Hi
On Wed, Apr 3, 2024 at 12:31 PM Vladimir Sementsov-Ogievskiy
<vsement...@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
> > <vsement...@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
