Patchwork [1/2] coroutine-ucontext: Help valgrind understand coroutines

login
register
mail settings
Submitter Kevin Wolf
Date July 12, 2012, 2:27 p.m.
Message ID <1342103249-20888-2-git-send-email-kwolf@redhat.com>
Download mbox | patch
Permalink /patch/170675/
State New
Headers show

Comments

Kevin Wolf - July 12, 2012, 2:27 p.m.
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(-)
Stefan Weil - July 12, 2012, 5:07 p.m.
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.
Peter Maydell - July 12, 2012, 5:14 p.m.
On 12 July 2012 15:27, Kevin Wolf <kwolf@redhat.com> wrote:
> valgrind tends to get confused and report false positives when you
> switch stacks and don't tell it about it.

Does the sigaltstack backend need anything similar?

-- PMM
Kevin Wolf - July 13, 2012, 8:36 a.m.
Am 12.07.2012 19:14, schrieb Peter Maydell:
> On 12 July 2012 15:27, Kevin Wolf <kwolf@redhat.com> wrote:
>> valgrind tends to get confused and report false positives when you
>> switch stacks and don't tell it about it.
> 
> Does the sigaltstack backend need anything similar?

Don't know, I never used valgrind with sigaltstack coroutines. All
examples that I found for the stack registration functions were using it
in the context of ucontext, but I wouldn't be surprised if sigaltstack
needs it as well. If you care enough, you can try it out and send a
patch, otherwise we can still add it when someone needs it.

Kevin
Kevin Wolf - July 13, 2012, 8:45 a.m.
Am 12.07.2012 19:07, schrieb Stefan Weil:
> 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.

Okay, I don't really mind, it was just the style of the check
immediately before the new one, so I used that. I can change it.

>> +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, 

Hm, indeed. Then the example code I looked at was wrong...

> although I know that you like "int" very much :-).

If you're alluding to something, I didn't get it.

Kevin
Kevin Wolf - July 13, 2012, 8:53 a.m.
Am 13.07.2012 10:45, schrieb Kevin Wolf:
> Am 12.07.2012 19:07, schrieb Stefan Weil:
>> 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.
> 
> Okay, I don't really mind, it was just the style of the check
> immediately before the new one, so I used that. I can change it.

Why did you #define a CONFIG_VALGRIND locally in commit c2a8238a? I
always thought, CONFIG_* macros should only ever be defined by
configure. Now this gives me a naming conflict with something that
depends on different semantics. I'll keep CONFIG_VALGRIND_H and leave it
to you to clean up oslib-posix.c if you care enough.

Kevin

Patch

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
+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;
+#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);
 }