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 <kw...@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
