On 3/27/20 3:09 AM, Peter Maydell wrote: > On Fri, 27 Mar 2020 at 09:49, Alex Bennée <alex.ben...@linaro.org> wrote: >> >> The undefined behaviour checker pointed out that a shift of 64 would >> lead to undefined behaviour. >> >> Signed-off-by: Alex Bennée <alex.ben...@linaro.org> >> --- >> fpu/softfloat.c | 11 ++++++++--- >> 1 file changed, 8 insertions(+), 3 deletions(-) >> >> diff --git a/fpu/softfloat.c b/fpu/softfloat.c >> index 301ce3b537b..444d35920dd 100644 >> --- a/fpu/softfloat.c >> +++ b/fpu/softfloat.c >> @@ -3834,9 +3834,14 @@ void normalizeFloatx80Subnormal(uint64_t aSig, >> int32_t *zExpPtr, >> { >> int8_t shiftCount; >> >> - shiftCount = clz64(aSig); >> - *zSigPtr = aSig<<shiftCount; >> - *zExpPtr = 1 - shiftCount; >> + if (aSig) { >> + shiftCount = clz64(aSig); >> + *zSigPtr = aSig << shiftCount; >> + *zExpPtr = 1 - shiftCount; >> + } else { >> + *zSigPtr = 0; >> + *zExpPtr = 1 - 64; >> + } >> } > > Ah yes, I saw this one in Coverity: CID 1421991. > > RTH marked the Coverity issue as a false positive with the rationale > "We assume an out-of-range shift count is merely IMPLEMENTATION DEFINED > and not UNDEFINED (in the Arm ARM sense), and so cannot turn a 0 value > into a non-zero value." > but I think I disagree with that. We can assume that for the TCG IR > where we get to define shift semantics because we're doing the codegen, > but we can't assume it in C code, because it's not included in the set > of extended guarantees provided by -fwrapv as far as I know.
Perhaps. Of course we also know from our broad knowledge of architectures, that a compiler would really have to go out of its way for this to happen. I really hate C in this way, sometimes. I wonder if I have the energy to petition the committee to drop, for C202? all of the "undefined" nonsense that only applies to sign-magnitute and ones-compliment computers, which haven't been seen since the 70's... > That said, is it valid for this function to be called with a zero > aSig value ? I think all these normalizeFloat*Subnormal() functions > assume non-zero sig input, and the only callsite where it's not clearly > obvious that this is obvious that the sig input is non-zero is the call to > normalizeFloatx80Subnormal() from addFloatx80Sigs(). So perhaps we > just need to check and fix that callsite ?? You're right -- addFloatx80Sigs is the only use out of 26 that doesn't have a preceding check for 0. r~