Re: [Qemu-devel] [PATCH v4 04/10] qapi: Emit implicit structs in generated C

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 03/08/2016 07:24 AM, Markus Armbruster wrote:
>> Eric Blake <address@hidden> 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.
>> 
>> Yes.
>> 
>>> 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[0] == ':'" feels a bit hacky, but beats changing the
>>> signature of the visit_object_type() callback to pass a new
>>> 'implicit' flag.
>> 
>> PRO: it matches what QAPISchemaObjectType.is_implicit() does.
>> 
>> CON: it duplicates what QAPISchemaObjectType.is_implicit() does.
>> 
>> Ways to adhere to DRY:
>> 
>> (1) Add a flag to visit_object_type() and, for consistency, to
>>     visit_object_type_flat().
>> 
>> (2) Change the QAPISchemaVisitor.visit_FOO() to take a full FOO instead
>>     of FOO.name, FOO.info and selected other members.
>> 
>> We've discussed (2) elsewhere.  The QAPISchemaEntity.visit() shrink to a
>> mere double-dispatch.  The QAPISchemaVisitor gain full access to the
>> things they visit.  The interface between the generators and the
>> internal representation changes from a narrow, explicit and inflexible
>> one to a much wider "anything goes" one.  Both the narrow and the wide
>> interface have advantages and disadvantages.  Have we outgrown the
>> narrow one?
>
> Your argument that the narrow view forces us to think about things may
> still hold.  I'm border-line on whether the switch is worth it, vs.
> adding flags as the visitors want to learn more and more about what they
> are visiting without having the actual Python object in hand.

I used to be on the "narrow" side of the fence, but I've come to sit on
it.

> I think what would sway me over the fence is looking at some of our
> constructs: for example, qapi-types.py has gen_object() which it now
> calls recursively.  When called directly from visit_object_type(), we
> have all the pieces; when called from visit_alternate_type(), we have to
> create a 1-element members array; and when called recursively, we have
> to manually explode base into the four pieces.

What could be improved if we changed visit_object_type() to take a
QAPISchemaObjectType instead of name, info, base, members, variants?

I believe we'd leave gen_object() unchanged to keep
visit_alternate_type() simple.

Here's a different one: we could drop visit_object_type_flat().

>>>                   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 ':empty' 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.
>>>
>>> The patch relies on the fact that all our implicit objects start
>>> with a leading ':', which can be transliteratated to a leading
>> 
>> transliterated
>> 
>>> single underscore, and we've already documented and enforce that
>> 
>> Uh, these are "always reserved for use as identifiers with file scope"
>> (C99 7.1.3).  I'm afraid we need to use the q_ prefix.
>> 
>>> the user can't create QAPI names with a leading underscore, so
>>> exposing the types won't create any collisions.  It is a bit
>>> unfortunate that the generated struct names don't match normal
>>> naming conventions, but it's not too bad, since they will only
>>> be used in generated code.
>> 
>> The problem is self-inflicted: we make up these names in
>> _make_implicit_object_type().  We could just as well make up names that
>> come out prettier.
>
> Any suggestions?  It seems easy enough to change, if we have something
> that we like.

For an array of T, we use TList.

For a simple union U's implicit enum, we use UKind.

Both predate the application of rational thought to clashes in generated
code ;)  Reserving these names was easier than changing them, so that's
what we did.

For implicit objects, we use :obj-NAME-ROLE.  We can change that, but
need to keep QAPISchemaMember._pretty_owner() working.

We could use c_name(q_obj_NAME_ROLE).

The resulting types aren't in CamelCase.  Making them CamelCase would be
easy enough when NAME is a type name (it is for ROLEs wrapper and base),
but for command and event names (ROLE arg) it involves playing stupid
games with case, dash and underscore.  Let's not go there, and pray the
coding style police looks the other way.

>> In fact, my first versions of the code had names starting with ':' for
>> *all* implicit types.  I abandoned that for enum and array types when I
>> realized I need C names for them, and the easiest way to get them making
>> up names so that a trivial .c_name() works.  We can do the same for
>> object types.
>> 
>>> The generated code grows substantially in size; in part because
>>> it was easier to output every implicit type rather than adding
>>> generator complexity to try and only output types as needed.
>> 
>> I happily trade larger .c files the optimizer can reduce for a simpler
>> generator.  I'm less sanguine about larger .h files when they get
>> included a lot.  qapi-types.h gets included basically everywhere, and
>> grows from ~4000 to ~5250 lines.  How much of this is avoidable?
>
> Not much: remember, because we use unboxed types in unions, all -wrapper
> structs have to be present in the .h for the compiler to not complain
> about incomplete types.
>
> For -arg types (and for upcoming -base types in 8/10), we could get away
> with only forward declarations of the type in the .h: the
> visit_type_ARG_members() function uses only a pointer so it can live
> with an incomplete type in the header and a complete type in a private
> helper file or in the .c.  But we have fewer -arg classes in comparison
> to the -wrapper classes, which means splitting on those lines would add
> a lot of complexity to the generator for very little .h savings.

Hmm.

I feel awful generating >100KiB of code that gets included pretty much
every time we compile anything.  Perhaps the proper fix for that is to
find out *why* we include qapi-types.h so much, then figure out how to
include it much, much less.

Here's a guilty one: error.h.  I believe it includes qapi-types.h just
for QapiErrorClass.  I guess it's only in the QAPI schema to have
QapiErrorClass_lookup[] generated.  I'd gladly maintain those nine(!)
lines of code manually if it helps me drop >100KiB of useless code from
many (most?) compiles.

Another one are builtin QAPI types.  A few headers want them.  We could
put them into a separate header instead of generating them into
qapi-types.h.  Could also enable getting rid of the ugly "spew builtins
into every header, but guard them with #ifdeffery" trick.