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

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 4255 bytes --]

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

Okay, I see a cause for part of my confusion.

>  
> +class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):

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

The new code goes to great lengths to separate all builtin decls up
front.  But...

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

...it is still interleaving builtin defns with everything else.


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

Meanwhile, the old code did builtin intList definitions up front...

> -
> -for expr in exprs:
> -    ret = ""
> -    if expr.has_key('struct'):

> -# 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"))

...but declared the cleanup functions at the end (so code motion 1 is
that the cleanup functions are now interleaved with the intList
declarations)...

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

...and wrote the definitions at the end (so code motion 2 is that the
definitions are now interleaved with all other definitions).

Attached a few quick hacks that reduce (but not eliminate) some of the
visitor's churn to the generated files, by consolidating the location of
the builtins and cleaning up struct declarations as separate cleanups
(to be applied prior to 26/47), then updating the visitor to use the
same builtin order (to be applied after or squashed with 26/47).

It still doesn't solve everything, but with those hacks, I was able to
get to a slightly more manageable:

 qapi-types.c                        |  864 ++++----
 qapi-types.h                        | 3602
++++++++++++++++++------------------
 qga/qapi-generated/qga-qapi-types.c |  110 -
 qga/qapi-generated/qga-qapi-types.h |  405 ++--
 4 files changed, 2585 insertions(+), 2396 deletions(-)

I'm sure there are further things that could be done, but at this point,
I hope you get my picture, and I'll quit focusing on this particular patch.

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

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: 0001-qapi-types-sort-and-consolidate-builtins.patch --]
[-- Type: text/x-patch; name="0001-qapi-types-sort-and-consolidate-builtins.patch", Size: 2098 bytes --]

From 00df19b3b0d497e0a4670308f971ca95383b20cd Mon Sep 17 00:00:00 2001
From: Eric Blake <eblake@redhat.com>
Date: Wed, 22 Jul 2015 13:59:20 -0600
Subject: [PATCH 1/4] qapi-types: sort and consolidate builtins

Just as sorting generated code based on user-defined structs
will help analysis of later patches, we also want to sort
builtin types rather than letting python randomly dump a
hash table.  We also want to consolidate the two conditional
areas of the generated .h file into one.
---
 scripts/qapi-types.py | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index a48ad9c..b7028f6 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -327,11 +327,6 @@ fdecl.write(mcgen('''

 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"))
-
 for expr in exprs:
     ret = ""
     if expr.has_key('struct'):
@@ -359,16 +354,17 @@ for expr in exprs:

 # 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(guardstart("QAPI_TYPES_BUILTIN"))
+for typename in sorted(builtin_types.keys()):
+    fdecl.write(generate_fwd_builtin(typename))
     fdecl.write(generate_type_cleanup_decl(typename + "List"))
-fdecl.write(guardend("QAPI_TYPES_BUILTIN_CLEANUP_DECL"))
+fdecl.write(guardend("QAPI_TYPES_BUILTIN"))

 # ...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():
+    for typename in sorted(builtin_types.keys()):
         fdef.write(generate_type_cleanup(typename + "List"))

 for expr in exprs:
-- 
2.4.3


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.3: 0002-qapi-update-typedef-usage.patch --]
[-- Type: text/x-patch; name="0002-qapi-update-typedef-usage.patch", Size: 2075 bytes --]

From eac8ce42799ecb17271a70aa52185b54173de399 Mon Sep 17 00:00:00 2001
From: Eric Blake <eblake@redhat.com>
Date: Wed, 22 Jul 2015 14:00:06 -0600
Subject: [PATCH 2/4] qapi: update typedef usage

Change from:

 typedef struct fooList {
     ...
     struct fooList *next;
 } fooList;

to:

 typedef struct fooList fooList;

 struct fooList {
     ...
     struct fooList *next;
 };

If desired, we could further eliminate the now-spurious 'struct'
from the 'next' member of the list.

Note that qemu does not have any standard on which of the two
forms is preferred, and that more uses tend to go with the
typedef and declaration smashed into one.
---
 scripts/qapi-types.py | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index b7028f6..d21381c 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -15,13 +15,15 @@ from qapi import *
 def generate_fwd_builtin(name):
     return mcgen('''

-typedef struct %(name)sList {
+typedef struct %(name)sList %(name)sList;
+
+struct %(name)sList {
     union {
         %(type)s value;
         uint64_t padding;
     };
     struct %(name)sList *next;
-} %(name)sList;
+};
 ''',
                  type=c_type(name),
                  name=name)
@@ -31,26 +33,30 @@ def generate_fwd_struct(name):

 typedef struct %(name)s %(name)s;

-typedef struct %(name)sList {
+typedef struct %(name)sList %(name)sList;
+
+struct %(name)sList {
     union {
         %(name)s *value;
         uint64_t padding;
     };
     struct %(name)sList *next;
-} %(name)sList;
+};
 ''',
                  name=c_name(name))

 def generate_fwd_enum_struct(name):
     return mcgen('''

-typedef struct %(name)sList {
+typedef struct %(name)sList %(name)sList;
+
+struct %(name)sList {
     union {
         %(name)s value;
         uint64_t padding;
     };
     struct %(name)sList *next;
-} %(name)sList;
+};
 ''',
                  name=c_name(name))

-- 
2.4.3


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.4: 0003-qapi-squash-to-visitor.patch --]
[-- Type: text/x-patch; name="0003-qapi-squash-to-visitor.patch", Size: 3244 bytes --]

From 348f3d3d3a35afe7dca4b695972677ce03da91ca Mon Sep 17 00:00:00 2001
From: Eric Blake <eblake@redhat.com>
Date: Wed, 22 Jul 2015 14:58:25 -0600
Subject: [PATCH 4/4] qapi: squash to visitor

Prior to conversion to a visitor, all code related to lists of
builtins were lumped together. Doing so in the visitor code
reduces the churn in comparing the generated files, making it
easier to prove the conversion to the visitor is correct.
---
 scripts/qapi-types.py | 31 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 16 deletions(-)

diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index d6185c6..0817628 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -240,27 +240,26 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
         self.defn = None
         self.fwdecl = None
         self.fwdefn = None
-        self.btin = None
+        self.btindecl = None
+        self.btindefn = None
     def visit_begin(self):
         self.decl = ''
         self.defn = ''
         self.fwdecl = ''
         self.fwdefn = ''
-        self.btin = guardstart('QAPI_TYPES_BUILTIN')
+        self.btindecl = guardstart('QAPI_TYPES_BUILTIN')
+        self.btindefn = ''
     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).
+        self.btindecl += guardend('QAPI_TYPES_BUILTIN')
+        self.decl = self.fwdecl + self.btindecl + self.decl
+        self.fwdecl = None
+        self.btindecl = None
+        self.defn = self.fwdefn + self.btindefn + self.defn
+        self.fwdefn = None
+        self.btindefn = None
     def _gen_type_cleanup(self, name):
         self.decl += generate_type_cleanup_decl(name)
         self.defn += generate_type_cleanup(name)
@@ -269,11 +268,11 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
         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)
+            self.btindecl += gen_fwd_object_or_array(name)
+            self.btindecl += gen_array(name, element_type)
+            self.btindecl += generate_type_cleanup_decl(name)
             if do_builtins:
-                self.defn += generate_type_cleanup(name)
+                self.btindefn += generate_type_cleanup(name)
         else:
             self.fwdecl += gen_fwd_object_or_array(name)
             self.decl += gen_array(name, element_type)
-- 
2.4.3


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]