| Message ID | 20200327094945.23768-6-alex.bennee@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | A selection of sanitiser fixes | expand |
On Fri, 27 Mar 2020 at 09:49, Alex Bennée <alex.bennee@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.bennee@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. 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 ?? thanks -- PMM
11:53 Pet, 27.03.2020. Alex Bennée <alex.bennee@linaro.org> је написао/ла: > > The undefined behaviour checker Alex, what exactly is "undefined behaviour checker"? If this is a test, can you give us a link? Sincerely, Aleksandar > pointed out that a shift of 64 would > lead to undefined behaviour. > > Signed-off-by: Alex Bennée <alex.bennee@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; > + } > } > > /*---------------------------------------------------------------------------- > -- > 2.20.1 > >
Aleksandar Markovic <aleksandar.qemu.devel@gmail.com> writes: > 11:53 Pet, 27.03.2020. Alex Bennée <alex.bennee@linaro.org> је написао/ла: >> >> The undefined behaviour checker > > Alex, what exactly is "undefined behaviour checker"? If this is a test, can > you give us a link? It's enabled by our sanitizers build: ../../configure --cc=clang-8 --cxx=clang++-8 --enable-sanitizers > > Sincerely, > Aleksandar > >> pointed out that a shift of 64 would >> lead to undefined behaviour. >> >> Signed-off-by: Alex Bennée <alex.bennee@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; >> + } >> } >> >> > /*---------------------------------------------------------------------------- >> -- >> 2.20.1 >> >>
On 3/27/20 3:09 AM, Peter Maydell wrote: > On Fri, 27 Mar 2020 at 09:49, Alex Bennée <alex.bennee@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.bennee@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~
On Fri, 27 Mar 2020 at 22:27, Richard Henderson <richard.henderson@linaro.org> wrote: > 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... There was certainly a proposal to do that (I think from a Google engineer) for C++, I forget whether the equivalent C change has also been proposed. > > 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. Mmm. My vote is for fixing addFloatx80Sigs -- now we just need to figure out what the desired behaviour is. thanks -- PMM
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; + } } /*----------------------------------------------------------------------------
The undefined behaviour checker pointed out that a shift of 64 would lead to undefined behaviour. Signed-off-by: Alex Bennée <alex.bennee@linaro.org> --- fpu/softfloat.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)