Patchwork [2/5] sparc: fix expression with uninitialized initial value

login
register
mail settings
Submitter Blue Swirl
Date July 30, 2012, 4:04 p.m.
Message ID <5ddedba0015c15117770b3d1860f7dd6d8d08a0c.1343664167.git.blauwirbel@gmail.com>
Download mbox | patch
Permalink /patch/174034/
State New
Headers show

Comments

Blue Swirl - July 30, 2012, 4:04 p.m.
From: Blue Swirl <blauwirbel@gmail.com>

err was uninitalized, it's not OK to use |=. Spotted by Clang
compiler.

Fix by replacing |= by =.

Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
---
 linux-user/signal.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Peter Maydell - July 30, 2012, 4:13 p.m.
On 30 July 2012 17:04,  <blauwirbel@gmail.com> wrote:
> From: Blue Swirl <blauwirbel@gmail.com>
>
> err was uninitalized, it's not OK to use |=. Spotted by Clang

"uninitialized" (feel free to just fix typo on commit).

> compiler.
>
> Fix by replacing |= by =.
>
> Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
> ---
>  linux-user/signal.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/linux-user/signal.c b/linux-user/signal.c
> index 97f30d9..3d6b5df 100644
> --- a/linux-user/signal.c
> +++ b/linux-user/signal.c
> @@ -2061,7 +2061,7 @@ restore_fpu_state(CPUSPARCState *env, qemu_siginfo_fpu_t *fpu)
>          err = __copy_from_user(&env->fpr[0], &fpu->si_float_regs[0],
>                                      (sizeof(unsigned long) * 32));
>  #endif
> -        err |= __get_user(env->fsr, &fpu->si_fsr);
> +        err = __get_user(env->fsr, &fpu->si_fsr);
>  #if 0
>          err |= __get_user(current->thread.fpqdepth, &fpu->si_fpqdepth);
>          if (current->thread.fpqdepth != 0)

This will need changing again if we ever fix the #if 0-d out
code in this function, but I guess that will be obvious to whoever
does that.

Incidentally, __get_user() can never fail [we catch unreadable memory
earlier when wo do the lock_user_struct] so you could also just
not do anything with its return value. Some of the other targets
rely on this in their signal save/restore code. I think the use
of the return value is mostly in code that was copy-and-pasted
from the Linux kernel (which does use a __get_user() that can fail).

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

-- PMM
Andreas Färber - July 30, 2012, 4:59 p.m.
Am 30.07.2012 18:13, schrieb Peter Maydell:
> On 30 July 2012 17:04,  <blauwirbel@gmail.com> wrote:
>> From: Blue Swirl <blauwirbel@gmail.com>
>>
>> err was uninitalized, it's not OK to use |=. Spotted by Clang
> 
> "uninitialized" (feel free to just fix typo on commit).
> 
>> compiler.
>>
>> Fix by replacing |= by =.
>>
>> Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
>> ---
>>  linux-user/signal.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/linux-user/signal.c b/linux-user/signal.c
>> index 97f30d9..3d6b5df 100644
>> --- a/linux-user/signal.c
>> +++ b/linux-user/signal.c
>> @@ -2061,7 +2061,7 @@ restore_fpu_state(CPUSPARCState *env, qemu_siginfo_fpu_t *fpu)
>>          err = __copy_from_user(&env->fpr[0], &fpu->si_float_regs[0],
>>                                      (sizeof(unsigned long) * 32));
>>  #endif
>> -        err |= __get_user(env->fsr, &fpu->si_fsr);
>> +        err = __get_user(env->fsr, &fpu->si_fsr);
>>  #if 0
>>          err |= __get_user(current->thread.fpqdepth, &fpu->si_fpqdepth);
>>          if (current->thread.fpqdepth != 0)
> 
> This will need changing again if we ever fix the #if 0-d out
> code in this function, but I guess that will be obvious to whoever
> does that.

You mean the #endif part? Would an explicit err = 0 make things better?

Andreas

> 
> Incidentally, __get_user() can never fail [we catch unreadable memory
> earlier when wo do the lock_user_struct] so you could also just
> not do anything with its return value. Some of the other targets
> rely on this in their signal save/restore code. I think the use
> of the return value is mostly in code that was copy-and-pasted
> from the Linux kernel (which does use a __get_user() that can fail).
> 
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> 
> -- PMM
Peter Maydell - July 30, 2012, 5:09 p.m.
On 30 July 2012 17:59, Andreas Färber <afaerber@suse.de> wrote:
> Am 30.07.2012 18:13, schrieb Peter Maydell:
>> This will need changing again if we ever fix the #if 0-d out
>> code in this function, but I guess that will be obvious to whoever
>> does that.
>
> You mean the #endif part? Would an explicit err = 0 make things better?

Not really, things would still need changing later. Really the
problem is that most of the function is #if 0'd out (and never
called from anywhere); it's just unused stub code so anything
that shuts the compiler up will do, really.

-- PMM
Blue Swirl - July 30, 2012, 5:20 p.m.
On Mon, Jul 30, 2012 at 5:09 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 30 July 2012 17:59, Andreas Färber <afaerber@suse.de> wrote:
>> Am 30.07.2012 18:13, schrieb Peter Maydell:
>>> This will need changing again if we ever fix the #if 0-d out
>>> code in this function, but I guess that will be obvious to whoever
>>> does that.
>>
>> You mean the #endif part? Would an explicit err = 0 make things better?
>
> Not really, things would still need changing later. Really the
> problem is that most of the function is #if 0'd out (and never
> called from anywhere); it's just unused stub code so anything
> that shuts the compiler up will do, really.

I'll implement the #if 0'd part.

>
> -- PMM
Peter Maydell - July 30, 2012, 5:57 p.m.
On 30 July 2012 18:20, Blue Swirl <blauwirbel@gmail.com> wrote:
> On Mon, Jul 30, 2012 at 5:09 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> Not really, things would still need changing later. Really the
>> problem is that most of the function is #if 0'd out (and never
>> called from anywhere); it's just unused stub code so anything
>> that shuts the compiler up will do, really.
>
> I'll implement the #if 0'd part.

That would be cool, but just to be clear, I don't in general
think we should block small cleanup/fix series on "implement
$feature first", so I'm not saying you have to do that now.

-- PMM

Patch

diff --git a/linux-user/signal.c b/linux-user/signal.c
index 97f30d9..3d6b5df 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -2061,7 +2061,7 @@  restore_fpu_state(CPUSPARCState *env, qemu_siginfo_fpu_t *fpu)
         err = __copy_from_user(&env->fpr[0], &fpu->si_float_regs[0],
 	                             (sizeof(unsigned long) * 32));
 #endif
-        err |= __get_user(env->fsr, &fpu->si_fsr);
+        err = __get_user(env->fsr, &fpu->si_fsr);
 #if 0
         err |= __get_user(current->thread.fpqdepth, &fpu->si_fpqdepth);
         if (current->thread.fpqdepth != 0)