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;
+ }
}
/*----------------------------------------------------------------------------
--
2.20.1
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
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
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
>>
>>
--
Alex Bennée
© 2016 - 2026 Red Hat, Inc.