| Message ID | 1453219845-30939-13-git-send-email-eblake@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Eric Blake <eblake@redhat.com> writes: > C compilers are allowed to represent enums as a smaller type > than int, if all enum values fit in the smaller type. There > are even compiler flags that force the use of this smaller > representation, and using them changes the ABI of a binary. Suggest "although using them". > Therefore, our generated code for visit_type_ENUM() (for all > qapi enums) was wrong for casting Enum* to int* when calling > visit_type_enum(). > > It appears that no one has been doing this for qemu, because > if they had, we are potentially dereferencing beyond bounds > or even risking a SIGBUS on platforms where unaligned pointer > dereferencing is fatal. Better is to avoid the practice > entirely, and just use the correct types. > > This matches the fix for alternate qapi types, done earlier in > commit 0426d53 "qapi: Simplify visiting of alternate types", > with generated code changing as: > > | void visit_type_GuestDiskBusType(Visitor *v, GuestDiskBusType *obj, const char *name, Error **errp) > | { > |- visit_type_enum(v, (int *)obj, GuestDiskBusType_lookup, "GuestDiskBusType", name, errp); > |+ int tmp = *obj; > |+ visit_type_enum(v, &tmp, GuestDiskBusType_lookup, "GuestDiskBusType", name, errp); > |+ *obj = tmp; > | } Long lines. Do we have an example with a shorter enum name handy? > Signed-off-by: Eric Blake <eblake@redhat.com> > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > --- > v9: mention earlier commit id, enhance commit message > v8: no change > v7: rebase on typo fix > v6: new patch > --- > scripts/qapi-visit.py | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py > index 4a4f67d..6bd188b 100644 > --- a/scripts/qapi-visit.py > +++ b/scripts/qapi-visit.py > @@ -178,12 +178,13 @@ out: > > > def gen_visit_enum(name): > - # FIXME cast from enum *obj to int * invalidly assumes enum is int > return mcgen(''' > > void visit_type_%(c_name)s(Visitor *v, %(c_name)s *obj, const char *name, Error **errp) > { > - visit_type_enum(v, (int *)obj, %(c_name)s_lookup, "%(name)s", name, errp); > + int tmp = *obj; > + visit_type_enum(v, &tmp, %(c_name)s_lookup, "%(name)s", name, errp); > + *obj = tmp; > } > ''', > c_name=c_name(name), name=name) Same pattern in qapi-visit-core.c, except we name the variable @value there. Your choice.
On 01/20/2016 11:08 AM, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> writes: > >> C compilers are allowed to represent enums as a smaller type >> than int, if all enum values fit in the smaller type. There >> are even compiler flags that force the use of this smaller >> representation, and using them changes the ABI of a binary. > > Suggest "although using them". > Okay. >> with generated code changing as: >> >> | void visit_type_GuestDiskBusType(Visitor *v, GuestDiskBusType *obj, const char *name, Error **errp) >> | { >> |- visit_type_enum(v, (int *)obj, GuestDiskBusType_lookup, "GuestDiskBusType", name, errp); >> |+ int tmp = *obj; >> |+ visit_type_enum(v, &tmp, GuestDiskBusType_lookup, "GuestDiskBusType", name, errp); >> |+ *obj = tmp; >> | } > > Long lines. Do we have an example with a shorter enum name handy? Shortest is QType; runner-ups RxState and TpmType. >> void visit_type_%(c_name)s(Visitor *v, %(c_name)s *obj, const char *name, Error **errp) >> { >> - visit_type_enum(v, (int *)obj, %(c_name)s_lookup, "%(name)s", name, errp); >> + int tmp = *obj; >> + visit_type_enum(v, &tmp, %(c_name)s_lookup, "%(name)s", name, errp); >> + *obj = tmp; >> } >> ''', >> c_name=c_name(name), name=name) > > Same pattern in qapi-visit-core.c, except we name the variable @value > there. Your choice. 'value' sounds consistent. An easy swap on a respin.
Eric Blake <eblake@redhat.com> writes: > On 01/20/2016 11:08 AM, Markus Armbruster wrote: >> Eric Blake <eblake@redhat.com> writes: >> >>> C compilers are allowed to represent enums as a smaller type >>> than int, if all enum values fit in the smaller type. There >>> are even compiler flags that force the use of this smaller >>> representation, and using them changes the ABI of a binary. >> >> Suggest "although using them". >> > > Okay. > > >>> with generated code changing as: >>> >>> | void visit_type_GuestDiskBusType(Visitor *v, GuestDiskBusType >>> | *obj, const char *name, Error **errp) >>> | { >>> |- visit_type_enum(v, (int *)obj, GuestDiskBusType_lookup, "GuestDiskBusType", name, errp); >>> |+ int tmp = *obj; >>> |+ visit_type_enum(v, &tmp, GuestDiskBusType_lookup, "GuestDiskBusType", name, errp); >>> |+ *obj = tmp; >>> | } >> >> Long lines. Do we have an example with a shorter enum name handy? > > Shortest is QType; runner-ups RxState and TpmType. QType comes out okay: | void visit_type_QType(Visitor *v, QType *obj, const char *name, Error **errp) | { |- visit_type_enum(v, (int *)obj, QType_lookup, "QType", name, errp); |+ int tmp = *obj; |+ visit_type_enum(v, &tmp, QType_lookup, "QType", name, errp); |+ *obj = tmp; | } Let's use it. >>> void visit_type_%(c_name)s(Visitor *v, %(c_name)s *obj, const char *name, Error **errp) >>> { >>> - visit_type_enum(v, (int *)obj, %(c_name)s_lookup, "%(name)s", name, errp); >>> + int tmp = *obj; >>> + visit_type_enum(v, &tmp, %(c_name)s_lookup, "%(name)s", name, errp); >>> + *obj = tmp; >>> } >>> ''', >>> c_name=c_name(name), name=name) >> >> Same pattern in qapi-visit-core.c, except we name the variable @value >> there. Your choice. > > 'value' sounds consistent. An easy swap on a respin. Okay.
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py index 4a4f67d..6bd188b 100644 --- a/scripts/qapi-visit.py +++ b/scripts/qapi-visit.py @@ -178,12 +178,13 @@ out: def gen_visit_enum(name): - # FIXME cast from enum *obj to int * invalidly assumes enum is int return mcgen(''' void visit_type_%(c_name)s(Visitor *v, %(c_name)s *obj, const char *name, Error **errp) { - visit_type_enum(v, (int *)obj, %(c_name)s_lookup, "%(name)s", name, errp); + int tmp = *obj; + visit_type_enum(v, &tmp, %(c_name)s_lookup, "%(name)s", name, errp); + *obj = tmp; } ''', c_name=c_name(name), name=name)
C compilers are allowed to represent enums as a smaller type than int, if all enum values fit in the smaller type. There are even compiler flags that force the use of this smaller representation, and using them changes the ABI of a binary. Therefore, our generated code for visit_type_ENUM() (for all qapi enums) was wrong for casting Enum* to int* when calling visit_type_enum(). It appears that no one has been doing this for qemu, because if they had, we are potentially dereferencing beyond bounds or even risking a SIGBUS on platforms where unaligned pointer dereferencing is fatal. Better is to avoid the practice entirely, and just use the correct types. This matches the fix for alternate qapi types, done earlier in commit 0426d53 "qapi: Simplify visiting of alternate types", with generated code changing as: | void visit_type_GuestDiskBusType(Visitor *v, GuestDiskBusType *obj, const char *name, Error **errp) | { |- visit_type_enum(v, (int *)obj, GuestDiskBusType_lookup, "GuestDiskBusType", name, errp); |+ int tmp = *obj; |+ visit_type_enum(v, &tmp, GuestDiskBusType_lookup, "GuestDiskBusType", name, errp); |+ *obj = tmp; | } Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- v9: mention earlier commit id, enhance commit message v8: no change v7: rebase on typo fix v6: new patch --- scripts/qapi-visit.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)