[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: |
Kevin Wolf |
Subject: |
Re: [PATCH v2 7/7] qobject atomics osdep: Make a few macros more hygienic |
Date: |
Thu, 21 Sep 2023 10:42:57 +0200 |
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
- [PATCH v2 5/7] block/vdi: 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