[Bug c/101953] New: bug on the default cast operator from double to unsigned short

[Bug c/101953] New: bug on the default cast operator from double to unsigned short

[mlwmohawk at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101953

            Bug ID: 101953
           Summary: bug on the default cast operator from double to
                    unsigned short
           Product: gcc
           Version: 8.3.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c
          Assignee: unassigned at gcc dot gnu.org
          Reporter: mlwmohawk at gmail dot com
  Target Milestone: ---

markw@snoopy:~$ gcc --version
gcc (Ubuntu 9.3.0-17ubuntu1~20.04) 9.3.0
markw@snoopy:~$ ./badpi
-32000 vs -32000

On Raspberry PI:
pi@raspberrypi:~ $ gcc --version
gcc (Raspbian 8.3.0-6+rpi1) 8.3.0
pi@raspberrypi:~ $ ./badpi
0 vs -32000

This is the code:
#include <stdio.h>
#include <stdint.h>
#include <math.h>

int main(int argc, char **argv)
{
    uint32_t foo;
    int bar;
    double val = -32000;

    bar = ceil(val);
    foo = ceil(val);

    printf("%d vs %d\n", foo, bar);
}

gcc -Wall -o badpi badpi.c -lm

[Bug c/101953] bug on the default cast operator from double to unsigned short

[mlwmohawk at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101953

--- Comment #1 from M W <mlwmohawk at gmail dot com> ---
This also happens on C++

[Bug c/101953] bug on the default cast operator from double to unsigned short

[schwab@linux-m68k.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101953

Andreas Schwab <schwab@linux-m68k.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |INVALID
             Status|UNCONFIRMED                 |RESOLVED

--- Comment #2 from Andreas Schwab <schwab@linux-m68k.org> ---
-32000.0 is outside the range of uint32_t, thus the conversion is undefined.

[Bug c/101953] bug on the default cast operator from double to unsigned short

[mlwmohawk at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101953

M W <mlwmohawk at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|INVALID                     |---
             Status|RESOLVED                    |UNCONFIRMED

--- Comment #3 from M W <mlwmohawk at gmail dot com> ---
(In reply to Andreas Schwab from comment #2)
> -32000.0 is outside the range of uint32_t, thus the conversion is undefined.

I understand your response, but I disagree. Why is it different than x86_64? I
have been using gcc for literally decades and have NEVER seen this evaluation.
Why is it different from 9.3.0?

On CentOS 7, we get

[root@localhost ~]# gcc --version
gcc (GCC) 4.8.5 20150623 (Red Hat 4.8.5-16)
Copyright (C) 2015 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

[root@localhost ~]# ./badpi 
-32000 vs -32000


This is a bug that breaks real code. There is an extra step being taken on
raspberry pi that breaks existing code. 

If you think this is the correct behavior, remember, it is unexpected. At least
give a warning when a value is set to zero instead of a faithful attempt to
return the correct bits.

[Bug c/101953] bug on the default cast operator from double to unsigned short

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101953

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
         Resolution|---                         |INVALID

--- Comment #4 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
>Why is it different than x86_64? 

Because of different instructions and such.

>This is a bug that breaks real code. 
This is NOT a bug, it is a bug in their code.

[Bug c/101953] bug on the default cast operator from double to unsigned short

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101953

--- Comment #5 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
>it is unexpected.
Unexpected because not understanding C/C++ standard is normal these days.

[Bug c/101953] bug on the default cast operator from double to unsigned short

[mlwmohawk at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101953

--- Comment #6 from M W <mlwmohawk at gmail dot com> ---

"Unexpected because not understanding C/C++ standard is normal these days."

I'm sort of ticked off by that. I have used some form of this code for over two
decades on a lot of different platforms. Mac, Windows, Linux, and Sun and have
never seen this behavior. 

At the very minimum, there should be an error that asserts that states "Result
set to zero for 'reason'" because there is code that assumes that a good faith
bitwise return would be appropriate.

[Bug c/101953] bug on the default cast operator from double to unsigned short

[mlwmohawk at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101953

M W <mlwmohawk at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |UNCONFIRMED
         Resolution|INVALID                     |---

--- Comment #7 from M W <mlwmohawk at gmail dot com> ---
Don't agree

[Bug c/101953] bug on the default cast operator from double to unsigned short

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101953

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |DUPLICATE
             Status|UNCONFIRMED                 |RESOLVED

--- Comment #8 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
.

*** This bug has been marked as a duplicate of bug 89604 ***

[Bug c/101953] bug on the default cast operator from double to unsigned short

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101953

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|DUPLICATE                   |INVALID

--- Comment #9 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Sorry wrong bug #.

[Bug c/101953] bug on the default cast operator from double to unsigned short

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101953

--- Comment #10 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
6.3.1.4 Real floating and integer
1 When a finite value of real floating type is converted to an integer type
other than _Bool,
the fractional part is discarded (i.e., the value is truncated toward zero). If
the value of
the integral part cannot be represented by the integer type, the behavior is
undefined.61)

[Bug c/101953] bug on the default cast operator from double to unsigned short

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101953

--- Comment #11 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Andrew Pinski from comment #10)
> 6.3.1.4 Real floating and integer
> 1 When a finite value of real floating type is converted to an integer type
> other than _Bool,
> the fractional part is discarded (i.e., the value is truncated toward zero).
> If the value of
> the integral part cannot be represented by the integer type, the behavior is
> undefined.61)

http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1548.pdf :).

[Bug c/101953] bug on the default cast operator from double to unsigned short

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101953

--- Comment #12 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
For aarch64 we do:
        fcvtzs  w0, d0 ; int
        fcvtzu  w0, d0 ; uint32_t
For ARM:
        vcvt.s32.f64    s15, d16 ; int
        vcvt.u32.f64    s15, d7 ; uint32_t



For x86 we do:
        cvttsd2sil      %xmm0, %eax ; int
        cvttsd2siq      %xmm0, %rax ;unsigned int 32

So in the case of x86, we convert it to long long and then truncate while on
aarch64/arm, it is a direct conversion.  This is because on x86 there is no
instruction which does unsigned; only signed.

[Bug c/101953] bug on the default cast operator from double to unsigned short

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101953

--- Comment #13 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Newer PowerPC does:
        fctiwz 0,0 ; int
        fctiwuz 0,0 ;unsigned

Older PPC does:
        fctiwz 0,0 ; int
        fctidz 0,0 ; unsigned

So it depends on which generation of PowerPC ISA you use.

[Bug c/101953] bug on the default cast operator from double to unsigned short

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101953

--- Comment #14 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
MIPS64 does:
        trunc.w.d $f0,$f0 ; int

        c.le.d  $fcc0,$f1,$f0
        bc1t    $fcc0,.L2
        nop

        trunc.w.d $f0,$f0
        mfc1    $2,$f0
        b       .L3
        nop

.L2:
        sub.d   $f0,$f0,$f1
        li      $3,-2147483648                  # 0xffffffff80000000
        trunc.w.d $f0,$f0
        mfc1    $2,$f0
        or      $2,$2,$3
.L3: ; unsigned

So MIPS64 does a comparison to see if the float is greater than INT_MAX and
then does a subtraction and then conversion and then adds back INT_MAX.

[Bug c/101953] bug on the default cast operator from double to unsigned short

[mlwmohawk at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101953

M W <mlwmohawk at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|INVALID                     |---
             Status|RESOLVED                    |UNCONFIRMED

--- Comment #15 from M W <mlwmohawk at gmail dot com> ---
So, let me get this straight, you honestly think that this is the correct
behavior? Seriously, porting between platforms is hard enough, but this is
crazy.

"-Wall" raises no warnings. 
It is inconsistent between platforms


A very common construct returns VERY different results across different
platforms? 

I'm quite sure that is impacting way more projects than have realized it.

[Bug c/101953] bug on the default cast operator from double to unsigned short

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101953

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |INVALID
             Status|UNCONFIRMED                 |RESOLVED

--- Comment #16 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Undefined mean it could do anything.

Sorry but if you want something that has defined behavior like this, a
different language is needed.

[Bug c/101953] bug on the default cast operator from double to unsigned short

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101953

--- Comment #17 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to M W from comment #15)
> I'm quite sure that is impacting way more projects than have realized it.

Almost all of the open source ones have been fixed years ago.  Both with IBM
and ARM doing a lot of the lifting.

[Bug c/101953] bug on the default cast operator from double to unsigned short

[mlwmohawk at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101953

--- Comment #18 from M W <mlwmohawk at gmail dot com> ---
I can't believe that this is acceptable. Shocked. in fact.

[Bug c/101953] bug on the default cast operator from double to unsigned short

[mlwmohawk at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101953

M W <mlwmohawk at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|INVALID                     |---
             Status|RESOLVED                    |UNCONFIRMED

--- Comment #19 from M W <mlwmohawk at gmail dot com> ---
There is a difference between "undefined" and "unpredictable." If returning "0"
instead of a binary representation of the number amounts to "undefined" (like
every other platform) then I think you are wrong. 

Done.

[Bug c/101953] bug on the default cast operator from double to unsigned short

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101953

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |INVALID
             Status|UNCONFIRMED                 |RESOLVED

--- Comment #20 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to M W from comment #19)
> There is a difference between "undefined" and "unpredictable." If returning
> "0" instead of a binary representation of the number amounts to "undefined"
> (like every other platform) then I think you are wrong. 
> 
> Done.

If the value of
the integral part cannot be represented by the integer type, the behavior is
undefined.

[Bug c/101953] bug on the default cast operator from double to unsigned short

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101953

--- Comment #21 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to M W from comment #18)
> I can't believe that this is acceptable. Shocked. in fact.

You should read bug 323.

[Bug c/101953] bug on the default cast operator from double to unsigned short

[harald at gigawatt dot nl]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101953

Harald van Dijk <harald at gigawatt dot nl> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |harald at gigawatt dot nl

--- Comment #22 from Harald van Dijk <harald at gigawatt dot nl> ---
(In reply to M W from comment #3)
> If you think this is the correct behavior, remember, it is unexpected. At
> least give a warning when a value is set to zero instead of a faithful
> attempt to return the correct bits.

Agreed. Without special options, I think this behaviour is defensible,
undefined is undefined, but I would have expected -fsanitize=undefined to catch
this. clang's -fsanitize=undefined does catch this, gcc's doesn't (tested in
Ubuntu's version of GCC 10).

[Bug c/101953] bug on the default cast operator from double to unsigned short

[msebor at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101953

Martin Sebor <msebor at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |msebor at gcc dot gnu.org

--- Comment #23 from Martin Sebor <msebor at gcc dot gnu.org> ---
With a constant argument it should be possible to issue a warning at compile
time.  If that's sufficient I would suggest to open a new bug requesting it. 
Similarly, if the sanitizers should detect it but don't please open a separate
request to make that work.  Link them to this one for background.

[Bug c/101953] bug on the default cast operator from double to unsigned short

[mlwmohawk at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101953

--- Comment #24 from M W <mlwmohawk at gmail dot com> ---
I know it is documented as "undefined," but it is also unexpected without even
a warning. Anyone maintaining code and/or porting to rpi will simply not expect
this behavior. I certainly didn't. I had been using my code for over a decade
and the default casting did the "correct" (expected) thing across many
different platforms.


I'm really not sure how anyone is OK with this behavior. I'm concerned about
the mountains of code out in the real world that may be failing silently. Now
that the RaspberryPI is being used more and more as a drop in replacement for
old x86 servers, this will impact a lot of people.

[Bug c/101953] bug on the default cast operator from double to unsigned short

[joseph at codesourcery dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101953

--- Comment #25 from joseph at codesourcery dot com <joseph at codesourcery dot com> ---
The option to use to detect this is -fsanitize=float-cast-overflow (note: 
I haven't tested if it detects this particular case).  As per the manual: 
"Unlike other similar options, @option{-fsanitize=float-cast-overflow} is 
not enabled by @option{-fsanitize=undefined}.".  (Annex F makes the result 
an unspecified value with "invalid" raised, instead of being undefined 
behavior, which justifies not including it in -fsanitize=undefined by 
default.  That means that each time such a conversion occurs in the 
abstract machine, some value representable in the result type must be 
produced; it can't be a "wobbly" value where different uses of the result 
of the conversion, that was only executed once in the abstract machine, 
act like different values or like values not of the result type.  If such 
a conversion does produce a wobbly value, that's a bug; see bug 93806 
comments 27 to 29 for examples of such a bug.)

[Bug c/101953] bug on the default cast operator from double to unsigned short

[mlwmohawk at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101953

--- Comment #26 from M W <mlwmohawk at gmail dot com> ---
pi@raspberrypi:~ $ gcc -fsanitize=float-cast-overflow -Wall -o badpi badpi.c
-lm
pi@raspberrypi:~ $ 

That flag doesn't work

[Bug c/101953] bug on the default cast operator from double to unsigned short

[harald at gigawatt dot nl]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101953

--- Comment #27 from Harald van Dijk <harald at gigawatt dot nl> ---
(In reply to joseph@codesourcery.com from comment #25)
> The option to use to detect this is -fsanitize=float-cast-overflow (note: 
> I haven't tested if it detects this particular case).  As per the manual: 
> "Unlike other similar options, @option{-fsanitize=float-cast-overflow} is 
> not enabled by @option{-fsanitize=undefined}.".  (Annex F makes the result 
> an unspecified value with "invalid" raised, instead of being undefined 
> behavior, which justifies not including it in -fsanitize=undefined by 
> default.

Have just tested that -fsanitize=float-cast-overflow does indeed catch this
case. Thanks for the explanation, that makes sense.

The fact that it's not included by -fsanitize=undefined even in compilations
where Annex F is not followed or does not apply is a bit weird, but changing it
to be included was bug #100591, closed as invalid; I won't open a new bug
asking for the same thing again.

(In reply to M W from comment #24)
> I know it is documented as "undefined," but it is also unexpected without
> even a warning.

Martin Sebor requested opening a new bug if you'd like to see a compile-time
warning for this, rather than tracking that as part of this bug.

(In reply to M W from comment #26)
> pi@raspberrypi:~ $ gcc -fsanitize=float-cast-overflow -Wall -o badpi badpi.c
> -lm
> pi@raspberrypi:~ $ 
> 
> That flag doesn't work

The flag does work, but it's a runtime warning, not a compile-time warning. Run
badpi and you should see it.

[Bug c/101953] bug on the default cast operator from double to unsigned short

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101953

--- Comment #28 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
That flag does work:
gcc -fsanitize=float-cast-overflow -Wall -o pr101953{,.c} -lm; ./pr101953
pr101953.c:12:11: runtime error: -32000 is outside the range of representable
values of type 'unsigned int'
-32000 vs -32000
Sanitizers are runtime instrumentation, so you actually need to run the
program.