On 27 July 2017 at 14:01, Pekka Paalanen <[email protected]> wrote: > On Wed, 26 Jul 2017 16:09:32 +0100 > Emil Velikov <[email protected]> wrote: > >> On 25 July 2017 at 10:24, Pekka Paalanen <[email protected]> wrote: >> > On Tue, 25 Jul 2017 15:25:58 +0800 >> > Jonas Ådahl <[email protected]> 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 <[email protected]> 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 <[email protected]> >> >> > > --- >> >> > > >> >> > > 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 _______________________________________________ wayland-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/wayland-devel
