By sticking the next pointer first, we don't need a union with 64-bit padding for smaller types. On 32-bit platforms, this can reduce the size of uint8List from 16 bytes (or 12, depending on whether 64-bit ints can tolerate 4-byte alignment) down to 8. It has no effect on 64-bit platforms (where alignment still dictates a 16-byte struct); but fewer anonymous unions is still a win in my book.
However, this requires visit_start_list() and visit_next_list() to gain a size parameter, to know what size element to allocate. I debated about going one step further, to allow for fewer casts, by doing: typedef GenericList GenericList; struct GenericList { GenericList *next; }; struct FooList { GenericList base; Foo value; }; so that you convert to 'GenericList *' by '&foolist->base', and back by 'container_of(generic, GenericList, base)' (as opposed to the existing '(GenericList *)foolist' and '(FooList *)generic'). But doing that would require hoisting the declaration of GenericList prior to inclusion of qapi-types.h, rather than its current spot in visitor.h; it also makes iteration a bit more verbose through 'foolist->base.next' instead of 'foolist->next'. Signed-off-by: Eric Blake <ebl...@redhat.com> --- v9: no change v8: rebase to earlier changes v7: new patch; probably too invasive to be worth it, especially if we can't prove that our current size for uint8List is a bottleneck --- hw/ppc/spapr_drc.c | 2 +- include/qapi/visitor-impl.h | 5 +++-- include/qapi/visitor.h | 39 +++++++++++++++++++-------------------- qapi/opts-visitor.c | 9 +++++---- qapi/qapi-dealloc-visitor.c | 5 +++-- qapi/qapi-visit-core.c | 14 +++++++++----- qapi/qmp-input-visitor.c | 8 ++++---- qapi/qmp-output-visitor.c | 5 +++-- qapi/string-input-visitor.c | 9 +++++---- qapi/string-output-visitor.c | 5 +++-- scripts/qapi-types.py | 5 +---- scripts/qapi-visit.py | 4 ++-- 12 files changed, 58 insertions(+), 52 deletions(-) diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c index 41f2da0..0eba901 100644 --- a/hw/ppc/spapr_drc.c +++ b/hw/ppc/spapr_drc.c @@ -299,7 +299,7 @@ static void prop_get_fdt(Object *obj, Visitor *v, const char *name, int i; prop = fdt_get_property_by_offset(fdt, fdt_offset, &prop_len); name = fdt_string(fdt, fdt32_to_cpu(prop->nameoff)); - visit_start_list(v, name, NULL, &err); + visit_start_list(v, name, NULL, 0, &err); if (err) { error_propagate(errp, err); return; diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h index 8df4ba1..dbbbcdb 100644 --- a/include/qapi/visitor-impl.h +++ b/include/qapi/visitor-impl.h @@ -41,9 +41,10 @@ struct Visitor /* Must be set */ bool (*start_list)(Visitor *v, const char *name, GenericList **list, - Error **errp); + size_t size, Error **errp); /* Must be set */ - GenericList *(*next_list)(Visitor *v, GenericList *element, Error **errp); + GenericList *(*next_list)(Visitor *v, GenericList *element, size_t size, + Error **errp); /* Must be set */ void (*end_list)(Visitor *v); diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h index 4eae633..c446726 100644 --- a/include/qapi/visitor.h +++ b/include/qapi/visitor.h @@ -56,11 +56,8 @@ * created by the qapi generator. */ typedef struct GenericList { - union { - void *value; - uint64_t padding; - }; struct GenericList *next; + char padding[]; } GenericList; /** @@ -130,19 +127,19 @@ void visit_end_implicit_struct(Visitor *v); /** * Prepare to visit a list tied to an object key @name. * @name will be NULL if this is visited as part of another list. - * Input visitors malloc a qapi List struct into *@list, or set it to - * NULL if there are no elements in the list; and output visitors - * expect *@list to point to the start of the list, if any. On - * return, if *@list is non-NULL, the caller should enter a loop + * Input visitors malloc a qapi List struct into *@list of size @size, + * or set it to NULL if there are no elements in the list; and output + * visitors expect *@list to point to the start of the list, if any. + * On return, if *@list is non-NULL, the caller should enter a loop * visiting the current element, then using visit_next_list() to * advance to the next element, until that returns NULL; then * visit_end_list() must be used to complete the visit. * - * If supported by a visitor, @list can be NULL to indicate that there - * is no qapi List struct, and that the upcoming visit calls are - * parsing input to or creating output from some other representation; - * in this case, visit_next_list() will not be needed, but - * visit_end_list() is still mandatory. + * If supported by a visitor, @list can be NULL (and @size 0) to + * indicate that there is no qapi List struct, and that the upcoming + * visit calls are parsing input to or creating output from some other + * representation; in this case, visit_next_list() will not be needed, + * but visit_end_list() is still mandatory. * * Returns true if *@list was allocated; if that happens, and an error * occurs any time before the matching visit_end_list(), then the @@ -150,17 +147,19 @@ void visit_end_implicit_struct(Visitor *v); * allocation before returning control further. */ bool visit_start_list(Visitor *v, const char *name, GenericList **list, - Error **errp); + size_t size, Error **errp); /** * Iterate over a GenericList during a list visit. * Before calling this function, the caller should use the appropriate - * visit_type_FOO() for the current list element at @element->value, and - * check for errors. @element must not be NULL; on the first iteration, - * it should be the value in *list after visit_start_list(); on other - * calls it should be the previous return value. This function - * returns NULL once there are no further list elements. + * visit_type_FOO() for the current list element at @element->value, + * and check for errors. @element must not be NULL; on the first + * iteration, it should be the value in *list after + * visit_start_list(); on other calls it should be the previous return + * value. @size should be the same as for visit_start_list(). This + * function returns NULL once there are no further list elements. */ -GenericList *visit_next_list(Visitor *v, GenericList *element, Error **errp); +GenericList *visit_next_list(Visitor *v, GenericList *element, size_t size, + Error **errp); /** * Complete the list started earlier. * Must be called after any successful use of visit_start_list(), diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c index 38d1e68..28f9a8a 100644 --- a/qapi/opts-visitor.c +++ b/qapi/opts-visitor.c @@ -214,7 +214,8 @@ lookup_distinct(const OptsVisitor *ov, const char *name, Error **errp) static bool -opts_start_list(Visitor *v, const char *name, GenericList **list, Error **errp) +opts_start_list(Visitor *v, const char *name, GenericList **list, size_t size, + Error **errp) { OptsVisitor *ov = to_ov(v); @@ -225,7 +226,7 @@ opts_start_list(Visitor *v, const char *name, GenericList **list, Error **errp) ov->repeated_opts = lookup_distinct(ov, name, errp); if (ov->repeated_opts) { ov->list_mode = LM_IN_PROGRESS; - *list = g_new0(GenericList, 1); + *list = g_malloc0(size); return true; } else { *list = NULL; @@ -235,7 +236,7 @@ opts_start_list(Visitor *v, const char *name, GenericList **list, Error **errp) static GenericList * -opts_next_list(Visitor *v, GenericList *list, Error **errp) +opts_next_list(Visitor *v, GenericList *list, size_t size, Error **errp) { OptsVisitor *ov = to_ov(v); @@ -269,7 +270,7 @@ opts_next_list(Visitor *v, GenericList *list, Error **errp) abort(); } - list->next = g_new0(GenericList, 1); + list->next = g_malloc0(size); return list->next; } diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c index 0990199..d498f29 100644 --- a/qapi/qapi-dealloc-visitor.c +++ b/qapi/qapi-dealloc-visitor.c @@ -90,7 +90,8 @@ static void qapi_dealloc_end_implicit_struct(Visitor *v) } static bool qapi_dealloc_start_list(Visitor *v, const char *name, - GenericList **list, Error **errp) + GenericList **list, size_t size, + Error **errp) { QapiDeallocVisitor *qov = to_qov(v); qapi_dealloc_push(qov, NULL); @@ -98,7 +99,7 @@ static bool qapi_dealloc_start_list(Visitor *v, const char *name, } static GenericList *qapi_dealloc_next_list(Visitor *v, GenericList *list, - Error **errp) + size_t size, Error **errp) { GenericList *next = list->next; g_free(list); diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c index 259e0cb..ed4de71 100644 --- a/qapi/qapi-visit-core.c +++ b/qapi/qapi-visit-core.c @@ -80,19 +80,23 @@ void visit_end_implicit_struct(Visitor *v) } bool visit_start_list(Visitor *v, const char *name, GenericList **list, - Error **errp) + size_t size, Error **errp) { - bool result = v->start_list(v, name, list, errp); + bool result; + + assert(list ? size : !size); + result = v->start_list(v, name, list, size, errp); if (result) { assert(list && *list); } return result; } -GenericList *visit_next_list(Visitor *v, GenericList *list, Error **errp) +GenericList *visit_next_list(Visitor *v, GenericList *list, size_t size, + Error **errp) { - assert(list); - return v->next_list(v, list, errp); + assert(list && size); + return v->next_list(v, list, size, errp); } void visit_end_list(Visitor *v) diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c index 6b4ad68..16c18b3 100644 --- a/qapi/qmp-input-visitor.c +++ b/qapi/qmp-input-visitor.c @@ -168,7 +168,7 @@ static bool qmp_input_start_implicit_struct(Visitor *v, void **obj, } static bool qmp_input_start_list(Visitor *v, const char *name, - GenericList **list, Error **errp) + GenericList **list, size_t size, Error **errp) { QmpInputVisitor *qiv = to_qiv(v); QObject *qobj = qmp_input_get_object(qiv, name, true); @@ -184,7 +184,7 @@ static bool qmp_input_start_list(Visitor *v, const char *name, qmp_input_push(qiv, qobj, entry, errp); if (list) { if (entry) { - *list = g_new0(GenericList, 1); + *list = g_malloc0(size); return true; } else { *list = NULL; @@ -194,7 +194,7 @@ static bool qmp_input_start_list(Visitor *v, const char *name, } static GenericList *qmp_input_next_list(Visitor *v, GenericList *list, - Error **errp) + size_t size, Error **errp) { QmpInputVisitor *qiv = to_qiv(v); StackObject *so = &qiv->stack[qiv->nb_stack - 1]; @@ -202,7 +202,7 @@ static GenericList *qmp_input_next_list(Visitor *v, GenericList *list, if (!so->entry) { return NULL; } - list->next = g_new0(GenericList, 1); + list->next = g_malloc0(size); return list->next; } diff --git a/qapi/qmp-output-visitor.c b/qapi/qmp-output-visitor.c index ce592d2..78567d0 100644 --- a/qapi/qmp-output-visitor.c +++ b/qapi/qmp-output-visitor.c @@ -120,7 +120,8 @@ static void qmp_output_end_struct(Visitor *v) } static bool qmp_output_start_list(Visitor *v, const char *name, - GenericList **listp, Error **errp) + GenericList **listp, size_t size, + Error **errp) { QmpOutputVisitor *qov = to_qov(v); QList *list = qlist_new(); @@ -131,7 +132,7 @@ static bool qmp_output_start_list(Visitor *v, const char *name, } static GenericList *qmp_output_next_list(Visitor *v, GenericList *list, - Error **errp) + size_t size, Error **errp) { return list->next; } diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c index 0e4a07c..86bddd1 100644 --- a/qapi/string-input-visitor.c +++ b/qapi/string-input-visitor.c @@ -121,7 +121,8 @@ error: } static bool -start_list(Visitor *v, const char *name, GenericList **list, Error **errp) +start_list(Visitor *v, const char *name, GenericList **list, size_t size, + Error **errp) { StringInputVisitor *siv = to_siv(v); Error *err = NULL; @@ -141,7 +142,7 @@ start_list(Visitor *v, const char *name, GenericList **list, Error **errp) if (r) { siv->cur = r->begin; } - *list = g_new0(GenericList, 1); + *list = g_malloc0(size); return true; } else { *list = NULL; @@ -150,7 +151,7 @@ start_list(Visitor *v, const char *name, GenericList **list, Error **errp) } static GenericList * -next_list(Visitor *v, GenericList *list, Error **errp) +next_list(Visitor *v, GenericList *list, size_t size, Error **errp) { StringInputVisitor *siv = to_siv(v); Range *r; @@ -176,7 +177,7 @@ next_list(Visitor *v, GenericList *list, Error **errp) siv->cur = r->begin; } - list->next = g_new0(GenericList, 1); + list->next = g_malloc0(size); return list->next; } diff --git a/qapi/string-output-visitor.c b/qapi/string-output-visitor.c index 2666802..104f8a4 100644 --- a/qapi/string-output-visitor.c +++ b/qapi/string-output-visitor.c @@ -264,7 +264,8 @@ static void print_type_number(Visitor *v, const char *name, double *obj, } static bool -start_list(Visitor *v, const char *name, GenericList **list, Error **errp) +start_list(Visitor *v, const char *name, GenericList **list, size_t size, + Error **errp) { StringOutputVisitor *sov = to_sov(v); @@ -280,7 +281,7 @@ start_list(Visitor *v, const char *name, GenericList **list, Error **errp) } static GenericList * -next_list(Visitor *v, GenericList *list, Error **errp) +next_list(Visitor *v, GenericList *list, size_t size, Error **errp) { StringOutputVisitor *sov = to_sov(v); GenericList *ret = list->next; diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py index 5cf20c2..47a6523 100644 --- a/scripts/qapi-types.py +++ b/scripts/qapi-types.py @@ -26,11 +26,8 @@ def gen_array(name, element_type): return mcgen(''' struct %(c_name)s { - union { - %(c_type)s value; - uint64_t padding; - }; %(c_name)s *next; + %(c_type)s value; }; ''', c_name=c_name(name), c_type=element_type.c_type()) diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py index eebb5f7..193a1b6 100644 --- a/scripts/qapi-visit.py +++ b/scripts/qapi-visit.py @@ -130,7 +130,7 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error %(c_name)s *elt; bool allocated; - allocated = visit_start_list(v, name, (GenericList **)obj, &err); + allocated = visit_start_list(v, name, (GenericList **)obj, sizeof(%(c_name)s), &err); if (err) { goto out; } @@ -140,7 +140,7 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error if (err) { break; } - elt = (%(c_name)s *)visit_next_list(v, (GenericList *)elt, &err); + elt = (%(c_name)s *)visit_next_list(v, (GenericList *)elt, sizeof(%(c_name)s), &err); if (err) { break; } -- 2.5.0