Re: [Qemu-devel] [PATCH v5 03/46] qapi: Test for C member name collisions

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 09/22/2015 09:23 AM, Markus Armbruster wrote:
>> Eric Blake <address@hidden> writes:
>> 
>>> Expose some weaknesses in the generator: we don't always forbid
>>> the generation of structs that contain multiple members that map
>> 
>> Slightly misleading.  args-name-clash is a clash between command
>> arguments.  These are a struct internally, but we don't currently
>> generate an actual struct for it, only an argument list.
>
> Maybe struct-member-clash?  Renames are easy enough, but only if patch
> 1/46 is okay to go in first. :)

Naming the test args-member-clash is fine, because that's what it tests.
It also covers struct member clashes due to they way arguments work.
The slightly misleading part is the commit message.

>>> to the same C name.  This has already been marked FIXME in
>>> qapi.py, but having more tests will make sure future patches
>>> produce desired behavior.
>> 
>> Point to commit d90675f?
>
> Sure, now that it finally landed.
>
>> 
>>> Some of these tests will be deleted later, and a positive test
>>> added to qapi-schema-test.json in its place, when the code is
>> 
>> "in their place"?
>> 
>
> Yep. (Perils of editing, I started with one test, then added more later
> and merged into one patch)
>
>>> reworked so that the collision no longer occurs.
>>>
>>> Signed-off-by: Eric Blake <address@hidden>
>>> ---
>
>>> +++ b/tests/qapi-schema/flat-union-branch-clash.json
>>> @@ -1,4 +1,4 @@
>>> -# we check for no duplicate keys between branches and base
>>> +# we check for no duplicate keys between branch members and base
>>>  { 'enum': 'TestEnum',
>>>    'data': [ 'value1', 'value2' ] }
>>>  { 'struct': 'Base',
>> 
>> This clashing business is awfully confusing as soon as unions come into
>> play.  When I'm confused, I need to think in writing.
>
> No kidding.  We already attempted to detect clashes, and caught some,
> but not all, types of clashes.  And there are indeed two types of member
> name clashes: those where the generated C struct has duplicate members
> (either because 2 user names map to the same C name, or because the
> generated code injects a C name for a purpose other than a "key":value
> name), and those where the qapi type would specify the same "key":value
> name more than once in the same {} object on the wire (even if the names
> would not collide in C because one is accessed through a box pointer).
> By patch 16/46, we should be catching all cases of member name clashes,
> but there's still work to do to catch collisions in 'command' and/or
> 'event' names.
>
> Also, by the time 16/46 is in, there are cases where we reject "clashes"
> where two member names with different spellings would map to the same C
> name, but where the corresponding C struct does not have a clash because
> the members are boxed behind different pointers.  Technically, we would
> not have to reject such cases, but the case is still confusing enough
> that rejecting it forces the qapi writer to consider a naming convention
> that is less confusing in the first place.

That's fair.

>> The basic case is clash between local, non-variant members.  Needs test
>> coverage.  args-name-clash.json provides it, because internally the
>> arguments are just another object type.
>
> Correct.  The test proves we don't yet catch the clash, and is fixed
> when later commits add the check.
>
>> 
>> With a base, the members inherited from base get added to the mix.  We
>> need to test a clash betwen local, non-variant member and a member
>> inherited from base.
>
> True for both structs and flat unions (the two places where we use
> 'base').  More on this below.
>
>> 
>> With unions, things get complicated, because we have multiple kinds of
>> clashes.  Best explained with an example.  Let's use UserDefFlatUnion
>> from qapi-schema-test.json.
>> 
>>     { 'union': 'UserDefFlatUnion',
>>       'base': 'UserDefUnionBase',   # intentional forward reference
>>       'discriminator': 'enum1',
>>       'data': { 'value1' : 'UserDefA',
>>                 'value2' : 'UserDefB',
>>                 'value3' : 'UserDefB' } }
>> 
>>     { 'struct': 'UserDefUnionBase',
>>       'base': 'UserDefZero',
>>       'data': { 'string': 'str', 'enum1': 'EnumOne' } }
>> 
>> Generated C looks like this:
>> 
>>     struct UserDefFlatUnion {
>>         /* Members inherited from UserDefUnionBase: */
>>         int64_t integer;
>>         char *string;
>>         EnumOne enum1;
>>         /* Own members: */
>>         // if the schema language supported adding non-variant local
>>         // members, they'd go right here
>>         union { /* union tag is @enum1 */
>>             void *data;
>>             UserDefA *value1;
>>             UserDefB *value2;
>>             UserDefB *value3;
>>         };
>>     };
>> 
>> Thus, what can clash in C is the tag values value1, value2, value3 with
>> the non-variant members integer, string, enum1.
>
> That is, the tag values now appear as C member names, even though they
> did not correspond to QMP "key":value names.  Likewise, the 'data' C
> member name can cause a clash.
>
> Was even worse before commit 0f61af3e, where we were also burning the C
> name 'kind'.
>
> Commit 1e6c1616 was where we quit burning the C member name 'base'.
> Prior to that time, members of base classes did not clash with variant
> names because of the C boxing.

For union types.  For struct types, we still box the base.  I'd like to
get rid of that.

Even when the base is boxed, the members still clash in QMP.

We also box the variants (e.g. UserDefA *value1 in the example above).
Would be nice to get rid of that, too.

> If we run into a situation where the enum values collide with base
> member names (both of which are ABI), we could still solve the collision
> by renaming the C member names for the enum values to something that
> don't collide (such as _tag_value1 rather than value1); this is because
> the C member names are not ABI and can be changed.  But we can cross
> that bridge later if the situation ever arises; for now, it's just
> easier to patch the generator to reject qapi where such a collision
> would occur.

Agreed.

>> On the wire, the union members are unboxed, i.e. we get just
>> 
>>     "boolean": false
>> 
>> instead of
>> 
>>     "value1": { "boolean": false }
>> 
>> Thus what can clash on the wire is the variant members with the
>> non-variant members: boolean with integer, string, enum1 when enum1 is
>> value1, and so forth.
>> 
>> This is the clash flat-union-branch-clash.json tests.  Its error message
>> is "Member name 'name' of branch 'value1' clashes with base 'Base'".
>> Suboptimal, it should say "with member 'name' of base 'Base'".
>
> Indeed, this is the other type of clash (QMP wire clashes, whether or
> not they cause a C member clash).
>
>>> +++ b/tests/qapi-schema/flat-union-branch-clash2.json
>>> @@ -0,0 +1,14 @@
>>> +# FIXME: we should check for no duplicate C names between branches and base
>>> +{ 'enum': 'TestEnum',
>>> +  'data': [ 'base', 'c-d' ] }
>>> +{ 'struct': 'Base',
>>> +  'data': { 'enum1': 'TestEnum', '*c_d': 'str' } }
>>> +{ 'struct': 'Branch1',
>>> +  'data': { 'string': 'str' } }
>>> +{ 'struct': 'Branch2',
>>> +  'data': { 'value': 'int' } }
>>> +{ 'union': 'TestUnion',
>>> +  'base': 'Base',
>>> +  'discriminator': 'enum1',
>>> +  'data': { 'base': 'Branch1',
>>> +            'c-d': 'Branch2' } }
>> 
>> This tests the other kind of clash: tag value 'c-d' clashes with
>> non-variant member name 'c_d'.
>> 
>> Please add a comment explaining what clash should be reported here.
>
> Will do; and by the end of the series the error is properly reported.
>
>>> +++ b/tests/qapi-schema/flat-union-cycle.json
>>> @@ -0,0 +1,6 @@
>>> +# we reject a loop in flat unions, due to member collision
>>> +{ 'enum': 'Enum', 'data': [ 'okay', 'loop' ] }
>>> +{ 'struct': 'Base', 'data': { 'switch': 'Enum' } }
>>> +{ 'struct': 'Okay', 'data': { 'int': 'int' } }
>>> +{ 'union': 'Union', 'base': 'Base', 'discriminator': 'switch',
>>> +  'data': { 'okay': 'Okay', 'loop': 'Base' } }
>> 
>> This isn't a loop, it's a fork: we get the members of Base via its use
>> as base, and again via its use as type of a variant case.
>> 
>> What does it add over flat-union-branch-clash.json?
>
> I wrote this test when I discovered the assertion failure in the parser
> bug as covered by patch 16/46 (a struct attempting to inherit directly
> or indirectly from itself is not nice).

Yes, that needs to be caught.

>                                         When I first wrote the test, I
> was trying to make sure that a flat union cannot inherit from itself,
> but then ran into the problem that a base class must be a struct and not
> a union.

Yes.  That means base loops can only consist of struct types right now.

>           So I changed the test to make sure that QMP cannot reuse the
> base class as a variant type, since that would require the members of
> the base type to occur in QMP more than once, without seeing if any
> other test already did that.
>
> You may have a point that this doesn't cover anything beyond
> flat-union-branch-clash, and since my later changes to detect
> self-inheritance didn't change the error message flagged for this case,
> we can probably safely drop this test as not adding anything.
>
> And I guess I should still test that self-inheritance attempts are
> rejected, even if we later relax things to allow a non-struct as a base
> class.

Replacing this test by one that actually tests base loops sounds good to
me.  If we ever accept other kinds of base types, we'll have to update
the loop test, but that's normal.

>>> +++ b/tests/qapi-schema/qapi-schema-test.json
>>> @@ -32,11 +32,12 @@
>>>              'dict1': 'UserDefTwoDict' } }
>>>
>>>  # for testing unions
>>> +# name collisions between branches should not clash
>>>  { 'struct': 'UserDefA',
>>> -  'data': { 'boolean': 'bool' } }
>>> +  'data': { 'boolean': 'bool', '*a_b': 'int' } }
>>>
>>>  { 'struct': 'UserDefB',
>>> -  'data': { 'intb': 'int' } }
>>> +  'data': { 'intb': 'int', '*a-b': 'bool' } }
>>>
>>>  { 'union': 'UserDefFlatUnion',
>>>    'base': 'UserDefUnionBase',   # intentional forward reference
>> 
>> This tests that different variants may have clashing names.  Okay.
>
> That is, even though the variant is accepted at the same QMP {} level,
> only one variant at a time can be active, so clashes in names between
> variants is not fatal to either QMP or to the generated C code.

Yes.

>> I'm afraid the comment is a bit too terse.  Not sure I'd make the
>> connection from it to member a_b and to UserDefB's a-b a fortnight from
>> now.
>> 
>
> Then I get to beef it up for the next round :)
>
>
>>> +++ b/tests/qapi-schema/struct-base-clash2.json
>>> @@ -0,0 +1,5 @@
>>> +# FIXME - a base class collides with a member named base
>>> +{ 'struct': 'Base', 'data': {} }
>>> +{ 'struct': 'Sub',
>>> +  'base': 'Base',
>>> +  'data': { 'base': 'str' } }
>> 
>> What's this about?  Hmm, I think it's about the way we do a struct
>> type's base.  For a union type, we add the base's members, as shown
>> above.  For a struct type, we add the base *boxed*, like this:
>> 
>>     struct Sub {
>>         // The base type
>>         Base *base;
>>         // Own members
>>         char *base;
>>     };
>> 
>> Therefore, a struct type with a base can't have a member named base.
>> But that's simply daft.  As soon as we change it to match union types,
>> this test case goes away.  If we change it soon, do we still need this
>> test?  Will it be done later in this series?
>
> Yes, we fix it up later in the series, at which point this test
> disappears. But having the test now makes it easier to see what the
> later patch is changing.

I'm not sure I'd bother myself, but I gladly defer to your judgement
here.

>>> +++ b/tests/qapi-schema/union-clash2.json
>>> @@ -0,0 +1,3 @@
>>> +# FIXME - a union branch named 'data' collides with generated C code
>>> +{ 'union': 'TestUnion',
>>> +  'data': { 'data': 'int' } }
>> 
>> This tests another stupid clash: we put a member named data in our
>> generated unions.  As soon as we stop doing that, this test will go
>> away.  If we stop soon, do we still need this test?  Will we stop later
>> in this series?
>
> Yes, we fix it up later in the series, at which point this test
> disappears. But having the test now makes it easier to see what the
> later patch is changing.

Likewise.