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

[Kevin Wolf <kwolf@redhat.com>]

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