Re: [Qemu-devel] [PATCH v5 05/14] qapi: Emit implicit structs in generated C

[Markus Armbruster <armbru@redhat.com>]

Eric Blake <eblake@redhat.com> writes:

> We already have several places that want to visit all the members
> of an implicit object within a larger context (simple union variant,
> event with anonymous data, command with anonymous arguments struct);
> and will be adding another one soon (the ability to declare an
> anonymous base for a flat union).  Having a C struct declared for
> these implicit types, along with a visit_type_FOO_members() helper
> function, will make for fewer special cases in our generator.
>
> We do not, however, need qapi_free_FOO() or visit_type_FOO()
> functions for implicit types, because they should not be used
> directly outside of the generated code.  This is done by adding a
> conditional in visit_object_type() for both qapi-types.py and
> qapi-visit.py based on the object name.  The comparison of
> "name.startswith('q_')" is a bit hacky (it's basically duplicating
> what .is_implicit() already uses), but beats changing the signature
> of the visit_object_type() callback to pass a new 'implicit' flag.
> The hack should be temporary: we are considering adding a future
> patch that consolidates the narrow visit_object_type() and
> visit_object_type_flat() [with different pieces already broken out]
> into a broader visit_object_type() [where the visitor can query the
> type object directly].
>
> Also, now that we WANT to output C code for implicits, we have to
> change the condition in the visit_needed() filter.  We have to
> special-case 'q_empty' in that filter as something that does not
> get output: because it is a built-in type, it would be emitted in
> multiple files (the main qapi-types.h and in qga-qapi-types.h)
> causing compilation failure due to redefinition.  But since it
> has no members, it's easier to just avoid an attempt to visit
> that particular type.

Not just easier.  ':empty' is a genuinely internal thing so far (it
exists only to make qapi-introspect.py simpler), and generating C for it
would be weird.

> The patch relies on the changed naming of implicit types in the
> previous patch.  It is a bit unfortunate that the generated struct
> names and visit_type_FOO_members() don't match normal naming
> conventions, but it's not too bad, since they will only be used in
> generated code.
>
> The generated code grows substantially in size: the implicit
> '-wrapper' types must be emitted in qapi-types.h before any union
> can include an unboxed member of that type.  Arguably, the '-args'
> types could be emitted in a private header for just qapi-visit.c
> and qmp-marshal.c, rather than polluting qapi-types.h; but adding
> complexity to the generator to split the output location according
> to role doesn't seem worth the maintenance costs.  Another idea
> for a later patch would be reworking error.h to not need to
> include all of qapi-types.h.

I got that in my tree.  If it goes in before this patch, the last
sentence should be dropped.

> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> v5: rebase onto different naming scheme, document future improvements
> v4: new patch
> ---
>  scripts/qapi.py       |  2 --
>  scripts/qapi-types.py | 14 ++++++++------
>  scripts/qapi-visit.py | 20 ++++++++++----------
>  3 files changed, 18 insertions(+), 18 deletions(-)
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index f6701f5..96fb216 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -1000,7 +1000,6 @@ class QAPISchemaObjectType(QAPISchemaType):
>          return self.name.startswith('q_')
>
>      def c_name(self):
> -        assert not self.is_implicit()
>          return QAPISchemaType.c_name(self)
>
>      def c_type(self):
> @@ -1008,7 +1007,6 @@ class QAPISchemaObjectType(QAPISchemaType):
>          return c_name(self.name) + pointer_suffix
>
>      def c_unboxed_type(self):
> -        assert not self.is_implicit()
>          return c_name(self.name)
>
>      def json_type(self):
> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> index f194bea..107eabe 100644
> --- a/scripts/qapi-types.py
> +++ b/scripts/qapi-types.py
> @@ -61,8 +61,7 @@ def gen_object(name, base, members, variants):
>      ret = ''
>      if variants:
>          for v in variants.variants:
> -            if (isinstance(v.type, QAPISchemaObjectType) and
> -                    not v.type.is_implicit()):
> +            if isinstance(v.type, QAPISchemaObjectType):
>                  ret += gen_object(v.type.name, v.type.base,
>                                    v.type.local_members, v.type.variants)
>
> @@ -197,9 +196,8 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
>          self._btin = None
>
>      def visit_needed(self, entity):
> -        # Visit everything except implicit objects
> -        return not (entity.is_implicit() and
> -                    isinstance(entity, QAPISchemaObjectType))
> +        # Visit everything except the special empty builtin
> +        return entity.name != 'q_empty'
>
>      def _gen_type_cleanup(self, name):
>          self.decl += gen_type_cleanup_decl(name)

Before: we visit all types except for implicit object types in "def
before use" order.  visit_needed() filters out implicit object types.
gen_object() sorts by recursing, but filters out non-object types and
implicit object types.  Note the shared "filters out implicit object
types" part.

After: we visit all types escept for the empty type in "def before use"
order.  visit_needed() filters out the empty type.  gen_object filters
out non-object types.  It lost its shared part.  If we somehow recurse
into the empty type, we visit it.  Oops.

Actually, we overcomplicated things.  visit_needed() exists so we can
filter out different kinds of entities in one condition.  Really
convenient in qapi-introspect.py.

But if you want to filter out just one kind (here: object types), like
we do here, it's simpler to filter in that kind's visit_ method (here:
visit_object_type().  More efficient, too, but that doesn't matter.

Since we recurse, we need to additionally filter there.  An easy way to
do that would be to start with objects_seen set to
set(schema.the_empty_object_type) instead of set().

> @@ -233,7 +231,11 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
>          self.decl += gen_object(name, base, members, variants)
>          if base:
>              self.decl += gen_upcast(name, base)
> -        self._gen_type_cleanup(name)
> +        # FIXME Worth changing the visitor signature, so we could
> +        # directly use rather than repeat type.is_implicit()?

Nitpick: this is a TODO, because it's not about fixing something that's
broken.

> +        if not name.startswith('q_'):
> +            # implicit types won't be directly allocated/freed
> +            self._gen_type_cleanup(name)
>
>      def visit_alternate_type(self, name, info, variants):
>          self._fwdecl += gen_fwd_object_or_array(name)
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index a712e9a..c486aaa 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -215,13 +215,11 @@ out:
>
>
>  def gen_visit_object(name, base, members, variants):
> -    ret = gen_visit_object_members(name, base, members, variants)
> -

See visit_object_type() below.

>      # FIXME: if *obj is NULL on entry, and visit_start_struct() assigns to
>      # *obj, but then visit_type_FOO_members() fails, we should clean up *obj
>      # rather than leaving it non-NULL. As currently written, the caller must
>      # call qapi_free_FOO() to avoid a memory leak of the partial FOO.
> -    ret += mcgen('''
> +    return mcgen('''
>
>  void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error **errp)
>  {
> @@ -245,8 +243,6 @@ out:
>  ''',
>                   c_name=c_name(name))
>
> -    return ret
> -
>
>  class QAPISchemaGenVisitVisitor(QAPISchemaVisitor):
>      def __init__(self):
> @@ -269,9 +265,8 @@ class QAPISchemaGenVisitVisitor(QAPISchemaVisitor):
>          self._btin = None
>
>      def visit_needed(self, entity):
> -        # Visit everything except implicit objects
> -        return not (entity.is_implicit() and
> -                    isinstance(entity, QAPISchemaObjectType))
> +        # Visit everything except the special empty builtin
> +        return entity.name != 'q_empty'

As in qapi-types.py, visit_needed() isn't necessary even before the
patch.

>      def visit_enum_type(self, name, info, values, prefix):
>          # Special case for our lone builtin enum type
> @@ -297,8 +292,13 @@ class QAPISchemaGenVisitVisitor(QAPISchemaVisitor):
>
>      def visit_object_type(self, name, info, base, members, variants):
>          self.decl += gen_visit_members_decl(name)
> -        self.decl += gen_visit_decl(name)
> -        self.defn += gen_visit_object(name, base, members, variants)
> +        self.defn += gen_visit_object_members(name, base, members, variants)

Moved from gen_visit_object(), the rest can be called conditionally:

> +        # FIXME Worth changing the visitor signature, so we could
> +        # directly use rather than repeat type.is_implicit()?
> +        if not name.startswith('q_'):
> +            # only explicit types need an allocating visit
> +            self.decl += gen_visit_decl(name)
> +            self.defn += gen_visit_object(name, base, members, variants)
>
>      def visit_alternate_type(self, name, info, variants):
>          self.decl += gen_visit_decl(name)

Okay.