[PATCH wayland 0/4] Untangle the symbol export duplication

[Pekka Paalanen]

On Fri, 16 Sep 2016 01:03:40 +0100
Emil Velikov <emil.l.velikov at gmail.com> wrote:

> On 15 September 2016 at 11:47, Pekka Paalanen <ppaalanen at gmail.com> wrote:
> > On Wed, 14 Sep 2016 16:57:17 +0100
> > Emil Velikov <emil.l.velikov at gmail.com> wrote:
> >  
> >> Hi Pekka,
> >>
> >> Huge thanks for the input.
> >>
> >> On 14 September 2016 at 14:35, Pekka Paalanen <ppaalanen at gmail.com> wrote:  
> >> > On Tue, 30 Aug 2016 18:24:18 +0100
> >> > Emil Velikov <emil.l.velikov at gmail.com> wrote:
> >> >  
> >> >> Hi all,
> >> >>
> >> >> For a while I've noticed that on mesa side we have few providers of the
> >> >> wl_drm_interface symbol and literally all our 'wayland binaries' are
> >> >> linked against both the client and server wayland libs.
> >> >>
> >> >> After having a look, it seems that:
> >> >>  - the server exposes the interface symbols for 'inheritance' purposes,
> >> >> thus other servers can build upon these primitives while designing their
> >> >> own protocols (interfaces ?).  

Hi,

there seems to be two different issues discussed here.

1) libwayland-client and libwayland-server export the same interface
symbols each, and cannot stop exporting them. You proposed to
consolidate them into a third library.

I don't think that is necessary.

2) Push everyone to build and install a .so and generated headers for
every protocol extension XML file they offer for public use. You claim
it would have benefits over the static build approach.

The static approach can break only when the interface symbols are
exported and therefore possible to have conflicting symbols from
different binaries all loaded into the same process. I think that is
solved well enough by making wayland-scanner to stop forcing the export
of the interface symbols.

Detailed replies are inline below.

> >> >
> >> > I have no idea what that means. wl_drm is private to Mesa.
> >> >
> >> > You are not supposed to use wl_drm in your apps, and *no-one* is
> >> > supposed to build on it by any other way than copying the XML file and
> >> > renaming everything. The recommended way would be to start writing XML
> >> > from scratch though.
> >> >  
> >> The wl_drm* symbols are what brought me here. AFAICT all the
> >> information/discussion is generic and not specific to it.
> >>
> >> See below for more.
> >>  
> >> >>  - at the same time the client needs to have a reference to the
> >> >> interface symbol in order to register an instance of said interface.  
> >> >
> >> > Anyway, those details don't really matter at all, see below.
> >> >  
> >> >> This means that if the "wrong" symbol gets picked at runtime and the
> >> >> client does not correctly manage older versions in its
> >> >> .registry_handle_global function (yes we have a case or two of those in
> >> >> the wild) things will end up horribly wrong.
> >> >>
> >> >> I think that a better option would have been to distinguish the two
> >> >> (call one instance or the other singleton) and let the scanner attribute
> >> >> for the name difference. Regardless of that is that it's too late to
> >> >> change things since this would lead to breaking the ABI.
> >> >>
> >> >> One way around it to update the scanner to provide newer symbols and
> >> >> annotate the old ones as deprecated. This way when/if we break the ABI
> >> >> we can untangle things. One day...
> >> >>
> >> >> Does the above sound about right - can anyone let me know if I'm loosing
> >> >> the plot ?
> >> >>
> >> >> That said, I've went ahead and removed duplication where possible - by
> >> >> folding the util symbols into libwayland-util. Please check with patch 3/4
> >> >> how compatibility with existing and new users is be preserved.  
> >> >
> >> > Actually, the <foo>_interface symbols are not meant to be exported *at
> >> > all*. But there is a catch that is the source for all the confusion.
> >> >
> >> > For historical reasons, libwayland-server and libwayland-client are
> >> > designed to export the wl_*_interface pointers, because libwayland
> >> > ships the headers generated from wayland.xml.  
> >>  
> >> > The headers cannot work
> >> > if something does not provide the symbols from the generated
> >> > wayland-protocol.c file.  
> >> Having a dull moment - what do you mean with the above ? How can
> >> headers "work" or "not work" ?  
> >
> > Hi,
> >
> > you can generate the headers and use them, but you cannot write all the
> > necessary code to get a working thing without also having access to the
> > symbols exported from wayland-protocol.c. To be specific, you can never
> > create any object type whose wl_*_interface symbol you do not have
> > access to. This applies both to server and client side.
> >
> > Furthermore, the client side generated headers explicitly use the
> > *_interface symbols. On server side they are used by the hand-written
> > code in a compositor.
> >  
> Afaict (I'm looking at the mesa generated files yet they should be
> similar/identical to others):
>  - The generated headers contain only the *_interface symbol
> declaration. None of the generated code makes use of it.

That is only because the XML does not define new interfaces to be
instantiated by statically typed requests. The only new object type
(wl_drm) is created via wl_registry. IOW, your example is lacking in
features.

For a better example, look at e.g.
https://cgit.freedesktop.org/wayland/wayland-protocols/tree/stable/viewporter/viewporter.xml
and search for the use of wp_viewport_interface in the generated
headers.

>  - Both client and server, take const pointer to the symbol in order
> to bind an instance (client) and create the global (?) resource
> (server).
> Which in itself requires a provider for the said symbol(s). I believe
> the latter is what you meant with "don't work", correct ?

That is the "you cannot write all the necessary code" part, yes.

The other part, which wayland-drm.xml does not excercise, is in the
generated client headers as I explained above.

> > (Confusingly, the server side also has *_interface as a struct type in
> > addition to the variable.)
> >  
> Indeed. It was fun first time I saw it ;-)
> 
> >> > IMHO and in hindsight, this was a mistake - we
> >> > should have installed only the XML file and require everyone to
> >> > generate their own wayland.xml bindings to keep things consistent. But,
> >> > it's done, and now it is API and ABI, so we cannot change that.
> >> >  
> >> That may have been a good idea indeed.
> >>  
> >> > That is the reason why wayland-scanner emits all the WL_EXPORTs in the
> >> > *-protocol.c file. IMO it really should not do that, except for
> >> > wayland.xml itself only when building libwayland.
> >> >
> >> > It's an unfortunate accident that all libs using Wayland extensions
> >> > from wayland-protocols or anywhere are exporting the *_interface
> >> > symbols. There is really no need for that as I see. If two pieces of
> >> > software use the same extension, they can just (generate and) build in
> >> > their own copies of the *-protocol.c file.
> >> >  
> >> Note: I'll be using protocol and interface interchangeably.
> >>
> >> Here's my take on it:
> >> Server foo implements protocol A which builds on top of the core
> >> wayland primitives/interfaces X and Y.
> >> For linking purposes server foo will depend on
> >> libwayland-{server,client} to provide the {X,Y}_interface symbols.  
> >
> > Yes and no.
> >
> > You do not need X_interface if you only use the type as an argument in
> > your own requests and events.
> >
> > X_interface is only needed when creating objects of that kind.
> >  
> Which (m)any new servers would like to do.

I'm not sure what you're trying to say.

If X_interface is something in the wayland.xml, then libwayland-server
will provide for the symbol. If X_interface is defined in any other XML
file, you need to use wayland-scanner on the XML file to generate
the .c file and build that into your program.

> >> Similarly on the client foo side, one needs to know the specifics of
> >> the interfaces A, X and/or Y in order to register/instantiate them.
> >> Which in itself, pulls the link time dependency of
> >> libwayland-{server,client}. With the latter being the recommended one
> >> afaict.  
> >
> > Again, only when creating objects of type X and Y, not when dealing
> > with any aspect of A.
> >  
> As above. Furthermore any client may wish to bind an instance of the
> new X and Y as well A (for whatever reason), even if A isn't created
> by libfoo-server. Disclaimer: not 100% sure if the latter will
> work/isn't against wayland design policy.

An interface can only ever be defined in one place, and no other place
can change that or inherit from it (create a new interface of the same
name but with added messages) because it will conflict eventually.

An interface can be augmented by adding new messages and bumping the
interface versions in the hierarchy according to the versioning rules.
This will always keep the newer version compatible with the older
version, so any version you have will always fully support any older
version too. This applies also to the C interface symbols we are
talking about.

Interface development by version bumps cannot branch, it must always be
kept linear, which means that we really want to have just one XML file
defining the interface.

> > You could technically have A create objects of type X, but we strongly
> > advice against it, because having the same type constructed from
> > independent parent-object trees will break interface versioning.
> >
> > The reason why libwayland exports the interface symbols is because it
> > does not create the object of those types itself. That's the only
> > reason to export them in general, too.
> >  
> >> The only way around this would be to have all the protocol A, X and Y
> >> code statically linked inside both client/server foo. Which in itself,
> >> will cause binary bloat. From memory we're talking about ~20K for the
> >> core wayland ones. That is 20K for each binary that uses wayland -
> >> surely less than ideal.  
> >
> > IMO the problems caused by sharing well outweigh that. Or would you
> > rather have every XML file turned into a library with its own .so and
> > headers to be installed? Also the private protocol extensions that are
> > not meant to be used out side of a given project?
> >  
> Having libfoo library which ships with respective header and exports
> the specific _interface symbol(s) is a good idea IMHO.
> Regardless of the language (used to write or utilise the library) and
> number of projects that use the interface, one can employ
> dependency/version tracking alongside the version checks that wayland
> provides (the ones you mention below).

Such libfoo would not include any text, only variables with data that
is constant and completely defined by the XML file it was created from.
All the functions in the headers will be inline and call into libwayland
directly.

All such libraries would be ignored by all other language bindings.
Language bindings have their own scanners generating wrappers suitable
for their language directly. The symbols we are talking about are just
a curiosity of the C language bindings.

I really do not see much benefit from that. I do see a hassle with
installing libraries and versioning everything properly.
Wayland-protocols would need to install 15 libraries already and that
number will just grow and never go down. Add to that all the work
needed in projects using these to start using the libraries.

A project cannot make use of an interface v5 if it only implements
support for v4. Using the library you suggest or not does not change
that, support for a new interface version always requires new
hand-written code for the new semantics. Therefore it is good enough to
build the interface v4 definitions into the program directly, and avoid
the runtime dependency hassle. The built-in v4 will work just fine,
even if the other side of IPC supported only up to v3 or all the way up
to v8.


> Projects that have a protocol/interface that is used only internally,
> can either static link or use a private shared library.

IOW, do what we already do with all protocol extensions (statically
built-in) except wayland.xml.

> >> > Libwayland has been written to cope just fine with multiple copies of
> >> > interface definitions, or it at least should.
> >> >  
> >> Can libwayland cope with clients which do not check versions and
> >> always assume that vX will be present ? I believe not, yet again I'm
> >> not sure if one can really achieve that. Speaking of which I should
> >> really send a patch or two to fix things :-\  
> >
> > I do not understand the question.
> >
> > The interface version negotiation happens when you use wl_registry to
> > bind a global. The server advertises the version
> > MIN(server_implemented, available_in_symbols). The client will choose
> > version MIN(advertised, client_implemented, available_in_symbols).
> >
> > People often ignore the available_in_symbols there, because they
> > generate their own .c and .h files straight from the XML file which
> > they know to be at least the expected version. Exporting the
> > *_interface symbols is what causes confusion there.
> >  
> When there are multiple providers for the *_interface symbol(s), which
> one gets picked is an interesting question.

There should never be multiple different providers, that is the core
underlying issue.

We can fix that for everything except libwayland by not exporting the
symbols.

> Combine that with the lack
> of MIN(...) in (some) clients, equals a recipe for disaster.
> Admittedly this (complete lack of MIN) is an application bug, yet it's
> something that cannot (should not) be fixed/handled in wayland,
> correct ?

Are you talking about ignoring available_in_symbols version, or not
doing version checks at all?

Not doing version checks at all is a bug, because you don't know at
build time which version the remote side of IPC will support.

Ignoring available_in_symbols version is totally ok if you build in
your own code generated from an XML file with the interface versions
that you need. By definition, the available_in_symbols version will
then be exactly right, and does not need checking at runtime.

> > After all, people already use the headers they know are good, so getting
> > a mismatching symbol for *_interface at runtime is quite a surprise.
> >  
> Personally I haven't had bad experiences but as we get more wayland
> programs and people using those, it is unlikely that we'll be so
> fortunate.

Yes, so let's stop the forced exporting where we can.

> > Hence, let's stop the exporting. That will solve the accidents of
> > getting not the *_interface symbol one compiled oneself.
> >
> >  
> >> >
> >> > I would seriously suggest that we make wayland-scanner *not* emit any
> >> > WL_EXPORTs for the *-protocol.c file if at all possible. I would like
> >> > that to be the default, while libwayland build would opt-in for the
> >> > exports.
> >> >
> >> > Can we do that, or would it break any ABI we actually care about?
> >> >  
> >> How will one resolve the dependencies of the now exported _interface
> >> symbols at link time ?  
> >
> > No-one should be using them to begin with, unless they have
> > intentionally written a library their own interfaces similar to
> > libwayland exposing wayland.xml interfaces. In the latter case, it is
> > only an API break as they just need to tell wayland-scanner to export
> > stuff.
> >
> > I would prefer wayland-scanner to default to not exporting, because I
> > believe most exports are accidental and might not get fixed otherwise.
> > If one exported intentionally, they will notice the break, and fix it.
> >  
> From my humble experience developers/engineers cannot invest into
> understanding how each block works in deatail. Thus they take many
> things as granted.
> That said, it's perfectly reasonable that they do rely upon those,
> even though they don't know/realise that, since things are exported by
> default.
> 
> Hiding the symbols is likely to lead to local workarounds, in the form
> of "let's check-in this generated files and fix them because
> wayland-scanner produces strange/broken files". Sadly we've all seen a
> fair few similar cases.

That will happen in any case. Let upstream at least try to do the right
thing where it still can, shall we?

> >> Hiding the interface symbols from libwayland-{server,client} will lead
> >> to a massive ABI break, since effectively everything (that I've
> >> looked) which uses wayland depends on a selection of those.  
> >
> > That is the only exception. I said: "while libwayland build would opt-in
> > for the exports." I never meant libwayland to stop exporting, just
> > everything else should stop exporting.
> >  
> >> "Everything" being: mesa (gbm, egl, vulkan), webkit2gtk,
> >> KF5/kwin/kscreen*, weston binaries, Qt wayland integration, mpv, vaapi
> >> (front end and drivers).  
> >
> > libwayland must continue exporting the wayland.xml symbols, yes. This
> > is the exception to the rule.
> >  
> Ack, I see - so libwayland-client and libwayland-server will expose
> the _interface symbols ?

They already do, have always done, and they must continue to do so,
because the headers they install cannot work otherwise, and the headers
have also made all built programs directly dependent on the symbols
even without anyone specifically writing code to rely on them.

No other library that I know of installs headers generated by
wayland-scanner. I certainly hope no-one does it.

> Isn't this the case that we started with - which afaict is far from
> ideal. This can easily be resolved by introducing a shared lib,
> libwayland-interface.so (as mentioned above). This is how this patch
> series resolves all the other duplicated symbols (by introducing
> libwayland-utils.so).

This is where I realized we were talking about the two different issues.

> Similar to libwayland-util.so, any old and new projects will continue
> to work and the symbol duplication will be resolved.

You only resolve the duplication between libwayland-server and
libwayland-client, which has never been a problem heard about in
upstream. The symbols exported by them are identical, so it does not
matter which one provides them.

I agree that problems can arise if you link several different versions
of libwayland-client and/or -server into the same program at runtime. Am
I the only one who thinks that is a distribution error?

Introducing libwayland-interface.so would not fix that case either,
when you mix one lib from before and one from after
libwayland-interface.so was introduced.

> Any projects which introduce add/their own protocol can be advised to
> have a shared lib, by updating wayland-scanner to produce a meaningful
> message.
> Obviously they can still continue to static link, with the message
> suggesting how to "hide" the _interface symbols if they choose to do
> so.
> 
> > Mesa must stop exporting the wl_drm.xml symbols.
> >  
> I know that giving vaapi as an example is discouraged, yet doing so
> will break the library :-\
> I'm not sure how many others are in a similar boat and deliberately
> breaking apps is (almost) never a wise move.

Does libva not carry a copy wayland-drm.xml and generate its own code
from it during build?

Or is it really depending on the... hmm, libEGL.so? - exporting
wl_drm_interface?

> tldr: I'm suggesting creating a libwayland-interface.so shared library
> and updating wayland-scanner to suggest/educate devs to either a)
> create their own shared ones as needed or b) how to hide the symbols
> if they opt to static link the protocol code.

They cannot hide the symbols until we fix wayland-scanner to not
force-export them.

> Using shared library:
> pros: code reuse, single provider for the _interface symbol.
> cons: extra bit in dependency tracking/tree
> 
> Using static linking:
> pros: simpler dependency tree/tracking
> cons: 'bloated' binaries, duplicated symbols which may or may not be
> conflicting (in one keeps the WL_EXPORT), broken applications (if one
> drops WL_EXPORT)

IMO, let's go with static only. There really isn't enough to share in
a .so to warrant all the work needed for it all over. An .so would not
make the checks on the available_in_symbols version any less necessary,
you could just trade it off to checking .so version at runtime.

wayland-scanner generates a .c file because putting that data inline in
the generated headers would actually be too much as you would get a
copy per each compilation unit. One copy per binary is the best
trade-off between simplicity and saving space in my opinion.

I wonder if anyone else would bother giving their opinion...


Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 811 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20160916/731742f5/attachment.sig>