[PATCH wayland v2 3/4] wayland-util: build/ship as separate shared library

[Emil Velikov]

On 21 March 2017 at 10:05, Pekka Paalanen <ppaalanen at gmail.com> wrote:
> On Mon, 20 Mar 2017 18:21:53 +0000
> Emil Velikov <emil.l.velikov at gmail.com> wrote:
>
>> On 17 March 2017 at 13:32, Pekka Paalanen <ppaalanen at gmail.com> wrote:
>> > On Fri, 17 Mar 2017 12:07:45 +0000
>> > Emil Velikov <emil.l.velikov at gmail.com> wrote:
>> >
>> >> On 17 March 2017 at 11:10, Pekka Paalanen <ppaalanen at gmail.com> wrote:
>> >> > On Thu, 16 Mar 2017 15:32:46 +0000
>> >> > Emil Velikov <emil.l.velikov at gmail.com> wrote:
>> >> >
>> >> >> On 16 March 2017 at 12:40, Pekka Paalanen <ppaalanen at gmail.com> wrote:
>> >
>> >> >> > Hi,
>> >> >> >
>> >> >> > since I have so much trouble making my mind on this patch, how about a
>> >> >> > silly counter-proposal?
>> >> >> >
>> >> >> > Let's squash libwayland-server.so and libwayland-client.so into a
>> >> >> > single libwayland.so.
>> >> >> >
>> >> >> > That would take care of the duplicate exported symbols issue not just
>> >> >> > for the util functions, but also for the exported variables produced by
>> >> >> > wayland-scanner. Since we have many programs (libEGL!) that necessarily
>> >> >> > already link to both, there cannot be problems from linking to
>> >> >> > everything always.
>> >> >> >
>> >> >> > We would still need to install dummy libwayland-server.so and
>> >> >> > libwayland-client.so just for pulling in libwayland.so, but that's no
>> >> >> > worse than Emil's proposition.
>> >> >> >
>> >> >> > Whether we have the existing .pc files telling to link to server/client
>> >> >> > or just libwayland.so would be up for debate. I'm not suggesting to
>> >> >> > merge the .pc files.
>> >> >> >
>> >> >> > libwayland-server.so and libwayland-client.so have exactly the same set
>> >> >> > of dependencies.
>> >> >> >
>> >> >> > Any downsides to this approach vs. doing nothing vs. Emil's
>> >> >> > libwayland-{client,server,util}.so?
>> >
>> >> How exactly do you see libwayland.so relative to the client/server one
>> >>  - which provides what symbols, etc.
>> >>  - are the client/server DSOs - symlinks/other ?
>> >
>> > This is how I imagined it:
>> >
>> > - libwayland.so contains all the code, and exports both server and
>> >   client symbols.
>> >
>> > - libwayland-server.so and libwayland-client.so are normal DSOs that
>> >   export essentially no symbols, they only depend on libwayland.so and
>> >   cause it to be loaded.
>> >
>> > In other words, you proposed to move util functions into a third
>> > library. I proposed to move everything into a third library.
>> >
>> I'm yet to see/hear of such a DSO, and I'd imagine it will be rather
>> confusing/misleading.
>>
>> >
>> > One thing now coming to my mind is this. With your
>> > https://github.com/evelikov/wl_link_test experiments, did you try the
>> > following?
>> > 1. build and install old wayland
>> > 2. build link test
>> > 3. build and install your patched wayland
>> > 4. run link test (do not rebuild)
>> >
>> > The point here is that the application executable nor library does not
>> > have libwayland-util.so as NEEDED themselves. Still, they need to
>> > continue working fine.
>> >
>> Dan pointed out the same concert - bumping the -version-info will handle that.
>
> No, that was a different case. Daniel was worried about downgrading
> libwayland*.so, I am asking about updating libwayland*.so without
> rebuilding users.
>
> How would bumping -version-info handle it?
>
> One updates libwayland*.so, gets a new version-info, while old programs
> were expecting the old ones. I can see only two possible outcomes:
> a) either version-info guarantees that the old programs no longer start
> as the library of that version is not found, or
> b) the version-info is still compatible, so nothing changes by bumping
> it.
>
> Regardless of that, the original question remains: the user binary (e.g.
> wl_link_test) will not have libwayland-util.so as NEEDED - does it
> still work?
>
> I've been running with the assumption that it still does work, and your
> commit message seemed to imply that too, hence I started wondering why
> you want to add libwayland-util.so as NEEDED in newly built binaries?
>
> Is it necessary, which would mean that all programs would need to be
> rebuilt, or is it not necessary, in which case it would only serve to
> break newly built programs in the case Daniel pointed out?
>
Summarising the points below. Highlighting the key words - hope it
provides some clarity.

If we don't bump the version-info:
A) w/o NEEDED, aka Requires.private: libwayland-util
 - Existing binaries _will_ work, as the symbols will be _implicitly_
resolved at _runtime_.
 - Newly build binaries _may_ (depending on a few factors) fail at _link_ time

B) with NEEDED, aka Requires: libwayland-util
 - Existing binaries will work, if we don't bump version-info
 - Newly build/linked binaries will fail to run with older wayland -
regardless of version-info.

If version-info is bumped, one should use 4:0:0 for wayland-client and
2:0:0 for wayland-server, to reflect the backward incompatible change.

TL;DR; It can be forward or backward compatible, but not both.
Note that even GLIBC does not guarantee compatibility in both directions.

>> > Then there is the case of dlopen()'ing these libraries. I think there
>> > might be something unexpected to expect because we have this:
>> > https://cgit.freedesktop.org/wayland/weston/tree/libweston/compositor-drm.c?id=2.0.0#n1575
>> > introduced in commit 97f2952b.
>> >
>> This one works perfectly fine with or w/o a version-info change - try
>> the wl-dl-test.
>>
>> > Does that mean that if someone writes a plugin for his app, and that
>> > plugin uses wl_list, your patch would require them to at least rebuild
>> > the plugin?
>> >
>> There's no requirement to rebuild anything. If we're concerned about
>> using FOO build against wayland X+1, while running on wayland X -
>> version-info will make that an explicit error, as it should be.
>> Similar thing has been done already twice, afaict.
>>
>> If that's not a concern one need not bother changing version-info.
>
> Ok, so it's just to ensure the user gets a runtime error they would get
> anyway. It does not solve anything, it just changes one error to
> another that happens more often.
>
> So, how come the hack is needed in Weston for some versions of Mesa,
> while in this case one should never need anything like it?
>
> The scenario is this:
>
> Someone writes a plugin (dlopen()'d DSO) for his program (an
> executable, or maybe loaded via library). The plugin uses
> libwayland-client (pkg-config name) and it uses wl_list API. His
> program and the plugin are built with the old libwayland. Then the
> Wayland libs get updated to a version including your patch. Now we have
> a situation where the program and the plugin do *not* explicitly link
> to libwayland-util, yet the plugin uses symbols exported only by
> libwayland-util. The plugin is dlopened(). Is this not exactly the same
> case as why we have the hack for Mesa in Weston?
>
FWIW the Mesa design decision was made way before I started any Mesa
development.
The case you're thinking of is very close yet subtly different from
the one we have around Mesa.

Here, when the plugin is linked against old wayland, it will have a
link against libwayland-client, libwayland-server or both.
Regardless of the API it uses.

As we plugin is dlopened, the libwayland-client/server dependency will
a) resolve the symbol dependency for older wayland or b) pull
libwayland-util which will provide the required symbols, in case of
newer wayland.

>> > ISTR libSDL dlopen()'s libwayland-client.so which is a slightly
>> > different case again.
>> >
>> Yes, dlopen/dlsym is safe, regardless of who is doing it.
>> To explain why/who - as one does dlsym(libwayland-client.so.0_handle,
>> "wl_list_goo") the linker will propagate to the direct dependencies
>> and if any of those provide the symbol a pointer to it will be
>> returned.
>
> Yes, I hope it works that way too. So what went wrong in Mesa in the
> case I referred to?
>
In Mesa the symbols are not resolved neither directly nor indirectly.
Here they are handled directly (old wayland) or indirectly (new
wayland).

>> > The case where one does rebuild the application is what you at least
>> > tested, as I understand.
>> >
>> Correct.
>>
>> FWIW: I think it would have saved us all time, if some/most/all of the
>> above were asked much earlier.
>
> I could not think of these questions earlier. It has taken hours and
> hours of reading and thinking and discussion to be able to even come up
> with ideas for these questions.
>
> I have no experience with these issues, which makes it extremely
> painful to even to try to make any kind of opinion here, let alone
> questions about the corner cases that may become the deal-breakers.
>
>> If you have any other concerns/issues please shout, pretty please ?
>
> See above. I do write them out as I they come to my mind.
>
> I am simply the wrong guy to poke here, even if I happen to be a
> maintainer for the library. The problem here is that you have to train
> me first to become able to review your patch. That takes a lot of time
> from the both of us. (An idea for TTT?)
>
I see - will try to work something out. I'm very grateful for the
examples and help.

> What makes things even more hard is that there is no concrete failure
> to investigate and solve. Not even an artificial one, is there?
>
> I would love to have tests added in the test suite for all the
> corner-cases we could come up with, but that would require somehow
> getting a hold of old releases in the test suite. Actually, we lack any
> kind of ABI backward-compatibility tests from the test suite.
>
The issues I have in mind, can perhaps be detected via Valgrind. It
will be very tricky though.
Other ABI changes - mentioned a few months back should be a lot easier
to manage. That's my next step as I finish some other wayland-scanner
patches.

>> >> > Indeed, that's what I need: Reviewed-by's and Acked-by's from people
>> >> > with proper reputation.
>> >> >
>> >> > Just like for any patch really, the strength of reviews needs to
>> >> > reflect the impact of the patch.
>> >> >
>> >> How about we do something like Daniel ?
>> >>  - he pointed out some possible flaws
>> >>  - I dismissed those and provided a simple test case to justify my claim
>> >>
>> >> If every 'issue' is refuted, shout for X weeks slap an Ack and let us
>> >> carry on with out lives ?
>> >
>> > I do not see Daniel's Reviewed-by's or Acked-by's on this revision of
>> > this patch, nor the earlier one.
>> >
>> > I will not give my R-b or A-b for this patch, because I don't feel
>> > qualified. The same goes for my counter-proposal, too. The
>> > counter-proposal would need your R-b and someone else's A-b as well.
>> >
>> Ack - your previous email was perfectly clear on these.
>>
>> I hope that one day you'll reconsider wrt Acked-by and I'd prefer if
>> that covers someone else's work than mine.
>
> Depends on if I can understand the patch and it's consequences as far
> as I know.
>
>> Some food for thought:
>> -  Do you have many experiences with bugs caused by symbol collision ?
>
> None.
>
Pray you don't run into them.

>>  - Have you considered issues, that you don't have personal experience
>> with, worth-while ? Where there many ?
>
> If I can understand an issue, or at least understand the fix enough to
> not cause any regressions, those are fine. In this case I know that I
> understand neither. You say there is a problem, and I don't see it.
> What's worse, I don't understand all the consequences of your suggested
> high-risk fix.
>
> As I have other things to do, some arguably more important even, and
> learning this topic properly would take a very considerable time, I am
> very much inclined to leave the reviewing of it for others who know
> better, until something causes its priority to rise (e.g. a release
> blocker).
>
I see - due to the shortage of knowledge/experience many changes are
considered high-risk.
That's perfectly reasonable assumption - I would do the exact same thing.

At the same time it's somewhat frustrating, when you've spent days
debugging similar issues in the past. Yet people state that it won't
be a problem :-\

Thanks again for the help. Let me know if you become a beer/wine fan -
I owe you a few!

-Emil