Re: [PATCH 06/19] block/stream: fix -Werror=maybe-uninitialized false-positives

[Marc-André Lureau]

Hi

On Tue, Apr 2, 2024 at 11:24 PM Vladimir Sementsov-Ogievskiy
<vsementsov@yandex-team.ru> wrote:
>
> On 02.04.24 18:34, Eric Blake wrote:
> > On Tue, Apr 02, 2024 at 12:58:43PM +0300, Vladimir Sementsov-Ogievskiy 
> > wrote:
> >>>> Again, same false-positives, because of WITH_GRAPH_RDLOCK_GUARD()..
> >>>>
> >>>> Didn't you try to change WITH_ macros somehow, so that compiler believe 
> >>>> in our good intentions?
> >>>>
> >>>
> >>>
> >>> #define WITH_QEMU_LOCK_GUARD_(x, var) \
> >>>       for (g_autoptr(QemuLockable) var = \
> >>>                   
> >>> qemu_lockable_auto_lock(QEMU_MAKE_LOCKABLE_NONNULL((x))); \
> >>>            var; \
> >>>            qemu_lockable_auto_unlock(var), var = NULL)
> >>>
> >>> I can't think of a clever way to rewrite this. The compiler probably
> >>> thinks the loop may not run, due to the "var" condition. But how to
> >>> convince it otherwise? it's hard to introduce another variable too..
> >>
> >>
> >> hmm. maybe like this?
> >>
> >> #define WITH_QEMU_LOCK_GUARD_(x, var) \
> >>      for (g_autoptr(QemuLockable) var = \
> >>                  qemu_lockable_auto_lock(QEMU_MAKE_LOCKABLE_NONNULL((x))), 
> >> \
> >>           var2 = (void *)(true); \
> >>           var2; \
> >>           qemu_lockable_auto_unlock(var), var2 = NULL)
> >>
> >>
> >> probably, it would be simpler for compiler to understand the logic this 
> >> way. Could you check?
> >
> > Wouldn't that attach __attribute__((cleanup(xxx))) to var2, at which
> > point we could cause the compiler to call xxx((void*)(true)) if the
> > user does an early return inside the lock guard, with disastrous
> > consequences?  Or is the __attribute__ applied only to the first out
> > of two declarations in a list?
> >
>
> Oh, most probably you are right, seems g_autoptr apply it to both variables. 
> Also, we don't need qemu_lockable_auto_unlock(var) separate call, if we 
> zero-out another variable. So, me fixing:
>
> #define WITH_QEMU_LOCK_GUARD_(x, var) \
>      for (QemuLockable *var 
> __attribute__((cleanup(qemu_lockable_auto_unlock))) = 
> qemu_lockable_auto_lock(QEMU_MAKE_LOCKABLE_NONNULL((x))), \
>           *var2 = (void *)(true); \
>           var2; \
>           var2 = NULL)
>
> (and we'll need to modify qemu_lockable_auto_unlock() to take "QemuLockable 
> **x" argument)
>

That's almost good enough. I fixed a few things to generate var2.

I applied a similar approach to WITH_GRAPH_RDLOCK_GUARD macro:

--- a/include/block/graph-lock.h
+++ b/include/block/graph-lock.h
@@ -224,13 +224,22 @@ graph_lockable_auto_unlock(GraphLockable *x)

 G_DEFINE_AUTOPTR_CLEANUP_FUNC(GraphLockable, graph_lockable_auto_unlock)

-#define WITH_GRAPH_RDLOCK_GUARD_(var)                                         \
-    for (g_autoptr(GraphLockable) var = graph_lockable_auto_lock(GML_OBJ_()); \
-         var;                                                                 \
-         graph_lockable_auto_unlock(var), var = NULL)
+static inline void TSA_NO_TSA coroutine_fn
+graph_lockable_auto_cleanup(GraphLockable **x)
+{
+    graph_lockable_auto_unlock(*x);
+}
+
+#define WITH_GRAPH_RDLOCK_GUARD__(var) \
+    GraphLockable *var \
+        __attribute__((cleanup(graph_lockable_auto_cleanup))) G_GNUC_UNUSED = \
+       graph_lockable_auto_lock(GML_OBJ_())
+
+#define WITH_GRAPH_RDLOCK_GUARD_(var, var2)                             \
+    for (WITH_GRAPH_RDLOCK_GUARD__(var), *var2 = (void *)true; var2;
var2 = NULL)

 #define WITH_GRAPH_RDLOCK_GUARD() \
-    WITH_GRAPH_RDLOCK_GUARD_(glue(graph_lockable_auto, __COUNTER__))
+    WITH_GRAPH_RDLOCK_GUARD_(glue(graph_lockable_auto, __COUNTER__),
glue(graph_lockable_auto, __COUNTER__))

Unfortunately, it doesn't work in all cases. It seems to have issues
with some guards:
../block/stream.c: In function ‘stream_run’:
../block/stream.c:216:12: error: ‘ret’ may be used uninitialized
[-Werror=maybe-uninitialized]
  216 |         if (ret < 0) {


What should we do? change the macros + cherry-pick the missing
false-positives, or keep this series as is?





-- 
Marc-André Lureau