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

[Kevin Wolf]

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 <address@hidden>
>>> ---
>>>   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