Re: [PULL 11/13] coroutine-ucontext: use QEMU_DEFINE_STATIC_CO_TLS()

[Kevin Wolf]

Am 13.05.2022 um 17:27 hat Peter Maydell geschrieben:
> On Wed, 4 May 2022 at 15:34, Kevin Wolf <kwolf@redhat.com> wrote:
> >
> > From: Stefan Hajnoczi <stefanha@redhat.com>
> >
> > Thread-Local Storage variables cannot be used directly from coroutine
> > code because the compiler may optimize TLS variable accesses across
> > qemu_coroutine_yield() calls. When the coroutine is re-entered from
> > another thread the TLS variables from the old thread must no longer be
> > used.
> >
> > Use QEMU_DEFINE_STATIC_CO_TLS() for the current and leader variables.
> >
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > Message-Id: <20220307153853.602859-2-stefanha@redhat.com>
> > Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  util/coroutine-ucontext.c | 38 ++++++++++++++++++++++++--------------
> >  1 file changed, 24 insertions(+), 14 deletions(-)
> >
> > diff --git a/util/coroutine-ucontext.c b/util/coroutine-ucontext.c
> > index ed368e1a3e..ddc98fb4f8 100644
> > --- a/util/coroutine-ucontext.c
> > +++ b/util/coroutine-ucontext.c
> > @@ -25,6 +25,7 @@
> >  #include "qemu/osdep.h"
> >  #include <ucontext.h>
> >  #include "qemu/coroutine_int.h"
> > +#include "qemu/coroutine-tls.h"
> >
> >  #ifdef CONFIG_VALGRIND_H
> >  #include <valgrind/valgrind.h>
> > @@ -66,8 +67,8 @@ typedef struct {
> >  /**
> >   * Per-thread coroutine bookkeeping
> >   */
> > -static __thread CoroutineUContext leader;
> > -static __thread Coroutine *current;
> > +QEMU_DEFINE_STATIC_CO_TLS(Coroutine *, current);
> > +QEMU_DEFINE_STATIC_CO_TLS(CoroutineUContext, leader);
> 
> Hi; Coverity complains about this change (CID 1488745):
> 
> # Big parameter passed by value (PASS_BY_VALUE)
> # pass_by_value: Passing parameter v of type CoroutineUContext
> # (size 304 bytes) by value, which exceeds the medium threshold
> # of 256 bytes.

The macro defines functions get_leader()/set_leader(), which would
indeed have this problem, but they are never called. Not sure if it's
worth having a separate macro that avoids generating these unused
functions for cases like this, but in practice it's a false positive.

Kevin