| Message ID | 1455778109-6278-14-git-send-email-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
Eric Blake <eblake@redhat.com> writes: > There's no reason to do two malloc's for a flat union; let's just > inline the branch struct directly into the C union branch of the > flat union. > > Surprisingly, fewer clients were actually using explicit references > to the branch types in comparison to the number of flat unions > thus modified. > > This lets us reduce the hack in qapi-types:gen_variants() added in > the previous patch; we no longer need to distinguish between > alternates and flat unions. > > The change to unboxed structs means that u.data (added in commit > cee2dedb) is now coincident with random fields of each branch of > the flat union, whereas beforehand it was only coincident with > pointers (since all branches of a flat union have to be objects). > Note that this was already the case for simple unions - but there > we got lucky. Remember, visit_start_union() blindly returns true > for all visitors except for the dealloc visitor, where it returns > the value !!obj->u.data, and that this result then controls > whether to proceed with the visit to the variant. Pre-patch, > this meant that flat unions were testing whether the boxed pointer > was still NULL, and thereby skipping visit_end_implicit_struct() > and avoiding a NULL dereference if the pointer had not been > allocated. The same was true for simple unions where the current > branch had pointer type, except there we bypassed visit_type_FOO(). > But for simple unions where the current branch had scalar type, the > contents of that scalar meant that the decision to call > visit_type_FOO() was data-dependent - the reason we got lucky there > is that visit_type_FOO() for all scalar types in the dealloc visitor > is a no-op (only the pointer variants had anything to free), so it > did not matter whether the dealloc visit was skipped. But with this > patch, we would risk leaking memory if we could skip a call to > visit_type_FOO_fields() based solely on a data-dependent decision. > > But notice: in the dealloc visitor, visit_type_FOO() already handles > a NULL obj - it was only the visit_type_implicit_FOO() that was > failing to check for NULL. And now that we have refactored things to > have the branch be part of the parent struct, we no longer have a > separate pointer that can be NULL in the first place. So we can just > delete the call to visit_start_union() altogether, and blindly visit > the branch type; there is no change in behavior except to the dealloc > visitor, where we now unconditionally visit the branch, but where that > visit is now always safe (for a flat union, we can no longer > dereference NULL, and for a simple union, visit_type_FOO() was already > safely handling NULL on pointer types). > > Unfortunately, simple unions are not as easy to switch to unboxed > layout; because we are special-casing the hidden implicit type with > a single 'data' member, we really DO need to keep calling another > layer of visit_start_struct(), with a second malloc; although there > are some cleanups planned for simple unions in later patches. > > Note that after this patch, visit_start_union() is unused, and the > only remaining use of visit_start_implicit_struct() is for alternate > types; the next couple of patches will do further cleanups based on > that fact. > > Signed-off-by: Eric Blake <eblake@redhat.com> Bonus: qapi-visit.c shrinks a bit, shedding all the visit_type_implicit_FOO(). > --- > v11: rebase to earlier changes, save cleanups for later, fix bug > with visit_start_union now being actively wrong > v10: new patch > --- > scripts/qapi-types.py | 13 +++---------- > scripts/qapi-visit.py | 7 ++----- > cpus.c | 18 ++++++------------ > hmp.c | 12 ++++++------ > tests/test-qmp-input-visitor.c | 10 +++++----- > tests/test-qmp-output-visitor.c | 6 ++---- > 6 files changed, 24 insertions(+), 42 deletions(-) > > diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py > index 4dabe91..eac90d2 100644 > --- a/scripts/qapi-types.py > +++ b/scripts/qapi-types.py > @@ -116,14 +116,6 @@ static inline %(base)s *qapi_%(c_name)s_base(const %(c_name)s *obj) > > > def gen_variants(variants): > - # HACK: Determine if this is an alternate (at least one variant > - # is not an object); unions have all branches as objects. > - unboxed = False > - for v in variants.variants: > - if not isinstance(v.type, QAPISchemaObjectType): > - unboxed = True > - break > - > # FIXME: What purpose does data serve, besides preventing a union that > # has a branch named 'data'? We use it in qapi-visit.py to decide > # whether to bypass the switch statement if visiting the discriminator > @@ -140,11 +132,12 @@ def gen_variants(variants): > > for var in variants.variants: > # Ugly special case for simple union TODO get rid of it > - typ = var.simple_union_type() or var.type > + simple_union_type = var.simple_union_type() > + typ = simple_union_type or var.type > ret += mcgen(''' > %(c_type)s %(c_name)s; > ''', > - c_type=typ.c_type(is_unboxed=unboxed), > + c_type=typ.c_type(is_unboxed=not simple_union_type), > c_name=c_name(var.name)) > > ret += mcgen(''' > diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py > index 61b7e72..367c459 100644 > --- a/scripts/qapi-visit.py > +++ b/scripts/qapi-visit.py > @@ -79,7 +79,7 @@ def gen_visit_struct_fields(name, base, members, variants=None): > for var in variants.variants: > # Ugly special case for simple union TODO get rid of it > if not var.simple_union_type(): > - ret += gen_visit_implicit_struct(var.type) > + ret += gen_visit_fields_decl(var.type) > > struct_fields_seen.add(name) > ret += mcgen(''' > @@ -102,9 +102,6 @@ static void visit_type_%(c_name)s_fields(Visitor *v, %(c_name)s *obj, Error **er > > if variants: > ret += mcgen(''' > - if (!visit_start_union(v, !!obj->u.data, &err) || err) { > - goto out; > - } > switch (obj->%(c_name)s) { > ''', > c_name=c_name(variants.tag_member.name)) > @@ -126,7 +123,7 @@ static void visit_type_%(c_name)s_fields(Visitor *v, %(c_name)s *obj, Error **er > c_name=c_name(var.name)) > else: > ret += mcgen(''' > - visit_type_implicit_%(c_type)s(v, &obj->u.%(c_name)s, &err); > + visit_type_%(c_type)s_fields(v, &obj->u.%(c_name)s, &err); > ''', > c_type=var.type.c_name(), > c_name=c_name(var.name)) The change has become quite simple. I take that as a good sign. [...]
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py index 4dabe91..eac90d2 100644 --- a/scripts/qapi-types.py +++ b/scripts/qapi-types.py @@ -116,14 +116,6 @@ static inline %(base)s *qapi_%(c_name)s_base(const %(c_name)s *obj) def gen_variants(variants): - # HACK: Determine if this is an alternate (at least one variant - # is not an object); unions have all branches as objects. - unboxed = False - for v in variants.variants: - if not isinstance(v.type, QAPISchemaObjectType): - unboxed = True - break - # FIXME: What purpose does data serve, besides preventing a union that # has a branch named 'data'? We use it in qapi-visit.py to decide # whether to bypass the switch statement if visiting the discriminator @@ -140,11 +132,12 @@ def gen_variants(variants): for var in variants.variants: # Ugly special case for simple union TODO get rid of it - typ = var.simple_union_type() or var.type + simple_union_type = var.simple_union_type() + typ = simple_union_type or var.type ret += mcgen(''' %(c_type)s %(c_name)s; ''', - c_type=typ.c_type(is_unboxed=unboxed), + c_type=typ.c_type(is_unboxed=not simple_union_type), c_name=c_name(var.name)) ret += mcgen(''' diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py index 61b7e72..367c459 100644 --- a/scripts/qapi-visit.py +++ b/scripts/qapi-visit.py @@ -79,7 +79,7 @@ def gen_visit_struct_fields(name, base, members, variants=None): for var in variants.variants: # Ugly special case for simple union TODO get rid of it if not var.simple_union_type(): - ret += gen_visit_implicit_struct(var.type) + ret += gen_visit_fields_decl(var.type) struct_fields_seen.add(name) ret += mcgen(''' @@ -102,9 +102,6 @@ static void visit_type_%(c_name)s_fields(Visitor *v, %(c_name)s *obj, Error **er if variants: ret += mcgen(''' - if (!visit_start_union(v, !!obj->u.data, &err) || err) { - goto out; - } switch (obj->%(c_name)s) { ''', c_name=c_name(variants.tag_member.name)) @@ -126,7 +123,7 @@ static void visit_type_%(c_name)s_fields(Visitor *v, %(c_name)s *obj, Error **er c_name=c_name(var.name)) else: ret += mcgen(''' - visit_type_implicit_%(c_type)s(v, &obj->u.%(c_name)s, &err); + visit_type_%(c_type)s_fields(v, &obj->u.%(c_name)s, &err); ''', c_type=var.type.c_name(), c_name=c_name(var.name)) diff --git a/cpus.c b/cpus.c index 898426c..9592163 100644 --- a/cpus.c +++ b/cpus.c @@ -1568,28 +1568,22 @@ CpuInfoList *qmp_query_cpus(Error **errp) info->value->thread_id = cpu->thread_id; #if defined(TARGET_I386) info->value->arch = CPU_INFO_ARCH_X86; - info->value->u.x86 = g_new0(CpuInfoX86, 1); - info->value->u.x86->pc = env->eip + env->segs[R_CS].base; + info->value->u.x86.pc = env->eip + env->segs[R_CS].base; #elif defined(TARGET_PPC) info->value->arch = CPU_INFO_ARCH_PPC; - info->value->u.ppc = g_new0(CpuInfoPPC, 1); - info->value->u.ppc->nip = env->nip; + info->value->u.ppc.nip = env->nip; #elif defined(TARGET_SPARC) info->value->arch = CPU_INFO_ARCH_SPARC; - info->value->u.q_sparc = g_new0(CpuInfoSPARC, 1); - info->value->u.q_sparc->pc = env->pc; - info->value->u.q_sparc->npc = env->npc; + info->value->u.q_sparc.pc = env->pc; + info->value->u.q_sparc.npc = env->npc; #elif defined(TARGET_MIPS) info->value->arch = CPU_INFO_ARCH_MIPS; - info->value->u.q_mips = g_new0(CpuInfoMIPS, 1); - info->value->u.q_mips->PC = env->active_tc.PC; + info->value->u.q_mips.PC = env->active_tc.PC; #elif defined(TARGET_TRICORE) info->value->arch = CPU_INFO_ARCH_TRICORE; - info->value->u.tricore = g_new0(CpuInfoTricore, 1); - info->value->u.tricore->PC = env->PC; + info->value->u.tricore.PC = env->PC; #else info->value->arch = CPU_INFO_ARCH_OTHER; - info->value->u.other = g_new0(CpuInfoOther, 1); #endif /* XXX: waiting for the qapi to support GSList */ diff --git a/hmp.c b/hmp.c index 996cb91..bfbd667 100644 --- a/hmp.c +++ b/hmp.c @@ -314,22 +314,22 @@ void hmp_info_cpus(Monitor *mon, const QDict *qdict) switch (cpu->value->arch) { case CPU_INFO_ARCH_X86: - monitor_printf(mon, " pc=0x%016" PRIx64, cpu->value->u.x86->pc); + monitor_printf(mon, " pc=0x%016" PRIx64, cpu->value->u.x86.pc); break; case CPU_INFO_ARCH_PPC: - monitor_printf(mon, " nip=0x%016" PRIx64, cpu->value->u.ppc->nip); + monitor_printf(mon, " nip=0x%016" PRIx64, cpu->value->u.ppc.nip); break; case CPU_INFO_ARCH_SPARC: monitor_printf(mon, " pc=0x%016" PRIx64, - cpu->value->u.q_sparc->pc); + cpu->value->u.q_sparc.pc); monitor_printf(mon, " npc=0x%016" PRIx64, - cpu->value->u.q_sparc->npc); + cpu->value->u.q_sparc.npc); break; case CPU_INFO_ARCH_MIPS: - monitor_printf(mon, " PC=0x%016" PRIx64, cpu->value->u.q_mips->PC); + monitor_printf(mon, " PC=0x%016" PRIx64, cpu->value->u.q_mips.PC); break; case CPU_INFO_ARCH_TRICORE: - monitor_printf(mon, " PC=0x%016" PRIx64, cpu->value->u.tricore->PC); + monitor_printf(mon, " PC=0x%016" PRIx64, cpu->value->u.tricore.PC); break; default: break; diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c index 47cf6aa..b05da5b 100644 --- a/tests/test-qmp-input-visitor.c +++ b/tests/test-qmp-input-visitor.c @@ -295,7 +295,7 @@ static void test_visitor_in_union_flat(TestInputVisitorData *data, g_assert_cmpint(tmp->enum1, ==, ENUM_ONE_VALUE1); g_assert_cmpstr(tmp->string, ==, "str"); g_assert_cmpint(tmp->integer, ==, 41); - g_assert_cmpint(tmp->u.value1->boolean, ==, true); + g_assert_cmpint(tmp->u.value1.boolean, ==, true); base = qapi_UserDefFlatUnion_base(tmp); g_assert(&base->enum1 == &tmp->enum1); @@ -330,8 +330,8 @@ static void test_visitor_in_alternate(TestInputVisitorData *data, g_assert_cmpint(tmp->u.udfu.integer, ==, 1); g_assert_cmpstr(tmp->u.udfu.string, ==, "str"); g_assert_cmpint(tmp->u.udfu.enum1, ==, ENUM_ONE_VALUE1); - g_assert_cmpint(tmp->u.udfu.u.value1->boolean, ==, true); - g_assert_cmpint(tmp->u.udfu.u.value1->has_a_b, ==, false); + g_assert_cmpint(tmp->u.udfu.u.value1.boolean, ==, true); + g_assert_cmpint(tmp->u.udfu.u.value1.has_a_b, ==, false); qapi_free_UserDefAlternate(tmp); v = visitor_input_test_init(data, "false"); @@ -358,8 +358,8 @@ static void test_visitor_in_alternate(TestInputVisitorData *data, g_assert_cmpint(wrap->alt->u.udfu.integer, ==, 1); g_assert_cmpstr(wrap->alt->u.udfu.string, ==, "str"); g_assert_cmpint(wrap->alt->u.udfu.enum1, ==, ENUM_ONE_VALUE1); - g_assert_cmpint(wrap->alt->u.udfu.u.value1->boolean, ==, true); - g_assert_cmpint(wrap->alt->u.udfu.u.value1->has_a_b, ==, false); + g_assert_cmpint(wrap->alt->u.udfu.u.value1.boolean, ==, true); + g_assert_cmpint(wrap->alt->u.udfu.u.value1.has_a_b, ==, false); qapi_free_WrapAlternate(wrap); } diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c index fe2f1a1..a7f8b45 100644 --- a/tests/test-qmp-output-visitor.c +++ b/tests/test-qmp-output-visitor.c @@ -403,9 +403,8 @@ static void test_visitor_out_union_flat(TestOutputVisitorData *data, UserDefFlatUnion *tmp = g_malloc0(sizeof(UserDefFlatUnion)); tmp->enum1 = ENUM_ONE_VALUE1; tmp->string = g_strdup("str"); - tmp->u.value1 = g_malloc0(sizeof(UserDefA)); tmp->integer = 41; - tmp->u.value1->boolean = true; + tmp->u.value1.boolean = true; visit_type_UserDefFlatUnion(data->ov, NULL, &tmp, &error_abort); arg = qmp_output_get_qobject(data->qov); @@ -460,8 +459,7 @@ static void test_visitor_out_alternate(TestOutputVisitorData *data, tmp->u.udfu.integer = 1; tmp->u.udfu.string = g_strdup("str"); tmp->u.udfu.enum1 = ENUM_ONE_VALUE1; - tmp->u.udfu.u.value1 = g_new0(UserDefA, 1); - tmp->u.udfu.u.value1->boolean = true; + tmp->u.udfu.u.value1.boolean = true; visit_type_UserDefAlternate(data->ov, NULL, &tmp, &error_abort); arg = qmp_output_get_qobject(data->qov);
There's no reason to do two malloc's for a flat union; let's just inline the branch struct directly into the C union branch of the flat union. Surprisingly, fewer clients were actually using explicit references to the branch types in comparison to the number of flat unions thus modified. This lets us reduce the hack in qapi-types:gen_variants() added in the previous patch; we no longer need to distinguish between alternates and flat unions. The change to unboxed structs means that u.data (added in commit cee2dedb) is now coincident with random fields of each branch of the flat union, whereas beforehand it was only coincident with pointers (since all branches of a flat union have to be objects). Note that this was already the case for simple unions - but there we got lucky. Remember, visit_start_union() blindly returns true for all visitors except for the dealloc visitor, where it returns the value !!obj->u.data, and that this result then controls whether to proceed with the visit to the variant. Pre-patch, this meant that flat unions were testing whether the boxed pointer was still NULL, and thereby skipping visit_end_implicit_struct() and avoiding a NULL dereference if the pointer had not been allocated. The same was true for simple unions where the current branch had pointer type, except there we bypassed visit_type_FOO(). But for simple unions where the current branch had scalar type, the contents of that scalar meant that the decision to call visit_type_FOO() was data-dependent - the reason we got lucky there is that visit_type_FOO() for all scalar types in the dealloc visitor is a no-op (only the pointer variants had anything to free), so it did not matter whether the dealloc visit was skipped. But with this patch, we would risk leaking memory if we could skip a call to visit_type_FOO_fields() based solely on a data-dependent decision. But notice: in the dealloc visitor, visit_type_FOO() already handles a NULL obj - it was only the visit_type_implicit_FOO() that was failing to check for NULL. And now that we have refactored things to have the branch be part of the parent struct, we no longer have a separate pointer that can be NULL in the first place. So we can just delete the call to visit_start_union() altogether, and blindly visit the branch type; there is no change in behavior except to the dealloc visitor, where we now unconditionally visit the branch, but where that visit is now always safe (for a flat union, we can no longer dereference NULL, and for a simple union, visit_type_FOO() was already safely handling NULL on pointer types). Unfortunately, simple unions are not as easy to switch to unboxed layout; because we are special-casing the hidden implicit type with a single 'data' member, we really DO need to keep calling another layer of visit_start_struct(), with a second malloc; although there are some cleanups planned for simple unions in later patches. Note that after this patch, visit_start_union() is unused, and the only remaining use of visit_start_implicit_struct() is for alternate types; the next couple of patches will do further cleanups based on that fact. Signed-off-by: Eric Blake <eblake@redhat.com> --- v11: rebase to earlier changes, save cleanups for later, fix bug with visit_start_union now being actively wrong v10: new patch --- scripts/qapi-types.py | 13 +++---------- scripts/qapi-visit.py | 7 ++----- cpus.c | 18 ++++++------------ hmp.c | 12 ++++++------ tests/test-qmp-input-visitor.c | 10 +++++----- tests/test-qmp-output-visitor.c | 6 ++---- 6 files changed, 24 insertions(+), 42 deletions(-)