Re: [Qemu-devel] [PATCH v9 05/17] qapi: Unbox base members

[Eric Blake]

On 10/20/2015 06:09 AM, Markus Armbruster wrote:
> Eric Blake <address@hidden> writes:
> 
>> Rather than storing a base class as a pointer to a box, just
>> store the fields of that base class in the same order, so that
>> a child struct can be safely cast to its parent.

^^^^

>> Compare to the earlier commit 1e6c1616a "qapi: Generate a nicer
>> struct for flat unions".
> 
> Should we mention the struct can be cast to its base?

As in the ^^^ sentence above? Or did you mean somewhere additionally in
the comments in the code below?

>> -def gen_struct_fields(members):
>> +def gen_struct_fields(members, base, nested=False):
>>      ret = ''
>> +    if base:
>> +        if not nested:
>> +            ret += mcgen('''
>> +    /* Members inherited from %(c_name)s: */
>> +''',
>> +                         c_name=base.c_name())
>> +        ret += gen_struct_fields(base.local_members, base.base, True)
>> +        if not nested:
>> +            ret += mcgen('''
>> +    /* Own members: */
>> +''')
> 
> Before: gen_struct_fields() emits *own* fields.
> 
> After: it emits *all* fields.
> 
> Since the signature changes, all callers are visible in the patch, and
> can be reviewed.
> 
> Code looks a bit awkward, but I don't have better ideas.  Perhaps we can
> do better when we fuse gen_struct() and gen_union().

I haven't tried to do that fusion yet; but we are probably quite close
to it.  I'll see whether it is worth adding on as an 18/17 in this
subset of the series, or saving for later.

> This is gen_union().
> 
>>  ''',
>>                  c_name=c_name(name))
>>      if base:
>> -        ret += mcgen('''
>> -    /* Members inherited from %(c_name)s: */
>> -''',
>> -                     c_name=c_name(base.name))
>> -        ret += gen_struct_fields(base.members)
>> -        ret += mcgen('''
>> -    /* Own members: */
>> -''')
>> +        ret += gen_struct_fields([], base)
>>      else:
>>          ret += mcgen('''
>>      %(c_type)s kind;
> 
> Before: emit base fields, then own fields.
> 
> After: emit all fields.  Good, but please mention in the commit message
> that you're touching gen_union().  You could also do this part in a
> separate patch, but that's probably overkill.

Sure, I can improve the commit message, and maybe split the patch. The
idea at play is that both structs and unions want to emit all fields, as
well as strategic comments about which fields are inherited, so a shared
function is ideal for that; this patch moved code from gen_union() into
gen_struct_fields() so that gen_struct() could reuse it.

>>      if base:
>> -        ret += gen_visit_implicit_struct(base)
>> +        ret += gen_visit_struct_fields(base.name, base.base,
>> +                                       base.local_members)
>>
>>      ret += mcgen('''
>>
> 
> This change looks innocent enough on first glance, but it's actually
> quite hairy.
> 
> = Before =
> 
> The visit_type_FOO_fields() are generated in QAPISchema visit order,
> i.e. right when QAPISchemaObjectType 'FOO' is visited.
> 
> The visit_type_implicit_FOO() are generated on demand, right before
> their first use.  It's used by visit_type_STRUCT_fields() when STRUCT
> has base FOO, and by visit_type_UNION() when flat UNION has a member
> FOO.
> 
> Aside: only flat unions, because simple unions are an ugly special case
> here.  To be unified.
> 
> Generating visit_type_implicit_FOO() on demand makes sense, because the
> function isn't always needed.
> 
> Since visit_type_implicit_FOO() calls visit_type_FOO_fields(), and the
> former can be generated before the latter, we may need a forward
> declaration.  struct_fields_seen is used to detect this.  See commit
> 8c3f8e7.
> 
> = After =
> 
> visit_type_implicit_FOO() is now used only when flat UNION has a member
> FOO.  May need a forward declaration of visit_type_FOO_fields() anyway.
> 
> visit_type_FOO_fields() is now also generated on demand, right before
> first use other than visit_type_implicit_FOO().  Weird.
> 
> The resulting code motion makes the change to generated code difficult
> to review.
> 
> Generating visit_type_FOO_fields() on demand makes less sense, because
> the function is always needed.  All it can accomplish is saving a few
> forward declarations, at the cost of making gen_visit_struct_fields()
> recursive, which begs the recursion termination question, and less
> uniform generated code.
> 
> The naive answer to the recursion termination question is that types
> can't contain themselves, therefore we can't ever get into a cycle.
> That begs the next question: why do we need the if name in
> struct_fields_seen conditional then?  We do need it (turning it into an
> assertion promptly fails it), but I can't tell why offhand.
> 
> I'm sure I could figure out why this works, but I don't want to.  Let's
> keep the code simple instead: keep generating visit_type_FOO_fields() in
> QAPISchema visit order, emit necessary forward declarations.
> 
> Suggest to factor the code to emit a forward declaration out of
> gen_visit_implicit_struct() and reuse it in gen_visit_struct_fields().

Sounds like I need to split this patch then, anyways.  I'll see what I
can come up with for v10.


>> +++ b/tests/qapi-schema/qapi-schema-test.json
>> @@ -40,6 +40,10 @@
>>    'data': { 'string0': 'str',
>>              'dict1': 'UserDefTwoDict' } }
>>
>> +# ensure that we don't have an artificial collision on 'base'
>> +{ 'struct': 'UserDefThree',
>> +  'base': 'UserDefOne', 'data': { 'base': 'str' } }
>> +

> This is the positive test I challenged above.

I can drop it if you don't like it; I felt better with it, but as you
say, it only proves that 'base' is usable as a member name, and not that
someone won't pick some other name when refactoring back into a boxed
base.  See also my comments below about using containers (rather than
boxes or inlined members), where we may need to deal with the naming
clash anyways.

>> @@ -218,9 +216,11 @@ static void channel_event(int event, 
>> SpiceChannelEventInfo *info)
>>      }
>>
>>      if (info->flags & SPICE_CHANNEL_EVENT_FLAG_ADDR_EXT) {
>> -        add_addr_info(client->base, (struct sockaddr *)&info->paddr_ext,
>> +        add_addr_info((SpiceBasicInfo *)client,
> 
> Ah, you're already exploiting the ability to cast to the base type!

Absolutely :)

> 
> Idea: should we generate a type-safe macro or inline function for this?

Hmm. DO_UPCAST() (and its more powerful underlying container_of())
doesn't fit here, because we inlined the fields rather than directly
including the base.

Below, I'm using this qapi:
{'struct':'Parent', 'data':{'i':'int'}}
{'struct':'Child', 'base':'Parent', 'data':{'b':'bool'}}

What we have without this patch is a box that requires separate
allocation (two layers of pointers, the boxing that we don't want):

struct Child {
    Parent *base;
    bool b;
};

And this patch takes things to be completely inlined (since that's what
we did earlier with flat unions), so that there is no intermediate
structure in the child (and thus nothing for DO_UPCAST() to grab):

struct Child {
    /* Inherited fields from Parent */
    int i;
    /* own fields */
    bool b;
};

But there is a third possible layout (if we do it, we should do it for
flat unions as well), which is using the base as a container rather than
a box, where at least we don't need separate allocation:

struct Child {
    Parent base;
    bool b;
};

or similarly, but in longhand:

struct Child {
    struct {
        int i;
    } base;
    bool b;
};

but then we are back to having to avoid a collision with the C name
'base' with a QMP name in own fields, and all client code has to spell
out child->base.i (instead of the boxed child->base->i, or the inlined
child->i).

There's also the ugly approach of exposing things in a dual naming
system via an anonymous union and struct:

struct Child {
    union {
        struct {
            int i;
        };
        Parent base;
    };
    bool b;
};

which would allow 'child->i' to be the same storage as 'child->base.i',
so that clients can use the short spelling when accessing fields but
also have handy access to the base member for DO_UPCAST().  I'm not sure
I want to go there, though.


Taking your idea from another review message, you mentioned that we
could allow 'base' by tweaking c_name() to munge the user's member
'base' into 'q_base', while reserving 'base' for ourselves; or we could
use '_base' for our own use, which shouldn't collide with user's names
(user names should not start with underscore except for downstream names
which have double-underscore).  Or use '_b' instead of 'base' - the
point remains that if we want to avoid a collision but still use the
container approach, we could pick the C name appropriately.  But
ultimately, we should either commit to the name munging pattern, or
outlaw the name that we plan to use internally, or stick to inlined
members that don't require munging in the first place.

Meanwhile, if we decide that we like the convenience of inlined data,
you are correct in the idea that we could have the qapi generator
automatically spit out an appropriate type-specific upcast inline
function for each type with a base.  So given the same example, this
might mean:

static inline Parent *qapi_Child_to_Parent(Child *child) {
    return (Parent*)child;
}

But while such a representation would add compiler type-safety (hiding
the cast in generated code, where we can prove the generator knew it was
safe, and so that clients don't have to cast), it also adds verbosity.
I can't think of any representation that would be shorter than making
the clients do the cast, short of using a container rather than inline
approach.  Even foo(qapi_baseof_Child(child), blah) is longer than
foo((Parent *)child, blah).

Preferences?

> Turned out nicely, I anticipated more churn.

Yes, that was my feeling as well - we haven't quite used base classes in
enough places to make the conversion painful - but the longer we wait,
the more usage of base classes will sneak into .json files making a
future conversion to a new layout more painful.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature