Message ID | 5ddedba0015c15117770b3d1860f7dd6d8d08a0c.1343664167.git.blauwirbel@gmail.com |
---|---|
State | New |
Headers | show |
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
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
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
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
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
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)