Message ID | 1457194595-16189-5-git-send-email-eblake@redhat.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
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. 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? > 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. 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? > Signed-off-by: Eric Blake <eblake@redhat.com> Can't find fault with the patch itself.
On 03/08/2016 07:24 AM, Markus Armbruster wrote: > 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. > > 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 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. > >> 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. > > 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.
Eric Blake <eblake@redhat.com> writes: > On 03/08/2016 07:24 AM, Markus Armbruster wrote: >> 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. >> >> 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.
On 03/08/2016 12:09 PM, Markus Armbruster wrote: > >> 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(). Indeed. But I'd rather get v5 of this series out sooner rather than later, so I'll save the change for a later day. >>> >>> 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. Seems doable. We already reserved q_ for ourselves (commit 9fb081e0), so far using it mainly by c_name. There's still the risk that c_name adds q_ onto something ticklish which then clashes with our use of q_ on an implicit type for something else; except that we haven't declared 'obj' as ticklish. > 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. Commit 13f59ae predates my work on qapi, but I think you're right that error.h including qapi-types.h is a big offender and appears to do so solely for ErrorClass. But how about as a followup series, so that v5 gets out the door sooner. > > 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. In that same series.
Eric Blake <eblake@redhat.com> writes: > On 03/08/2016 12:09 PM, Markus Armbruster wrote: > >> >>> 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(). > > Indeed. But I'd rather get v5 of this series out sooner rather than > later, so I'll save the change for a later day. I agree that we've wandered beyond the scope of this series here. It's not about the visitor, it merely struggles with the visitor's narrow view on the visited objects. That led us to discuss whether that view still makes sense. We're not sure. >>>> 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. > > Seems doable. We already reserved q_ for ourselves (commit 9fb081e0), so > far using it mainly by c_name. There's still the risk that c_name adds > q_ onto something ticklish which then clashes with our use of q_ on an > implicit type for something else; except that we haven't declared 'obj' > as ticklish. As far as I can tell, c_name() is still the only user of q_. If we add more, we need to update the "Reserve the entire 'q_' namespace for c_name()" comment, and we should try to keep track of our use of q_ somehow. Alternatively, reserve another chunk of the name space for implicit object types. Might be easier to maintain. >> 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. > > Commit 13f59ae predates my work on qapi, but I think you're right that > error.h including qapi-types.h is a big offender and appears to do so > solely for ErrorClass. But how about as a followup series, so that v5 > gets out the door sooner. Teaching error.h manners doesn't depend on anything in your pipe. I'm invoking "Error Reporting" maintainer privileges: the task is mine :) >> 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. > > In that same series. I'd expect this to be a series of its own, going through the QAPI tree.
diff --git a/scripts/qapi.py b/scripts/qapi.py index b1b87ee..6cf0a75 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -999,7 +999,6 @@ class QAPISchemaObjectType(QAPISchemaType): return self.name[0] == ':' def c_name(self): - assert not self.is_implicit() return QAPISchemaType.c_name(self) def c_type(self): @@ -1476,7 +1475,7 @@ def c_enum_const(type_name, const_name, prefix=None): type_name = prefix return camel_to_upper(type_name) + '_' + c_name(const_name, False).upper() -c_name_trans = string.maketrans('.-', '__') +c_name_trans = string.maketrans('.-:', '___') # Map @name to a valid C identifier. diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py index f194bea..fa2d6af 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 != ':empty' def _gen_type_cleanup(self, name): self.decl += gen_type_cleanup_decl(name) @@ -233,7 +231,9 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor): self.decl += gen_object(name, base, members, variants) if base: self.decl += gen_upcast(name, base) - self._gen_type_cleanup(name) + if name[0] != ':': + # 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..b4303a7 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) - # 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 != ':empty' def visit_enum_type(self, name, info, values, prefix): # Special case for our lone builtin enum type @@ -297,8 +292,11 @@ 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) + if name[0] != ':': + # 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)
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[0] == ':'" feels a bit hacky, but beats changing the signature of the visit_object_type() callback to pass a new 'implicit' flag. 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 single underscore, and we've already documented and enforce that 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 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. Signed-off-by: Eric Blake <eblake@redhat.com> --- v4: new patch --- scripts/qapi.py | 3 +-- scripts/qapi-types.py | 12 ++++++------ scripts/qapi-visit.py | 18 ++++++++---------- 3 files changed, 15 insertions(+), 18 deletions(-)