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

Tue Mar 21 10:05:43 UTC 2017

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?

> > 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?

> > 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?

> > 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?)

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.

> >> > 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.

>  - 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).

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