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

[Emil Velikov]

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

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.

HTH
Emil