diff mbox

[v2,20/29] qapi/types qapi/visit: Generate built-in stuff into separate files

Message ID 20180211093607.27351-21-armbru@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Markus Armbruster Feb. 11, 2018, 9:35 a.m. UTC
Linking code from multiple separate QAPI schemata into the same
program is possible, but involves some weirdness around built-in
types:

* We generate code for built-in types into .c only with option
  --builtins.  The user is responsible for generating code for exactly
  one QAPI schema per program with --builtins.

* We generate code for built-in types into .h regardless of
  --builtins, but guarded by #ifndef QAPI_VISIT_BUILTIN.  Because all
  copies of this code are exactly the same, including any combination
  of these headers works.

Replace this contraption by something more conventional: generate code
for built-in types into their very own files: qapi-builtin-types.c,
qapi-builtin-visit.c, qapi-builtin-types.h, qapi-builtin-visit.h, but
only with --builtins.  Obey --output-dir, but ignore --prefix for
them.

Make qapi-types.h include qapi-builtin-types.h.  With multiple
schemata you now have multiple qapi-types.[ch], but only one
qapi-builtin-types.[ch].  Same for qapi-visit.[ch] and
qapi-builtin-visit.[ch].

Bonus: if all you need is built-in stuff, you can include a much
smaller header.  To be exploited shortly.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 .gitignore             |  2 ++
 Makefile               | 13 ++++++----
 Makefile.objs          |  2 ++
 scripts/qapi/common.py | 61 ++++++++++++++++++++++++++++++++++++++++-------
 scripts/qapi/types.py  | 61 +++++++++++++++++++----------------------------
 scripts/qapi/visit.py  | 64 +++++++++++++++++++++-----------------------------
 6 files changed, 116 insertions(+), 87 deletions(-)

Comments

Eric Blake Feb. 12, 2018, 9:44 p.m. UTC | #1
On 02/11/2018 03:35 AM, Markus Armbruster wrote:
> Linking code from multiple separate QAPI schemata into the same
> program is possible, but involves some weirdness around built-in
> types:
> 
> * We generate code for built-in types into .c only with option
>    --builtins.  The user is responsible for generating code for exactly
>    one QAPI schema per program with --builtins.
> 
> * We generate code for built-in types into .h regardless of
>    --builtins, but guarded by #ifndef QAPI_VISIT_BUILTIN.  Because all
>    copies of this code are exactly the same, including any combination
>    of these headers works.
> 
> Replace this contraption by something more conventional: generate code
> for built-in types into their very own files: qapi-builtin-types.c,
> qapi-builtin-visit.c, qapi-builtin-types.h, qapi-builtin-visit.h, but
> only with --builtins.  Obey --output-dir, but ignore --prefix for
> them.
> 
> Make qapi-types.h include qapi-builtin-types.h.  With multiple
> schemata you now have multiple qapi-types.[ch], but only one
> qapi-builtin-types.[ch].  Same for qapi-visit.[ch] and
> qapi-builtin-visit.[ch].
> 
> Bonus: if all you need is built-in stuff, you can include a much
> smaller header.  To be exploited shortly.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> @@ -2046,6 +2046,7 @@ class QAPIGenH(QAPIGenC):
>   
>   
>   class QAPIGenDoc(QAPIGen):
> +
>       def _top(self, fname):
>           return (QAPIGen._top(self, fname)
>                   + '@c AUTOMATICALLY GENERATED, DO NOT MODIFY\n\n')

Does this hunk belong in an earlier patch?

Otherwise,
Reviewed-by: Eric Blake <eblake@redhat.com>
Marc-André Lureau Feb. 13, 2018, 3:25 p.m. UTC | #2
Hi

On Sun, Feb 11, 2018 at 10:35 AM, Markus Armbruster <armbru@redhat.com> wrote:
> Linking code from multiple separate QAPI schemata into the same
> program is possible, but involves some weirdness around built-in
> types:
>
> * We generate code for built-in types into .c only with option
>   --builtins.  The user is responsible for generating code for exactly
>   one QAPI schema per program with --builtins.
>
> * We generate code for built-in types into .h regardless of
>   --builtins, but guarded by #ifndef QAPI_VISIT_BUILTIN.  Because all
>   copies of this code are exactly the same, including any combination
>   of these headers works.
>
> Replace this contraption by something more conventional: generate code
> for built-in types into their very own files: qapi-builtin-types.c,
> qapi-builtin-visit.c, qapi-builtin-types.h, qapi-builtin-visit.h, but
> only with --builtins.  Obey --output-dir, but ignore --prefix for
> them.
>
> Make qapi-types.h include qapi-builtin-types.h.  With multiple
> schemata you now have multiple qapi-types.[ch], but only one
> qapi-builtin-types.[ch].  Same for qapi-visit.[ch] and
> qapi-builtin-visit.[ch].
>
> Bonus: if all you need is built-in stuff, you can include a much
> smaller header.  To be exploited shortly.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> ---
>  .gitignore             |  2 ++
>  Makefile               | 13 ++++++----
>  Makefile.objs          |  2 ++
>  scripts/qapi/common.py | 61 ++++++++++++++++++++++++++++++++++++++++-------
>  scripts/qapi/types.py  | 61 +++++++++++++++++++----------------------------
>  scripts/qapi/visit.py  | 64 +++++++++++++++++++++-----------------------------
>  6 files changed, 116 insertions(+), 87 deletions(-)
>
> diff --git a/.gitignore b/.gitignore
> index 7d783e6e66..9477a08b6b 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -29,6 +29,8 @@
>  /qga/qapi-generated
>  /qapi-generated
>  /qapi-gen-timestamp
> +/qapi-builtin-types.[ch]
> +/qapi-builtin-visit.[ch]
>  /qapi-types.[ch]
>  /qapi-visit.[ch]
>  /qapi-event.[ch]
> diff --git a/Makefile b/Makefile
> index 164a38578e..60ddc9c945 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -90,10 +90,13 @@ endif
>  include $(SRC_PATH)/rules.mak
>
>  GENERATED_FILES = qemu-version.h config-host.h qemu-options.def
> -GENERATED_FILES += qmp-commands.h qapi-types.h qapi-visit.h qapi-event.h
> -GENERATED_FILES += qmp-commands.c qapi-types.c qapi-visit.c qapi-event.c
> -GENERATED_FILES += qmp-introspect.h
> -GENERATED_FILES += qmp-introspect.c
> +GENERATED_FILES += qapi-builtin-types.h qapi-builtin-types.c
> +GENERATED_FILES += qapi-types.h qapi-types.c
> +GENERATED_FILES += qapi-builtin-visit.h qapi-builtin-visit.c
> +GENERATED_FILES += qapi-visit.h qapi-visit.c
> +GENERATED_FILES += qmp-commands.h qmp-commands.c
> +GENERATED_FILES += qapi-event.h qapi-event.c
> +GENERATED_FILES += qmp-introspect.c qmp-introspect.h
>  GENERATED_FILES += qapi-doc.texi
>
>  GENERATED_FILES += trace/generated-tcg-tracers.h
> @@ -520,7 +523,9 @@ qapi-modules = $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/qapi/common.json \
>                 $(SRC_PATH)/qapi/transaction.json \
>                 $(SRC_PATH)/qapi/ui.json
>
> +qapi-builtin-types.c qapi-builtin-types.h \
>  qapi-types.c qapi-types.h \
> +qapi-builtin-visit.c qapi-builtin-visit.h \
>  qapi-visit.c qapi-visit.h \
>  qmp-commands.h qmp-commands.c \
>  qapi-event.c qapi-event.h \
> diff --git a/Makefile.objs b/Makefile.objs
> index d255aaf194..2813e984fd 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -2,6 +2,8 @@
>  # Common libraries for tools and emulators
>  stub-obj-y = stubs/ crypto/
>  util-obj-y = util/ qobject/ qapi/
> +util-obj-y += qapi-builtin-types.o
> +util-obj-y += qapi-builtin-visit.o
>  util-obj-y += qmp-introspect.o qapi-types.o qapi-visit.o qapi-event.o
>
>  chardev-obj-y = chardev/
> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
> index 31d2f73e7e..de12f8469a 100644
> --- a/scripts/qapi/common.py
> +++ b/scripts/qapi/common.py
> @@ -1531,11 +1531,10 @@ class QAPISchema(object):
>
>      def _def_builtin_type(self, name, json_type, c_type):
>          self._def_entity(QAPISchemaBuiltinType(name, json_type, c_type))
> -        # TODO As long as we have QAPI_TYPES_BUILTIN to share multiple
> -        # qapi-types.h from a single .c, all arrays of builtins must be
> -        # declared in the first file whether or not they are used.  Nicer
> -        # would be to use lazy instantiation, while figuring out how to
> -        # avoid compilation issues with multiple qapi-types.h.
> +        # Instantiating only the arrays that are actually used would
> +        # be nice, but we can't as long as their generated code
> +        # (qapi-builtin-types.[ch]) may be shared by some other
> +        # schema.
>          self._make_array_type(name, None)
>
>      def _def_predefineds(self):
> @@ -1992,14 +1991,15 @@ class QAPIGen(object):
>          return ''
>
>      def write(self, output_dir, fname):
> -        if output_dir:
> +        pathname = os.path.join(output_dir, fname)
> +        dir = os.path.dirname(pathname)
> +        if dir:
>              try:
> -                os.makedirs(output_dir)
> +                os.makedirs(dir)
>              except os.error as e:
>                  if e.errno != errno.EEXIST:
>                      raise
> -        fd = os.open(os.path.join(output_dir, fname),
> -                     os.O_RDWR | os.O_CREAT, 0666)
> +        fd = os.open(pathname, os.O_RDWR | os.O_CREAT, 0666)
>          f = os.fdopen(fd, 'r+')
>          text = (self._top(fname) + self._preamble + self._body
>                  + self._bottom(fname))
> @@ -2046,6 +2046,7 @@ class QAPIGenH(QAPIGenC):
>
>
>  class QAPIGenDoc(QAPIGen):
> +
>      def _top(self, fname):
>          return (QAPIGen._top(self, fname)
>                  + '@c AUTOMATICALLY GENERATED, DO NOT MODIFY\n\n')
> @@ -2062,3 +2063,45 @@ class QAPISchemaMonolithicCVisitor(QAPISchemaVisitor):
>      def write(self, output_dir):
>          self._genc.write(output_dir, self._prefix + self._what + '.c')
>          self._genh.write(output_dir, self._prefix + self._what + '.h')
> +
> +
> +class QAPISchemaModularCVisitor(QAPISchemaVisitor):
> +
> +    def __init__(self, prefix, what, blurb, pydoc):
> +        self._prefix = prefix
> +        self._what = what
> +        self._blurb = blurb
> +        self._pydoc = pydoc
> +        self._module = {}
> +
> +    def _module_basename(self, what, name):
> +        if name is None:
> +            return re.sub(r'-', '-builtin-', what)
> +        return self._prefix + what
> +
> +    def _add_module(self, name, blurb):
> +        genc = QAPIGenC(blurb, self._pydoc)
> +        genh = QAPIGenH(blurb, self._pydoc)
> +        self._module[name] = (genc, genh)
> +        self._set_module(name)
> +
> +    def _set_module(self, name):
> +        self._genc, self._genh = self._module[name]
> +
> +    def write(self, output_dir, opt_builtins):
> +        for name in self._module:
> +            if name is None and not opt_builtins:
> +                continue
> +            basename = self._module_basename(self._what, name)
> +            (genc, genh) = self._module[name]
> +            genc.write(output_dir, basename + '.c')
> +            genh.write(output_dir, basename + '.h')
> +
> +    def _begin_module(self, name):
> +        pass
> +
> +    def visit_module(self, name):
> +        if len(self._module) != 1:
> +            return
> +        self._add_module(name, self._blurb)
> +        self._begin_module(name)
> diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
> index d2b8423479..59826b1162 100644
> --- a/scripts/qapi/types.py
> +++ b/scripts/qapi/types.py
> @@ -167,64 +167,51 @@ void qapi_free_%(c_name)s(%(c_name)s *obj)
>      return ret
>
>
> -class QAPISchemaGenTypeVisitor(QAPISchemaMonolithicCVisitor):
> +class QAPISchemaGenTypeVisitor(QAPISchemaModularCVisitor):
>
> -    def __init__(self, prefix, opt_builtins):
> -        QAPISchemaMonolithicCVisitor.__init__(
> +    def __init__(self, prefix):
> +        QAPISchemaModularCVisitor.__init__(
>              self, prefix, 'qapi-types', ' * Schema-defined QAPI types',
>              __doc__)
> -        self._opt_builtins = opt_builtins
> +        self._add_module(None, ' * Built-in QAPI types')
> +        self._genc.preamble_add(mcgen('''
> +#include "qemu/osdep.h"
> +#include "qapi/dealloc-visitor.h"
> +#include "qapi-builtin-types.h"
> +#include "qapi-builtin-visit.h"
> +'''))
> +        self._genh.preamble_add(mcgen('''
> +#include "qapi/util.h"
> +'''))
> +
> +    def _begin_module(self, name):
>          self._genc.preamble_add(mcgen('''
>  #include "qemu/osdep.h"
>  #include "qapi/dealloc-visitor.h"
>  #include "%(prefix)sqapi-types.h"
>  #include "%(prefix)sqapi-visit.h"
>  ''',
> -                                      prefix=prefix))
> +                                      prefix=self._prefix))
>          self._genh.preamble_add(mcgen('''
> -#include "qapi/util.h"
> +#include "qapi-builtin-types.h"
>  '''))
> -        self._btin = '\n' + guardstart('QAPI_TYPES_BUILTIN')
>
>      def visit_begin(self, schema):
>          # gen_object() is recursive, ensure it doesn't visit the empty type
>          objects_seen.add(schema.the_empty_object_type.name)
>
> -    def visit_end(self):
> -        # To avoid header dependency hell, we always generate
> -        # declarations for built-in types in our header files and
> -        # simply guard them.  See also opt_builtins (command line
> -        # option -b).
> -        self._btin += guardend('QAPI_TYPES_BUILTIN')
> -        self._genh.preamble_add(self._btin)
> -        self._btin = None
> -
>      def _gen_type_cleanup(self, name):
>          self._genh.add(gen_type_cleanup_decl(name))
>          self._genc.add(gen_type_cleanup(name))
>
>      def visit_enum_type(self, name, info, values, prefix):
> -        # Special case for our lone builtin enum type
> -        # TODO use something cleaner than existence of info
> -        if not info:
> -            self._btin += gen_enum(name, values, prefix)
> -            if self._opt_builtins:
> -                self._genc.add(gen_enum_lookup(name, values, prefix))
> -        else:
> -            self._genh.preamble_add(gen_enum(name, values, prefix))
> -            self._genc.add(gen_enum_lookup(name, values, prefix))
> +        self._genh.preamble_add(gen_enum(name, values, prefix))
> +        self._genc.add(gen_enum_lookup(name, values, prefix))
>
>      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 += gen_type_cleanup_decl(name)
> -            if self._opt_builtins:
> -                self._genc.add(gen_type_cleanup(name))
> -        else:
> -            self._genh.preamble_add(gen_fwd_object_or_array(name))
> -            self._genh.add(gen_array(name, element_type))
> -            self._gen_type_cleanup(name)
> +        self._genh.preamble_add(gen_fwd_object_or_array(name))
> +        self._genh.add(gen_array(name, element_type))
> +        self._gen_type_cleanup(name)
>
>      def visit_object_type(self, name, info, base, members, variants):
>          # Nothing to do for the special empty builtin
> @@ -248,6 +235,6 @@ class QAPISchemaGenTypeVisitor(QAPISchemaMonolithicCVisitor):
>
>
>  def gen_types(schema, output_dir, prefix, opt_builtins):
> -    vis = QAPISchemaGenTypeVisitor(prefix, opt_builtins)
> +    vis = QAPISchemaGenTypeVisitor(prefix)
>      schema.visit(vis)
> -    vis.write(output_dir)
> +    vis.write(output_dir, opt_builtins)
> diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
> index 3d09d44265..9b678e7263 100644
> --- a/scripts/qapi/visit.py
> +++ b/scripts/qapi/visit.py
> @@ -263,13 +263,27 @@ out:
>                   c_name=c_name(name))
>
>
> -class QAPISchemaGenVisitVisitor(QAPISchemaMonolithicCVisitor):
> +class QAPISchemaGenVisitVisitor(QAPISchemaModularCVisitor):
>
> -    def __init__(self, prefix, opt_builtins):
> -        QAPISchemaMonolithicCVisitor.__init__(
> +    def __init__(self, prefix):
> +        QAPISchemaModularCVisitor.__init__(
>              self, prefix, 'qapi-visit', ' * Schema-defined QAPI visitors',
>              __doc__)
> -        self._opt_builtins = opt_builtins
> +        self._add_module(None, ' * Built-in QAPI visitors')
> +        self._genc.preamble_add(mcgen('''
> +#include "qemu/osdep.h"
> +#include "qemu-common.h"
> +#include "qapi/error.h"
> +#include "qapi-builtin-visit.h"
> +'''))
> +        self._genh.preamble_add(mcgen('''
> +#include "qapi/visitor.h"
> +#include "qapi-builtin-types.h"
> +
> +''',
> +                                      prefix=prefix))
> +
> +    def _begin_module(self, name):
>          self._genc.preamble_add(mcgen('''
>  #include "qemu/osdep.h"
>  #include "qemu-common.h"
> @@ -277,45 +291,21 @@ class QAPISchemaGenVisitVisitor(QAPISchemaMonolithicCVisitor):
>  #include "qapi/qmp/qerror.h"
>  #include "%(prefix)sqapi-visit.h"
>  ''',
> -                                      prefix=prefix))
> +                                      prefix=self._prefix))
>          self._genh.preamble_add(mcgen('''
> -#include "qapi/visitor.h"
> +#include "qapi-builtin-visit.h"
>  #include "%(prefix)sqapi-types.h"
>
>  ''',
> -                                      prefix=prefix))
> -        self._btin = guardstart('QAPI_VISIT_BUILTIN')
> -
> -    def visit_end(self):
> -        # To avoid header dependency hell, we always generate
> -        # declarations for built-in types in our header files and
> -        # simply guard them.  See also opt_builtins (command line
> -        # option -b).
> -        self._btin += guardend('QAPI_VISIT_BUILTIN')
> -        self._genh.preamble_add(self._btin)
> -        self._btin = None
> +                                      prefix=self._prefix))
>
>      def visit_enum_type(self, name, info, values, prefix):
> -        # Special case for our lone builtin enum type
> -        # TODO use something cleaner than existence of info
> -        if not info:
> -            self._btin += gen_visit_decl(name, scalar=True)
> -            if self._opt_builtins:
> -                self._genc.add(gen_visit_enum(name))
> -        else:
> -            self._genh.add(gen_visit_decl(name, scalar=True))
> -            self._genc.add(gen_visit_enum(name))
> +        self._genh.add(gen_visit_decl(name, scalar=True))
> +        self._genc.add(gen_visit_enum(name))
>
>      def visit_array_type(self, name, info, element_type):
> -        decl = gen_visit_decl(name)
> -        defn = gen_visit_list(name, element_type)
> -        if isinstance(element_type, QAPISchemaBuiltinType):
> -            self._btin += decl
> -            if self._opt_builtins:
> -                self._genc.add(defn)
> -        else:
> -            self._genh.add(decl)
> -            self._genc.add(defn)
> +        self._genh.add(gen_visit_decl(name))
> +        self._genc.add(gen_visit_list(name, element_type))
>
>      def visit_object_type(self, name, info, base, members, variants):
>          # Nothing to do for the special empty builtin
> @@ -336,6 +326,6 @@ class QAPISchemaGenVisitVisitor(QAPISchemaMonolithicCVisitor):
>
>
>  def gen_visit(schema, output_dir, prefix, opt_builtins):
> -    vis = QAPISchemaGenVisitVisitor(prefix, opt_builtins)
> +    vis = QAPISchemaGenVisitVisitor(prefix)
>      schema.visit(vis)
> -    vis.write(output_dir)
> +    vis.write(output_dir, opt_builtins)
> --
> 2.13.6
>
Michael Roth Feb. 19, 2018, 7:30 p.m. UTC | #3
Quoting Markus Armbruster (2018-02-11 03:35:58)
> Linking code from multiple separate QAPI schemata into the same
> program is possible, but involves some weirdness around built-in
> types:
> 
> * We generate code for built-in types into .c only with option
>   --builtins.  The user is responsible for generating code for exactly
>   one QAPI schema per program with --builtins.
> 
> * We generate code for built-in types into .h regardless of
>   --builtins, but guarded by #ifndef QAPI_VISIT_BUILTIN.  Because all
>   copies of this code are exactly the same, including any combination
>   of these headers works.
> 
> Replace this contraption by something more conventional: generate code
> for built-in types into their very own files: qapi-builtin-types.c,
> qapi-builtin-visit.c, qapi-builtin-types.h, qapi-builtin-visit.h, but
> only with --builtins.  Obey --output-dir, but ignore --prefix for
> them.
> 
> Make qapi-types.h include qapi-builtin-types.h.  With multiple
> schemata you now have multiple qapi-types.[ch], but only one
> qapi-builtin-types.[ch].  Same for qapi-visit.[ch] and
> qapi-builtin-visit.[ch].
> 
> Bonus: if all you need is built-in stuff, you can include a much
> smaller header.  To be exploited shortly.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>

> ---
>  .gitignore             |  2 ++
>  Makefile               | 13 ++++++----
>  Makefile.objs          |  2 ++
>  scripts/qapi/common.py | 61 ++++++++++++++++++++++++++++++++++++++++-------
>  scripts/qapi/types.py  | 61 +++++++++++++++++++----------------------------
>  scripts/qapi/visit.py  | 64 +++++++++++++++++++++-----------------------------
>  6 files changed, 116 insertions(+), 87 deletions(-)
> 
> diff --git a/.gitignore b/.gitignore
> index 7d783e6e66..9477a08b6b 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -29,6 +29,8 @@
>  /qga/qapi-generated
>  /qapi-generated
>  /qapi-gen-timestamp
> +/qapi-builtin-types.[ch]
> +/qapi-builtin-visit.[ch]
>  /qapi-types.[ch]
>  /qapi-visit.[ch]
>  /qapi-event.[ch]
> diff --git a/Makefile b/Makefile
> index 164a38578e..60ddc9c945 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -90,10 +90,13 @@ endif
>  include $(SRC_PATH)/rules.mak
> 
>  GENERATED_FILES = qemu-version.h config-host.h qemu-options.def
> -GENERATED_FILES += qmp-commands.h qapi-types.h qapi-visit.h qapi-event.h
> -GENERATED_FILES += qmp-commands.c qapi-types.c qapi-visit.c qapi-event.c
> -GENERATED_FILES += qmp-introspect.h
> -GENERATED_FILES += qmp-introspect.c
> +GENERATED_FILES += qapi-builtin-types.h qapi-builtin-types.c
> +GENERATED_FILES += qapi-types.h qapi-types.c
> +GENERATED_FILES += qapi-builtin-visit.h qapi-builtin-visit.c
> +GENERATED_FILES += qapi-visit.h qapi-visit.c
> +GENERATED_FILES += qmp-commands.h qmp-commands.c
> +GENERATED_FILES += qapi-event.h qapi-event.c
> +GENERATED_FILES += qmp-introspect.c qmp-introspect.h
>  GENERATED_FILES += qapi-doc.texi
> 
>  GENERATED_FILES += trace/generated-tcg-tracers.h
> @@ -520,7 +523,9 @@ qapi-modules = $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/qapi/common.json \
>                 $(SRC_PATH)/qapi/transaction.json \
>                 $(SRC_PATH)/qapi/ui.json
> 
> +qapi-builtin-types.c qapi-builtin-types.h \
>  qapi-types.c qapi-types.h \
> +qapi-builtin-visit.c qapi-builtin-visit.h \
>  qapi-visit.c qapi-visit.h \
>  qmp-commands.h qmp-commands.c \
>  qapi-event.c qapi-event.h \
> diff --git a/Makefile.objs b/Makefile.objs
> index d255aaf194..2813e984fd 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -2,6 +2,8 @@
>  # Common libraries for tools and emulators
>  stub-obj-y = stubs/ crypto/
>  util-obj-y = util/ qobject/ qapi/
> +util-obj-y += qapi-builtin-types.o
> +util-obj-y += qapi-builtin-visit.o
>  util-obj-y += qmp-introspect.o qapi-types.o qapi-visit.o qapi-event.o
> 
>  chardev-obj-y = chardev/
> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
> index 31d2f73e7e..de12f8469a 100644
> --- a/scripts/qapi/common.py
> +++ b/scripts/qapi/common.py
> @@ -1531,11 +1531,10 @@ class QAPISchema(object):
> 
>      def _def_builtin_type(self, name, json_type, c_type):
>          self._def_entity(QAPISchemaBuiltinType(name, json_type, c_type))
> -        # TODO As long as we have QAPI_TYPES_BUILTIN to share multiple
> -        # qapi-types.h from a single .c, all arrays of builtins must be
> -        # declared in the first file whether or not they are used.  Nicer
> -        # would be to use lazy instantiation, while figuring out how to
> -        # avoid compilation issues with multiple qapi-types.h.
> +        # Instantiating only the arrays that are actually used would
> +        # be nice, but we can't as long as their generated code
> +        # (qapi-builtin-types.[ch]) may be shared by some other
> +        # schema.
>          self._make_array_type(name, None)
> 
>      def _def_predefineds(self):
> @@ -1992,14 +1991,15 @@ class QAPIGen(object):
>          return ''
> 
>      def write(self, output_dir, fname):
> -        if output_dir:
> +        pathname = os.path.join(output_dir, fname)
> +        dir = os.path.dirname(pathname)
> +        if dir:
>              try:
> -                os.makedirs(output_dir)
> +                os.makedirs(dir)
>              except os.error as e:
>                  if e.errno != errno.EEXIST:
>                      raise
> -        fd = os.open(os.path.join(output_dir, fname),
> -                     os.O_RDWR | os.O_CREAT, 0666)
> +        fd = os.open(pathname, os.O_RDWR | os.O_CREAT, 0666)
>          f = os.fdopen(fd, 'r+')
>          text = (self._top(fname) + self._preamble + self._body
>                  + self._bottom(fname))
> @@ -2046,6 +2046,7 @@ class QAPIGenH(QAPIGenC):
> 
> 
>  class QAPIGenDoc(QAPIGen):
> +
>      def _top(self, fname):
>          return (QAPIGen._top(self, fname)
>                  + '@c AUTOMATICALLY GENERATED, DO NOT MODIFY\n\n')
> @@ -2062,3 +2063,45 @@ class QAPISchemaMonolithicCVisitor(QAPISchemaVisitor):
>      def write(self, output_dir):
>          self._genc.write(output_dir, self._prefix + self._what + '.c')
>          self._genh.write(output_dir, self._prefix + self._what + '.h')
> +
> +
> +class QAPISchemaModularCVisitor(QAPISchemaVisitor):
> +
> +    def __init__(self, prefix, what, blurb, pydoc):
> +        self._prefix = prefix
> +        self._what = what
> +        self._blurb = blurb
> +        self._pydoc = pydoc
> +        self._module = {}
> +
> +    def _module_basename(self, what, name):
> +        if name is None:
> +            return re.sub(r'-', '-builtin-', what)
> +        return self._prefix + what
> +
> +    def _add_module(self, name, blurb):
> +        genc = QAPIGenC(blurb, self._pydoc)
> +        genh = QAPIGenH(blurb, self._pydoc)
> +        self._module[name] = (genc, genh)
> +        self._set_module(name)
> +
> +    def _set_module(self, name):
> +        self._genc, self._genh = self._module[name]
> +
> +    def write(self, output_dir, opt_builtins):
> +        for name in self._module:
> +            if name is None and not opt_builtins:
> +                continue
> +            basename = self._module_basename(self._what, name)
> +            (genc, genh) = self._module[name]
> +            genc.write(output_dir, basename + '.c')
> +            genh.write(output_dir, basename + '.h')
> +
> +    def _begin_module(self, name):
> +        pass
> +
> +    def visit_module(self, name):
> +        if len(self._module) != 1:
> +            return
> +        self._add_module(name, self._blurb)
> +        self._begin_module(name)
> diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
> index d2b8423479..59826b1162 100644
> --- a/scripts/qapi/types.py
> +++ b/scripts/qapi/types.py
> @@ -167,64 +167,51 @@ void qapi_free_%(c_name)s(%(c_name)s *obj)
>      return ret
> 
> 
> -class QAPISchemaGenTypeVisitor(QAPISchemaMonolithicCVisitor):
> +class QAPISchemaGenTypeVisitor(QAPISchemaModularCVisitor):
> 
> -    def __init__(self, prefix, opt_builtins):
> -        QAPISchemaMonolithicCVisitor.__init__(
> +    def __init__(self, prefix):
> +        QAPISchemaModularCVisitor.__init__(
>              self, prefix, 'qapi-types', ' * Schema-defined QAPI types',
>              __doc__)
> -        self._opt_builtins = opt_builtins
> +        self._add_module(None, ' * Built-in QAPI types')
> +        self._genc.preamble_add(mcgen('''
> +#include "qemu/osdep.h"
> +#include "qapi/dealloc-visitor.h"
> +#include "qapi-builtin-types.h"
> +#include "qapi-builtin-visit.h"
> +'''))
> +        self._genh.preamble_add(mcgen('''
> +#include "qapi/util.h"
> +'''))
> +
> +    def _begin_module(self, name):
>          self._genc.preamble_add(mcgen('''
>  #include "qemu/osdep.h"
>  #include "qapi/dealloc-visitor.h"
>  #include "%(prefix)sqapi-types.h"
>  #include "%(prefix)sqapi-visit.h"
>  ''',
> -                                      prefix=prefix))
> +                                      prefix=self._prefix))
>          self._genh.preamble_add(mcgen('''
> -#include "qapi/util.h"
> +#include "qapi-builtin-types.h"
>  '''))
> -        self._btin = '\n' + guardstart('QAPI_TYPES_BUILTIN')
> 
>      def visit_begin(self, schema):
>          # gen_object() is recursive, ensure it doesn't visit the empty type
>          objects_seen.add(schema.the_empty_object_type.name)
> 
> -    def visit_end(self):
> -        # To avoid header dependency hell, we always generate
> -        # declarations for built-in types in our header files and
> -        # simply guard them.  See also opt_builtins (command line
> -        # option -b).
> -        self._btin += guardend('QAPI_TYPES_BUILTIN')
> -        self._genh.preamble_add(self._btin)
> -        self._btin = None
> -
>      def _gen_type_cleanup(self, name):
>          self._genh.add(gen_type_cleanup_decl(name))
>          self._genc.add(gen_type_cleanup(name))
> 
>      def visit_enum_type(self, name, info, values, prefix):
> -        # Special case for our lone builtin enum type
> -        # TODO use something cleaner than existence of info
> -        if not info:
> -            self._btin += gen_enum(name, values, prefix)
> -            if self._opt_builtins:
> -                self._genc.add(gen_enum_lookup(name, values, prefix))
> -        else:
> -            self._genh.preamble_add(gen_enum(name, values, prefix))
> -            self._genc.add(gen_enum_lookup(name, values, prefix))
> +        self._genh.preamble_add(gen_enum(name, values, prefix))
> +        self._genc.add(gen_enum_lookup(name, values, prefix))
> 
>      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 += gen_type_cleanup_decl(name)
> -            if self._opt_builtins:
> -                self._genc.add(gen_type_cleanup(name))
> -        else:
> -            self._genh.preamble_add(gen_fwd_object_or_array(name))
> -            self._genh.add(gen_array(name, element_type))
> -            self._gen_type_cleanup(name)
> +        self._genh.preamble_add(gen_fwd_object_or_array(name))
> +        self._genh.add(gen_array(name, element_type))
> +        self._gen_type_cleanup(name)
> 
>      def visit_object_type(self, name, info, base, members, variants):
>          # Nothing to do for the special empty builtin
> @@ -248,6 +235,6 @@ class QAPISchemaGenTypeVisitor(QAPISchemaMonolithicCVisitor):
> 
> 
>  def gen_types(schema, output_dir, prefix, opt_builtins):
> -    vis = QAPISchemaGenTypeVisitor(prefix, opt_builtins)
> +    vis = QAPISchemaGenTypeVisitor(prefix)
>      schema.visit(vis)
> -    vis.write(output_dir)
> +    vis.write(output_dir, opt_builtins)
> diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
> index 3d09d44265..9b678e7263 100644
> --- a/scripts/qapi/visit.py
> +++ b/scripts/qapi/visit.py
> @@ -263,13 +263,27 @@ out:
>                   c_name=c_name(name))
> 
> 
> -class QAPISchemaGenVisitVisitor(QAPISchemaMonolithicCVisitor):
> +class QAPISchemaGenVisitVisitor(QAPISchemaModularCVisitor):
> 
> -    def __init__(self, prefix, opt_builtins):
> -        QAPISchemaMonolithicCVisitor.__init__(
> +    def __init__(self, prefix):
> +        QAPISchemaModularCVisitor.__init__(
>              self, prefix, 'qapi-visit', ' * Schema-defined QAPI visitors',
>              __doc__)
> -        self._opt_builtins = opt_builtins
> +        self._add_module(None, ' * Built-in QAPI visitors')
> +        self._genc.preamble_add(mcgen('''
> +#include "qemu/osdep.h"
> +#include "qemu-common.h"
> +#include "qapi/error.h"
> +#include "qapi-builtin-visit.h"
> +'''))
> +        self._genh.preamble_add(mcgen('''
> +#include "qapi/visitor.h"
> +#include "qapi-builtin-types.h"
> +
> +''',
> +                                      prefix=prefix))
> +
> +    def _begin_module(self, name):
>          self._genc.preamble_add(mcgen('''
>  #include "qemu/osdep.h"
>  #include "qemu-common.h"
> @@ -277,45 +291,21 @@ class QAPISchemaGenVisitVisitor(QAPISchemaMonolithicCVisitor):
>  #include "qapi/qmp/qerror.h"
>  #include "%(prefix)sqapi-visit.h"
>  ''',
> -                                      prefix=prefix))
> +                                      prefix=self._prefix))
>          self._genh.preamble_add(mcgen('''
> -#include "qapi/visitor.h"
> +#include "qapi-builtin-visit.h"
>  #include "%(prefix)sqapi-types.h"
> 
>  ''',
> -                                      prefix=prefix))
> -        self._btin = guardstart('QAPI_VISIT_BUILTIN')
> -
> -    def visit_end(self):
> -        # To avoid header dependency hell, we always generate
> -        # declarations for built-in types in our header files and
> -        # simply guard them.  See also opt_builtins (command line
> -        # option -b).
> -        self._btin += guardend('QAPI_VISIT_BUILTIN')
> -        self._genh.preamble_add(self._btin)
> -        self._btin = None
> +                                      prefix=self._prefix))
> 
>      def visit_enum_type(self, name, info, values, prefix):
> -        # Special case for our lone builtin enum type
> -        # TODO use something cleaner than existence of info
> -        if not info:
> -            self._btin += gen_visit_decl(name, scalar=True)
> -            if self._opt_builtins:
> -                self._genc.add(gen_visit_enum(name))
> -        else:
> -            self._genh.add(gen_visit_decl(name, scalar=True))
> -            self._genc.add(gen_visit_enum(name))
> +        self._genh.add(gen_visit_decl(name, scalar=True))
> +        self._genc.add(gen_visit_enum(name))
> 
>      def visit_array_type(self, name, info, element_type):
> -        decl = gen_visit_decl(name)
> -        defn = gen_visit_list(name, element_type)
> -        if isinstance(element_type, QAPISchemaBuiltinType):
> -            self._btin += decl
> -            if self._opt_builtins:
> -                self._genc.add(defn)
> -        else:
> -            self._genh.add(decl)
> -            self._genc.add(defn)
> +        self._genh.add(gen_visit_decl(name))
> +        self._genc.add(gen_visit_list(name, element_type))
> 
>      def visit_object_type(self, name, info, base, members, variants):
>          # Nothing to do for the special empty builtin
> @@ -336,6 +326,6 @@ class QAPISchemaGenVisitVisitor(QAPISchemaMonolithicCVisitor):
> 
> 
>  def gen_visit(schema, output_dir, prefix, opt_builtins):
> -    vis = QAPISchemaGenVisitVisitor(prefix, opt_builtins)
> +    vis = QAPISchemaGenVisitVisitor(prefix)
>      schema.visit(vis)
> -    vis.write(output_dir)
> +    vis.write(output_dir, opt_builtins)
> -- 
> 2.13.6
>
Markus Armbruster Feb. 23, 2018, 5:36 p.m. UTC | #4
Eric Blake <eblake@redhat.com> writes:

> On 02/11/2018 03:35 AM, Markus Armbruster wrote:
>> Linking code from multiple separate QAPI schemata into the same
>> program is possible, but involves some weirdness around built-in
>> types:
>>
>> * We generate code for built-in types into .c only with option
>>    --builtins.  The user is responsible for generating code for exactly
>>    one QAPI schema per program with --builtins.
>>
>> * We generate code for built-in types into .h regardless of
>>    --builtins, but guarded by #ifndef QAPI_VISIT_BUILTIN.  Because all
>>    copies of this code are exactly the same, including any combination
>>    of these headers works.
>>
>> Replace this contraption by something more conventional: generate code
>> for built-in types into their very own files: qapi-builtin-types.c,
>> qapi-builtin-visit.c, qapi-builtin-types.h, qapi-builtin-visit.h, but
>> only with --builtins.  Obey --output-dir, but ignore --prefix for
>> them.
>>
>> Make qapi-types.h include qapi-builtin-types.h.  With multiple
>> schemata you now have multiple qapi-types.[ch], but only one
>> qapi-builtin-types.[ch].  Same for qapi-visit.[ch] and
>> qapi-builtin-visit.[ch].
>>
>> Bonus: if all you need is built-in stuff, you can include a much
>> smaller header.  To be exploited shortly.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>> @@ -2046,6 +2046,7 @@ class QAPIGenH(QAPIGenC):
>>       class QAPIGenDoc(QAPIGen):
>> +
>>       def _top(self, fname):
>>           return (QAPIGen._top(self, fname)
>>                   + '@c AUTOMATICALLY GENERATED, DO NOT MODIFY\n\n')
>
> Does this hunk belong in an earlier patch?

Yes: PATCH 05.

> Otherwise,
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!
diff mbox

Patch

diff --git a/.gitignore b/.gitignore
index 7d783e6e66..9477a08b6b 100644
--- a/.gitignore
+++ b/.gitignore
@@ -29,6 +29,8 @@ 
 /qga/qapi-generated
 /qapi-generated
 /qapi-gen-timestamp
+/qapi-builtin-types.[ch]
+/qapi-builtin-visit.[ch]
 /qapi-types.[ch]
 /qapi-visit.[ch]
 /qapi-event.[ch]
diff --git a/Makefile b/Makefile
index 164a38578e..60ddc9c945 100644
--- a/Makefile
+++ b/Makefile
@@ -90,10 +90,13 @@  endif
 include $(SRC_PATH)/rules.mak
 
 GENERATED_FILES = qemu-version.h config-host.h qemu-options.def
-GENERATED_FILES += qmp-commands.h qapi-types.h qapi-visit.h qapi-event.h
-GENERATED_FILES += qmp-commands.c qapi-types.c qapi-visit.c qapi-event.c
-GENERATED_FILES += qmp-introspect.h
-GENERATED_FILES += qmp-introspect.c
+GENERATED_FILES += qapi-builtin-types.h qapi-builtin-types.c
+GENERATED_FILES += qapi-types.h qapi-types.c
+GENERATED_FILES += qapi-builtin-visit.h qapi-builtin-visit.c
+GENERATED_FILES += qapi-visit.h qapi-visit.c
+GENERATED_FILES += qmp-commands.h qmp-commands.c
+GENERATED_FILES += qapi-event.h qapi-event.c
+GENERATED_FILES += qmp-introspect.c qmp-introspect.h
 GENERATED_FILES += qapi-doc.texi
 
 GENERATED_FILES += trace/generated-tcg-tracers.h
@@ -520,7 +523,9 @@  qapi-modules = $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/qapi/common.json \
                $(SRC_PATH)/qapi/transaction.json \
                $(SRC_PATH)/qapi/ui.json
 
+qapi-builtin-types.c qapi-builtin-types.h \
 qapi-types.c qapi-types.h \
+qapi-builtin-visit.c qapi-builtin-visit.h \
 qapi-visit.c qapi-visit.h \
 qmp-commands.h qmp-commands.c \
 qapi-event.c qapi-event.h \
diff --git a/Makefile.objs b/Makefile.objs
index d255aaf194..2813e984fd 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -2,6 +2,8 @@ 
 # Common libraries for tools and emulators
 stub-obj-y = stubs/ crypto/
 util-obj-y = util/ qobject/ qapi/
+util-obj-y += qapi-builtin-types.o
+util-obj-y += qapi-builtin-visit.o
 util-obj-y += qmp-introspect.o qapi-types.o qapi-visit.o qapi-event.o
 
 chardev-obj-y = chardev/
diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
index 31d2f73e7e..de12f8469a 100644
--- a/scripts/qapi/common.py
+++ b/scripts/qapi/common.py
@@ -1531,11 +1531,10 @@  class QAPISchema(object):
 
     def _def_builtin_type(self, name, json_type, c_type):
         self._def_entity(QAPISchemaBuiltinType(name, json_type, c_type))
-        # TODO As long as we have QAPI_TYPES_BUILTIN to share multiple
-        # qapi-types.h from a single .c, all arrays of builtins must be
-        # declared in the first file whether or not they are used.  Nicer
-        # would be to use lazy instantiation, while figuring out how to
-        # avoid compilation issues with multiple qapi-types.h.
+        # Instantiating only the arrays that are actually used would
+        # be nice, but we can't as long as their generated code
+        # (qapi-builtin-types.[ch]) may be shared by some other
+        # schema.
         self._make_array_type(name, None)
 
     def _def_predefineds(self):
@@ -1992,14 +1991,15 @@  class QAPIGen(object):
         return ''
 
     def write(self, output_dir, fname):
-        if output_dir:
+        pathname = os.path.join(output_dir, fname)
+        dir = os.path.dirname(pathname)
+        if dir:
             try:
-                os.makedirs(output_dir)
+                os.makedirs(dir)
             except os.error as e:
                 if e.errno != errno.EEXIST:
                     raise
-        fd = os.open(os.path.join(output_dir, fname),
-                     os.O_RDWR | os.O_CREAT, 0666)
+        fd = os.open(pathname, os.O_RDWR | os.O_CREAT, 0666)
         f = os.fdopen(fd, 'r+')
         text = (self._top(fname) + self._preamble + self._body
                 + self._bottom(fname))
@@ -2046,6 +2046,7 @@  class QAPIGenH(QAPIGenC):
 
 
 class QAPIGenDoc(QAPIGen):
+
     def _top(self, fname):
         return (QAPIGen._top(self, fname)
                 + '@c AUTOMATICALLY GENERATED, DO NOT MODIFY\n\n')
@@ -2062,3 +2063,45 @@  class QAPISchemaMonolithicCVisitor(QAPISchemaVisitor):
     def write(self, output_dir):
         self._genc.write(output_dir, self._prefix + self._what + '.c')
         self._genh.write(output_dir, self._prefix + self._what + '.h')
+
+
+class QAPISchemaModularCVisitor(QAPISchemaVisitor):
+
+    def __init__(self, prefix, what, blurb, pydoc):
+        self._prefix = prefix
+        self._what = what
+        self._blurb = blurb
+        self._pydoc = pydoc
+        self._module = {}
+
+    def _module_basename(self, what, name):
+        if name is None:
+            return re.sub(r'-', '-builtin-', what)
+        return self._prefix + what
+
+    def _add_module(self, name, blurb):
+        genc = QAPIGenC(blurb, self._pydoc)
+        genh = QAPIGenH(blurb, self._pydoc)
+        self._module[name] = (genc, genh)
+        self._set_module(name)
+
+    def _set_module(self, name):
+        self._genc, self._genh = self._module[name]
+
+    def write(self, output_dir, opt_builtins):
+        for name in self._module:
+            if name is None and not opt_builtins:
+                continue
+            basename = self._module_basename(self._what, name)
+            (genc, genh) = self._module[name]
+            genc.write(output_dir, basename + '.c')
+            genh.write(output_dir, basename + '.h')
+
+    def _begin_module(self, name):
+        pass
+
+    def visit_module(self, name):
+        if len(self._module) != 1:
+            return
+        self._add_module(name, self._blurb)
+        self._begin_module(name)
diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
index d2b8423479..59826b1162 100644
--- a/scripts/qapi/types.py
+++ b/scripts/qapi/types.py
@@ -167,64 +167,51 @@  void qapi_free_%(c_name)s(%(c_name)s *obj)
     return ret
 
 
-class QAPISchemaGenTypeVisitor(QAPISchemaMonolithicCVisitor):
+class QAPISchemaGenTypeVisitor(QAPISchemaModularCVisitor):
 
-    def __init__(self, prefix, opt_builtins):
-        QAPISchemaMonolithicCVisitor.__init__(
+    def __init__(self, prefix):
+        QAPISchemaModularCVisitor.__init__(
             self, prefix, 'qapi-types', ' * Schema-defined QAPI types',
             __doc__)
-        self._opt_builtins = opt_builtins
+        self._add_module(None, ' * Built-in QAPI types')
+        self._genc.preamble_add(mcgen('''
+#include "qemu/osdep.h"
+#include "qapi/dealloc-visitor.h"
+#include "qapi-builtin-types.h"
+#include "qapi-builtin-visit.h"
+'''))
+        self._genh.preamble_add(mcgen('''
+#include "qapi/util.h"
+'''))
+
+    def _begin_module(self, name):
         self._genc.preamble_add(mcgen('''
 #include "qemu/osdep.h"
 #include "qapi/dealloc-visitor.h"
 #include "%(prefix)sqapi-types.h"
 #include "%(prefix)sqapi-visit.h"
 ''',
-                                      prefix=prefix))
+                                      prefix=self._prefix))
         self._genh.preamble_add(mcgen('''
-#include "qapi/util.h"
+#include "qapi-builtin-types.h"
 '''))
-        self._btin = '\n' + guardstart('QAPI_TYPES_BUILTIN')
 
     def visit_begin(self, schema):
         # gen_object() is recursive, ensure it doesn't visit the empty type
         objects_seen.add(schema.the_empty_object_type.name)
 
-    def visit_end(self):
-        # To avoid header dependency hell, we always generate
-        # declarations for built-in types in our header files and
-        # simply guard them.  See also opt_builtins (command line
-        # option -b).
-        self._btin += guardend('QAPI_TYPES_BUILTIN')
-        self._genh.preamble_add(self._btin)
-        self._btin = None
-
     def _gen_type_cleanup(self, name):
         self._genh.add(gen_type_cleanup_decl(name))
         self._genc.add(gen_type_cleanup(name))
 
     def visit_enum_type(self, name, info, values, prefix):
-        # Special case for our lone builtin enum type
-        # TODO use something cleaner than existence of info
-        if not info:
-            self._btin += gen_enum(name, values, prefix)
-            if self._opt_builtins:
-                self._genc.add(gen_enum_lookup(name, values, prefix))
-        else:
-            self._genh.preamble_add(gen_enum(name, values, prefix))
-            self._genc.add(gen_enum_lookup(name, values, prefix))
+        self._genh.preamble_add(gen_enum(name, values, prefix))
+        self._genc.add(gen_enum_lookup(name, values, prefix))
 
     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 += gen_type_cleanup_decl(name)
-            if self._opt_builtins:
-                self._genc.add(gen_type_cleanup(name))
-        else:
-            self._genh.preamble_add(gen_fwd_object_or_array(name))
-            self._genh.add(gen_array(name, element_type))
-            self._gen_type_cleanup(name)
+        self._genh.preamble_add(gen_fwd_object_or_array(name))
+        self._genh.add(gen_array(name, element_type))
+        self._gen_type_cleanup(name)
 
     def visit_object_type(self, name, info, base, members, variants):
         # Nothing to do for the special empty builtin
@@ -248,6 +235,6 @@  class QAPISchemaGenTypeVisitor(QAPISchemaMonolithicCVisitor):
 
 
 def gen_types(schema, output_dir, prefix, opt_builtins):
-    vis = QAPISchemaGenTypeVisitor(prefix, opt_builtins)
+    vis = QAPISchemaGenTypeVisitor(prefix)
     schema.visit(vis)
-    vis.write(output_dir)
+    vis.write(output_dir, opt_builtins)
diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
index 3d09d44265..9b678e7263 100644
--- a/scripts/qapi/visit.py
+++ b/scripts/qapi/visit.py
@@ -263,13 +263,27 @@  out:
                  c_name=c_name(name))
 
 
-class QAPISchemaGenVisitVisitor(QAPISchemaMonolithicCVisitor):
+class QAPISchemaGenVisitVisitor(QAPISchemaModularCVisitor):
 
-    def __init__(self, prefix, opt_builtins):
-        QAPISchemaMonolithicCVisitor.__init__(
+    def __init__(self, prefix):
+        QAPISchemaModularCVisitor.__init__(
             self, prefix, 'qapi-visit', ' * Schema-defined QAPI visitors',
             __doc__)
-        self._opt_builtins = opt_builtins
+        self._add_module(None, ' * Built-in QAPI visitors')
+        self._genc.preamble_add(mcgen('''
+#include "qemu/osdep.h"
+#include "qemu-common.h"
+#include "qapi/error.h"
+#include "qapi-builtin-visit.h"
+'''))
+        self._genh.preamble_add(mcgen('''
+#include "qapi/visitor.h"
+#include "qapi-builtin-types.h"
+
+''',
+                                      prefix=prefix))
+
+    def _begin_module(self, name):
         self._genc.preamble_add(mcgen('''
 #include "qemu/osdep.h"
 #include "qemu-common.h"
@@ -277,45 +291,21 @@  class QAPISchemaGenVisitVisitor(QAPISchemaMonolithicCVisitor):
 #include "qapi/qmp/qerror.h"
 #include "%(prefix)sqapi-visit.h"
 ''',
-                                      prefix=prefix))
+                                      prefix=self._prefix))
         self._genh.preamble_add(mcgen('''
-#include "qapi/visitor.h"
+#include "qapi-builtin-visit.h"
 #include "%(prefix)sqapi-types.h"
 
 ''',
-                                      prefix=prefix))
-        self._btin = guardstart('QAPI_VISIT_BUILTIN')
-
-    def visit_end(self):
-        # To avoid header dependency hell, we always generate
-        # declarations for built-in types in our header files and
-        # simply guard them.  See also opt_builtins (command line
-        # option -b).
-        self._btin += guardend('QAPI_VISIT_BUILTIN')
-        self._genh.preamble_add(self._btin)
-        self._btin = None
+                                      prefix=self._prefix))
 
     def visit_enum_type(self, name, info, values, prefix):
-        # Special case for our lone builtin enum type
-        # TODO use something cleaner than existence of info
-        if not info:
-            self._btin += gen_visit_decl(name, scalar=True)
-            if self._opt_builtins:
-                self._genc.add(gen_visit_enum(name))
-        else:
-            self._genh.add(gen_visit_decl(name, scalar=True))
-            self._genc.add(gen_visit_enum(name))
+        self._genh.add(gen_visit_decl(name, scalar=True))
+        self._genc.add(gen_visit_enum(name))
 
     def visit_array_type(self, name, info, element_type):
-        decl = gen_visit_decl(name)
-        defn = gen_visit_list(name, element_type)
-        if isinstance(element_type, QAPISchemaBuiltinType):
-            self._btin += decl
-            if self._opt_builtins:
-                self._genc.add(defn)
-        else:
-            self._genh.add(decl)
-            self._genc.add(defn)
+        self._genh.add(gen_visit_decl(name))
+        self._genc.add(gen_visit_list(name, element_type))
 
     def visit_object_type(self, name, info, base, members, variants):
         # Nothing to do for the special empty builtin
@@ -336,6 +326,6 @@  class QAPISchemaGenVisitVisitor(QAPISchemaMonolithicCVisitor):
 
 
 def gen_visit(schema, output_dir, prefix, opt_builtins):
-    vis = QAPISchemaGenVisitVisitor(prefix, opt_builtins)
+    vis = QAPISchemaGenVisitVisitor(prefix)
     schema.visit(vis)
-    vis.write(output_dir)
+    vis.write(output_dir, opt_builtins)