diff mbox

[RFC,v2,08/47] qapi-visit: Fix generated code when schema has forward refs

Message ID 1435782155-31412-9-git-send-email-armbru@redhat.com
State New
Headers show

Commit Message

Markus Armbruster July 1, 2015, 8:21 p.m. UTC 
The visit_type_implicit_FOO() are generated on demand, right before
their first use.  Used by visit_type_STRUCT_fields() when STRUCT has
base FOO, and by visit_type_UNION() when flat UNION has member a FOO.

If the schema defines FOO after its first use as struct base or flat
union member, visit_type_implicit_FOO() calls
visit_type_implicit_FOO() before its definition, which doesn't
compile.

Rearrange qapi-schema-test.json to demonstrate the bug.

Fix by generating the necessary forward declaration.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/qapi-visit.py                   | 15 ++++++++++++++-
 tests/qapi-schema/qapi-schema-test.json | 30 +++++++++++++++++-------------
 tests/qapi-schema/qapi-schema-test.out  | 10 +++++-----
 3 files changed, 36 insertions(+), 19 deletions(-)

Comments

Eric Blake July 20, 2015, 11:19 p.m. UTC | #1 
On 07/01/2015 02:21 PM, Markus Armbruster wrote:
> The visit_type_implicit_FOO() are generated on demand, right before
> their first use.  Used by visit_type_STRUCT_fields() when STRUCT has
> base FOO, and by visit_type_UNION() when flat UNION has member a FOO.
> 
> If the schema defines FOO after its first use as struct base or flat
> union member, visit_type_implicit_FOO() calls
> visit_type_implicit_FOO() before its definition, which doesn't
> compile.

None of our public qapi .json files currently do this, so no difference
to the generated code used in qemu proper.

> 
> Rearrange qapi-schema-test.json to demonstrate the bug.

Indeed, without testsuite exposure, nothing was relying on this fix.

> 
> Fix by generating the necessary forward declaration.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  scripts/qapi-visit.py                   | 15 ++++++++++++++-
>  tests/qapi-schema/qapi-schema-test.json | 30 +++++++++++++++++-------------
>  tests/qapi-schema/qapi-schema-test.out  | 10 +++++-----
>  3 files changed, 36 insertions(+), 19 deletions(-)
> 

> +++ b/scripts/qapi-visit.py
> @@ -17,13 +17,23 @@ from qapi import *
>  import re
>  
>  implicit_structs = []
> +struct_fields_seen = set()
>  
>  def generate_visit_implicit_struct(type):
>      global implicit_structs
>      if type in implicit_structs:
>          return ''
>      implicit_structs.append(type)
> -    return mcgen('''
...
> +    ret += mcgen('''

Oddly enough, the ''' is at the same indentation,...

>  
>  static void visit_type_implicit_%(c_type)s(Visitor *m, %(c_type)s **obj, Error **errp)
>  {
> @@ -38,8 +48,11 @@ static void visit_type_implicit_%(c_type)s(Visitor *m, %(c_type)s **obj, Error *
>  }
>  ''',
>                   c_type=type_name(type))
> +    return ret

so nothing needed to be reindented here :)

Reviewed-by: Eric Blake <eblake@redhat.com>
Markus Armbruster July 27, 2015, 7:31 a.m. UTC | #2 
Eric Blake <eblake@redhat.com> writes:

> On 07/01/2015 02:21 PM, Markus Armbruster wrote:
>> The visit_type_implicit_FOO() are generated on demand, right before
>> their first use.  Used by visit_type_STRUCT_fields() when STRUCT has
>> base FOO, and by visit_type_UNION() when flat UNION has member a FOO.
>> 
>> If the schema defines FOO after its first use as struct base or flat
>> union member, visit_type_implicit_FOO() calls
>> visit_type_implicit_FOO() before its definition, which doesn't
>> compile.
>
> None of our public qapi .json files currently do this, so no difference
> to the generated code used in qemu proper.

Yes, we've intentionally avoided forward references.

Nevertheless, the generators either have to catch and reject them, or
make them work.  Making them work is easy, so I did that.

>> Rearrange qapi-schema-test.json to demonstrate the bug.
>
> Indeed, without testsuite exposure, nothing was relying on this fix.
>
>> 
>> Fix by generating the necessary forward declaration.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  scripts/qapi-visit.py                   | 15 ++++++++++++++-
>>  tests/qapi-schema/qapi-schema-test.json | 30 +++++++++++++++++-------------
>>  tests/qapi-schema/qapi-schema-test.out  | 10 +++++-----
>>  3 files changed, 36 insertions(+), 19 deletions(-)
>> 
>
>> +++ b/scripts/qapi-visit.py
>> @@ -17,13 +17,23 @@ from qapi import *
>>  import re
>>  
>>  implicit_structs = []
>> +struct_fields_seen = set()
>>  
>>  def generate_visit_implicit_struct(type):
>>      global implicit_structs
>>      if type in implicit_structs:
>>          return ''
>>      implicit_structs.append(type)
>> -    return mcgen('''
> ...
>> +    ret += mcgen('''
>
> Oddly enough, the ''' is at the same indentation,...
>
>>  
>>  static void visit_type_implicit_%(c_type)s(Visitor *m, %(c_type)s **obj, Error **errp)
>>  {
>> @@ -38,8 +48,11 @@ static void visit_type_implicit_%(c_type)s(Visitor *m, %(c_type)s **obj, Error *
>>  }
>>  ''',
>>                   c_type=type_name(type))
>> +    return ret
>
> so nothing needed to be reindented here :)
>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!
diff mbox

Patch

diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 4ec79a6..b3a308f 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -17,13 +17,23 @@  from qapi import *
 import re
 
 implicit_structs = []
+struct_fields_seen = set()
 
 def generate_visit_implicit_struct(type):
     global implicit_structs
     if type in implicit_structs:
         return ''
     implicit_structs.append(type)
-    return mcgen('''
+    ret = ''
+    if type not in struct_fields_seen:
+        # Need a forward declaration
+        ret += mcgen('''
+
+static void visit_type_%(c_type)s_fields(Visitor *m, %(c_type)s **obj, Error **errp);
+''',
+                     c_type=type_name(type))
+
+    ret += mcgen('''
 
 static void visit_type_implicit_%(c_type)s(Visitor *m, %(c_type)s **obj, Error **errp)
 {
@@ -38,8 +48,11 @@  static void visit_type_implicit_%(c_type)s(Visitor *m, %(c_type)s **obj, Error *
 }
 ''',
                  c_type=type_name(type))
+    return ret
 
 def generate_visit_struct_fields(name, members, base = None):
+    struct_fields_seen.add(name)
+
     ret = ''
 
     if base:
diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index c7eaa86..ccadb13 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -7,13 +7,13 @@ 
   'data': { 'enum1': 'EnumOne', '*enum2': 'EnumOne', 'enum3': 'EnumOne', '*enum4': 'EnumOne' } }
 
 # for testing nested structs
-{ 'struct': 'UserDefZero',
-  'data': { 'integer': 'int' } }
-
 { 'struct': 'UserDefOne',
-  'base': 'UserDefZero',
+  'base': 'UserDefZero',        # intentional forward reference
   'data': { 'string': 'str', '*enum1': 'EnumOne' } }
 
+{ 'struct': 'UserDefZero',
+  'data': { 'integer': 'int' } }
+
 { 'struct': 'UserDefTwoDictDict',
   'data': { 'userdef': 'UserDefOne', 'string': 'str' } }
 
@@ -33,29 +33,33 @@ 
 { 'struct': 'UserDefB',
   'data': { 'integer': 'int' } }
 
-{ 'struct': 'UserDefC',
-  'data': { 'string1': 'str', 'string2': 'str' } }
-
-{ 'struct': 'UserDefUnionBase',
-  'data': { 'string': 'str', 'enum1': 'EnumOne' } }
-
 { 'union': 'UserDefFlatUnion',
-  'base': 'UserDefUnionBase',
+  'base': 'UserDefUnionBase',   # intentional forward reference
   'discriminator': 'enum1',
-  'data': { 'value1' : 'UserDefA', 'value2' : 'UserDefB', 'value3' : 'UserDefB' } }
+  'data': { 'value1' : 'UserDefA',
+            'value2' : 'UserDefB',
+            'value3' : 'UserDefB' } }
 # FIXME generated struct UserDefFlatUnion has members for direct base
 # UserDefOne, but lacks members for indirect base UserDefZero
 
+{ 'struct': 'UserDefUnionBase',
+  'data': { 'string': 'str', 'enum1': 'EnumOne' } }
+
 # this variant of UserDefFlatUnion defaults to a union that uses fields with
 # allocated types to test corner cases in the cleanup/dealloc visitor
 { 'union': 'UserDefFlatUnion2',
   'base': 'UserDefUnionBase',
   'discriminator': 'enum1',
-  'data': { 'value1' : 'UserDefC', 'value2' : 'UserDefB', 'value3' : 'UserDefA' } }
+  'data': { 'value1' : 'UserDefC', # intentional forward reference
+            'value2' : 'UserDefB',
+            'value3' : 'UserDefA' } }
 
 { 'alternate': 'UserDefAlternate',
   'data': { 'uda': 'UserDefA', 's': 'str', 'i': 'int' } }
 
+{ 'struct': 'UserDefC',
+  'data': { 'string1': 'str', 'string2': 'str' } }
+
 # for testing native lists
 { 'union': 'UserDefNativeListUnion',
   'data': { 'integer': ['int'],
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index cf0ccc4..a4291db 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -1,17 +1,17 @@ 
 [OrderedDict([('enum', 'EnumOne'), ('data', ['value1', 'value2', 'value3'])]),
  OrderedDict([('struct', 'NestedEnumsOne'), ('data', OrderedDict([('enum1', 'EnumOne'), ('*enum2', 'EnumOne'), ('enum3', 'EnumOne'), ('*enum4', 'EnumOne')]))]),
- OrderedDict([('struct', 'UserDefZero'), ('data', OrderedDict([('integer', 'int')]))]),
  OrderedDict([('struct', 'UserDefOne'), ('base', 'UserDefZero'), ('data', OrderedDict([('string', 'str'), ('*enum1', 'EnumOne')]))]),
+ OrderedDict([('struct', 'UserDefZero'), ('data', OrderedDict([('integer', 'int')]))]),
  OrderedDict([('struct', 'UserDefTwoDictDict'), ('data', OrderedDict([('userdef', 'UserDefOne'), ('string', 'str')]))]),
  OrderedDict([('struct', 'UserDefTwoDict'), ('data', OrderedDict([('string1', 'str'), ('dict2', 'UserDefTwoDictDict'), ('*dict3', 'UserDefTwoDictDict')]))]),
  OrderedDict([('struct', 'UserDefTwo'), ('data', OrderedDict([('string0', 'str'), ('dict1', 'UserDefTwoDict')]))]),
  OrderedDict([('struct', 'UserDefA'), ('data', OrderedDict([('boolean', 'bool')]))]),
  OrderedDict([('struct', 'UserDefB'), ('data', OrderedDict([('integer', 'int')]))]),
- OrderedDict([('struct', 'UserDefC'), ('data', OrderedDict([('string1', 'str'), ('string2', 'str')]))]),
- OrderedDict([('struct', 'UserDefUnionBase'), ('data', OrderedDict([('string', 'str'), ('enum1', 'EnumOne')]))]),
  OrderedDict([('union', 'UserDefFlatUnion'), ('base', 'UserDefUnionBase'), ('discriminator', 'enum1'), ('data', OrderedDict([('value1', 'UserDefA'), ('value2', 'UserDefB'), ('value3', 'UserDefB')]))]),
+ OrderedDict([('struct', 'UserDefUnionBase'), ('data', OrderedDict([('string', 'str'), ('enum1', 'EnumOne')]))]),
  OrderedDict([('union', 'UserDefFlatUnion2'), ('base', 'UserDefUnionBase'), ('discriminator', 'enum1'), ('data', OrderedDict([('value1', 'UserDefC'), ('value2', 'UserDefB'), ('value3', 'UserDefA')]))]),
  OrderedDict([('alternate', 'UserDefAlternate'), ('data', OrderedDict([('uda', 'UserDefA'), ('s', 'str'), ('i', 'int')]))]),
+ OrderedDict([('struct', 'UserDefC'), ('data', OrderedDict([('string1', 'str'), ('string2', 'str')]))]),
  OrderedDict([('union', 'UserDefNativeListUnion'), ('data', OrderedDict([('integer', ['int']), ('s8', ['int8']), ('s16', ['int16']), ('s32', ['int32']), ('s64', ['int64']), ('u8', ['uint8']), ('u16', ['uint16']), ('u32', ['uint32']), ('u64', ['uint64']), ('number', ['number']), ('boolean', ['bool']), ('string', ['str']), ('sizes', ['size'])]))]),
  OrderedDict([('command', 'user_def_cmd'), ('data', OrderedDict())]),
  OrderedDict([('command', 'user_def_cmd1'), ('data', OrderedDict([('ud1a', 'UserDefOne')]))]),
@@ -39,15 +39,15 @@ 
  {'enum_name': '__org.qemu_x-Union1Kind', 'enum_values': None},
  {'enum_name': '__org.qemu_x-AltKind', 'enum_values': None}]
 [OrderedDict([('struct', 'NestedEnumsOne'), ('data', OrderedDict([('enum1', 'EnumOne'), ('*enum2', 'EnumOne'), ('enum3', 'EnumOne'), ('*enum4', 'EnumOne')]))]),
- OrderedDict([('struct', 'UserDefZero'), ('data', OrderedDict([('integer', 'int')]))]),
  OrderedDict([('struct', 'UserDefOne'), ('base', 'UserDefZero'), ('data', OrderedDict([('string', 'str'), ('*enum1', 'EnumOne')]))]),
+ OrderedDict([('struct', 'UserDefZero'), ('data', OrderedDict([('integer', 'int')]))]),
  OrderedDict([('struct', 'UserDefTwoDictDict'), ('data', OrderedDict([('userdef', 'UserDefOne'), ('string', 'str')]))]),
  OrderedDict([('struct', 'UserDefTwoDict'), ('data', OrderedDict([('string1', 'str'), ('dict2', 'UserDefTwoDictDict'), ('*dict3', 'UserDefTwoDictDict')]))]),
  OrderedDict([('struct', 'UserDefTwo'), ('data', OrderedDict([('string0', 'str'), ('dict1', 'UserDefTwoDict')]))]),
  OrderedDict([('struct', 'UserDefA'), ('data', OrderedDict([('boolean', 'bool')]))]),
  OrderedDict([('struct', 'UserDefB'), ('data', OrderedDict([('integer', 'int')]))]),
- OrderedDict([('struct', 'UserDefC'), ('data', OrderedDict([('string1', 'str'), ('string2', 'str')]))]),
  OrderedDict([('struct', 'UserDefUnionBase'), ('data', OrderedDict([('string', 'str'), ('enum1', 'EnumOne')]))]),
+ OrderedDict([('struct', 'UserDefC'), ('data', OrderedDict([('string1', 'str'), ('string2', 'str')]))]),
  OrderedDict([('struct', 'UserDefOptions'), ('data', OrderedDict([('*i64', ['int']), ('*u64', ['uint64']), ('*u16', ['uint16']), ('*i64x', 'int'), ('*u64x', 'uint64')]))]),
  OrderedDict([('struct', 'EventStructOne'), ('data', OrderedDict([('struct1', 'UserDefOne'), ('string', 'str'), ('*enum2', 'EnumOne')]))]),
  OrderedDict([('struct', '__org.qemu_x-Base'), ('data', OrderedDict([('__org.qemu_x-member1', '__org.qemu_x-Enum')]))]),