[PATCH 1/2] scanner: Add --visibility flag for setting symbol visibility
Pekka Paalanen
ppaalanen at gmail.com
Tue Jul 25 11:44:29 UTC 2017
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()?
>
> 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.
> }
>
> 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."?
> + 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
-------------- 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/20170725/c25623b0/attachment.sig>
More information about the wayland-devel
mailing list