Re: [PULL v2 00/61] Misc patches for soft freeze

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 3/17/20 3:47 PM, Paolo Bonzini wrote:
> On 17/03/20 15:26, Stefan Hajnoczi wrote:
>> Yes, looks like the compiler can't figure out the control flow on
>> NetBSD.
>>
>> We could drop the WITH_QEMU_LOCK_GUARD() macro and use this idiom
>> instead:
>>
>>    {
>>        QEMU_LOCK_GUARD(&mutex);
>>        ...
>>    }
>>
>> But it's unusual for C code to create scopes without a statement (for,
>> if, while).
> 
> After staring at compiler dumps for a while I have just concluded that
> this could actually be considered a bug in WITH_QEMU_LOCK_GUARD.
> 
> QEMU_MAKE_LOCKABLE returns NULL if passed a NULL argument.  This is the
> root cause of the NetBSD failure, as the compiler doesn't figure out
> that &timer_list->active_timers_lock is non-NULL and therefore doesn't
> simplify the qemu_make_lockable function.
> 
> But why does that cause an uninitialized variable warning?  Because if
> WITH_QEMU_LOCK_GUARD were passed NULL, it would not execute its body!
> 
> So I'm going to squash the following in the series, mostly through a new
> patch "lockable: introduce QEMU_MAKE_LOCKABLE_NONNULL":
> 
> diff --git a/include/qemu/lockable.h b/include/qemu/lockable.h
> index 44b3f4b..1aeb2cb 100644
> --- a/include/qemu/lockable.h
> +++ b/include/qemu/lockable.h
> @@ -67,7 +67,7 @@ qemu_make_lockable(void *x, QemuLockable *lockable)
>    * In C++ it would be different, but then C++ wouldn't need QemuLockable
>    * either...
>    */
> -#define QEMU_MAKE_LOCKABLE_(x) qemu_make_lockable((x), &(QemuLockable) {    \
> +#define QEMU_MAKE_LOCKABLE_(x) (&(QemuLockable) {    \
>           .object = (x),                               \
>           .lock = QEMU_LOCK_FUNC(x),                   \
>           .unlock = QEMU_UNLOCK_FUNC(x),               \
> @@ -75,14 +75,27 @@ qemu_make_lockable(void *x, QemuLockable *lockable)
>   
>   /* QEMU_MAKE_LOCKABLE - Make a polymorphic QemuLockable
>    *
> - * @x: a lock object (currently one of QemuMutex, CoMutex, QemuSpin).
> + * @x: a lock object (currently one of QemuMutex, QemuRecMutex, CoMutex, QemuSpin).
>    *
>    * Returns a QemuLockable object that can be passed around
> - * to a function that can operate with locks of any kind.
> + * to a function that can operate with locks of any kind, or
> + * NULL if @x is %NULL.
>    */
>   #define QEMU_MAKE_LOCKABLE(x)                        \
>       QEMU_GENERIC(x,                                  \
>                    (QemuLockable *, (x)),              \
> +                 qemu_make_lockable((x), QEMU_MAKE_LOCKABLE_(x)))
> +
> +/* QEMU_MAKE_LOCKABLE_NONNULL - Make a polymorphic QemuLockable
> + *
> + * @x: a lock object (currently one of QemuMutex, QemuRecMutex, CoMutex, QemuSpin).
> + *
> + * Returns a QemuLockable object that can be passed around
> + * to a function that can operate with locks of any kind.
> + */
> +#define QEMU_MAKE_LOCKABLE_NONNULL(x)                \
> +    QEMU_GENERIC(x,                                  \
> +                 (QemuLockable *, (x)),              \
>                    QEMU_MAKE_LOCKABLE_(x))
>   
>   static inline void qemu_lockable_lock(QemuLockable *x)
> @@ -112,7 +125,7 @@ G_DEFINE_AUTOPTR_CLEANUP_FUNC(QemuLockable, qemu_lockable_auto_unlock)
>   
>   #define WITH_QEMU_LOCK_GUARD_(x, var) \
>       for (g_autoptr(QemuLockable) var = \
> -                qemu_lockable_auto_lock(QEMU_MAKE_LOCKABLE((x))); \
> +                qemu_lockable_auto_lock(QEMU_MAKE_LOCKABLE_NONNULL((x))); \
>            var; \
>            qemu_lockable_auto_unlock(var), var = NULL)
>   
> 
> So thank you NetBSD compiler, I guess. :P

Yep, new patch looks good.

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> 
> Paolo
>