Re: [Qemu-devel] [PATCH v11 13/15] 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.
>
> The change to unboxed structs means that u.data (added in commit
> cee2dedb) is now coincident with random fields of each branch of
> the flat union, whereas beforehand it was only coincident with
> pointers (since all branches of a flat union have to be objects).
> Note that this was already the case for simple unions - but there
> we got lucky.  Remember, visit_start_union() blindly returns true
> for all visitors except for the dealloc visitor, where it returns
> the value !!obj->u.data, and that this result then controls
> whether to proceed with the visit to the variant.  Pre-patch,
> this meant that flat unions were testing whether the boxed pointer
> was still NULL, and thereby skipping visit_end_implicit_struct()
> and avoiding a NULL dereference if the pointer had not been
> allocated.  The same was true for simple unions where the current
> branch had pointer type, except there we bypassed visit_type_FOO().
> But for simple unions where the current branch had scalar type, the
> contents of that scalar meant that the decision to call
> visit_type_FOO() was data-dependent - the reason we got lucky there
> is that visit_type_FOO() for all scalar types in the dealloc visitor
> is a no-op (only the pointer variants had anything to free), so it
> did not matter whether the dealloc visit was skipped.  But with this
> patch, we would risk leaking memory if we could skip a call to
> visit_type_FOO_fields() based solely on a data-dependent decision.
>
> But notice: in the dealloc visitor, visit_type_FOO() already handles
> a NULL obj - it was only the visit_type_implicit_FOO() that was
> failing to check for NULL. And now that we have refactored things to
> have the branch be part of the parent struct, we no longer have a
> separate pointer that can be NULL in the first place.  So we can just
> delete the call to visit_start_union() altogether, and blindly visit
> the branch type; there is no change in behavior except to the dealloc
> visitor, where we now unconditionally visit the branch, but where that
> visit is now always safe (for a flat union, we can no longer
> dereference NULL, and for a simple union, visit_type_FOO() was already
> safely handling NULL on pointer types).
>
> Unfortunately, simple unions are not as easy to switch to unboxed
> layout; 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; although there
> are some cleanups planned for simple unions in later patches.
>
> Note that after this patch, visit_start_union() is unused, and 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.
>
> Signed-off-by: Eric Blake <address@hidden>

Bonus: qapi-visit.c shrinks a bit, shedding all the
visit_type_implicit_FOO().

> ---
> v11: rebase to earlier changes, save cleanups for later, fix bug
> with visit_start_union now being actively wrong
> v10: new patch
> ---
>  scripts/qapi-types.py           | 13 +++----------
>  scripts/qapi-visit.py           |  7 ++-----
>  cpus.c                          | 18 ++++++------------
>  hmp.c                           | 12 ++++++------
>  tests/test-qmp-input-visitor.c  | 10 +++++-----
>  tests/test-qmp-output-visitor.c |  6 ++----
>  6 files changed, 24 insertions(+), 42 deletions(-)
>
> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> index 4dabe91..eac90d2 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.
> -    unboxed = False
> -    for v in variants.variants:
> -        if not isinstance(v.type, QAPISchemaObjectType):
> -            unboxed = 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
> @@ -140,11 +132,12 @@ 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
> +        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_unboxed=unboxed),
> +                     c_type=typ.c_type(is_unboxed=not simple_union_type),
>                       c_name=c_name(var.name))
>
>      ret += mcgen('''
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index 61b7e72..367c459 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -79,7 +79,7 @@ def gen_visit_struct_fields(name, base, members, 
> variants=None):
>          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)
>
>      struct_fields_seen.add(name)
>      ret += mcgen('''
> @@ -102,9 +102,6 @@ static void visit_type_%(c_name)s_fields(Visitor *v, 
> %(c_name)s *obj, Error **er
>
>      if variants:
>          ret += mcgen('''
> -    if (!visit_start_union(v, !!obj->u.data, &err) || err) {
> -        goto out;
> -    }
>      switch (obj->%(c_name)s) {
>  ''',
>                       c_name=c_name(variants.tag_member.name))
> @@ -126,7 +123,7 @@ static void visit_type_%(c_name)s_fields(Visitor *v, 
> %(c_name)s *obj, Error **er
>                               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))

The change has become quite simple.  I take that as a good sign.

[...]