[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 7/7] qobject atomics osdep: Make a few macros more hygieni
From: |
Markus Armbruster |
Subject: |
Re: [PATCH v2 7/7] qobject atomics osdep: Make a few macros more hygienic |
Date: |
Thu, 21 Sep 2023 07:14:48 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/28.2 (gnu/linux) |
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!
- Re: [PATCH v2 6/7] block: Clean up local variable shadowing, (continued)
- [PATCH v2 5/7] block/vdi: Clean up local variable shadowing, Markus Armbruster, 2023/09/20
- [PATCH v2 1/7] migration/rdma: Fix save_page method to fail on polling error, Markus Armbruster, 2023/09/20
- [PATCH v2 4/7] block/dirty-bitmap: Clean up local variable shadowing, Markus Armbruster, 2023/09/20
- [PATCH v2 2/7] migration: Clean up local variable shadowing, Markus Armbruster, 2023/09/20
- [PATCH v2 7/7] qobject atomics osdep: Make a few macros more hygienic, Markus Armbruster, 2023/09/20
- [PATCH v2 3/7] ui: Clean up local variable shadowing, Markus Armbruster, 2023/09/20