[Qemu-devel] Build broken

[Qemu-devel] Build broken

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

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] Build broken

[Kevin Wolf]

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

Re: [Qemu-devel] Build broken

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

Stefan

Re: [Qemu-devel] Build broken

[Kevin Wolf]

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

Re: [Qemu-devel] Build broken

[Stefan Hajnoczi]

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

Re: [Qemu-devel] Build broken

[Stefan Hajnoczi]

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

Re: [Qemu-devel] Build broken

[malc]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1282 bytes --]

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.

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] Build broken

[Stefan Hajnoczi]

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

Re: [Qemu-devel] Build broken

[Kevin Wolf]

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>

Re: [Qemu-devel] Build broken

[malc]

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. 

-- 
mailto:av1474@comtv.ru