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

[Eric Blake]

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. :)

> 
>> 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.

> 
> 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.

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.

> 
> 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). 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.  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.


>> +++ 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.

> 
> 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.

>> +++ 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.

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

signature.asc
Description: OpenPGP digital signature