| Message ID | 20180321134005.8822-3-marcandre.lureau@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | RFC: simplify qobject refcount | expand |
On 21/03/2018 14:40, Marc-André Lureau wrote: > +/* A typecast, checking for the type of arguments */ > +/* QObject is at offset 0, for all QObject-derived types */ > +#define QOBJECT(x) QEMU_GENERIC(x, \ > + (QNull *, (QObject *) x), \ > + (const QNull *, (const QObject *) x), \ > + (QNum *, (QObject *) x), \ > + (const QNum *, (const QObject *) x), \ > + (QString *, (QObject *) x), \ > + (const QString *, (const QObject *) x), \ > + (QDict *, (QObject *) x), \ > + (const QDict *, (const QObject *) x), \ > + (QList *, (QObject *) x), \ > + (const QList *, (const QObject *) x), \ > + (QBool *, (QObject *) x), \ > + (const QBool *, (const QObject *) x), \ > + (QObject *, x), \ > + (const QObject *, x), \ > + qobject_unknown_type(x)) Why not just QEMU_GENERIC(x, (QObject *, x), (const QObject *, x), ({ \ QEMU_BUILD_BUG_ON(offsetof(typeof(*x), base)); &(x)->base; })) That is just an extension of what was being done before, and it is resilient against people putting a random "QObject base" in the middle of a struct. Paolo
On 03/21/2018 08:40 AM, Marc-André Lureau wrote: > Simplify casting to QObject by not requiring the definition of the > type and assuming base QObject is at offset 0. Add a static check for > this. > > Use the QEMU_GENERIC macro for keeping type safety check and error > when given an unknown type. > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > include/qapi/qmp/qobject.h | 36 ++++++++++++++++++++++++++++++++++-- > qobject/qobject.c | 9 +++++++++ > 2 files changed, 43 insertions(+), 2 deletions(-) > +++ b/qobject/qobject.c > @@ -16,6 +16,15 @@ > #include "qapi/qmp/qlist.h" > #include "qapi/qmp/qstring.h" > > +QEMU_BUILD_BUG_MSG( > + offsetof(QNull, base) != 0 || > + offsetof(QNum, base) != 0 || > + offsetof(QString, base) != 0 || > + offsetof(QDict, base) != 0 || > + offsetof(QList, base) != 0 || > + offsetof(QBool, base) != 0, > + "base qobject must be at offset 0"); Interesting. If we do this, we should also simplify qobject_to() from its current: #define qobject_to(type, obj) ({ \ QObject *_tmp = qobject_check_type(obj, glue(QTYPE_CAST_TO_, type)); \ _tmp ? container_of(_tmp, type, base) : (type *)NULL; }) to the simpler: #define qobject_to(type, obj) \ ((type *)qobject_check_type(obj, glue(QTYPE_CAST_TO_, type)))
On 21/03/2018 15:01, Eric Blake wrote: >> > > Interesting. If we do this, we should also simplify qobject_to() from > its current: > > #define qobject_to(type, obj) ({ \ > QObject *_tmp = qobject_check_type(obj, glue(QTYPE_CAST_TO_, type)); \ > _tmp ? container_of(_tmp, type, base) : (type *)NULL; }) > > to the simpler: > > #define qobject_to(type, obj) \ > ((type *)qobject_check_type(obj, glue(QTYPE_CAST_TO_, type))) Yes, indeed! Paolo
Hi On Wed, Mar 21, 2018 at 3:01 PM, Paolo Bonzini <pbonzini@redhat.com> wrote: > On 21/03/2018 14:40, Marc-André Lureau wrote: >> +/* A typecast, checking for the type of arguments */ >> +/* QObject is at offset 0, for all QObject-derived types */ >> +#define QOBJECT(x) QEMU_GENERIC(x, \ >> + (QNull *, (QObject *) x), \ >> + (const QNull *, (const QObject *) x), \ >> + (QNum *, (QObject *) x), \ >> + (const QNum *, (const QObject *) x), \ >> + (QString *, (QObject *) x), \ >> + (const QString *, (const QObject *) x), \ >> + (QDict *, (QObject *) x), \ >> + (const QDict *, (const QObject *) x), \ >> + (QList *, (QObject *) x), \ >> + (const QList *, (const QObject *) x), \ >> + (QBool *, (QObject *) x), \ >> + (const QBool *, (const QObject *) x), \ >> + (QObject *, x), \ >> + (const QObject *, x), \ >> + qobject_unknown_type(x)) > > Why not just > > QEMU_GENERIC(x, > (QObject *, x), > (const QObject *, x), > ({ \ > QEMU_BUILD_BUG_ON(offsetof(typeof(*x), base)); > &(x)->base; > })) > > That is just an extension of what was being done before, and it is > resilient against people putting a random "QObject base" in the middle > of a struct. > Yeah, I tried a few of those approaches. Here the problem is that QObject doesn't have base field. So you get a compile time error with a QObject * as argument. thanks
On 03/21/2018 09:08 AM, Marc-André Lureau wrote: > Hi > > On Wed, Mar 21, 2018 at 3:01 PM, Paolo Bonzini <pbonzini@redhat.com> wrote: >> On 21/03/2018 14:40, Marc-André Lureau wrote: >>> +/* A typecast, checking for the type of arguments */ >>> +/* QObject is at offset 0, for all QObject-derived types */ >>> +#define QOBJECT(x) QEMU_GENERIC(x, \ >>> + (QNull *, (QObject *) x), \ >>> + (const QNull *, (const QObject *) x), \ >>> + (QNum *, (QObject *) x), \ >>> + (const QNum *, (const QObject *) x), \ >>> + (QString *, (QObject *) x), \ >>> + (const QString *, (const QObject *) x), \ >>> + (QDict *, (QObject *) x), \ >>> + (const QDict *, (const QObject *) x), \ >>> + (QList *, (QObject *) x), \ >>> + (const QList *, (const QObject *) x), \ >>> + (QBool *, (QObject *) x), \ >>> + (const QBool *, (const QObject *) x), \ >>> + (QObject *, x), \ >>> + (const QObject *, x), \ >>> + qobject_unknown_type(x)) >> >> Why not just >> >> QEMU_GENERIC(x, >> (QObject *, x), >> (const QObject *, x), >> ({ \ >> QEMU_BUILD_BUG_ON(offsetof(typeof(*x), base)); >> &(x)->base; >> })) >> >> That is just an extension of what was being done before, and it is >> resilient against people putting a random "QObject base" in the middle >> of a struct. >> > > Yeah, I tried a few of those approaches. Here the problem is that > QObject doesn't have base field. So you get a compile time error with > a QObject * as argument. So the compiler requires &(x)->base to resolve even when it is not on the branch that gets selected?
Hi On Wed, Mar 21, 2018 at 3:19 PM, Eric Blake <eblake@redhat.com> wrote: > On 03/21/2018 09:08 AM, Marc-André Lureau wrote: >> >> Hi >> >> On Wed, Mar 21, 2018 at 3:01 PM, Paolo Bonzini <pbonzini@redhat.com> >> wrote: >>> >>> On 21/03/2018 14:40, Marc-André Lureau wrote: >>>> >>>> +/* A typecast, checking for the type of arguments */ >>>> +/* QObject is at offset 0, for all QObject-derived types */ >>>> +#define QOBJECT(x) QEMU_GENERIC(x, \ >>>> + (QNull *, (QObject *) x), \ >>>> + (const QNull *, (const QObject *) x), \ >>>> + (QNum *, (QObject *) x), \ >>>> + (const QNum *, (const QObject *) x), \ >>>> + (QString *, (QObject *) x), \ >>>> + (const QString *, (const QObject *) x), \ >>>> + (QDict *, (QObject *) x), \ >>>> + (const QDict *, (const QObject *) x), \ >>>> + (QList *, (QObject *) x), \ >>>> + (const QList *, (const QObject *) x), \ >>>> + (QBool *, (QObject *) x), \ >>>> + (const QBool *, (const QObject *) x), \ >>>> + (QObject *, x), \ >>>> + (const QObject *, x), \ >>>> + qobject_unknown_type(x)) >>> >>> >>> Why not just >>> >>> QEMU_GENERIC(x, >>> (QObject *, x), >>> (const QObject *, x), >>> ({ \ >>> QEMU_BUILD_BUG_ON(offsetof(typeof(*x), base)); >>> &(x)->base; >>> })) >>> >>> That is just an extension of what was being done before, and it is >>> resilient against people putting a random "QObject base" in the middle >>> of a struct. >>> >> >> Yeah, I tried a few of those approaches. Here the problem is that >> QObject doesn't have base field. So you get a compile time error with >> a QObject * as argument. > > > So the compiler requires &(x)->base to resolve even when it is not on the > branch that gets selected? > Unfortunately, yes, all branches must compile apparently (I know)...
On 21/03/2018 15:21, Marc-André Lureau wrote: >>> Yeah, I tried a few of those approaches. Here the problem is that >>> QObject doesn't have base field. So you get a compile time error with >>> a QObject * as argument. >> >> So the compiler requires &(x)->base to resolve even when it is not on the >> branch that gets selected? > > Unfortunately, yes, all branches must compile apparently (I know)... Ugh, and that's indeed true of _Generic too. These don't compile: struct s1 { int y; }; struct s2 { int z; }; #define f(x) _Generic(x, struct s1: (x).y, struct s2: (x).z) int f1(struct s1 *s) { return f(*s); } int f2(struct s2 *s) { return f(*s); } :( Then I guess Marc-André's realization is ugly but unavoidable. Paolo
On 03/21/2018 09:49 AM, Paolo Bonzini wrote: > On 21/03/2018 15:21, Marc-André Lureau wrote: >>>> Yeah, I tried a few of those approaches. Here the problem is that >>>> QObject doesn't have base field. So you get a compile time error with >>>> a QObject * as argument. >>> >>> So the compiler requires &(x)->base to resolve even when it is not on the >>> branch that gets selected? >> >> Unfortunately, yes, all branches must compile apparently (I know)... > > Ugh, and that's indeed true of _Generic too. These don't compile: > > struct s1 { int y; }; > struct s2 { int z; }; > > #define f(x) _Generic(x, struct s1: (x).y, struct s2: (x).z) > > int f1(struct s1 *s) { return f(*s); } > int f2(struct s2 *s) { return f(*s); } > > :( Then I guess Marc-André's realization is ugly but unavoidable. Not necessarily - can we use multiple layers of macros? (Untested) #define QOBJECT_0(x) x #define QOBJECT_1(x) ({ \ QEMU_BUILD_BUG_ON(offsetof(typeof(*x), base)); \ &(x)->base; }) #define QOBJECT(x) QOBJECT_ ## QEMU_GENERIC(x, \ (QObject *, 0), (const QObject *, 0), 1)(x) or with an additional layer of glue() if needed That is, reduce the QEMU_GENERIC expansion into something that generates only a single preprocessor token, where we then use to decide which OTHER macro to expand, so that we are only evaluating &(x)->base when we selected the derived types.
On 21/03/2018 16:29, Eric Blake wrote: > > Not necessarily - can we use multiple layers of macros? (Untested) > > #define QOBJECT_0(x) x > #define QOBJECT_1(x) ({ \ > QEMU_BUILD_BUG_ON(offsetof(typeof(*x), base)); \ > &(x)->base; }) > #define QOBJECT(x) QOBJECT_ ## QEMU_GENERIC(x, \ > (QObject *, 0), > (const QObject *, 0), > 1)(x) > > or with an additional layer of glue() if needed > > That is, reduce the QEMU_GENERIC expansion into something that generates > only a single preprocessor token, where we then use to decide which > OTHER macro to expand, so that we are only evaluating &(x)->base when we > selected the derived types. I don't think so. Macro expansion happens way earlier. Paolo
Hi On Wed, Mar 21, 2018 at 3:01 PM, Paolo Bonzini <pbonzini@redhat.com> wrote: > On 21/03/2018 14:40, Marc-André Lureau wrote: >> +/* A typecast, checking for the type of arguments */ >> +/* QObject is at offset 0, for all QObject-derived types */ >> +#define QOBJECT(x) QEMU_GENERIC(x, \ >> + (QNull *, (QObject *) x), \ >> + (const QNull *, (const QObject *) x), \ >> + (QNum *, (QObject *) x), \ >> + (const QNum *, (const QObject *) x), \ >> + (QString *, (QObject *) x), \ >> + (const QString *, (const QObject *) x), \ >> + (QDict *, (QObject *) x), \ >> + (const QDict *, (const QObject *) x), \ >> + (QList *, (QObject *) x), \ >> + (const QList *, (const QObject *) x), \ >> + (QBool *, (QObject *) x), \ >> + (const QBool *, (const QObject *) x), \ >> + (QObject *, x), \ >> + (const QObject *, x), \ >> + qobject_unknown_type(x)) > > Why not just > > QEMU_GENERIC(x, > (QObject *, x), > (const QObject *, x), > ({ \ > QEMU_BUILD_BUG_ON(offsetof(typeof(*x), base)); > &(x)->base; > })) > > That is just an extension of what was being done before, and it is > resilient against people putting a random "QObject base" in the middle > of a struct. > That would work with an anonymous union though: struct QObject { union { QType type; QType base; }; size_t refcnt; }; If it's acceptable, I think I'll take this approach.
On 21/03/2018 16:55, Marc-André Lureau wrote: > That would work with an anonymous union though: > > struct QObject { > union { > QType type; > QType base; > }; > size_t refcnt; > }; > > If it's acceptable, I think I'll take this approach. You don't need QEMU_GENERIC at all then, don't you? Paolo
Hi On Wed, Mar 21, 2018 at 5:00 PM, Paolo Bonzini <pbonzini@redhat.com> wrote: > On 21/03/2018 16:55, Marc-André Lureau wrote: >> That would work with an anonymous union though: >> >> struct QObject { >> union { >> QType type; >> QType base; >> }; >> size_t refcnt; >> }; >> >> If it's acceptable, I think I'll take this approach. > > You don't need QEMU_GENERIC at all then, don't you? Ah ah, that's what I thought too, but the type of QObject * &x->base isn't QObject * :) I don't think we can fool it there (without loosing some type safety)
On 21/03/2018 17:11, Marc-André Lureau wrote: > Hi > > On Wed, Mar 21, 2018 at 5:00 PM, Paolo Bonzini <pbonzini@redhat.com> wrote: >> On 21/03/2018 16:55, Marc-André Lureau wrote: >>> That would work with an anonymous union though: >>> >>> struct QObject { >>> union { >>> QType type; >>> QType base; >>> }; >>> size_t refcnt; >>> }; >>> >>> If it's acceptable, I think I'll take this approach. >> >> You don't need QEMU_GENERIC at all then, don't you? > > Ah ah, that's what I thought too, but the type of QObject * &x->base > isn't QObject * :) > > I don't think we can fool it there (without loosing some type safety) Hmm, perhaps by making it "struct {} base"? Or even: struct QObjectCommon { QType type; size_t refcnt; } struct QObject { QObjectCommon base; } struct QString { QObjectCommon base; ... } Paolo
Hi On Wed, Mar 21, 2018 at 5:23 PM, Paolo Bonzini <pbonzini@redhat.com> wrote: > On 21/03/2018 17:11, Marc-André Lureau wrote: >> Hi >> >> On Wed, Mar 21, 2018 at 5:00 PM, Paolo Bonzini <pbonzini@redhat.com> wrote: >>> On 21/03/2018 16:55, Marc-André Lureau wrote: >>>> That would work with an anonymous union though: >>>> >>>> struct QObject { >>>> union { >>>> QType type; >>>> QType base; >>>> }; >>>> size_t refcnt; >>>> }; >>>> >>>> If it's acceptable, I think I'll take this approach. >>> >>> You don't need QEMU_GENERIC at all then, don't you? >> >> Ah ah, that's what I thought too, but the type of QObject * &x->base >> isn't QObject * :) >> >> I don't think we can fool it there (without loosing some type safety) > > Hmm, perhaps by making it "struct {} base"? > > Or even: > > struct QObjectCommon { > QType type; > size_t refcnt; > } > > struct QObject { > QObjectCommon base; > } > > struct QString { > QObjectCommon base; > ... > } I fail to see what that solves. You are moving the problem to QObjectCommon: we would have to replace all QObject * user to take QObjectCommon *, and then we wouldn't solve the problem for QObjectCommon.
On 03/21/2018 11:59 AM, Marc-André Lureau wrote: >> Hmm, perhaps by making it "struct {} base"? >> >> Or even: >> >> struct QObjectCommon { >> QType type; >> size_t refcnt; >> } >> >> struct QObject { >> QObjectCommon base; >> } >> >> struct QString { >> QObjectCommon base; >> ... >> } > > I fail to see what that solves. You are moving the problem to > QObjectCommon: we would have to replace all QObject * user to take > QObjectCommon *, and then we wouldn't solve the problem for > QObjectCommon. Here, QObjectCommon is never used outside of qobject.h; all existing code continues to uwe QObject *. But the conversion from QString* or QObject * to QObjectCommon * is trivial (&obj->base), and the conversion from QObjectCommon * to the public QObject * is trivial (container_of style cast). So the QOBJECT() macro becomes comparable to a C++ reinterpret_cast<>() that converts any descendent of QObjectCommon into QObject, even if the original type was not QObject; and we never offer a public API to convert anything into or out of QObjectCommon (it only exists as a dummy type to make our other casts easier).
On Wed, Mar 21, 2018 at 6:08 PM, Eric Blake <eblake@redhat.com> wrote: > On 03/21/2018 11:59 AM, Marc-André Lureau wrote: >>> >>> Hmm, perhaps by making it "struct {} base"? >>> >>> Or even: >>> >>> struct QObjectCommon { >>> QType type; >>> size_t refcnt; >>> } >>> >>> struct QObject { >>> QObjectCommon base; >>> } >>> >>> struct QString { >>> QObjectCommon base; >>> ... >>> } >> >> >> I fail to see what that solves. You are moving the problem to >> QObjectCommon: we would have to replace all QObject * user to take >> QObjectCommon *, and then we wouldn't solve the problem for >> QObjectCommon. > > > Here, QObjectCommon is never used outside of qobject.h; all existing code > continues to uwe QObject *. But the conversion from QString* or QObject * > to QObjectCommon * is trivial (&obj->base), and the conversion from > QObjectCommon * to the public QObject * is trivial (container_of style > cast). So the QOBJECT() macro becomes comparable to a C++ > reinterpret_cast<>() that converts any descendent of QObjectCommon into > QObject, even if the original type was not QObject; and we never offer a > public API to convert anything into or out of QObjectCommon (it only exists > as a dummy type to make our other casts easier). What is the QOJECT() macro you proposed with that? thanks
On 03/21/2018 12:23 PM, Marc-André Lureau wrote: >>>> struct QObjectCommon { >>>> QType type; >>>> size_t refcnt; >>>> } >>>> >>>> struct QObject { >>>> QObjectCommon base; >>>> } >>>> >>>> struct QString { >>>> QObjectCommon base; >>>> ... >>>> } > What is the QOJECT() macro you proposed with that? Here's what you can do while still leaving all "subtypes" of QObject free to put base at a non-zero offset: #define QOBJECT(o) ((QObject *)(&(o)->base)) QEMU_BUILD_BUG_MSG(offsetof(QObject, base), "base of QObject must be at offset 0"); #define qobject_to(type, obj) ({ \ QObjectCommon *_tmp = qobject_check_type(obj, glue(QTYPE_CAST_TO_, type)); \ _tmp ? container_of(_tmp, type, base) : (type *)NULL; }) static inline QObjectCommon *qobject_check_type(const QObject *obj, QType type) { if (obj && qobject_type(obj) == type) { return QOBJECT(obj); } else { return NULL; } } Or, you keep your idea of requiring ALL subtypes to put base at offset 0, so that qobject_to() also becomes simpler.
On 21/03/2018 18:23, Marc-André Lureau wrote: >> >> Here, QObjectCommon is never used outside of qobject.h; all existing code >> continues to uwe QObject *. But the conversion from QString* or QObject * >> to QObjectCommon * is trivial (&obj->base), and the conversion from >> QObjectCommon * to the public QObject * is trivial (container_of style >> cast). So the QOBJECT() macro becomes comparable to a C++ >> reinterpret_cast<>() that converts any descendent of QObjectCommon into >> QObject, even if the original type was not QObject; and we never offer a >> public API to convert anything into or out of QObjectCommon (it only exists >> as a dummy type to make our other casts easier). > What is the QOJECT() macro you proposed with that? #define QOBJECT(x) \ container_of(&(x)->base, QObject, base) where the double dereference-and-container_of provides some type safety (container_of fails if &(x)->base is not a QObjectCommon*). Paolo
diff --git a/include/qapi/qmp/qobject.h b/include/qapi/qmp/qobject.h index e022707578..d0aead35ec 100644 --- a/include/qapi/qmp/qobject.h +++ b/include/qapi/qmp/qobject.h @@ -39,8 +39,40 @@ struct QObject { size_t refcnt; }; -/* Get the 'base' part of an object */ -#define QOBJECT(obj) (&(obj)->base) +/* This function gives an error if an invalid pointer type is passed + * to QOBJECT. For optimized builds, we can rely on dead-code + * elimination from the compiler, and give the errors already at link + * time. + */ +#if defined(__OPTIMIZE__) && !defined(__SANITIZE_ADDRESS__) +const void * qobject_unknown_type(const void *); +#else +static inline const void * +qobject_unknown_type(const void *unused) +{ + abort(); + return NULL; +} +#endif + +/* A typecast, checking for the type of arguments */ +/* QObject is at offset 0, for all QObject-derived types */ +#define QOBJECT(x) QEMU_GENERIC(x, \ + (QNull *, (QObject *) x), \ + (const QNull *, (const QObject *) x), \ + (QNum *, (QObject *) x), \ + (const QNum *, (const QObject *) x), \ + (QString *, (QObject *) x), \ + (const QString *, (const QObject *) x), \ + (QDict *, (QObject *) x), \ + (const QDict *, (const QObject *) x), \ + (QList *, (QObject *) x), \ + (const QList *, (const QObject *) x), \ + (QBool *, (QObject *) x), \ + (const QBool *, (const QObject *) x), \ + (QObject *, x), \ + (const QObject *, x), \ + qobject_unknown_type(x)) /* High-level interface for qobject_incref() */ #define QINCREF(obj) \ diff --git a/qobject/qobject.c b/qobject/qobject.c index 23600aa1c1..87649c5be5 100644 --- a/qobject/qobject.c +++ b/qobject/qobject.c @@ -16,6 +16,15 @@ #include "qapi/qmp/qlist.h" #include "qapi/qmp/qstring.h" +QEMU_BUILD_BUG_MSG( + offsetof(QNull, base) != 0 || + offsetof(QNum, base) != 0 || + offsetof(QString, base) != 0 || + offsetof(QDict, base) != 0 || + offsetof(QList, base) != 0 || + offsetof(QBool, base) != 0, + "base qobject must be at offset 0"); + static void (*qdestroy[QTYPE__MAX])(QObject *) = { [QTYPE_NONE] = NULL, /* No such object exists */ [QTYPE_QNULL] = NULL, /* qnull_ is indestructible */
Simplify casting to QObject by not requiring the definition of the type and assuming base QObject is at offset 0. Add a static check for this. Use the QEMU_GENERIC macro for keeping type safety check and error when given an unknown type. Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- include/qapi/qmp/qobject.h | 36 ++++++++++++++++++++++++++++++++++-- qobject/qobject.c | 9 +++++++++ 2 files changed, 43 insertions(+), 2 deletions(-)