[PATCH 1/2] scanner: Add --visibility flag for setting symbol visibility
Jonas Ådahl
jadahl at gmail.com
Wed Jul 26 07:49:57 UTC 2017
On Tue, Jul 25, 2017 at 02:44:29PM +0300, Pekka Paalanen wrote:
> On Tue, 25 Jul 2017 18:39:56 +0800
> Jonas Ådahl <jadahl at gmail.com> wrote:
>
> > Add a --visibility flag that enables the user to tweak the visibility
> > of the symbols generated by wayland-scanner. Three alternatives are
> > exposed:
> >
> > 'export': as has always been done up until now, export the symbols
> > using WL_EXPORT, making making them exposed externally. This is the
> > default in order to not break backward compatibility.
> >
> > 'compiler-default': use whatever visibility the compiler defaults to.
> > This is most likely the most visibility that protocol implementations
> > or users actually wants, as it doesn't expose any unwanted
> > implementation details.
> >
> > 'static': each symbol will only be visible to the compilation unit it
> > is included in. This means that a protocol implementations and users
> > needs to include both the 'code' file and either the 'client-header' or
> > 'server-header' (or both) in the source file implementing or using the
> > protocol in question.
> >
> > Using 'static' is a method to avoid situations where otherwise exposed
> > symbols of different protocols would conflict, for example if they have
> > the same interface name.
> >
> > When no visibility is specified, 'export' is assumed, but a warning is
> > printed to stderr, as it is unlikely that 'export' is what is actually
> > desired.
> >
> > Signed-off-by: Jonas Ådahl <jadahl at gmail.com>
> > ---
> > Makefile.am | 10 ++---
> > src/scanner.c | 124 +++++++++++++++++++++++++++++++++++++++++++++++++++-------
> > 2 files changed, 114 insertions(+), 20 deletions(-)
>
> Hi Jonas,
>
> thanks for writing this patch. The commit message is well written.
>
> I wonder where we could write down the guidelines on what visiblity to
> prefer:
>
> - always go for 'compiler-default' unless...
>
> - you already exported the symbols previously and cannot stop exporting
> them so choose 'export' to stop the warnings, or
>
> - you have to support protocols with conflicting symbol names and you
> have no choice but to use 'static' with the caveat it implies.
>
> Maybe this could be a new paragraph in usage()?
I don't see why not.
>
> >
> > diff --git a/Makefile.am b/Makefile.am
> > index d0c8bd3..d570525 100644
> > --- a/Makefile.am
> > +++ b/Makefile.am
> > @@ -97,19 +97,19 @@ nodist_libwayland_client_la_SOURCES = \
> > pkgconfig_DATA += src/wayland-client.pc src/wayland-server.pc
> >
> > protocol/%-protocol.c : $(top_srcdir)/protocol/%.xml
> > - $(AM_V_GEN)$(MKDIR_P) $(dir $@) && $(wayland_scanner) code < $< > $@
> > + $(AM_V_GEN)$(MKDIR_P) $(dir $@) && $(wayland_scanner) -Vexport code < $< > $@
> >
> > protocol/%-server-protocol.h : $(top_srcdir)/protocol/%.xml
> > - $(AM_V_GEN)$(MKDIR_P) $(dir $@) && $(wayland_scanner) server-header < $< > $@
> > + $(AM_V_GEN)$(MKDIR_P) $(dir $@) && $(wayland_scanner) -Vexport server-header < $< > $@
> >
> > protocol/%-client-protocol.h : $(top_srcdir)/protocol/%.xml
> > - $(AM_V_GEN)$(MKDIR_P) $(dir $@) && $(wayland_scanner) client-header < $< > $@
> > + $(AM_V_GEN)$(MKDIR_P) $(dir $@) && $(wayland_scanner) -Vexport client-header < $< > $@
> >
> > protocol/%-server-protocol-core.h : $(top_srcdir)/protocol/%.xml
> > - $(AM_V_GEN)$(MKDIR_P) $(dir $@) && $(wayland_scanner) server-header -c < $< > $@
> > + $(AM_V_GEN)$(MKDIR_P) $(dir $@) && $(wayland_scanner) -Vexport server-header -c < $< > $@
> >
> > protocol/%-client-protocol-core.h : $(top_srcdir)/protocol/%.xml
> > - $(AM_V_GEN)$(MKDIR_P) $(dir $@) && $(wayland_scanner) client-header -c < $< > $@
> > + $(AM_V_GEN)$(MKDIR_P) $(dir $@) && $(wayland_scanner) -Vexport client-header -c < $< > $@
> >
> > BUILT_SOURCES = \
> > $(nodist_libwayland_server_la_SOURCES) \
> > diff --git a/src/scanner.c b/src/scanner.c
> > index 517068c..d9152e3 100644
> > --- a/src/scanner.c
> > +++ b/src/scanner.c
> > @@ -57,6 +57,12 @@ enum side {
> > SERVER,
> > };
> >
> > +enum visibility {
> > + VISIBILITY_EXPORT,
> > + VISIBILITY_COMPILER_DEFAULT,
> > + VISIBILITY_STATIC
> > +};
> > +
> > static int
> > usage(int ret)
> > {
> > @@ -72,7 +78,16 @@ usage(int ret)
> > " the scanner was built against.\n"
> > " -c, --include-core-only include the core version of the headers,\n"
> > " that is e.g. wayland-client-core.h instead\n"
> > - " of wayland-client.h.\n");
> > + " of wayland-client.h.\n"
> > + " -V, --visibility=[export|compiler-default|static]\n"
> > + " select what type of visibility protocol\n"
> > + " symbols should have. 'export' will cause the\n"
> > + " symbols to be exported, 'compiler-default'\n"
> > + " will use the compiler default (often visible\n"
> > + " only within the same DSO), 'static' will make\n"
> > + " all symbols static (note that the file\n"
> > + " generated with 'code' must be included in the\n"
> > + " same file as the symbols are used\n");
> > exit(ret);
> > }
> >
> > @@ -226,6 +241,7 @@ struct entry {
> > };
> >
> > struct parse_context {
> > + enum visibility visibility;
> > struct location loc;
> > XML_Parser parser;
> > struct protocol *protocol;
> > @@ -1507,11 +1523,14 @@ emit_mainpage_blurb(const struct protocol *protocol, enum side side)
> > }
> >
> > static void
> > -emit_header(struct protocol *protocol, enum side side)
> > +emit_header(struct parse_context *ctx,
> > + struct protocol *protocol,
> > + enum side side)
> > {
> > struct interface *i, *i_next;
> > struct wl_array types;
> > const char *s = (side == SERVER) ? "SERVER" : "CLIENT";
> > + const char *symbol_decl_tag = NULL;
> > char **p, *prev;
> >
> > printf("/* Generated by %s %s */\n\n", PROGRAM_NAME, WAYLAND_VERSION);
> > @@ -1556,6 +1575,18 @@ emit_header(struct protocol *protocol, enum side side)
> > wl_array_release(&types);
> > printf("\n");
> >
> > + switch (ctx->visibility) {
> > + case VISIBILITY_EXPORT:
> > + symbol_decl_tag = "extern ";
> > + break;
> > + case VISIBILITY_COMPILER_DEFAULT:
> > + symbol_decl_tag = "";
> > + break;
> > + case VISIBILITY_STATIC:
> > + symbol_decl_tag = "static ";
> > + break;
> > + }
> > +
> > wl_list_for_each(i, &protocol->interface_list, link) {
> > printf("/**\n"
> > " * @page page_iface_%s %s\n",
> > @@ -1575,8 +1606,9 @@ emit_header(struct protocol *protocol, enum side side)
> > if (i->description && i->description->text)
> > format_text_to_comment(i->description->text, false);
> > printf(" */\n");
> > - printf("extern const struct wl_interface "
> > - "%s_interface;\n", i->name);
> > + printf("%sconst struct wl_interface "
> > + "%s_interface;\n",
> > + symbol_decl_tag, i->name);
>
> I believe the "extern" here is correct and required in all cases, so it
> should be left untouched. It just tells the compiler that this is not
> the definition of the global symbol, it is just a declaration.
>
> If you drop "extern", this becomes a definition filled with zeroes.
>
> What I do not understand is why do I not see any compiler warnings
> about duplicate variable definitions. When I looked through the
> generated files, we clearly have the same static variable defined
> multiple times in the same compilation unit - in case of the .inc file
> even in literally the same file.
We need to make it 'static ' when using static visibility still,
otherwise we get a compilation error:
error: static declaration of ‘tiny_intf_obj_interface’ follows non-static declaration
For 'compiler-default' I suppose it makes more sense to still use
'extern', as 'compiler-default' vs 'export' only affects how the same
"type" of variable is made visible externally.
>
> > }
> >
> > printf("\n");
> > @@ -1713,11 +1745,13 @@ emit_messages(struct wl_list *message_list,
> > }
> >
> > static void
> > -emit_code(struct protocol *protocol)
> > +emit_code(struct parse_context *ctx, struct protocol *protocol)
> > {
> > struct interface *i, *next;
> > struct wl_array types;
> > char **p, *prev;
> > + const char *visibility_tag = NULL;
> > + const char *symbol_decl_tag = NULL;
> >
> > printf("/* Generated by %s %s */\n\n", PROGRAM_NAME, WAYLAND_VERSION);
> >
> > @@ -1728,6 +1762,18 @@ emit_code(struct protocol *protocol)
> > "#include <stdint.h>\n"
> > "#include \"wayland-util.h\"\n\n");
> >
> > + switch (ctx->visibility) {
> > + case VISIBILITY_EXPORT:
> > + symbol_decl_tag = "extern ";
> > + break;
> > + case VISIBILITY_COMPILER_DEFAULT:
> > + symbol_decl_tag = "";
> > + break;
> > + case VISIBILITY_STATIC:
> > + symbol_decl_tag = "static ";
> > + break;
> > + }
> > +
> > wl_array_init(&types);
> > wl_list_for_each(i, &protocol->interface_list, link) {
> > emit_types_forward_declarations(protocol, &i->request_list, &types);
> > @@ -1738,7 +1784,8 @@ emit_code(struct protocol *protocol)
> > wl_array_for_each(p, &types) {
> > if (prev && strcmp(*p, prev) == 0)
> > continue;
> > - printf("extern const struct wl_interface %s_interface;\n", *p);
> > + printf("%sconst struct wl_interface %s_interface;\n",
> > + symbol_decl_tag, *p);
>
> Here is the same thing, we are emitting declarations ahead of time, not
> definitions, so the "extern" is required.
>
> > prev = *p;
> > }
> > wl_array_release(&types);
> > @@ -1752,14 +1799,26 @@ emit_code(struct protocol *protocol)
> > }
> > printf("};\n\n");
> >
> > + switch (ctx->visibility) {
> > + case VISIBILITY_EXPORT:
> > + visibility_tag = "WL_EXPORT ";
> > + break;
> > + case VISIBILITY_COMPILER_DEFAULT:
> > + visibility_tag = "";
> > + break;
> > + case VISIBILITY_STATIC:
> > + visibility_tag = "static ";
> > + break;
> > + }
> > wl_list_for_each_safe(i, next, &protocol->interface_list, link) {
> >
> > emit_messages(&i->request_list, i, "requests");
> > emit_messages(&i->event_list, i, "events");
> >
> > - printf("WL_EXPORT const struct wl_interface "
> > + printf("%sconst struct wl_interface "
> > "%s_interface = {\n"
> > "\t\"%s\", %d,\n",
> > + visibility_tag,
> > i->name, i->name, i->version);
>
> This is the important and correct hunk.
>
> >
> > if (!wl_list_empty(&i->request_list))
> > @@ -1790,12 +1849,32 @@ free_protocol(struct protocol *protocol)
> > free_description(protocol->description);
> > }
> >
> > +static int
> > +parse_visibility_str(const char *visibility_str,
> > + enum visibility *visibility)
> > +{
> > + if (strcmp(visibility_str, "export") == 0) {
> > + *visibility = VISIBILITY_EXPORT;
> > + } else if (strcmp(visibility_str, "compiler-default") == 0) {
> > + *visibility = VISIBILITY_COMPILER_DEFAULT;
> > + } else if (strcmp(visibility_str, "static") == 0) {
> > + *visibility = VISIBILITY_STATIC;
> > + } else {
> > + fprintf(stderr, "Error: invalid visibility string '%s'\n",
> > + visibility_str);
> > + return -1;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > int main(int argc, char *argv[])
> > {
> > struct parse_context ctx;
> > struct protocol protocol;
> > FILE *input = stdin;
> > char *input_filename = NULL;
> > + char *visibility_str = NULL;
> > int len;
> > void *buf;
> > bool help = false;
> > @@ -1803,6 +1882,7 @@ int main(int argc, char *argv[])
> > bool version = false;
> > bool fail = false;
> > int opt;
> > + enum visibility visibility = 0;
> > enum {
> > CLIENT_HEADER,
> > SERVER_HEADER,
> > @@ -1810,14 +1890,15 @@ int main(int argc, char *argv[])
> > } mode;
> >
> > static const struct option options[] = {
> > - { "help", no_argument, NULL, 'h' },
> > - { "version", no_argument, NULL, 'v' },
> > - { "include-core-only", no_argument, NULL, 'c' },
> > - { 0, 0, NULL, 0 }
> > + { "help", no_argument, NULL, 'h' },
> > + { "version", no_argument, NULL, 'v' },
> > + { "include-core-only", no_argument, NULL, 'c' },
> > + { "visibility", required_argument, NULL, 'V' },
> > + { 0, 0, NULL, 0 }
> > };
> >
> > while (1) {
> > - opt = getopt_long(argc, argv, "hvc", options, NULL);
> > + opt = getopt_long(argc, argv, "hvcV:", options, NULL);
> >
> > if (opt == -1)
> > break;
> > @@ -1832,6 +1913,9 @@ int main(int argc, char *argv[])
> > case 'c':
> > core_headers = true;
> > break;
> > + case 'V':
> > + visibility_str = optarg;
> > + break;
> > default:
> > fail = true;
> > break;
> > @@ -1858,6 +1942,15 @@ int main(int argc, char *argv[])
> > else
> > usage(EXIT_FAILURE);
> >
> > + if (!visibility_str) {
> > + fprintf(stderr,
> > + "Warning: protocol symbols will be exported, "
> > + "make sure this is desired.\n");
>
> Maybe add "See --help for more information on visibility."?
Good idea.
Jonas
>
> > + visibility = VISIBILITY_EXPORT;
> > + } else if (parse_visibility_str(visibility_str, &visibility) != 0) {
> > + usage(EXIT_FAILURE);
> > + }
> > +
> > if (argc == 3) {
> > input_filename = argv[1];
> > input = fopen(input_filename, "r");
> > @@ -1882,6 +1975,7 @@ int main(int argc, char *argv[])
> > /* initialize context */
> > memset(&ctx, 0, sizeof ctx);
> > ctx.protocol = &protocol;
> > + ctx.visibility = visibility;
> > if (input == stdin)
> > ctx.loc.filename = "<stdin>";
> > else
> > @@ -1931,13 +2025,13 @@ int main(int argc, char *argv[])
> >
> > switch (mode) {
> > case CLIENT_HEADER:
> > - emit_header(&protocol, CLIENT);
> > + emit_header(&ctx, &protocol, CLIENT);
> > break;
> > case SERVER_HEADER:
> > - emit_header(&protocol, SERVER);
> > + emit_header(&ctx, &protocol, SERVER);
> > break;
> > case CODE:
> > - emit_code(&protocol);
> > + emit_code(&ctx, &protocol);
> > break;
> > }
> >
>
> The rest looks good.
>
> Acked-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
>
> but for merging the issue of multiple definitions needs to be solved.
>
>
> Thanks,
> pq
More information about the wayland-devel
mailing list