Re: [Qemu-devel] [PATCH v10 11/13] qapi: Don't box branches of flat unions

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> There's no reason to do two malloc's for a flat union; let's just
> inline the branch struct directly into the C union branch of the
> flat union.
>
> Surprisingly, fewer clients were actually using explicit references
> to the branch types in comparison to the number of flat unions
> thus modified.
>
> This lets us reduce the hack in qapi-types:gen_variants() added in
> the previous patch; we no longer need to distinguish between
> alternates and flat unions.  It also lets us get rid of all traces
> of 'visit_type_implicit_FOO()' in qapi-visit, and reduce one (but
> not all) special cases of simplie unions.

simple

>
> Unfortunately, simple unions are not as easy to convert; because
> we are special-casing the hidden implicit type with a single 'data'
> member, we really DO need to keep calling another layer of
> visit_start_struct(), with a second malloc.  Hence,
> gen_visit_fields_decl() has to special case implicit types (the
> type for a simple union variant).

Simple unions should be mere sugar for the equivalent flat union, as
explained in qapi-code-gen.txt.  That they aren't now on the C side is
accidental complexity.  I hope we can clean that up relatively soon.

In the long run, I'd like to replace the whole struct / flat union /
simple union mess by a variant record type.

> Note that after this patch, the only remaining use of
> visit_start_implicit_struct() is for alternate types; the next
> couple of patches will do further cleanups based on that fact.

Remind me, what makes alternates need visit_start_implicit_struct()?

> Signed-off-by: Eric Blake <address@hidden>
>
> ---
> v10: new patch
>
> If anything, we could match our simple union wire format more closely
> by teaching qapi-types to expose implicit types inline, and write:
>
> struct SU {
>     SUKind type;
>     union {
>         struct {
>           Branch1 *data;
>       } branch1;
>       struct {
>           Branch2 *data;
>       } branch2;
>     } u;
> };
>
> where we would then access su.u.branch1.data->member instead of
> the current su.u.branch1->member.

Looks like the cleanup I mentioned above.

> ---
>  scripts/qapi-types.py           | 10 +--------
>  scripts/qapi-visit.py           | 45 
> +++++++----------------------------------
>  cpus.c                          | 18 ++++++-----------
>  hmp.c                           | 12 +++++------
>  tests/test-qmp-input-visitor.c  |  2 +-
>  tests/test-qmp-output-visitor.c |  5 ++---
>  6 files changed, 23 insertions(+), 69 deletions(-)
>
> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> index aba2847..5071817 100644
> --- a/scripts/qapi-types.py
> +++ b/scripts/qapi-types.py
> @@ -116,14 +116,6 @@ static inline %(base)s *qapi_%(c_name)s_base(const 
> %(c_name)s *obj)
>
>
>  def gen_variants(variants):
> -    # HACK: Determine if this is an alternate (at least one variant
> -    # is not an object); unions have all branches as objects.
> -    inline = False
> -    for v in variants.variants:
> -        if not isinstance(v.type, QAPISchemaObjectType):
> -            inline = True
> -            break
> -
>      # FIXME: What purpose does data serve, besides preventing a union that
>      # has a branch named 'data'? We use it in qapi-visit.py to decide
>      # whether to bypass the switch statement if visiting the discriminator
> @@ -144,7 +136,7 @@ def gen_variants(variants):
       for var in variants.variants:
           # Ugly special case for simple union TODO get rid of it
           typ = var.simple_union_type() or var.type
>          ret += mcgen('''
>          %(c_type)s %(c_name)s;
>  ''',
> -                     c_type=typ.c_type(is_member=inline),
> +                     c_type=typ.c_type(is_member=not 
> var.simple_union_type()),
>                       c_name=c_name(var.name))

This is where we generate flat union members unboxed: is_member=True
suppresses the pointer suffix.  Still dislike the name is_member :)

Perhaps:

           # Ugly special case for simple union TODO get rid of it
           simple_union_type = var.simple_union_type()
           typ = simple_union_type or var.type
           ret += mcgen('''
           %(c_type)s %(c_name)s;
   ''',
                        c_type=typ.c_type(is_member=not simple_union_type),
                        c_name=c_name(var.name))

Slightly more readable, and makes it more clear that "ugly special case"
applies to is_member=... as well.

>
>      ret += mcgen('''
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index 948bde4..68354d8 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -15,10 +15,6 @@
>  from qapi import *
>  import re
>
> -# visit_type_implicit_FOO() is emitted as needed; track if it has already
> -# been output.
> -implicit_structs_seen = set()
> -
>  # visit_type_alternate_FOO() is emitted as needed; track if it has already
>  # been output.
>  alternate_structs_seen = set()
> @@ -39,40 +35,15 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, 
> %(c_type)sobj, Error **
>
>
>  def gen_visit_fields_decl(typ):
> -    ret = ''
> -    if typ.name not in struct_fields_seen:
> -        ret += mcgen('''
> +    if typ.is_implicit() or typ.name in struct_fields_seen:
> +        return ''
> +    struct_fields_seen.add(typ.name)
> +
> +    return mcgen('''
>
>  static void visit_type_%(c_type)s_fields(Visitor *v, %(c_type)s *obj, Error 
> **errp);
>  ''',
> -                     c_type=typ.c_name())
> -        struct_fields_seen.add(typ.name)
> -    return ret

Two changes squashed together.  First step is mere style:

diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 948bde4..ac7d504 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -39,15 +39,14 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, 
%(c_type)sobj, Error **
 
 
 def gen_visit_fields_decl(typ):
-    ret = ''
-    if typ.name not in struct_fields_seen:
-        ret += mcgen('''
+    if typ.name in struct_fields_seen:
+        return ''
+    struct_fields_seen.add(typ.name)
+
+    return mcgen('''
 
 static void visit_type_%(c_type)s_fields(Visitor *v, %(c_type)s *obj, Error 
**errp);
 ''',
                      c_type=typ.c_name())
-        struct_fields_seen.add(typ.name)
-    return ret
 
 
 def gen_visit_implicit_struct(typ):

The blank line before the return seems superfluous.

Second step is the actual change:

@@ -35,7 +39,7 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, 
%(c_type)sobj, Error **
 
 
 def gen_visit_fields_decl(typ):
-    if typ.name in struct_fields_seen:
+    if typ.is_implicit() or typ.name in struct_fields_seen:
         return ''
     struct_fields_seen.add(typ.name)
 
Much easier to see what's going on now.

I guess you add .is_implicit() here so that gen_visit_object() can call
it unconditionally.  It's odd; other gen_ functions don't check
.is_implicit().

> -
> -
> -def gen_visit_implicit_struct(typ):
> -    if typ in implicit_structs_seen:
> -        return ''
> -    implicit_structs_seen.add(typ)
> -
> -    ret = gen_visit_fields_decl(typ)
> -
> -    ret += mcgen('''
> -
> -static void visit_type_implicit_%(c_type)s(Visitor *v, %(c_type)s **obj, 
> Error **errp)
> -{
> -    Error *err = NULL;
> -
> -    visit_start_implicit_struct(v, (void **)obj, sizeof(%(c_type)s), &err);
> -    if (!err) {
> -        visit_type_%(c_type)s_fields(v, *obj, errp);
> -        visit_end_implicit_struct(v);
> -    }
> -    error_propagate(errp, err);
> -}
> -''',
>                   c_type=typ.c_name())
> -    return ret
>
>
>  def gen_visit_alternate_struct(typ):
> @@ -250,9 +221,7 @@ def gen_visit_object(name, base, members, variants):
>
>      if variants:
>          for var in variants.variants:
> -            # Ugly special case for simple union TODO get rid of it
> -            if not var.simple_union_type():
> -                ret += gen_visit_implicit_struct(var.type)
> +            ret += gen_visit_fields_decl(var.type)

Before: if this is a flat union member of type FOO, we're going to call
visit_type_implicit_FOO(), as you can see in the next hunk.  Ensure it's
in scope by generating it unless it's been generated already.

After: we're going to call visit_type_FOO_fields() instead.  Generate a
forward declaration unless either the function or the forward
declaration has been generated already.  Except don't generate it when
FOO is an implicit type, because then the member is simple rather than
flat.

Doesn't this unduly hide the ugly special case?

To keep it in view, I'd write

               # Ugly special case for simple union TODO get rid of it
               if not var.simple_union_type():
  -                ret += gen_visit_implicit_struct(var.type)
  +                ret += gen_visit_fields_decl(var.type)

and drop the .is_implicit() from gen_visit_fields_decl().

Would this work?

>
>      # FIXME: if *obj is NULL on entry, and visit_start_struct() assigns to
>      # *obj, but then visit_type_FOO_fields() fails, we should clean up *obj
> @@ -300,7 +269,7 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, 
> %(c_name)s **obj, Error
>                               c_name=c_name(var.name))
>              else:
>                  ret += mcgen('''
> -        visit_type_implicit_%(c_type)s(v, &(*obj)->u.%(c_name)s, &err);
> +        visit_type_%(c_type)s_fields(v, &(*obj)->u.%(c_name)s, &err);
>  ''',
>                               c_type=var.type.c_name(),
>                               c_name=c_name(var.name))

Before: we call visit_type_implicit_FOO() to visit the *boxed* member in
C and the *inlined* member in QMP.  visit_type_implicit_FOO() looks like
this:

    static void visit_type_implicit_FOO(Visitor *v, FOO **obj, Error **errp)
    {
        Error *err = NULL;

        visit_start_implicit_struct(v, (void **)obj, sizeof(FOO), &err);
        if (!err) {
            visit_type_FOO_fields(v, *obj, errp);
            visit_end_implicit_struct(v);
        }
        error_propagate(errp, err);
    }

After: we skip visit_start_implicit_struct() and
visit_end_implicit_struct(), i.e. the memory allocation stuff, and pass
&(*obj)->u.MEMBER instead of (*obj)->u.MEMBER to
visit_type_FOO_fields().  Okay.

Every time I come across "implicit" structs, I get confused, and have to
dig to unconfuse myself.  Good to get rid of one.

> diff --git a/cpus.c b/cpus.c
> index 898426c..9592163 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -1568,28 +1568,22 @@ CpuInfoList *qmp_query_cpus(Error **errp)
>          info->value->thread_id = cpu->thread_id;
>  #if defined(TARGET_I386)
>          info->value->arch = CPU_INFO_ARCH_X86;
> -        info->value->u.x86 = g_new0(CpuInfoX86, 1);
> -        info->value->u.x86->pc = env->eip + env->segs[R_CS].base;
> +        info->value->u.x86.pc = env->eip + env->segs[R_CS].base;
>  #elif defined(TARGET_PPC)
>          info->value->arch = CPU_INFO_ARCH_PPC;
> -        info->value->u.ppc = g_new0(CpuInfoPPC, 1);
> -        info->value->u.ppc->nip = env->nip;
> +        info->value->u.ppc.nip = env->nip;
>  #elif defined(TARGET_SPARC)
>          info->value->arch = CPU_INFO_ARCH_SPARC;
> -        info->value->u.q_sparc = g_new0(CpuInfoSPARC, 1);
> -        info->value->u.q_sparc->pc = env->pc;
> -        info->value->u.q_sparc->npc = env->npc;
> +        info->value->u.q_sparc.pc = env->pc;
> +        info->value->u.q_sparc.npc = env->npc;
>  #elif defined(TARGET_MIPS)
>          info->value->arch = CPU_INFO_ARCH_MIPS;
> -        info->value->u.q_mips = g_new0(CpuInfoMIPS, 1);
> -        info->value->u.q_mips->PC = env->active_tc.PC;
> +        info->value->u.q_mips.PC = env->active_tc.PC;
>  #elif defined(TARGET_TRICORE)
>          info->value->arch = CPU_INFO_ARCH_TRICORE;
> -        info->value->u.tricore = g_new0(CpuInfoTricore, 1);
> -        info->value->u.tricore->PC = env->PC;
> +        info->value->u.tricore.PC = env->PC;
>  #else
>          info->value->arch = CPU_INFO_ARCH_OTHER;
> -        info->value->u.other = g_new0(CpuInfoOther, 1);
>  #endif
>
>          /* XXX: waiting for the qapi to support GSList */
> diff --git a/hmp.c b/hmp.c
> index c6419da..1c16ed9 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -313,22 +313,22 @@ void hmp_info_cpus(Monitor *mon, const QDict *qdict)
>
>          switch (cpu->value->arch) {
>          case CPU_INFO_ARCH_X86:
> -            monitor_printf(mon, " pc=0x%016" PRIx64, cpu->value->u.x86->pc);
> +            monitor_printf(mon, " pc=0x%016" PRIx64, cpu->value->u.x86.pc);
>              break;
>          case CPU_INFO_ARCH_PPC:
> -            monitor_printf(mon, " nip=0x%016" PRIx64, 
> cpu->value->u.ppc->nip);
> +            monitor_printf(mon, " nip=0x%016" PRIx64, cpu->value->u.ppc.nip);
>              break;
>          case CPU_INFO_ARCH_SPARC:
>              monitor_printf(mon, " pc=0x%016" PRIx64,
> -                           cpu->value->u.q_sparc->pc);
> +                           cpu->value->u.q_sparc.pc);
>              monitor_printf(mon, " npc=0x%016" PRIx64,
> -                           cpu->value->u.q_sparc->npc);
> +                           cpu->value->u.q_sparc.npc);
>              break;
>          case CPU_INFO_ARCH_MIPS:
> -            monitor_printf(mon, " PC=0x%016" PRIx64, 
> cpu->value->u.q_mips->PC);
> +            monitor_printf(mon, " PC=0x%016" PRIx64, 
> cpu->value->u.q_mips.PC);
>              break;
>          case CPU_INFO_ARCH_TRICORE:
> -            monitor_printf(mon, " PC=0x%016" PRIx64, 
> cpu->value->u.tricore->PC);
> +            monitor_printf(mon, " PC=0x%016" PRIx64, 
> cpu->value->u.tricore.PC);
>              break;
>          default:
>              break;

That's not bad at all.

> diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c
> index 139ff7d..27e5ab9 100644
> --- a/tests/test-qmp-input-visitor.c
> +++ b/tests/test-qmp-input-visitor.c
> @@ -295,7 +295,7 @@ static void 
> test_visitor_in_union_flat(TestInputVisitorData *data,
>      g_assert_cmpint(tmp->enum1, ==, ENUM_ONE_VALUE1);
>      g_assert_cmpstr(tmp->string, ==, "str");
>      g_assert_cmpint(tmp->integer, ==, 41);
> -    g_assert_cmpint(tmp->u.value1->boolean, ==, true);
> +    g_assert_cmpint(tmp->u.value1.boolean, ==, true);
>
>      base = qapi_UserDefFlatUnion_base(tmp);
>      g_assert(&base->enum1 == &tmp->enum1);
> diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c
> index 965f298..c5514a1 100644
> --- a/tests/test-qmp-output-visitor.c
> +++ b/tests/test-qmp-output-visitor.c
> @@ -1,7 +1,7 @@
>  /*
>   * QMP Output Visitor unit-tests.
>   *
> - * Copyright (C) 2011, 2015 Red Hat Inc.
> + * Copyright (C) 2011-2016 Red Hat Inc.
>   *
>   * Authors:
>   *  Luiz Capitulino <address@hidden>
> @@ -403,9 +403,8 @@ static void 
> test_visitor_out_union_flat(TestOutputVisitorData *data,
>      UserDefFlatUnion *tmp = g_malloc0(sizeof(UserDefFlatUnion));
>      tmp->enum1 = ENUM_ONE_VALUE1;
>      tmp->string = g_strdup("str");
> -    tmp->u.value1 = g_malloc0(sizeof(UserDefA));
>      tmp->integer = 41;
> -    tmp->u.value1->boolean = true;
> +    tmp->u.value1.boolean = true;
>
>      visit_type_UserDefFlatUnion(data->ov, NULL, &tmp, &error_abort);
>      arg = qmp_output_get_qobject(data->qov);