Re: [PATCH v3 7/7] qobject atomics osdep: Make a few macros more hygienic

[Philippe Mathieu-Daudé <philmd@xxxxxxxxxx>]

On 21/9/23 14:13, Markus Armbruster wrote:
> 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@xxxxxxxxxx>
> Reviewed-by: Eric Blake <eblake@xxxxxxxxxx>
> ---
>   include/qapi/qmp/qobject.h | 10 ++++++++--
>   include/qemu/atomic.h      | 17 ++++++++++++-----
>   include/qemu/compiler.h    |  3 +++
>   include/qemu/osdep.h       | 27 ++++++++++++++++++++-------
>   4 files changed, 43 insertions(+), 14 deletions(-)
Reviewed-by: Philippe Mathieu-Daudé <philmd@xxxxxxxxxx>