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

[Marc-André Lureau]

Hi

On Wed, Apr 3, 2024 at 12:31 PM Vladimir Sementsov-Ogievskiy
<vsementsov@yandex-team.ru> wrote:
>
> On 03.04.24 11:11, Marc-André Lureau wrote:
> > 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) {
> >
> >
>
> So, updated macro helps in some cases, but doesn't help here? Intersting, why.
>
> > What should we do? change the macros + cherry-pick the missing
> > false-positives, or keep this series as is?
> >
> >
>
> I think marco + missing is better. No reason to add dead-initializations in 
> cases where new macros helps.

Ok

> Still, would be good to understand, what's the difference, why it help on 
> some cases and not help in another.

I don't know, it's like if the analyzer was lazy for this particular
case, although there is nothing much different from other usages.

If I replace:
for (... *var2 = (void *)true; var2;
with:
for (... *var2 = (void *)true; var2 || true;

then it doesn't warn..

Interestingly as well, if I change:
    for (... *var2 = (void *)true; var2; var2 = NULL)
for:
    for (... *var2 = GML_OBJ_(); var2; var2 = NULL)

GML_OBJ_() simply being &(GraphLockable) { }), an empty compound
literal, then it doesn't work, in all usages.

All in all, I am not sure the trick of using var2 is really reliable either.

-- 
Marc-André Lureau