Coverity complains (CID 1390847) about some dead code in
do_sigreturn(). This is an if (err) clause that can never be
true, copied from the kernel (where __get_user returns an error).
The one code path that could report an error is in the
currently commented-out pseudocode for handling FPU register
restoring, so move the if into that comment (and fix the
broken indent in the comment in the process).

(The new position for the error check is also the semantically
correct one -- we should not restore the signal mask from
the signal frame if we get an error here, so the check must
be done before set_sigmask(), not after.)

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 linux-user/sparc/signal.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/linux-user/sparc/signal.c b/linux-user/sparc/signal.c
index 295e415b1e6..ead169fbaa2 100644
--- a/linux-user/sparc/signal.c
+++ b/linux-user/sparc/signal.c
@@ -282,7 +282,7 @@ long do_sigreturn(CPUSPARCState *env)
     uint32_t up_psr, pc, npc;
     target_sigset_t set;
     sigset_t host_set;
-    int err=0, i;
+    int i;
 
     sf_addr = env->regwptr[UREG_FP];
     trace_user_do_sigreturn(env, sf_addr);
@@ -320,10 +320,13 @@ long do_sigreturn(CPUSPARCState *env)
     }
 
     /* FIXME: implement FPU save/restore:
-         * __get_user(fpu_save, &sf->fpu_save);
-         * if (fpu_save)
-         *        err |= restore_fpu_state(env, fpu_save);
-         */
+     * __get_user(fpu_save, &sf->fpu_save);
+     * if (fpu_save) {
+     *     if (restore_fpu_state(env, fpu_save)) {
+     *         goto segv_and_exit;
+     *     }
+     * }
+     */
 
     /* This is pretty much atomic, no amount locking would prevent
          * the races which exist anyways.
@@ -336,9 +339,6 @@ long do_sigreturn(CPUSPARCState *env)
     target_to_host_sigset_internal(&host_set, &set);
     set_sigmask(&host_set);
 
-    if (err) {
-        goto segv_and_exit;
-    }
     unlock_user_struct(sf, sf_addr, 0);
     return -TARGET_QEMU_ESIGRETURN;
 
-- 
2.19.1

On 15/11/2018 12:46, Peter Maydell wrote:
> Coverity complains (CID 1390847) about some dead code in
> do_sigreturn(). This is an if (err) clause that can never be
> true, copied from the kernel (where __get_user returns an error).
> The one code path that could report an error is in the
> currently commented-out pseudocode for handling FPU register
> restoring, so move the if into that comment (and fix the
> broken indent in the comment in the process).
> 
> (The new position for the error check is also the semantically
> correct one -- we should not restore the signal mask from
> the signal frame if we get an error here, so the check must
> be done before set_sigmask(), not after.)
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  linux-user/sparc/signal.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

Do you apply it directly to the master or do you want a linux-user pull
request?

Thanks,
Laurent

On 15 November 2018 at 11:59, Laurent Vivier <laurent@vivier.eu> wrote:
> On 15/11/2018 12:46, Peter Maydell wrote:
>> Coverity complains (CID 1390847) about some dead code in
>> do_sigreturn(). This is an if (err) clause that can never be
>> true, copied from the kernel (where __get_user returns an error).
>> The one code path that could report an error is in the
>> currently commented-out pseudocode for handling FPU register
>> restoring, so move the if into that comment (and fix the
>> broken indent in the comment in the process).
>>
>> (The new position for the error check is also the semantically
>> correct one -- we should not restore the signal mask from
>> the signal frame if we get an error here, so the check must
>> be done before set_sigmask(), not after.)
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>>  linux-user/sparc/signal.c | 16 ++++++++--------
>>  1 file changed, 8 insertions(+), 8 deletions(-)
>>
>
> Reviewed-by: Laurent Vivier <laurent@vivier.eu>
>
> Do you apply it directly to the master or do you want a linux-user pull
> request?

Applied to master, thanks.

-- PMM