From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56817) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZI1Sl-0007S4-Lc for qemu-devel@nongnu.org; Wed, 22 Jul 2015 17:22:05 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZI1Sh-00057I-9x for qemu-devel@nongnu.org; Wed, 22 Jul 2015 17:22:03 -0400 Received: from mx1.redhat.com ([209.132.183.28]:49569) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZI1Sg-00057C-WC for qemu-devel@nongnu.org; Wed, 22 Jul 2015 17:21:59 -0400 References: <1435782155-31412-1-git-send-email-armbru@redhat.com> <1435782155-31412-27-git-send-email-armbru@redhat.com> From: Eric Blake Message-ID: <55B00970.2060206@redhat.com> Date: Wed, 22 Jul 2015 15:21:52 -0600 MIME-Version: 1.0 In-Reply-To: <1435782155-31412-27-git-send-email-armbru@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="rwCAgaaS2dgBbA7NaqXxgbW0tarPVO6LO" Subject: Re: [Qemu-devel] [PATCH RFC v2 26/47] qapi-types: Convert to QAPISchemaVisitor, fixing flat unions List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster , qemu-devel@nongnu.org Cc: kwolf@redhat.com, berto@igalia.com, mdroth@linux.vnet.ibm.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --rwCAgaaS2dgBbA7NaqXxgbW0tarPVO6LO Content-Type: multipart/mixed; boundary="------------090504050702080803050602" This is a multi-part message in MIME format. --------------090504050702080803050602 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable 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: >=20 Okay, I see a cause for part of my confusion. > =20 > +class QAPISchemaGenTypeVisitor(QAPISchemaVisitor): > + def visit_end(self): > + self.decl =3D self.fwdecl + self.decl > + self.fwdecl =3D None > + self.defn =3D self.fwdefn + self.defn > + self.fwdefn =3D None > + # To avoid header dependency hell, we always generate > + # declarations for built-in types in our header files and > + # simply guard them. > + self.btin +=3D guardend('QAPI_TYPES_BUILTIN') > + self.decl =3D self.btin + self.decl > + self.btin =3D 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 +=3D generate_type_cleanup_decl(name) > + self.defn +=3D generate_type_cleanup(name) > + def visit_enum_type(self, name, info, values): > + self.fwdecl +=3D generate_enum(name, values) > + self.fwdefn +=3D generate_enum_lookup(name, values) > + def visit_array_type(self, name, info, element_type): > + if isinstance(element_type, QAPISchemaBuiltinType): > + self.btin +=3D gen_fwd_object_or_array(name) > + self.btin +=3D gen_array(name, element_type) > + self.btin +=3D generate_type_cleanup_decl(name) > + if do_builtins: > + self.defn +=3D generate_type_cleanup(name) =2E..it is still interleaving builtin defns with everything else. > -exprs =3D 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 =3D "" > - 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")) =2E..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 tha= t > -# 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")) =2E..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 patc= h. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --------------090504050702080803050602 Content-Type: text/x-patch; name="0001-qapi-types-sort-and-consolidate-builtins.patch" Content-Transfer-Encoding: quoted-printable Content-Disposition: attachment; filename="0001-qapi-types-sort-and-consolidate-builtins.patch" =46rom 00df19b3b0d497e0a4670308f971ca95383b20cd Mon Sep 17 00:00:00 2001 From: Eric Blake 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 =3D 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 =3D "" 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: --=20 2.4.3 --------------090504050702080803050602 Content-Type: text/x-patch; name="0002-qapi-update-typedef-usage.patch" Content-Transfer-Encoding: quoted-printable Content-Disposition: attachment; filename="0002-qapi-update-typedef-usage.patch" =46rom eac8ce42799ecb17271a70aa52185b54173de399 Mon Sep 17 00:00:00 2001 From: Eric Blake 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=3Dc_type(name), name=3Dname) @@ -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=3Dc_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=3Dc_name(name)) --=20 2.4.3 --------------090504050702080803050602 Content-Type: text/x-patch; name="0003-qapi-squash-to-visitor.patch" Content-Transfer-Encoding: quoted-printable Content-Disposition: attachment; filename="0003-qapi-squash-to-visitor.patch" =46rom 348f3d3d3a35afe7dca4b695972677ce03da91ca Mon Sep 17 00:00:00 2001 From: Eric Blake 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 =3D None self.fwdecl =3D None self.fwdefn =3D None - self.btin =3D None + self.btindecl =3D None + self.btindefn =3D None def visit_begin(self): self.decl =3D '' self.defn =3D '' self.fwdecl =3D '' self.fwdefn =3D '' - self.btin =3D guardstart('QAPI_TYPES_BUILTIN') + self.btindecl =3D guardstart('QAPI_TYPES_BUILTIN') + self.btindefn =3D '' def visit_end(self): - self.decl =3D self.fwdecl + self.decl - self.fwdecl =3D None - self.defn =3D self.fwdefn + self.defn - self.fwdefn =3D None # To avoid header dependency hell, we always generate # declarations for built-in types in our header files and # simply guard them. - self.btin +=3D guardend('QAPI_TYPES_BUILTIN') - self.decl =3D self.btin + self.decl - self.btin =3D 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 +=3D guardend('QAPI_TYPES_BUILTIN') + self.decl =3D self.fwdecl + self.btindecl + self.decl + self.fwdecl =3D None + self.btindecl =3D None + self.defn =3D self.fwdefn + self.btindefn + self.defn + self.fwdefn =3D None + self.btindefn =3D None def _gen_type_cleanup(self, name): self.decl +=3D generate_type_cleanup_decl(name) self.defn +=3D generate_type_cleanup(name) @@ -269,11 +268,11 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor): self.fwdefn +=3D generate_enum_lookup(name, values) def visit_array_type(self, name, info, element_type): if isinstance(element_type, QAPISchemaBuiltinType): - self.btin +=3D gen_fwd_object_or_array(name) - self.btin +=3D gen_array(name, element_type) - self.btin +=3D generate_type_cleanup_decl(name) + self.btindecl +=3D gen_fwd_object_or_array(name) + self.btindecl +=3D gen_array(name, element_type) + self.btindecl +=3D generate_type_cleanup_decl(name) if do_builtins: - self.defn +=3D generate_type_cleanup(name) + self.btindefn +=3D generate_type_cleanup(name) else: self.fwdecl +=3D gen_fwd_object_or_array(name) self.decl +=3D gen_array(name, element_type) --=20 2.4.3 --------------090504050702080803050602-- --rwCAgaaS2dgBbA7NaqXxgbW0tarPVO6LO Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJVsAlwAAoJEKeha0olJ0NqEj8IAIyf8jdWu1HCFYJVfJ/tcRy+ LwMQTN5cqjgdTU6GldvoGcMwCyxQvkHtkVD96U++63LU+Qs2ejXHG6DCAOnpgAt/ 4wp/w6G6Z5IW54RglfrbZ7Q8aOb1alrQ2RvgmGtRjCqwa+hhzSGspxA/99VQ0VER ah/KSOZf7SbyIsTcLza2L0TkepKPRuAl/w0qCb6tH/+xjWPHDNqPSZoJpZYbaJ8s 7jQhzhhn3264tyb/+5OsmBnP2yKE1DLPLzhqVAyTXI6AxElX4jEwPmj8Mqh0Q1rb XabmuUFKFB3Y4lJVezbX4L7F6sGBcjatW+71Os4FYaJZbUE2g4EIAhl3cFwLjPM= =Yp/X -----END PGP SIGNATURE----- --rwCAgaaS2dgBbA7NaqXxgbW0tarPVO6LO--