Re: [Qemu-devel] [PATCH 1/2] coroutine-ucontext: Help valgrind understand coroutines

[Stefan Weil <sw@weilnetz.de>]

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 <kwolf@redhat.com>
> ---
>   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 <valgrind/valgrind.h>
> +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 <valgrind/valgrind.h>
> +#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.