Re: [Qemu-devel] [PATCH RFC v2 26/47] qapi-types: Convert to QAPISchemaVisitor, fixing flat unions

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 07/01/2015 02:22 PM, Markus Armbruster wrote:
>> Fixes flat unions to get the base's base members.  Test case is from
>> commit 2fc0043, in qapi-schema-test.json:
>> 
>
>> 
>> Flat union visitors remain broken.  They'll be fixed next.
>
> Sadly, the generated files had a huge diffstat, making it very hard to
> determine if this change is sane:
>
>  qapi-types.c                        | 1412 ++++++-------
>  qapi-types.h                        | 3705 
> ++++++++++++++++++++----------------
>  qga/qapi-generated/qga-qapi-types.c |  110 -
>  qga/qapi-generated/qga-qapi-types.h |  542 +++--
>  4 files changed, 3208 insertions(+), 2561 deletions(-)
>
> At first, it looks easy, as in:
>
> qapi-types.c:
> +const int BlockdevRef_qtypes[QTYPE_MAX] = {
> ...
>  const char *const BlockdevRefKind_lookup[] = {
> ...
> -const int BlockdevRef_qtypes[QTYPE_MAX] = {
> ...
>
> (that is, the new visitor outputs the two arrays in a different order -
> I can live with that).

The old code generates enum lookup tables for implicit union or
alternate enumerations from within the union or alternate code.

The new code doesn't, because it makes all these enums full-fledged
types, so the ordinary enum generator does everything we need.

And that's why generating stuff in alphabetical doesn't help here: the
new types get inserted in the sorted sequence, but that's not where the
old code generated the implicit enum stuff.

> Then it gets a bit crazy when using normal diff algorithms:
>
> -void qapi_free_int32List(int32List *obj)
> +void qapi_free_ACPIOSTInfo(ACPIOSTInfo *obj)
>
> where declarations are rearranged (so the previous commit, which
> attempted to sort declarations to make this one easier, did not quite
> succeed).

I found that quite disappointing myself, but I didn't have better ideas
at the time.

> Even in qapi-types.h things were rearranged:
>
>
> -#ifndef QAPI_TYPES_BUILTIN_STRUCT_DECL
> -#define QAPI_TYPES_BUILTIN_STRUCT_DECL
> +#ifndef QAPI_TYPES_BUILTIN
> +#define QAPI_TYPES_BUILTIN
>
>
> -typedef struct int32List {
> +typedef struct boolList boolList;
> +
> +struct boolList {
>
> I can understand the minor change in #ifdef name, and even the
> separation between typedef and struct declaration (even though the old
> approach of doing it all at once works).

Artifact of my attempt to DRY: I sometimes need the typedef in one place
(say .h) and the actual struct in another place (say .c).  I make do
with the functions generating that even when I want both of them in the
same place.

>                                           But the overall diff in the
> generated files would be easier to review if done in stages, and either
> make 25/47 sort closer to what this patch does, or add yet more
> prerequisite patches that further tweak sorting.  Ideally, this patch
> will be a lot easier to review if the generated code is much closer to
> the pre-patch version (that is, separating sorting changes from other
> changes).  On the other hand, yes, it's kind of a pain to patch old code
> to do things differently just before throwing it away with the new code
> in its place,

Especially when you're throwing away the old code because you can't
stand working with it :)

>               so it becomes a judgment call of how confident we want to
> be that the new implementation isn't breaking things.

Different idea: I hack up a script to split file FOO into one part per
top-level syntactical construct (typedef, function, whatever), and store
each part in a file named like NUM-NAME-FOO.  The script will most
likely be hairy, so use diff FOO <(cat *-FOO) to verify it works.  Then
diff old/*-NAME-FOO new/*-NAME-FOO to see what changed.

Writing the script will take non-trivial effort, but I'll try if you
think it's needed.

For what it's worth, I sanity-checked with my "diff sorted" trick
mentioned in my reply to your review of PATCH 25.

> I was able to make a bit more sense of things by using git's 'diff
> patience' algorithm, which showed things more as code motion rather than
> one-or-two-line changes to lots of common boilerplate, but even that
> diffstat is still big:
>
>  qapi-types1.c | 2030 +++++++++++++++----------------
>  qapi-types1.h | 3707 
> +++++++++++++++++++++++++++++++++------------------------
>  2 files changed, 3148 insertions(+), 2589 deletions(-)
>
> That said, I'll still at least review this code by inspection, and
> things still compile fine, so although I'm reluctant to give R-b while
> the patch is in RFC stage (because I didn't want to take the time to be
> certain the results are the same amidst so much churn), they are mostly
> sane.

Fair enough.

>> Signed-off-by: Markus Armbruster <address@hidden>
>> ---
>>  scripts/qapi-types.py | 268 +++++++++++++-------------------
>>  tests/qapi-schema/qapi-schema-test.json |   4 +-
>>  2 files changed, 114 insertions(+), 158 deletions(-)
>> 
>> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
>> index a48ad9c..d6185c6 100644
>> --- a/scripts/qapi-types.py
>> +++ b/scripts/qapi-types.py
>> @@ -2,88 +2,67 @@
>>  # QAPI types generator
>>  #
>>  # Copyright IBM, Corp. 2011
>> +# Copyright (c) 2013-2015 Red Hat Inc.
>>  #
>>  # Authors:
>>  #  Anthony Liguori <address@hidden>
>> +#  Markus Armbruster <address@hidden>
>>  #
>>  # This work is licensed under the terms of the GNU GPL, version 2.
>>  # See the COPYING file in the top-level directory.
>>  
>> -from ordereddict import OrderedDict
>>  from qapi import *
>>  
>> -def generate_fwd_builtin(name):
>> -    return mcgen('''
>> -
>> -typedef struct %(name)sList {
>
> For example, the change of splitting this into:
>
> typedef struct %(name)sList;
>
> struct %(name)sList {
>
> done as a separate patch would make the generated diff of this patch
> smaller.

I'm willing to mess with the old code *locally* to reduce the diffs, if
that helps.

>> -def generate_struct_fields(members):
>> +def gen_struct_field(name, typ, optional):
>>      ret = ''
>>  
>> -    for argname, argentry, optional in parse_args(members):
>> -        if optional:
>> -            ret += mcgen('''
>> +    if optional:
>> +        ret += mcgen('''
>
> (sometimes it's annoying that python requires indentation changes when
> changing control flow; for C files, sometimes I split a patch that
> merely adds {} and indentation from another patch that changes logic, so
> that the logic change isn't drowned by the reindentation)

I sometimes use C-c C-w in diff-mode to view a patch hunk without its
whitespace changes.

>> +class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
>> +    def __init__(self):
>> +        self.decl = None
>> +        self.defn = None
>> +        self.fwdecl = None
>> +        self.fwdefn = None
>> +        self.btin = None
>> +    def visit_begin(self):
>> +        self.decl = ''
>> +        self.defn = ''
>> +        self.fwdecl = ''
>> +        self.fwdefn = ''
>> +        self.btin = guardstart('QAPI_TYPES_BUILTIN')
>> +    def visit_end(self):
>> +        self.decl = self.fwdecl + self.decl
>> +        self.fwdecl = None
>> +        self.defn = self.fwdefn + self.defn
>> +        self.fwdefn = None
>> +        # To avoid header dependency hell, we always generate
>> +        # declarations for built-in types in our header files and
>> +        # simply guard them.
>> +        self.btin += guardend('QAPI_TYPES_BUILTIN')
>> +        self.decl = self.btin + self.decl
>> +        self.btin = None
>> +        # Doesn't work for cases where we link in multiple objects
>> +        # that have the functions defined, so generate them only with
>> +        # option -b (do_builtins).
>
> Does this comment...
>
>> +    def _gen_type_cleanup(self, name):
>> +        self.decl += generate_type_cleanup_decl(name)
>> +        self.defn += generate_type_cleanup(name)
>> +    def visit_enum_type(self, name, info, values):
>> +        self.fwdecl += generate_enum(name, values)
>> +        self.fwdefn += generate_enum_lookup(name, values)
>> +    def visit_array_type(self, name, info, element_type):
>> +        if isinstance(element_type, QAPISchemaBuiltinType):
>> +            self.btin += gen_fwd_object_or_array(name)
>> +            self.btin += gen_array(name, element_type)
>> +            self.btin += generate_type_cleanup_decl(name)
>> +            if do_builtins:
>> +                self.defn += generate_type_cleanup(name)
>
> ...fit better here?

If I rephrase it, probably yes.

>> +        else:
>> +            self.fwdecl += gen_fwd_object_or_array(name)
>> +            self.decl += gen_array(name, element_type)
>> +            self._gen_type_cleanup(name)
>> +    def visit_object_type(self, name, info, base, members, variants):
>> +        if info:
>
> So to make sure I understand, this ensures that implicit types (those
> handled merely by parameters in a function or as members of a struct)
> have no declarations required in the -types files.

Yes, (not info) means the object type is implicit.  We create implicit
object types for command parameters, command returns, and event
paramaters that aren't type names.

We don't generate C for these types, they're purely internal.  We
generate *parameter lists* for them.

Hmm, that's actually not true for command returns.  And current master
duly bombs if I add such a command to qapi-schema-test.json:

    { 'command': 'user_def_cmd4', 'returns': { 'a': 'int' } }

      GEN   tests/test-qmp-commands.h
    Traceback (most recent call last):
      File "/work/armbru/qemu/scripts/qapi-commands.py", line 360, in <module>
        ret = generate_command_decl(cmd['command'], arglist, ret_type) + "\n"
      File "/work/armbru/qemu/scripts/qapi-commands.py", line 29, in 
generate_command_decl
        ret_type=c_type(ret_type), name=c_name(name),
      File "/work/armbru/qemu/scripts/qapi.py", line 924, in c_type
        assert isinstance(value, str) and value != ""
    AssertionError

The easy fix is to outlaw it.

>> +            self.fwdecl += gen_fwd_object_or_array(name)
>> +            if variants:
>> +                self.decl += gen_union(name, base, variants)
>
> Is it worth 'assert not members' at this point?

Yes, with a TODO, because it's an artificial restriction I'd like to
lift some day.

>> +            else:
>> +                self.decl += gen_struct(name, base, members)
>> +            self._gen_type_cleanup(name)
>> +    def visit_alternate_type(self, name, info, variants):
>> +        self.fwdecl += gen_fwd_object_or_array(name)
>> +        self.fwdefn += gen_alternate_qtypes(name, variants)
>> +        self.decl += gen_union(name, None, variants)
>> +        self.decl += gen_alternate_qtypes_decl(name)
>> +        self._gen_type_cleanup(name)
>> +
>
> The visitor looks reasonable from inspection review.
>
>>  do_builtins = False
>>  
>>  (input_file, output_dir, do_c, do_h, prefix, opts) = \
>> @@ -325,77 +348,10 @@ fdecl.write(mcgen('''
>>  #include <stdint.h>
>>  '''))
>>  
>> -exprs = QAPISchema(input_file).get_exprs()
>> -
>> -fdecl.write(guardstart("QAPI_TYPES_BUILTIN_STRUCT_DECL"))
>> -for typename in builtin_types.keys():
>> -    fdecl.write(generate_fwd_builtin(typename))
>> -fdecl.write(guardend("QAPI_TYPES_BUILTIN_STRUCT_DECL"))
>
> This is a place where adding sorting in 25/47 (or a separate patch)
> would make the impact on generated code from this patch smaller.

You mean iterating over sorted(builtin_types.keys())?  I can try and see
whether it helps.

>> -
>> -for expr in exprs:
>> -    ret = ""
>> -    if expr.has_key('struct'):
>> -        ret += generate_fwd_struct(expr['struct'])
>> -    elif expr.has_key('enum'):
>> -        ret += generate_enum(expr['enum'], expr['data'])
>> -        ret += generate_fwd_enum_struct(expr['enum'])
>> -        fdef.write(generate_enum_lookup(expr['enum'], expr['data']))
>> -    elif expr.has_key('union'):
>> -        ret += generate_fwd_struct(expr['union'])
>> -        enum_define = discriminator_find_enum_define(expr)
>> -        if not enum_define:
>> - ret += generate_enum('%sKind' % expr['union'],
>> expr['data'].keys())
>> -            fdef.write(generate_enum_lookup('%sKind' % expr['union'],
>> -                                            expr['data'].keys()))
>> -    elif expr.has_key('alternate'):
>> -        ret += generate_fwd_struct(expr['alternate'])
>> - ret += generate_enum('%sKind' % expr['alternate'],
>> expr['data'].keys())
>> -        fdef.write(generate_enum_lookup('%sKind' % expr['alternate'],
>> -                                        expr['data'].keys()))
>
> Tweaking the order of _qtypes[] vs. _lookup[] here in a pre-req patch
> would also help review of the diff generated by this patch.

Can try and see.

>> -        fdef.write(generate_alternate_qtypes(expr))
>> -    else:
>> -        continue
>> -    fdecl.write(ret)
>> -
>> -# to avoid header dependency hell, we always generate declarations
>> -# for built-in types in our header files and simply guard them
>> -fdecl.write(guardstart("QAPI_TYPES_BUILTIN_CLEANUP_DECL"))
>> -for typename in builtin_types.keys():
>> -    fdecl.write(generate_type_cleanup_decl(typename + "List"))
>> -fdecl.write(guardend("QAPI_TYPES_BUILTIN_CLEANUP_DECL"))
>> -
>> -# ...this doesn't work for cases where we link in multiple objects that
>> -# have the functions defined, so we use -b option to provide control
>> -# over these cases
>> -if do_builtins:
>> -    for typename in builtin_types.keys():
>> -        fdef.write(generate_type_cleanup(typename + "List"))
>
> Another iteration worth sorting in an earlier patch.

Can try and see.

>> -
>> -for expr in exprs:
>> -    ret = ""
>> -    if expr.has_key('struct'):
>> -        ret += generate_struct(expr) + "\n"
>> -        ret += generate_type_cleanup_decl(expr['struct'] + "List")
>> -        fdef.write(generate_type_cleanup(expr['struct'] + "List"))
>> -        ret += generate_type_cleanup_decl(expr['struct'])
>> -        fdef.write(generate_type_cleanup(expr['struct']))
>
> I think part of the large size of the generated diff comes from whether
> array fooList types are emitted in a different order via the visitor.

That's harder to avoid.  The old code generates them from within the
code generating the element type.  The new code doesn't, because it
makes all array types full-fledged types, which get visited like any
other type.

>> -    elif expr.has_key('union'):
>> -        ret += generate_union(expr, 'union') + "\n"
>> -        ret += generate_type_cleanup_decl(expr['union'] + "List")
>> -        fdef.write(generate_type_cleanup(expr['union'] + "List"))
>> -        ret += generate_type_cleanup_decl(expr['union'])
>> -        fdef.write(generate_type_cleanup(expr['union']))
>> -    elif expr.has_key('alternate'):
>> -        ret += generate_union(expr, 'alternate') + "\n"
>> -        ret += generate_type_cleanup_decl(expr['alternate'] + "List")
>> -        fdef.write(generate_type_cleanup(expr['alternate'] + "List"))
>> -        ret += generate_type_cleanup_decl(expr['alternate'])
>> -        fdef.write(generate_type_cleanup(expr['alternate']))
>> -    elif expr.has_key('enum'):
>> -        ret += "\n" + generate_type_cleanup_decl(expr['enum'] + "List")
>> -        fdef.write(generate_type_cleanup(expr['enum'] + "List"))
>> -    else:
>> -        continue
>> -    fdecl.write(ret)
>> +schema = QAPISchema(input_file)
>> +gen = QAPISchemaGenTypeVisitor()
>> +schema.visit(gen)
>> +fdef.write(gen.defn)
>> +fdecl.write(gen.decl)
>>  
>>  close_output(fdef, fdecl)
>> diff --git a/tests/qapi-schema/qapi-schema-test.json 
>> b/tests/qapi-schema/qapi-schema-test.json
>> index a9e5aab..257b4d4 100644
>> --- a/tests/qapi-schema/qapi-schema-test.json
>> +++ b/tests/qapi-schema/qapi-schema-test.json
>> @@ -39,8 +39,8 @@
>>    'data': { 'value1' : 'UserDefA',
>>              'value2' : 'UserDefB',
>>              'value3' : 'UserDefB' } }
>> -# FIXME generated struct UserDefFlatUnion has members for direct base
>> -# UserDefUnionBase, but lacks members for indirect base UserDefZero
>> +# FIXME generated visit_type_UserDefFlatUnion_fields() fails to visit
>> +# members of indirect base UserDefZero
>>  
>>  { 'struct': 'UserDefUnionBase',
>>    'base': 'UserDefZero',
>> 
>
> Overall, moving in the right direction.

Thanks!