[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 1/2] coroutine-ucontext: Help valgrind understan
From: |
Stefan Weil |
Subject: |
Re: [Qemu-devel] [PATCH 1/2] coroutine-ucontext: Help valgrind understand coroutines |
Date: |
Thu, 12 Jul 2012 19:07:18 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux i686; rv:13.0) Gecko/20120615 Thunderbird/13.0.1 |
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.
+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, Paolo Bonzini, 2012/07/12