From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:43644) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SpMrS-0003Rp-Li for qemu-devel@nongnu.org; Thu, 12 Jul 2012 13:07:31 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SpMrO-0003Fk-B6 for qemu-devel@nongnu.org; Thu, 12 Jul 2012 13:07:30 -0400 Received: from v220110690675601.yourvserver.net ([78.47.199.172]:48173) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SpMrO-0003Es-14 for qemu-devel@nongnu.org; Thu, 12 Jul 2012 13:07:26 -0400 Message-ID: <4FFF0446.7000009@weilnetz.de> Date: Thu, 12 Jul 2012 19:07:18 +0200 From: Stefan Weil MIME-Version: 1.0 References: <1342103249-20888-1-git-send-email-kwolf@redhat.com> <1342103249-20888-2-git-send-email-kwolf@redhat.com> In-Reply-To: <1342103249-20888-2-git-send-email-kwolf@redhat.com> Content-Type: text/plain; charset=ISO-8859-15; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 1/2] coroutine-ucontext: Help valgrind understand coroutines List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: qemu-devel@nongnu.org Great that you address this issue! I have two annotations, please see below. Am 12.07.2012 16:27, schrieb Kevin Wolf: > valgrind tends to get confused and report false positives when you > switch stacks and don't tell it about it. > > Signed-off-by: Kevin Wolf > --- > configure | 18 ++++++++++++++++++ > coroutine-ucontext.c | 21 +++++++++++++++++++++ > 2 files changed, 39 insertions(+), 0 deletions(-) > > diff --git a/configure b/configure > index 500fe24..b424fcf 100755 > --- a/configure > +++ b/configure > @@ -2855,6 +2855,20 @@ if compile_prog "" "" ; then > fi > > ######################################## > +# check if we have valgrind/valgrind.h > + > +valgrind_h=no > +cat > $TMPC << EOF > +#include > +int main(void) { > + return 0; > +} > +EOF > +if compile_prog "" "" ; then > + valgrind_h=yes > +fi > + > +######################################## > # check if environ is declared > > has_environ=no > @@ -3380,6 +3394,10 @@ if test "$linux_magic_h" = "yes" ; then > echo "CONFIG_LINUX_MAGIC_H=y" >> $config_host_mak > fi > > +if test "$valgrind_h" = "yes" ; then > + echo "CONFIG_VALGRIND_H=y" >> $config_host_mak I'd prefer CONFIG_VALGRIND instead of CONFIG_VALGRIND_H. The important feature is Valgrind, not the valgrind.h which is needed to get that feature. Of course that is a matter of personal taste, and there are already a few CONFIG_SOMETHING_H macros, but most macros omit the _H even if there _is_ a related h file. > > +fi > + > if test "$has_environ" = "yes" ; then > echo "CONFIG_HAS_ENVIRON=y" >> $config_host_mak > fi > diff --git a/coroutine-ucontext.c b/coroutine-ucontext.c > index 5f43083..db4ba88 100644 > --- a/coroutine-ucontext.c > +++ b/coroutine-ucontext.c > @@ -30,6 +30,10 @@ > #include "qemu-common.h" > #include "qemu-coroutine-int.h" > > +#ifdef CONFIG_VALGRIND_H > +#include > +#endif > + > enum { > /* Maximum free pool size prevents holding too many freed coroutines */ > POOL_MAX_SIZE = 64, > @@ -43,6 +47,11 @@ typedef struct { > Coroutine base; > void *stack; > jmp_buf env; > + > +#ifdef CONFIG_VALGRIND_H > + int valgrind_stack_id; Stack ids are "unsigned" in valgrind.h, so please use "unsigned" here, too, although I know that you like "int" very much :-). > > +#endif > + > } CoroutineUContext; > > /** > @@ -159,6 +168,11 @@ static Coroutine *coroutine_new(void) > uc.uc_stack.ss_size = stack_size; > uc.uc_stack.ss_flags = 0; > > +#ifdef CONFIG_VALGRIND_H > + co->valgrind_stack_id = > + VALGRIND_STACK_REGISTER(co->stack, co->stack + stack_size); > +#endif > + > arg.p = co; > > makecontext(&uc, (void (*)(void))coroutine_trampoline, > @@ -196,6 +210,13 @@ void qemu_coroutine_delete(Coroutine *co_) > return; > } > > +#ifdef CONFIG_VALGRIND_H > + /* Work around an unused variable in the valgrind.h macro... */ > + #pragma GCC diagnostic push > + #pragma GCC diagnostic ignored "-Wunused-but-set-variable" > + VALGRIND_STACK_DEREGISTER(co->valgrind_stack_id); > + #pragma GCC diagnostic pop > +#endif > g_free(co->stack); > g_free(co); > } Regards, Stefan W.