| Message ID | 1342103249-20888-2-git-send-email-kwolf@redhat.com |
|---|---|
| State | New |
| Headers | show |
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.
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
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
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
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
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); }
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(-)