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

[Emil Velikov]

On 27 July 2017 at 14:01, Pekka Paalanen <ppaalanen at gmail.com> wrote:
> 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?
>

Tl:Dr; I think that everything should be prefixed. See below for details.

 - ABI symbols - to prevent symbol collision
 - inline/other API - to minimise ambiguity, confusion, plus you can
have multiple implementations in the same translation unit*


*Some projects may want to support xdg-shell vX and vX+1 in the same file.
 - The inline API from vX may work with vY or vise-versa. Yet it's
easier to pass the wrong object to the inline function.
 - One can (and will have) cases, where API foo() is available for
only one version.
 - If the function signature (of the inline API) differs the compiler
will barf at you - say vX+1 adds extra argument to function bar()

Thanks
Emil