Eric Blake <ebl...@redhat.com> 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 <ebl...@redhat.com>
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.
[...]
