diff mbox

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

Message ID 1342193500-5708-1-git-send-email-kwolf@redhat.com
State New
Headers show

Commit Message

Kevin Wolf July 13, 2012, 3:31 p.m. UTC 
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(-)

Comments

Peter Maydell July 13, 2012, 3:37 p.m. UTC | #1 
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
Kevin Wolf July 13, 2012, 4:06 p.m. UTC | #2 
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
Eric Blake July 13, 2012, 4:13 p.m. UTC | #3 
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?
Kevin Wolf July 13, 2012, 4:18 p.m. UTC | #4 
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
diff mbox

Patch

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