| Message ID | 20230920183149.1105333-8-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/7] migration/rdma: Fix save_page method to fail on polling error | expand |
On Wed, Sep 20, 2023 at 08:31:49PM +0200, Markus Armbruster wrote: ... > The only reliable way to prevent unintended variable name capture is > -Wshadow. > > One blocker for enabling it is shadowing hiding in function-like > macros like > > qdict_put(dict, "name", qobject_ref(...)) > > qdict_put() wraps its last argument in QOBJECT(), and the last > argument here contains another QOBJECT(). > > Use dark preprocessor sorcery to make the macros that give us this > problem use different variable names on every call. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > Reviewed-by: Eric Blake <eblake@redhat.com> It's changed (for the better) since v1, so I'm re-reviewing. > --- > include/qapi/qmp/qobject.h | 11 +++++++++-- > include/qemu/atomic.h | 17 ++++++++++++----- > include/qemu/compiler.h | 3 +++ > include/qemu/osdep.h | 31 +++++++++++++++++++++++-------- > 4 files changed, 47 insertions(+), 15 deletions(-) > > diff --git a/include/qapi/qmp/qobject.h b/include/qapi/qmp/qobject.h > index 9003b71fd3..d36cc97805 100644 > --- a/include/qapi/qmp/qobject.h > +++ b/include/qapi/qmp/qobject.h > @@ -45,10 +45,17 @@ struct QObject { > struct QObjectBase_ base; > }; > > -#define QOBJECT(obj) ({ \ > +/* > + * Preprocessory sorcery ahead: use a different identifier for the s/Preprocessory/Preprocessor/ (multiple times in the patch) > + * local variable in each expansion, so we can nest macro calls > + * without shadowing variables. > + */ > +#define QOBJECT_INTERNAL(obj, _obj) ({ \ > typeof(obj) _obj = (obj); \ > - _obj ? container_of(&(_obj)->base, QObject, base) : NULL; \ > + _obj \ > + ? container_of(&(_obj)->base, QObject, base) : NULL; \ As pointed out before, you can write &_obj->base instead of &(_obj)->base, now that we know _obj is a single identifier rather than an arbitrary expression. Not strictly necessary since the extra () doesn't change semantics... > }) > +#define QOBJECT(obj) QOBJECT_INTERNAL((obj), MAKE_IDENTFIER(_obj)) > > /* Required for qobject_to() */ > #define QTYPE_CAST_TO_QNull QTYPE_QNULL > diff --git a/include/qemu/atomic.h b/include/qemu/atomic.h > index d95612f7a0..d4cbd01909 100644 > --- a/include/qemu/atomic.h > +++ b/include/qemu/atomic.h > @@ -157,13 +157,20 @@ > smp_read_barrier_depends(); > #endif > > -#define qatomic_rcu_read(ptr) \ > - ({ \ > +/* > + * Preprocessory sorcery ahead: use a different identifier for the > + * local variable in each expansion, so we can nest macro calls > + * without shadowing variables. > + */ > +#define qatomic_rcu_read_internal(ptr, _val) \ > + ({ \ > qemu_build_assert(sizeof(*ptr) <= ATOMIC_REG_SIZE); \ > - typeof_strip_qual(*ptr) _val; \ > - qatomic_rcu_read__nocheck(ptr, &_val); \ > - _val; \ > + typeof_strip_qual(*ptr) _val; \ > + qatomic_rcu_read__nocheck(ptr, &_val); \ ...but it looks odd for the patch to not be consistent on that front. > + _val; \ > }) > +#define qatomic_rcu_read(ptr) \ > + qatomic_rcu_read_internal((ptr), MAKE_IDENTFIER(_val)) > > #define qatomic_rcu_set(ptr, i) do { \ > qemu_build_assert(sizeof(*ptr) <= ATOMIC_REG_SIZE); \ > diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h > index a309f90c76..03236d830c 100644 > --- a/include/qemu/compiler.h > +++ b/include/qemu/compiler.h > @@ -37,6 +37,9 @@ > #define tostring(s) #s > #endif > > +/* Expands into an identifier stemN, where N is another number each time */ > +#define MAKE_IDENTFIER(stem) glue(stem, __COUNTER__) I like how this turned out. With the spelling fix, and optionally with the redundant () dropped, you can keep my R-b.
Eric Blake <eblake@redhat.com> writes: > On Wed, Sep 20, 2023 at 08:31:49PM +0200, Markus Armbruster wrote: > ... >> The only reliable way to prevent unintended variable name capture is >> -Wshadow. >> >> One blocker for enabling it is shadowing hiding in function-like >> macros like >> >> qdict_put(dict, "name", qobject_ref(...)) >> >> qdict_put() wraps its last argument in QOBJECT(), and the last >> argument here contains another QOBJECT(). >> >> Use dark preprocessor sorcery to make the macros that give us this >> problem use different variable names on every call. >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> Reviewed-by: Eric Blake <eblake@redhat.com> > > It's changed (for the better) since v1, so I'm re-reviewing. > >> --- >> include/qapi/qmp/qobject.h | 11 +++++++++-- >> include/qemu/atomic.h | 17 ++++++++++++----- >> include/qemu/compiler.h | 3 +++ >> include/qemu/osdep.h | 31 +++++++++++++++++++++++-------- >> 4 files changed, 47 insertions(+), 15 deletions(-) >> >> diff --git a/include/qapi/qmp/qobject.h b/include/qapi/qmp/qobject.h >> index 9003b71fd3..d36cc97805 100644 >> --- a/include/qapi/qmp/qobject.h >> +++ b/include/qapi/qmp/qobject.h >> @@ -45,10 +45,17 @@ struct QObject { >> struct QObjectBase_ base; >> }; >> >> -#define QOBJECT(obj) ({ \ >> +/* >> + * Preprocessory sorcery ahead: use a different identifier for the > > s/Preprocessory/Preprocessor/ (multiple times in the patch) Dang! Will fix. >> + * local variable in each expansion, so we can nest macro calls >> + * without shadowing variables. >> + */ >> +#define QOBJECT_INTERNAL(obj, _obj) ({ \ >> typeof(obj) _obj = (obj); \ >> - _obj ? container_of(&(_obj)->base, QObject, base) : NULL; \ >> + _obj \ >> + ? container_of(&(_obj)->base, QObject, base) : NULL; \ > > As pointed out before, you can write &_obj->base instead of > &(_obj)->base, now that we know _obj is a single identifier rather > than an arbitrary expression. Not strictly necessary since the extra > () doesn't change semantics... Makes sense, I just forgot here. >> }) >> +#define QOBJECT(obj) QOBJECT_INTERNAL((obj), MAKE_IDENTFIER(_obj)) >> >> /* Required for qobject_to() */ >> #define QTYPE_CAST_TO_QNull QTYPE_QNULL >> diff --git a/include/qemu/atomic.h b/include/qemu/atomic.h >> index d95612f7a0..d4cbd01909 100644 >> --- a/include/qemu/atomic.h >> +++ b/include/qemu/atomic.h >> @@ -157,13 +157,20 @@ >> smp_read_barrier_depends(); >> #endif >> >> -#define qatomic_rcu_read(ptr) \ >> - ({ \ >> +/* >> + * Preprocessory sorcery ahead: use a different identifier for the >> + * local variable in each expansion, so we can nest macro calls >> + * without shadowing variables. >> + */ >> +#define qatomic_rcu_read_internal(ptr, _val) \ >> + ({ \ >> qemu_build_assert(sizeof(*ptr) <= ATOMIC_REG_SIZE); \ >> - typeof_strip_qual(*ptr) _val; \ >> - qatomic_rcu_read__nocheck(ptr, &_val); \ >> - _val; \ >> + typeof_strip_qual(*ptr) _val; \ >> + qatomic_rcu_read__nocheck(ptr, &_val); \ > > ...but it looks odd for the patch to not be consistent on that front. > >> + _val; \ >> }) >> +#define qatomic_rcu_read(ptr) \ >> + qatomic_rcu_read_internal((ptr), MAKE_IDENTFIER(_val)) >> >> #define qatomic_rcu_set(ptr, i) do { \ >> qemu_build_assert(sizeof(*ptr) <= ATOMIC_REG_SIZE); \ >> diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h >> index a309f90c76..03236d830c 100644 >> --- a/include/qemu/compiler.h >> +++ b/include/qemu/compiler.h >> @@ -37,6 +37,9 @@ >> #define tostring(s) #s >> #endif >> >> +/* Expands into an identifier stemN, where N is another number each time */ >> +#define MAKE_IDENTFIER(stem) glue(stem, __COUNTER__) > > I like how this turned out. > > With the spelling fix, and optionally with the redundant () dropped, > you can keep my R-b. Thanks!
Am 20.09.2023 um 20:31 hat Markus Armbruster geschrieben: > Variables declared in macros can shadow other variables. Much of the > time, this is harmless, e.g.: > > #define _FDT(exp) \ > do { \ > int ret = (exp); \ > if (ret < 0) { \ > error_report("error creating device tree: %s: %s", \ > #exp, fdt_strerror(ret)); \ > exit(1); \ > } \ > } while (0) > > Harmless shadowing in h_client_architecture_support(): > > target_ulong ret; > > [...] > > ret = do_client_architecture_support(cpu, spapr, vec, fdt_bufsize); > if (ret == H_SUCCESS) { > _FDT((fdt_pack(spapr->fdt_blob))); > [...] > } > > return ret; > > However, we can get in trouble when the shadowed variable is used in a > macro argument: > > #define QOBJECT(obj) ({ \ > typeof(obj) o = (obj); \ > o ? container_of(&(o)->base, QObject, base) : NULL; \ > }) > > QOBJECT(o) expands into > > ({ > ---> typeof(o) o = (o); > o ? container_of(&(o)->base, QObject, base) : NULL; > }) > > Unintended variable name capture at --->. We'd be saved by > -Winit-self. But I could certainly construct more elaborate death > traps that don't trigger it. > > To reduce the risk of trapping ourselves, we use variable names in > macros that no sane person would use elsewhere. Here's our actual > definition of QOBJECT(): > > #define QOBJECT(obj) ({ \ > typeof(obj) _obj = (obj); \ > _obj ? container_of(&(_obj)->base, QObject, base) : NULL; \ > }) > > Works well enough until we nest macro calls. For instance, with > > #define qobject_ref(obj) ({ \ > typeof(obj) _obj = (obj); \ > qobject_ref_impl(QOBJECT(_obj)); \ > _obj; \ > }) > > the expression qobject_ref(obj) expands into > > ({ > typeof(obj) _obj = (obj); > qobject_ref_impl( > ({ > ---> typeof(_obj) _obj = (_obj); > _obj ? container_of(&(_obj)->base, QObject, base) : NULL; > })); > _obj; > }) > > Unintended variable name capture at --->. > > The only reliable way to prevent unintended variable name capture is > -Wshadow. > > One blocker for enabling it is shadowing hiding in function-like > macros like > > qdict_put(dict, "name", qobject_ref(...)) > > qdict_put() wraps its last argument in QOBJECT(), and the last > argument here contains another QOBJECT(). > > Use dark preprocessor sorcery to make the macros that give us this > problem use different variable names on every call. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > Reviewed-by: Eric Blake <eblake@redhat.com> > --- > include/qapi/qmp/qobject.h | 11 +++++++++-- > include/qemu/atomic.h | 17 ++++++++++++----- > include/qemu/compiler.h | 3 +++ > include/qemu/osdep.h | 31 +++++++++++++++++++++++-------- > 4 files changed, 47 insertions(+), 15 deletions(-) > > diff --git a/include/qapi/qmp/qobject.h b/include/qapi/qmp/qobject.h > index 9003b71fd3..d36cc97805 100644 > --- a/include/qapi/qmp/qobject.h > +++ b/include/qapi/qmp/qobject.h > @@ -45,10 +45,17 @@ struct QObject { > struct QObjectBase_ base; > }; > > -#define QOBJECT(obj) ({ \ > +/* > + * Preprocessory sorcery ahead: use a different identifier for the > + * local variable in each expansion, so we can nest macro calls > + * without shadowing variables. > + */ > +#define QOBJECT_INTERNAL(obj, _obj) ({ \ > typeof(obj) _obj = (obj); \ > - _obj ? container_of(&(_obj)->base, QObject, base) : NULL; \ > + _obj \ > + ? container_of(&(_obj)->base, QObject, base) : NULL; \ What happened here? The code in this line (or now two lines) seems to be unchanged apart from a strange looking newline. > }) > +#define QOBJECT(obj) QOBJECT_INTERNAL((obj), MAKE_IDENTFIER(_obj)) Kevin
Kevin Wolf <kwolf@redhat.com> writes: > Am 20.09.2023 um 20:31 hat Markus Armbruster geschrieben: [...] >> diff --git a/include/qapi/qmp/qobject.h b/include/qapi/qmp/qobject.h >> index 9003b71fd3..d36cc97805 100644 >> --- a/include/qapi/qmp/qobject.h >> +++ b/include/qapi/qmp/qobject.h >> @@ -45,10 +45,17 @@ struct QObject { >> struct QObjectBase_ base; >> }; >> >> -#define QOBJECT(obj) ({ \ >> +/* >> + * Preprocessory sorcery ahead: use a different identifier for the >> + * local variable in each expansion, so we can nest macro calls >> + * without shadowing variables. >> + */ >> +#define QOBJECT_INTERNAL(obj, _obj) ({ \ >> typeof(obj) _obj = (obj); \ >> - _obj ? container_of(&(_obj)->base, QObject, base) : NULL; \ >> + _obj \ >> + ? container_of(&(_obj)->base, QObject, base) : NULL; \ > > What happened here? The code in this line (or now two lines) seems to be > unchanged apart from a strange looking newline. Accident, will fix, thanks! >> }) >> +#define QOBJECT(obj) QOBJECT_INTERNAL((obj), MAKE_IDENTFIER(_obj)) > > Kevin
diff --git a/include/qapi/qmp/qobject.h b/include/qapi/qmp/qobject.h index 9003b71fd3..d36cc97805 100644 --- a/include/qapi/qmp/qobject.h +++ b/include/qapi/qmp/qobject.h @@ -45,10 +45,17 @@ struct QObject { struct QObjectBase_ base; }; -#define QOBJECT(obj) ({ \ +/* + * Preprocessory sorcery ahead: use a different identifier for the + * local variable in each expansion, so we can nest macro calls + * without shadowing variables. + */ +#define QOBJECT_INTERNAL(obj, _obj) ({ \ typeof(obj) _obj = (obj); \ - _obj ? container_of(&(_obj)->base, QObject, base) : NULL; \ + _obj \ + ? container_of(&(_obj)->base, QObject, base) : NULL; \ }) +#define QOBJECT(obj) QOBJECT_INTERNAL((obj), MAKE_IDENTFIER(_obj)) /* Required for qobject_to() */ #define QTYPE_CAST_TO_QNull QTYPE_QNULL diff --git a/include/qemu/atomic.h b/include/qemu/atomic.h index d95612f7a0..d4cbd01909 100644 --- a/include/qemu/atomic.h +++ b/include/qemu/atomic.h @@ -157,13 +157,20 @@ smp_read_barrier_depends(); #endif -#define qatomic_rcu_read(ptr) \ - ({ \ +/* + * Preprocessory sorcery ahead: use a different identifier for the + * local variable in each expansion, so we can nest macro calls + * without shadowing variables. + */ +#define qatomic_rcu_read_internal(ptr, _val) \ + ({ \ qemu_build_assert(sizeof(*ptr) <= ATOMIC_REG_SIZE); \ - typeof_strip_qual(*ptr) _val; \ - qatomic_rcu_read__nocheck(ptr, &_val); \ - _val; \ + typeof_strip_qual(*ptr) _val; \ + qatomic_rcu_read__nocheck(ptr, &_val); \ + _val; \ }) +#define qatomic_rcu_read(ptr) \ + qatomic_rcu_read_internal((ptr), MAKE_IDENTFIER(_val)) #define qatomic_rcu_set(ptr, i) do { \ qemu_build_assert(sizeof(*ptr) <= ATOMIC_REG_SIZE); \ diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h index a309f90c76..03236d830c 100644 --- a/include/qemu/compiler.h +++ b/include/qemu/compiler.h @@ -37,6 +37,9 @@ #define tostring(s) #s #endif +/* Expands into an identifier stemN, where N is another number each time */ +#define MAKE_IDENTFIER(stem) glue(stem, __COUNTER__) + #ifndef likely #define likely(x) __builtin_expect(!!(x), 1) #define unlikely(x) __builtin_expect(!!(x), 0) diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h index 2897720fac..0d39bb1876 100644 --- a/include/qemu/osdep.h +++ b/include/qemu/osdep.h @@ -383,19 +383,28 @@ void QEMU_ERROR("code path is reachable") * determined by the pre-processor instead of the compiler, you'll * have to open-code it. Sadly, Coverity is severely confused by the * constant variants, so we have to dumb things down there. + * + * Preprocessory sorcery ahead: use different identifiers for the + * local variables in each expansion, so we can nest macro calls + * without shadowing variables. */ -#undef MIN -#define MIN(a, b) \ +#define MIN_INTERNAL(a, b, _a, _b) \ ({ \ typeof(1 ? (a) : (b)) _a = (a), _b = (b); \ _a < _b ? _a : _b; \ }) -#undef MAX -#define MAX(a, b) \ +#undef MIN +#define MIN(a, b) \ + MIN_INTERNAL((a), (b), MAKE_IDENTFIER(_a), MAKE_IDENTFIER(_b)) + +#define MAX_INTERNAL(a, b, _a, _b) \ ({ \ typeof(1 ? (a) : (b)) _a = (a), _b = (b); \ _a > _b ? _a : _b; \ }) +#undef MAX +#define MAX(a, b) \ + MAX_INTERNAL((a), (b), MAKE_IDENTFIER(_a), MAKE_IDENTFIER(_b)) #ifdef __COVERITY__ # define MIN_CONST(a, b) ((a) < (b) ? (a) : (b)) @@ -416,14 +425,20 @@ void QEMU_ERROR("code path is reachable") /* * Minimum function that returns zero only if both values are zero. * Intended for use with unsigned values only. + * + * Preprocessory sorcery ahead: use different identifiers for the + * local variables in each expansion, so we can nest macro calls + * without shadowing variables. */ -#ifndef MIN_NON_ZERO -#define MIN_NON_ZERO(a, b) \ +#define MIN_NON_ZERO_INTERNAL(a, b, _a, _b) \ ({ \ typeof(1 ? (a) : (b)) _a = (a), _b = (b); \ - _a == 0 ? _b : (_b == 0 || _b > _a) ? _a : _b; \ + _a == 0 ? _b \ + : (_b == 0 || _b > _a) ? _a \ + : _b; \ }) -#endif +#define MIN_NON_ZERO(a, b) \ + MIN_NON_ZERO_INTERNAL((a), (b), MAKE_IDENTFIER(_a), MAKE_IDENTFIER(_b)) /* * Round number down to multiple. Safe when m is not a power of 2 (see
Variables declared in macros can shadow other variables. Much of the time, this is harmless, e.g.: #define _FDT(exp) \ do { \ int ret = (exp); \ if (ret < 0) { \ error_report("error creating device tree: %s: %s", \ #exp, fdt_strerror(ret)); \ exit(1); \ } \ } while (0) Harmless shadowing in h_client_architecture_support(): target_ulong ret; [...] ret = do_client_architecture_support(cpu, spapr, vec, fdt_bufsize); if (ret == H_SUCCESS) { _FDT((fdt_pack(spapr->fdt_blob))); [...] } return ret; However, we can get in trouble when the shadowed variable is used in a macro argument: #define QOBJECT(obj) ({ \ typeof(obj) o = (obj); \ o ? container_of(&(o)->base, QObject, base) : NULL; \ }) QOBJECT(o) expands into ({ ---> typeof(o) o = (o); o ? container_of(&(o)->base, QObject, base) : NULL; }) Unintended variable name capture at --->. We'd be saved by -Winit-self. But I could certainly construct more elaborate death traps that don't trigger it. To reduce the risk of trapping ourselves, we use variable names in macros that no sane person would use elsewhere. Here's our actual definition of QOBJECT(): #define QOBJECT(obj) ({ \ typeof(obj) _obj = (obj); \ _obj ? container_of(&(_obj)->base, QObject, base) : NULL; \ }) Works well enough until we nest macro calls. For instance, with #define qobject_ref(obj) ({ \ typeof(obj) _obj = (obj); \ qobject_ref_impl(QOBJECT(_obj)); \ _obj; \ }) the expression qobject_ref(obj) expands into ({ typeof(obj) _obj = (obj); qobject_ref_impl( ({ ---> typeof(_obj) _obj = (_obj); _obj ? container_of(&(_obj)->base, QObject, base) : NULL; })); _obj; }) Unintended variable name capture at --->. The only reliable way to prevent unintended variable name capture is -Wshadow. One blocker for enabling it is shadowing hiding in function-like macros like qdict_put(dict, "name", qobject_ref(...)) qdict_put() wraps its last argument in QOBJECT(), and the last argument here contains another QOBJECT(). Use dark preprocessor sorcery to make the macros that give us this problem use different variable names on every call. Signed-off-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> --- include/qapi/qmp/qobject.h | 11 +++++++++-- include/qemu/atomic.h | 17 ++++++++++++----- include/qemu/compiler.h | 3 +++ include/qemu/osdep.h | 31 +++++++++++++++++++++++-------- 4 files changed, 47 insertions(+), 15 deletions(-)