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) { What should we do? change the macros + cherry-pick the missing false-positives, or keep this series as is? -- Marc-André Lureau