| Message ID | alpine.LNX.2.00.1108051019170.14809@linmac |
|---|---|
| State | New |
| Headers | show |
Am 05.08.2011 08:22, schrieb malc: > > /home/malc/x/rcs/git/qemuorg/coroutine-ucontext.c: In function 'coroutine_new': > /home/malc/x/rcs/git/qemuorg/coroutine-ucontext.c:160:16: error: 'arg.i[1]' may be used uninitialized in this function > /home/malc/x/rcs/git/qemuorg/coroutine-ucontext.c:136:18: note: 'arg.i[1]' was declared here > > diff --git a/coroutine-ucontext.c b/coroutine-ucontext.c > index 41c2379..42dc3e2 100644 > --- a/coroutine-ucontext.c > +++ b/coroutine-ucontext.c > @@ -133,7 +133,7 @@ static Coroutine *coroutine_new(void) > CoroutineUContext *co; > ucontext_t old_uc, uc; > jmp_buf old_env; > - union cc_arg arg; > + union cc_arg arg = {0}; > > /* The ucontext functions preserve signal masks which incurs a system call > * overhead. setjmp()/longjmp() does not preserve signal masks but only > > I guess gcc should yell not only here on ppc32 but on any machine where > pointer size is less than the size of two ints. Stefan, why does this code even exist again? I think at some point I had it changed to just use a static variable in order to avoid doing this kind of tricks with unions. Interestingly, the buildbot doesn't seem to have failed on i386. Kevin
On Fri, Aug 5, 2011 at 8:29 AM, Kevin Wolf <kwolf@redhat.com> wrote: > Am 05.08.2011 08:22, schrieb malc: >> >> /home/malc/x/rcs/git/qemuorg/coroutine-ucontext.c: In function 'coroutine_new': >> /home/malc/x/rcs/git/qemuorg/coroutine-ucontext.c:160:16: error: 'arg.i[1]' may be used uninitialized in this function >> /home/malc/x/rcs/git/qemuorg/coroutine-ucontext.c:136:18: note: 'arg.i[1]' was declared here >> >> diff --git a/coroutine-ucontext.c b/coroutine-ucontext.c >> index 41c2379..42dc3e2 100644 >> --- a/coroutine-ucontext.c >> +++ b/coroutine-ucontext.c >> @@ -133,7 +133,7 @@ static Coroutine *coroutine_new(void) >> CoroutineUContext *co; >> ucontext_t old_uc, uc; >> jmp_buf old_env; >> - union cc_arg arg; >> + union cc_arg arg = {0}; >> >> /* The ucontext functions preserve signal masks which incurs a system call >> * overhead. setjmp()/longjmp() does not preserve signal masks but only >> >> I guess gcc should yell not only here on ppc32 but on any machine where >> pointer size is less than the size of two ints. > > Stefan, why does this code even exist again? I think at some point I had > it changed to just use a static variable in order to avoid doing this > kind of tricks with unions. virtfs are using coroutines in multiple threads at the same time. Introducing a global variable wouldn't be thread-safe. The real problem is that makecontext(3) has a bad function signature. There's no nice fix - whatever we do will be ugly. Using a union is the way it should be done in C. The code doesn't look pretty but it doesn't introduce global state. Stefan
On Fri, Aug 5, 2011 at 7:22 AM, malc <av1474@comtv.ru> wrote: > > /home/malc/x/rcs/git/qemuorg/coroutine-ucontext.c: In function 'coroutine_new': > /home/malc/x/rcs/git/qemuorg/coroutine-ucontext.c:160:16: error: 'arg.i[1]' may be used uninitialized in this function > /home/malc/x/rcs/git/qemuorg/coroutine-ucontext.c:136:18: note: 'arg.i[1]' was declared here > > diff --git a/coroutine-ucontext.c b/coroutine-ucontext.c > index 41c2379..42dc3e2 100644 > --- a/coroutine-ucontext.c > +++ b/coroutine-ucontext.c > @@ -133,7 +133,7 @@ static Coroutine *coroutine_new(void) > CoroutineUContext *co; > ucontext_t old_uc, uc; > jmp_buf old_env; > - union cc_arg arg; > + union cc_arg arg = {0}; > > /* The ucontext functions preserve signal masks which incurs a system call > * overhead. setjmp()/longjmp() does not preserve signal masks but only > > I guess gcc should yell not only here on ppc32 but on any machine where > pointer size is less than the size of two ints. Makes sense. Are you going to commit a fix or send a signed-off-by patch? Stefan
Am 05.08.2011 10:48, schrieb Stefan Hajnoczi: > On Fri, Aug 5, 2011 at 8:29 AM, Kevin Wolf <kwolf@redhat.com> wrote: >> Am 05.08.2011 08:22, schrieb malc: >>> >>> /home/malc/x/rcs/git/qemuorg/coroutine-ucontext.c: In function 'coroutine_new': >>> /home/malc/x/rcs/git/qemuorg/coroutine-ucontext.c:160:16: error: 'arg.i[1]' may be used uninitialized in this function >>> /home/malc/x/rcs/git/qemuorg/coroutine-ucontext.c:136:18: note: 'arg.i[1]' was declared here >>> >>> diff --git a/coroutine-ucontext.c b/coroutine-ucontext.c >>> index 41c2379..42dc3e2 100644 >>> --- a/coroutine-ucontext.c >>> +++ b/coroutine-ucontext.c >>> @@ -133,7 +133,7 @@ static Coroutine *coroutine_new(void) >>> CoroutineUContext *co; >>> ucontext_t old_uc, uc; >>> jmp_buf old_env; >>> - union cc_arg arg; >>> + union cc_arg arg = {0}; >>> >>> /* The ucontext functions preserve signal masks which incurs a system call >>> * overhead. setjmp()/longjmp() does not preserve signal masks but only >>> >>> I guess gcc should yell not only here on ppc32 but on any machine where >>> pointer size is less than the size of two ints. >> >> Stefan, why does this code even exist again? I think at some point I had >> it changed to just use a static variable in order to avoid doing this >> kind of tricks with unions. > > virtfs are using coroutines in multiple threads at the same time. > Introducing a global variable wouldn't be thread-safe. > > The real problem is that makecontext(3) has a bad function signature. > There's no nice fix - whatever we do will be ugly. > > Using a union is the way it should be done in C. The code doesn't > look pretty but it doesn't introduce global state. But it makes assumptions about the pointer size, which isn't a nice thing. TLS isn't an option for compatibility with some OSes/architectures? Kevin
On Fri, Aug 5, 2011 at 10:09 AM, Kevin Wolf <kwolf@redhat.com> wrote: > Am 05.08.2011 10:48, schrieb Stefan Hajnoczi: >> On Fri, Aug 5, 2011 at 8:29 AM, Kevin Wolf <kwolf@redhat.com> wrote: >>> Am 05.08.2011 08:22, schrieb malc: >>>> >>>> /home/malc/x/rcs/git/qemuorg/coroutine-ucontext.c: In function 'coroutine_new': >>>> /home/malc/x/rcs/git/qemuorg/coroutine-ucontext.c:160:16: error: 'arg.i[1]' may be used uninitialized in this function >>>> /home/malc/x/rcs/git/qemuorg/coroutine-ucontext.c:136:18: note: 'arg.i[1]' was declared here >>>> >>>> diff --git a/coroutine-ucontext.c b/coroutine-ucontext.c >>>> index 41c2379..42dc3e2 100644 >>>> --- a/coroutine-ucontext.c >>>> +++ b/coroutine-ucontext.c >>>> @@ -133,7 +133,7 @@ static Coroutine *coroutine_new(void) >>>> CoroutineUContext *co; >>>> ucontext_t old_uc, uc; >>>> jmp_buf old_env; >>>> - union cc_arg arg; >>>> + union cc_arg arg = {0}; >>>> >>>> /* The ucontext functions preserve signal masks which incurs a system call >>>> * overhead. setjmp()/longjmp() does not preserve signal masks but only >>>> >>>> I guess gcc should yell not only here on ppc32 but on any machine where >>>> pointer size is less than the size of two ints. >>> >>> Stefan, why does this code even exist again? I think at some point I had >>> it changed to just use a static variable in order to avoid doing this >>> kind of tricks with unions. >> >> virtfs are using coroutines in multiple threads at the same time. >> Introducing a global variable wouldn't be thread-safe. >> >> The real problem is that makecontext(3) has a bad function signature. >> There's no nice fix - whatever we do will be ugly. >> >> Using a union is the way it should be done in C. The code doesn't >> look pretty but it doesn't introduce global state. > > But it makes assumptions about the pointer size, which isn't a nice > thing. TLS isn't an option for compatibility with some OSes/architectures? GThread TLS is portable but slow in my testing. You are right, when we move to 128-bit pointers this code will break :). We could at add a #warning if pointer size > 64-bits. Stefan
On Fri, 5 Aug 2011, Stefan Hajnoczi wrote: > On Fri, Aug 5, 2011 at 7:22 AM, malc <av1474@comtv.ru> wrote: > > > > /home/malc/x/rcs/git/qemuorg/coroutine-ucontext.c: In function 'coroutine_new': > > /home/malc/x/rcs/git/qemuorg/coroutine-ucontext.c:160:16: error: 'arg.i[1]' may be used uninitialized in this function > > /home/malc/x/rcs/git/qemuorg/coroutine-ucontext.c:136:18: note: 'arg.i[1]' was declared here > > > > diff --git a/coroutine-ucontext.c b/coroutine-ucontext.c > > index 41c2379..42dc3e2 100644 > > --- a/coroutine-ucontext.c > > +++ b/coroutine-ucontext.c > > @@ -133,7 +133,7 @@ static Coroutine *coroutine_new(void) > > CoroutineUContext *co; > > ucontext_t old_uc, uc; > > jmp_buf old_env; > > - union cc_arg arg; > > + union cc_arg arg = {0}; > > > > /* The ucontext functions preserve signal masks which incurs a system call > > * overhead. setjmp()/longjmp() does not preserve signal masks but only > > > > I guess gcc should yell not only here on ppc32 but on any machine where > > pointer size is less than the size of two ints. > > Makes sense. Are you going to commit a fix or send a signed-off-by patch? > If the author(s)(you and Kevin? just you?) agree with the above i can just push it.
On Fri, Aug 5, 2011 at 5:49 PM, malc <av1474@comtv.ru> wrote: > On Fri, 5 Aug 2011, Stefan Hajnoczi wrote: > >> On Fri, Aug 5, 2011 at 7:22 AM, malc <av1474@comtv.ru> wrote: >> > >> > /home/malc/x/rcs/git/qemuorg/coroutine-ucontext.c: In function 'coroutine_new': >> > /home/malc/x/rcs/git/qemuorg/coroutine-ucontext.c:160:16: error: 'arg.i[1]' may be used uninitialized in this function >> > /home/malc/x/rcs/git/qemuorg/coroutine-ucontext.c:136:18: note: 'arg.i[1]' was declared here >> > >> > diff --git a/coroutine-ucontext.c b/coroutine-ucontext.c >> > index 41c2379..42dc3e2 100644 >> > --- a/coroutine-ucontext.c >> > +++ b/coroutine-ucontext.c >> > @@ -133,7 +133,7 @@ static Coroutine *coroutine_new(void) >> > CoroutineUContext *co; >> > ucontext_t old_uc, uc; >> > jmp_buf old_env; >> > - union cc_arg arg; >> > + union cc_arg arg = {0}; >> > >> > /* The ucontext functions preserve signal masks which incurs a system call >> > * overhead. setjmp()/longjmp() does not preserve signal masks but only >> > >> > I guess gcc should yell not only here on ppc32 but on any machine where >> > pointer size is less than the size of two ints. >> >> Makes sense. Are you going to commit a fix or send a signed-off-by patch? >> > > If the author(s)(you and Kevin? just you?) agree with the above i can just > push it. The change makes sense to me. Kevin? Stefan
Am 05.08.2011 18:49, schrieb malc: > On Fri, 5 Aug 2011, Stefan Hajnoczi wrote: > >> On Fri, Aug 5, 2011 at 7:22 AM, malc <av1474@comtv.ru> wrote: >>> >>> /home/malc/x/rcs/git/qemuorg/coroutine-ucontext.c: In function 'coroutine_new': >>> /home/malc/x/rcs/git/qemuorg/coroutine-ucontext.c:160:16: error: 'arg.i[1]' may be used uninitialized in this function >>> /home/malc/x/rcs/git/qemuorg/coroutine-ucontext.c:136:18: note: 'arg.i[1]' was declared here >>> >>> diff --git a/coroutine-ucontext.c b/coroutine-ucontext.c >>> index 41c2379..42dc3e2 100644 >>> --- a/coroutine-ucontext.c >>> +++ b/coroutine-ucontext.c >>> @@ -133,7 +133,7 @@ static Coroutine *coroutine_new(void) >>> CoroutineUContext *co; >>> ucontext_t old_uc, uc; >>> jmp_buf old_env; >>> - union cc_arg arg; >>> + union cc_arg arg = {0}; >>> >>> /* The ucontext functions preserve signal masks which incurs a system call >>> * overhead. setjmp()/longjmp() does not preserve signal masks but only >>> >>> I guess gcc should yell not only here on ppc32 but on any machine where >>> pointer size is less than the size of two ints. >> >> Makes sense. Are you going to commit a fix or send a signed-off-by patch? >> > > If the author(s)(you and Kevin? just you?) agree with the above i can just > push it. Feel free to push it. (Original code was by Anthony, then heavily modified by me, and after that modified again by Stefan) Acked-by: Kevin Wolf <kwolf@redhat.com>
On Mon, 8 Aug 2011, Kevin Wolf wrote: > Am 05.08.2011 18:49, schrieb malc: > > On Fri, 5 Aug 2011, Stefan Hajnoczi wrote: > > [..snip..] > > Feel free to push it. (Original code was by Anthony, then heavily > modified by me, and after that modified again by Stefan) > > Acked-by: Kevin Wolf <kwolf@redhat.com> Pushed.
diff --git a/coroutine-ucontext.c b/coroutine-ucontext.c index 41c2379..42dc3e2 100644 --- a/coroutine-ucontext.c +++ b/coroutine-ucontext.c @@ -133,7 +133,7 @@ static Coroutine *coroutine_new(void) CoroutineUContext *co; ucontext_t old_uc, uc; jmp_buf old_env; - union cc_arg arg; + union cc_arg arg = {0}; /* The ucontext functions preserve signal masks which incurs a system call * overhead. setjmp()/longjmp() does not preserve signal masks but only