[PATCH wayland 2/3] scanner: Allow adding a prefix to exported symbols

[Pekka Paalanen]

On Wed, 26 Jul 2017 16:09:32 +0100
Emil Velikov <emil.l.velikov at gmail.com> wrote:

> On 25 July 2017 at 10:24, Pekka Paalanen <ppaalanen at gmail.com> wrote:
> > On Tue, 25 Jul 2017 15:25:58 +0800
> > Jonas Ådahl <jadahl at gmail.com> wrote:
> >  
> >> On Mon, Jul 24, 2017 at 02:16:04PM +0300, Pekka Paalanen wrote:  
> >> > On Mon,  3 Jul 2017 17:16:45 +0800
> >> > Jonas Ådahl <jadahl at gmail.com> wrote:
> >> >  
> >> > > Two different protocols may use interfaces with identical names.
> >> > > Implementing support for both those protocols would result in symbol
> >> > > clashes, as wayland-scanner generates symbols from the interface names.
> >> > >
> >> > > Make it possible to avoiding these clashes by adding a way to add a
> >> > > prefix to the symbols generated by wayland-scanner. Implementations
> >> > > (servers and clients) can then use these prefix:ed symbols to implement
> >> > > different objects with the same name.
> >> > >
> >> > > Signed-off-by: Jonas Ådahl <jadahl at gmail.com>
> >> > > ---
> >> > >
> >> > > Something like this would be needed if a compositor/client wants to implement
> >> > > xdg-shell unstable v5 alongside xdg-shell stable, unless we want to rename all
> >> > > our xdg-shell interfaces. Implementing xdg-shell unstable v6 alongside
> >> > > xdg-shell stable does not have this issue.
> >> > >
> >> > > See issue raised here:
> >> > > https://lists.freedesktop.org/archives/wayland-devel/2017-June/034380.html
> >> > >
> >> > >
> >> > > Jonas
> >> > >
> >> > >
> >> > >  src/scanner.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++-------------
> >> > >  1 file changed, 73 insertions(+), 21 deletions(-)  
> >> >
> >> >
> >> > Hi,
> >> >
> >> > while this seems to change the ABI symbol names, it does not change the
> >> > names in the documentation, and it does not change the names of
> >> > #defines of enums, or the inline functions. That means that this is not
> >> > enough to fulfill the purpose: being able to use two similarly named
> >> > but different protocols by adding a prefix.  
> >>
> >> The idea I had was rather that one would avoid changing any names on
> >> non-symbols. It'd still be possible to implement both by doing it in
> >> separate C files. I can see the point in adding the prefix on everything
> >> though, so I'll provide a patch for that.
> >>  
> >
> > Hi,
> >
> > recapping the discussion from IRC, we pretty much agreed that prefixing
> > is not a nice solution. Jonas found out that we cannot actually prefix
> > everything, because there usually are references to other protocol
> > things (like you would never want to prefix wl_surface). So it becomes
> > very hard to prefix things appropriately.
> >
> > The alternative we discussed is solving a different problem: scanner
> > makes all the public symbols in the generated .c files WL_EXPORT, which
> > makes them leak into DSO ABI, which is bad. In my opinion, it should
> > have never happened in the first place. But we missed it, and now it has
> > spread, so we cannot just fix scanner to stop exporting, the decision
> > must be with the consumers. So we need a scanner option to stop
> > exporting.
> >
> > Quentin proposed we add a scanner option
> > --visibility={static|compiler|export}. It would affect all the symbols
> > exported from the generated .c files as follows:
> >
> > - static: the symbols will be static.
> > - compiler: the symbols will get whatever the default visibility is
> >   with the compiler, i.e. not explicitly static and not exported
> > - export: the symbols are exported (this the old behaviour, and will be
> >   the default)
> >
> > Obviously, the only way to actually make use of the 'static' option is
> > for the consumer to #include the generated .c file. It's ugly, yes, but
> > it solves the conflicting symbol names issue Jonas was looking into.
> >
> > In my opinion, the prefixing approach where we still cannot prefix
> > everything in a way that one could use conflicting protocols in the
> > same compilation unit, and where e.g. the static inline functions are
> > not prefixed, is more ugly than the 'static' option.
> >
> > We are going to need an option to stop the exports anyway, and it seems
> > like we can piggyback on that solution for the problem underlying the
> > prefixing proposal as well.
> >  
> It sounds like Quentin proposal is trying to address two distinct
> things at the same time:
>  - expose correct symbol visiblity
>  - avoid symbol crashes due to conflicting naming used for "different" protocols

Yes, it indeed is.

> Former is addressed by swapping WL_EXPORT with WL_PRIVATE, or alike
> While the latter can be tackled in a couple of says:
>  - manually update the sources to use separate distinct name for the protocol
>  - convince the generator (scanner) to produce ones for us
> 
> I think convincing the scanner is a perfectly reasonable solution.

Does that mean you are in favour of this patch 2/3 as is?
Is it ok to prefix only the "ABI symbols" and leave everything else in
the API, e.g. static inline functions, unprefixed?


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/20170727/1022a4a7/attachment.sig>