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

[Paolo Bonzini]

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

Paolo

signature.asc
Description: OpenPGP digital signature