[PATCH wayland v2 1/4] scanner: Add --visibility flag for setting symbol visibility
Pekka Paalanen
ppaalanen at gmail.com
Wed Jul 26 13:13:01 UTC 2017
On Wed, 26 Jul 2017 16:16:33 +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>
> ---
>
> Changes since v1:
>
> - Use 'extern' tag also for compiler default declarations.
>
> - "see --help" message to the warning
>
> - Added 'what visibility to choose' kind of documentation to --help
>
>
> Makefile.am | 10 ++---
> src/scanner.c | 135 +++++++++++++++++++++++++++++++++++++++++++++++++++-------
> 2 files changed, 125 insertions(+), 20 deletions(-)
Hi,
patches 2-4 are:
Reviewed-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
Patch 1 looks good, except for one detail we have missed completely and
ruins almost everything.
> diff --git a/src/scanner.c b/src/scanner.c
> index 517068c..3e43f90 100644
> --- a/src/scanner.c
> +++ b/src/scanner.c
> @@ -1507,11 +1537,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 +1589,16 @@ emit_header(struct protocol *protocol, enum side side)
> wl_array_release(&types);
> printf("\n");
>
> + switch (ctx->visibility) {
> + case VISIBILITY_EXPORT:
> + case VISIBILITY_COMPILER_DEFAULT:
> + symbol_decl_tag = "extern ";
> + 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 +1618,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);
> }
>
> printf("\n");
> @@ -1713,11 +1757,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 +1774,16 @@ emit_code(struct protocol *protocol)
> "#include <stdint.h>\n"
> "#include \"wayland-util.h\"\n\n");
>
> + switch (ctx->visibility) {
> + case VISIBILITY_EXPORT:
> + case VISIBILITY_COMPILER_DEFAULT:
> + symbol_decl_tag = "extern ";
> + 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 +1794,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);
In the static case, we emit a thing like:
static const struct wl_interface intf_not_here_interface;
But that is wrong, because 'intf_not_here' could as well be
'wl_surface', i.e. an interface defined in another XML file.
> prev = *p;
> }
> wl_array_release(&types);
> @@ -1752,14 +1809,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);
>
> if (!wl_list_empty(&i->request_list))
However, it is curious.
$ git ngrep -p intf_not_here -- tests/data/static-small-*
tests/data/static-small-client-core.h=46=struct intf_A;
tests/data/static-small-client-core.h:47:struct intf_not_here;
tests/data/static-small-client-core.h=155=intf_A_rq1(struct intf_A *intf_A, const struct wl_interface *interface, uint32_t version)
tests/data/static-small-client-core.h:168:static inline struct intf_not_here *
tests/data/static-small-client-core.h=169=intf_A_rq2(struct intf_A *intf_A, const char *str, int32_t i, uint32_t u, wl_fixed_t f, int32_t fd, struct another_intf *obj)
tests/data/static-small-client-core.h:174: INTF_A_RQ2, &intf_not_here_interface, NULL, str, i, u, f, fd, obj);
tests/data/static-small-client-core.h:176: return (struct intf_not_here *) typed_new;
tests/data/static-small-code-core.c=32=static const struct wl_interface another_intf_interface;
tests/data/static-small-code-core.c:33:static const struct wl_interface intf_not_here_interface;
tests/data/static-small-code-core.c=35=static const struct wl_interface *types[] = {
tests/data/static-small-code-core.c:37: &intf_not_here_interface,
tests/data/static-small-server-core.h=49=struct intf_A;
tests/data/static-small-server-core.h:50:struct intf_not_here;
The problematic
static const struct wl_interface intf_not_here_interface;
is there. But I would have expected to see another one in the client
header, because it uses intf_not_here_interface. Instead it seems to
rely on including the depended-on protocol's client header first.
What to do?
Make only local interfaces static and keep external interfaces always
as 'extern'?
It would break down if one wanted to have two interacting protocol
extensions both generated as static, but I suppose that's so far into
corner-case territory that we can ignore it?
Could we drop the declaration for external interfaces completely, and
rely on the user #including the depended-on header, like the
client-headers already do?
That would be cleaner, except if we do it only in the static case, but
OTOH if we did it in all cases, it might break someone's build. Well,
given it's a .c file we generate, it would definitely break builds, so
we can drop the declarations only in the static case where the code
file must be #included anyway.
Opinions?
Given all this, I'm leaning on giving up hope on landing this for the
ongoing release. We need more soaking time for whatever we do. Not
quite sure if asking for a second RC would be enough either, and
personally I will be on holidays for two weeks starting on Friday.
Maybe omitting the declarations for external interfaces in the static
case is the way to go?
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/20170726/973be167/attachment-0001.sig>
More information about the wayland-devel
mailing list