| Message ID | 20250211130626.3940412-1-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | target/alpha: Don't corrupt error_code with unknown softfloat flags | expand |
On 11/2/25 14:06, Peter Maydell wrote: > In do_cvttq() we set env->error_code with what is supposed to be a > set of FPCR exception bit values. However, if the set of float > exception flags we get back from softfloat for the conversion > includes a flag which is not one of the three we expect here > (invalid_cvti, invalid, inexact) then we will fall through the > if-ladder and set env->error_code to the unconverted softfloat > exception_flag value. This will then cause us to take a spurious > exception. > > This is harmless now, but when we add new floating point exception > flags to softfloat it will cause problems. Add an else clause to the > if-ladder to make it ignore any float exception flags it doesn't care > about. > > Specifically, without this fix, 'make check-tcg' will fail for Alpha > when the commit adding float_flag_input_denormal_used lands. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > I guess I forgot to run a 'make check-tcg' for all targets until > I was getting around to queuing the FEAT_AFP series :-/ > > target/alpha/fpu_helper.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/target/alpha/fpu_helper.c b/target/alpha/fpu_helper.c > index 63d9e9ce39c..f810a9b6a47 100644 > --- a/target/alpha/fpu_helper.c > +++ b/target/alpha/fpu_helper.c > @@ -476,6 +476,8 @@ static uint64_t do_cvttq(CPUAlphaState *env, uint64_t a, int roundmode) > exc = FPCR_INV; > } else if (exc & float_flag_inexact) { > exc = FPCR_INE; > + } else { > + exc = 0; > } > } > env->error_code = exc; Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> But (pre-existing) it would be clearer to not use the same variable for 2 distinct things. As a preliminary cleanup: -- >8 -- diff --git a/target/alpha/fpu_helper.c b/target/alpha/fpu_helper.c index 63d9e9ce39c..d1a17b71423 100644 --- a/target/alpha/fpu_helper.c +++ b/target/alpha/fpu_helper.c @@ -455,26 +455,27 @@ static uint64_t do_cvttq(CPUAlphaState *env, uint64_t a, int roundmode) { float64 fa; int64_t ret; - uint32_t exc; + uint32_t exc = 0; + int flags; fa = t_to_float64(a); ret = float64_to_int64_modulo(fa, roundmode, &FP_STATUS); - exc = get_float_exception_flags(&FP_STATUS); - if (unlikely(exc)) { + flags = get_float_exception_flags(&FP_STATUS); + if (unlikely(flags)) { set_float_exception_flags(0, &FP_STATUS); /* We need to massage the resulting exceptions. */ - if (exc & float_flag_invalid_cvti) { + if (flags & float_flag_invalid_cvti) { /* Overflow, either normal or infinity. */ if (float64_is_infinity(fa)) { exc = FPCR_INV; } else { exc = FPCR_IOV | FPCR_INE; } - } else if (exc & float_flag_invalid) { + } else if (flags & float_flag_invalid) { exc = FPCR_INV; - } else if (exc & float_flag_inexact) { + } else if (flags & float_flag_inexact) { exc = FPCR_INE; } } ---
On 2/11/25 05:06, Peter Maydell wrote: > In do_cvttq() we set env->error_code with what is supposed to be a > set of FPCR exception bit values. However, if the set of float > exception flags we get back from softfloat for the conversion > includes a flag which is not one of the three we expect here > (invalid_cvti, invalid, inexact) then we will fall through the > if-ladder and set env->error_code to the unconverted softfloat > exception_flag value. This will then cause us to take a spurious > exception. > > This is harmless now, but when we add new floating point exception > flags to softfloat it will cause problems. Add an else clause to the > if-ladder to make it ignore any float exception flags it doesn't care > about. > > Specifically, without this fix, 'make check-tcg' will fail for Alpha > when the commit adding float_flag_input_denormal_used lands. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > I guess I forgot to run a 'make check-tcg' for all targets until > I was getting around to queuing the FEAT_AFP series :-/ > > target/alpha/fpu_helper.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/target/alpha/fpu_helper.c b/target/alpha/fpu_helper.c > index 63d9e9ce39c..f810a9b6a47 100644 > --- a/target/alpha/fpu_helper.c > +++ b/target/alpha/fpu_helper.c > @@ -476,6 +476,8 @@ static uint64_t do_cvttq(CPUAlphaState *env, uint64_t a, int roundmode) > exc = FPCR_INV; > } else if (exc & float_flag_inexact) { > exc = FPCR_INE; > + } else { > + exc = 0; > } > } > env->error_code = exc; Reviewed-by: Richard Henderson <richard.henderson@linaro.org> And I think there's a case for Fixes: aa3bad5b59e7 ("target/alpha: Use float64_to_int64_modulo for CVTTQ") r~
diff --git a/target/alpha/fpu_helper.c b/target/alpha/fpu_helper.c index 63d9e9ce39c..f810a9b6a47 100644 --- a/target/alpha/fpu_helper.c +++ b/target/alpha/fpu_helper.c @@ -476,6 +476,8 @@ static uint64_t do_cvttq(CPUAlphaState *env, uint64_t a, int roundmode) exc = FPCR_INV; } else if (exc & float_flag_inexact) { exc = FPCR_INE; + } else { + exc = 0; } } env->error_code = exc;
In do_cvttq() we set env->error_code with what is supposed to be a set of FPCR exception bit values. However, if the set of float exception flags we get back from softfloat for the conversion includes a flag which is not one of the three we expect here (invalid_cvti, invalid, inexact) then we will fall through the if-ladder and set env->error_code to the unconverted softfloat exception_flag value. This will then cause us to take a spurious exception. This is harmless now, but when we add new floating point exception flags to softfloat it will cause problems. Add an else clause to the if-ladder to make it ignore any float exception flags it doesn't care about. Specifically, without this fix, 'make check-tcg' will fail for Alpha when the commit adding float_flag_input_denormal_used lands. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- I guess I forgot to run a 'make check-tcg' for all targets until I was getting around to queuing the FEAT_AFP series :-/ target/alpha/fpu_helper.c | 2 ++ 1 file changed, 2 insertions(+)