[Qemu-devel] [PATCH 0/2] qemu-iotests: valgrind support

[Qemu-devel] [PATCH 0/2] qemu-iotests: valgrind support

[Kevin Wolf]

Kevin Wolf (2):
  coroutine-ucontext: Help valgrind understand coroutines
  qemu-iotests: Valgrind support

 configure                    |   18 ++++++++++++++++++
 coroutine-ucontext.c         |   21 +++++++++++++++++++++
 tests/qemu-iotests/common    |   11 +++++++++++
 tests/qemu-iotests/common.rc |   10 ++++++++++
 4 files changed, 60 insertions(+), 0 deletions(-)

-- 
1.7.6.5

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

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

[Qemu-devel] [PATCH 2/2] qemu-iotests: Valgrind support

[Kevin Wolf]

check -valgrind wraps all qemu-io calls with valgrind. This makes it a
bit easier to debug problems that occur somewhere deep in a test case.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/qemu-iotests/common    |   11 +++++++++++
 tests/qemu-iotests/common.rc |   10 ++++++++++
 2 files changed, 21 insertions(+), 0 deletions(-)

diff --git a/tests/qemu-iotests/common b/tests/qemu-iotests/common
index eeb70cb..1f6fdf5 100644
--- a/tests/qemu-iotests/common
+++ b/tests/qemu-iotests/common
@@ -41,6 +41,7 @@ sortme=false
 expunge=true
 have_test_arg=false
 randomize=false
+valgrind=false
 rm -f $tmp.list $tmp.tmp $tmp.sed
 
 export IMGFMT=raw
@@ -212,6 +213,11 @@ testlist options
 	    xpand=false
 	    ;;
 
+    -valgrind)
+        valgrind=true
+	    xpand=false
+        ;;
+
 	-g)	# -g group ... pick from group file
 	    group=true
 	    xpand=false
@@ -345,3 +351,8 @@ fi
 [ "$QEMU" = "" ] && _fatal "qemu not found"
 [ "$QEMU_IMG" = "" ] && _fatal "qemu-img not found"
 [ "$QEMU_IO" = "" ] && _fatal "qemu-img not found"
+
+if $valgrind; then
+    export REAL_QEMU_IO="$QEMU_IO_PROG"
+    export QEMU_IO_PROG=valgrind_qemu_io
+fi
diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
index e535874..5e3a524 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -53,6 +53,16 @@ else
     TEST_IMG=$IMGPROTO:$TEST_DIR/t.$IMGFMT
 fi
 
+function valgrind_qemu_io()
+{
+    valgrind --log-file=/tmp/$$.valgrind --error-exitcode=99 $REAL_QEMU_IO "$@"
+    if [ $? != 0 ]; then
+        cat /tmp/$$.valgrind
+    fi
+    rm -f /tmp/$$.valgrind
+}
+
+
 _optstr_add()
 {
     if [ -n "$1" ]; then
-- 
1.7.6.5

Re: [Qemu-devel] [PATCH 0/2] qemu-iotests: valgrind support

[Paolo Bonzini]

Il 12/07/2012 16:27, Kevin Wolf ha scritto:
> Kevin Wolf (2):
>   coroutine-ucontext: Help valgrind understand coroutines
>   qemu-iotests: Valgrind support
> 
>  configure                    |   18 ++++++++++++++++++
>  coroutine-ucontext.c         |   21 +++++++++++++++++++++
>  tests/qemu-iotests/common    |   11 +++++++++++
>  tests/qemu-iotests/common.rc |   10 ++++++++++
>  4 files changed, 60 insertions(+), 0 deletions(-)
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

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

[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.


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

Re: [Qemu-devel] [PATCH 0/2] qemu-iotests: valgrind support

[Stefan Weil]

Am 12.07.2012 18:51, schrieb Paolo Bonzini:
> Il 12/07/2012 16:27, Kevin Wolf ha scritto:
>> Kevin Wolf (2):
>>    coroutine-ucontext: Help valgrind understand coroutines
>>    qemu-iotests: Valgrind support
>>
>>   configure                    |   18 ++++++++++++++++++
>>   coroutine-ucontext.c         |   21 +++++++++++++++++++++
>>   tests/qemu-iotests/common    |   11 +++++++++++
>>   tests/qemu-iotests/common.rc |   10 ++++++++++
>>   4 files changed, 60 insertions(+), 0 deletions(-)
>>
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Hi Paolo,

at least the data type of the Valgrind stack id should be fixed
(see my previous mail which I wrote while you was sending
your review).

Cheers,

Stefan

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

[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?

-- PMM

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

[Kevin Wolf]

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

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

[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.

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

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

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

[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>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
---

v2:
- Use unsigned for the stack ID
- Older gccs don't know #pragma diagnostic push/pop, so replace them
  by explicitly switching back to error afterwards
- They also don't like it inside a function...
- Check in configure that the macro and the #pragma work

 configure            |   20 ++++++++++++++++++++
 coroutine-ucontext.c |   28 ++++++++++++++++++++++++++++
 2 files changed, 48 insertions(+), 0 deletions(-)

diff --git a/configure b/configure
index 500fe24..aae73f4 100755
--- a/configure
+++ b/configure
@@ -2855,6 +2855,22 @@ if compile_prog "" "" ; then
 fi
 
 ########################################
+# check if we have valgrind/valgrind.h
+
+valgrind_h=no
+cat > $TMPC << EOF
+#include <valgrind/valgrind.h>
+#pragma GCC diagnostic ignored "-Wunused-but-set-variable"
+int main(void) {
+  VALGRIND_STACK_DEREGISTER(0);
+  return 0;
+}
+EOF
+if compile_prog "" "" ; then
+    valgrind_h=yes
+fi
+
+########################################
 # check if environ is declared
 
 has_environ=no
@@ -3380,6 +3396,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..e3c450b 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
+    unsigned 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,
@@ -185,6 +199,16 @@ Coroutine *qemu_coroutine_new(void)
     return co;
 }
 
+#ifdef CONFIG_VALGRIND_H
+/* Work around an unused variable in the valgrind.h macro... */
+#pragma GCC diagnostic ignored "-Wunused-but-set-variable"
+static inline void valgrind_stack_deregister(CoroutineUContext *co)
+{
+    VALGRIND_STACK_DEREGISTER(co->valgrind_stack_id);
+}
+#pragma GCC diagnostic error "-Wunused-but-set-variable"
+#endif
+
 void qemu_coroutine_delete(Coroutine *co_)
 {
     CoroutineUContext *co = DO_UPCAST(CoroutineUContext, base, co_);
@@ -196,6 +220,10 @@ void qemu_coroutine_delete(Coroutine *co_)
         return;
     }
 
+#ifdef CONFIG_VALGRIND_H
+    valgrind_stack_deregister(co);
+#endif
+
     g_free(co->stack);
     g_free(co);
 }
-- 
1.7.6.5

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

[Peter Maydell]

On 13 July 2012 16:31, Kevin Wolf <kwolf@redhat.com> wrote:
> +#ifdef CONFIG_VALGRIND_H
> +/* Work around an unused variable in the valgrind.h macro... */
> +#pragma GCC diagnostic ignored "-Wunused-but-set-variable"
> +static inline void valgrind_stack_deregister(CoroutineUContext *co)
> +{
> +    VALGRIND_STACK_DEREGISTER(co->valgrind_stack_id);
> +}
> +#pragma GCC diagnostic error "-Wunused-but-set-variable"
> +#endif

'#pragma .. error' will defeat the configure code which makes warnings
not fatal in release builds.

-- PMM

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

[Kevin Wolf]

Am 13.07.2012 17:37, schrieb Peter Maydell:
> On 13 July 2012 16:31, Kevin Wolf <kwolf@redhat.com> wrote:
>> +#ifdef CONFIG_VALGRIND_H
>> +/* Work around an unused variable in the valgrind.h macro... */
>> +#pragma GCC diagnostic ignored "-Wunused-but-set-variable"
>> +static inline void valgrind_stack_deregister(CoroutineUContext *co)
>> +{
>> +    VALGRIND_STACK_DEREGISTER(co->valgrind_stack_id);
>> +}
>> +#pragma GCC diagnostic error "-Wunused-but-set-variable"
>> +#endif
> 
> '#pragma .. error' will defeat the configure code which makes warnings
> not fatal in release builds.

I know. What's your suggestion? Switch only to warning? Then it would be
easy to miss warnings. Disabling the valgrind code for gcc < 4.6 is
better, but still not really nice. I thought having part of one file
always use -Werror for this one warning is the best compromise, but I
won't insist on it.

Kevin

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1247 bytes --]

On 07/13/2012 10:06 AM, Kevin Wolf wrote:
> Am 13.07.2012 17:37, schrieb Peter Maydell:
>> On 13 July 2012 16:31, Kevin Wolf <kwolf@redhat.com> wrote:
>>> +#ifdef CONFIG_VALGRIND_H
>>> +/* Work around an unused variable in the valgrind.h macro... */
>>> +#pragma GCC diagnostic ignored "-Wunused-but-set-variable"
>>> +static inline void valgrind_stack_deregister(CoroutineUContext *co)
>>> +{
>>> +    VALGRIND_STACK_DEREGISTER(co->valgrind_stack_id);
>>> +}
>>> +#pragma GCC diagnostic error "-Wunused-but-set-variable"
>>> +#endif
>>
>> '#pragma .. error' will defeat the configure code which makes warnings
>> not fatal in release builds.
> 
> I know. What's your suggestion? Switch only to warning? Then it would be
> easy to miss warnings. Disabling the valgrind code for gcc < 4.6 is
> better, but still not really nice.

But you're already disabling the valgrind code for gcc too old to honor

#pragma GCC diagnostic ignored "-Wunused-but-set-variable"

so what's the difference in making your configure check for
CONFIG_VALGRIND_H _also_ check that gcc is new enough to honor push/pop
of diagnostic?

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]

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

[Kevin Wolf]

Am 13.07.2012 18:13, schrieb Eric Blake:
> On 07/13/2012 10:06 AM, Kevin Wolf wrote:
>> Am 13.07.2012 17:37, schrieb Peter Maydell:
>>> On 13 July 2012 16:31, Kevin Wolf <kwolf@redhat.com> wrote:
>>>> +#ifdef CONFIG_VALGRIND_H
>>>> +/* Work around an unused variable in the valgrind.h macro... */
>>>> +#pragma GCC diagnostic ignored "-Wunused-but-set-variable"
>>>> +static inline void valgrind_stack_deregister(CoroutineUContext *co)
>>>> +{
>>>> +    VALGRIND_STACK_DEREGISTER(co->valgrind_stack_id);
>>>> +}
>>>> +#pragma GCC diagnostic error "-Wunused-but-set-variable"
>>>> +#endif
>>>
>>> '#pragma .. error' will defeat the configure code which makes warnings
>>> not fatal in release builds.
>>
>> I know. What's your suggestion? Switch only to warning? Then it would be
>> easy to miss warnings. Disabling the valgrind code for gcc < 4.6 is
>> better, but still not really nice.
> 
> But you're already disabling the valgrind code for gcc too old to honor
> 
> #pragma GCC diagnostic ignored "-Wunused-but-set-variable"
> 
> so what's the difference in making your configure check for
> CONFIG_VALGRIND_H _also_ check that gcc is new enough to honor push/pop
> of diagnostic?

The practical difference for me is that the RHEL 6 gcc knows
ignored/warning/error (since gcc 4.2), but not push/pop (since gcc 4.6),
so my test machine still wouldn't have valgrind support and I could drop
the patch wholesale.

Kevin