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

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 02/17/2016 10:44 AM, Markus Armbruster wrote:
>> 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.
>
> We're getting closer :)
>
>> 
>>> 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()?
>
> Here's what the two functions do:
>
> visit_start_struct(): optionally allocate, and consume {}
> visit_start_implicit_struct(): allocate, but do not consume {}
>
> When visiting an alternate, we need to allocate the C struct that
> contains the various alternate branches (visit_start_implicit_struct(),
> unchanged by this patch, but renamed visit_start_alternate in 13/13),
> then within that struct, if the next token is '{' we need to visit the C
> struct for the object branch of the alternate (pre-series, that was
> boxed, so we used visit_type_FOO(&obj) which calls visit_start_struct()
> for a full allocation and consumption of {}; but with the previous
> patch, it is now already allocated, so we now use visit_type_FOO(NULL)
> to skip the allocation while still consuming the {}).
>
> I can try to work something along the lines of that text into my commit
> messages for v11.

I've since made it to PATCH 13, and found the fused
visit_start_alternate().  Much easier to comprehend than the
visit_start_implicit_struct() + visit_get_next_type() combo.

>>> 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.
>
> Yay, I'm glad you like it! I've already written the patch for it, but it
> was big enough (and needs several other prerequisite cleanups in the
> codebase to use C99 initializers for things like SocketAddress to make
> the switch easier to review) that I haven't posted it yet.  And yes, it
> completely gets rid of the simple_union_type() hack.

Looking forward to its demise.

>>> @@ -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.
>
> It gets renamed to is_unboxed after the review on 10/13.  But even after
> my patch to convert simple unions, this code will still be
> c_type=typ.c_type(is_unboxed=True), unless I figure out a way to rework
> .c_type() to not need two separate boolean flags for the three different
> contexts in which we use a type name (declaring an unboxed member to a
> struct, declaring a local variable, and declaring a const parameter).

A possible alternative to a single c_type() with flags for context would
be separate c_CONTEXT_type().

In QAPISchemaType:

    def c_type(self):
        pass

    def c_param_type(self):
        return self.c_type()

QAPISchemaBuiltinType overrides:

    def c_type(self):
        return self._c_type_name

    def c_param_type(self):
        if self.name == 'str':
            return 'const ' + self._c_type_name
        return self._c_type_name

QAPISchemaEnumType:

    def c_type(self):
        return c_name(self.name)

QAPISchemaArrayType:

    def c_type(self):
        return c_name(self.name) + pointer_suffix

QAPISchemaObjectType:

    def c_type(self):
        assert not self.is_implicit()
        return c_name(self.name) + pointer_suffix

    def c_unboxed_type(self):
        return c_name(self.name)

QAPISchemaAlternateType:

    def c_type(self):
        return c_name(self.name) + pointer_suffix

Lots of trivial code, as so often with OO.

>>>  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:
>
> Then I'll split into two patches for v11.
>
>> 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().
>
> Although I have more plans to use .is_implicit() - I have patches in my
> local tree that allow:
>
> { 'enum': 'Enum', 'data': [ 'branch' ] }
> { 'union': 'U', 'base': { 'anonymous1': 'Enum' },
>   'discriminator': 'anonymous1',
>   'data': { 'branch': { 'anonymous2': 'str' } } }
>
> that is, both an anonymous base, and an anonymous branch.  It results in
> more places where we'll need to suppress declarations if the type is
> implicit; so doing it here instead of in each caller proves easier in
> the long run.
>
> But for this patch, I can probably go along with your idea of keeping
> the complexity in the caller, where we highlight that simple unions are
> still special cases for a bit longer...

Yes, please.

>>> @@ -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?
>
> ...It should; I'm testing it now.
>
>> 
>> Every time I come across "implicit" structs, I get confused, and have to
>> dig to unconfuse myself.  Good to get rid of one.
>
> Yep - and it makes my stalled work on documenting visitor.h easier with
> fewer ugly things to document :)

I welcome a smaller visitor.h; reviewing the first iteration of the
documentation patch was tough going.

>>>          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.
>
> I was actually pleasantly shocked at how few places in code needed
> changing.  The conversion of simple unions sitting in my local tree was
> more complex (much of that because we use SocketAddress in a LOT more
> places).