[Bug tree-optimization/100653] New: usage of scalar_storage_order produces incorrect result

[Bug tree-optimization/100653] New: usage of scalar_storage_order produces incorrect result

[jan.smets at nokia dot com]

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

            Bug ID: 100653
           Summary: usage of scalar_storage_order produces incorrect
                    result
           Product: gcc
           Version: 11.1.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: tree-optimization
          Assignee: unassigned at gcc dot gnu.org
          Reporter: jan.smets at nokia dot com
  Target Milestone: ---

Created attachment 50840
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=50840&action=edit
scalar_storage_order test case

Attached test case produces invalid results with -O2, good with -O1.

Known to fail : 11.1 10.3 9.3 8.5 7.5
Known to work: 6.5 
Target: x86_64-linux-gnu

$ gcc test.c -o /tmp/test -O2 -Wall -Wextra && /tmp/test
a0a1e01 EQ? 11e0a0a 0
$ gcc test.c -o /tmp/test -O1 -Wall -Wextra && /tmp/test
a0a1e01 EQ? a0a1e01 1

Thank you

[Bug tree-optimization/100653] usage of scalar_storage_order produces incorrect result

[pinskia at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
      Known to fail|                            |10.1.0, 11.1.0, 12.0, 7.3.0
     Ever confirmed|0                           |1
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2021-05-19
           Keywords|                            |wrong-code

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Confirmed.  It looks like some pass loses REF_REVERSE_STORAGE_ORDER.
As far as I can tell it has never worked.

[Bug tree-optimization/100653] usage of scalar_storage_order produces incorrect result

[ebotcazou at gcc dot gnu.org]

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

Eric Botcazou <ebotcazou at gcc dot gnu.org> changed:

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

--- Comment #2 from Eric Botcazou <ebotcazou at gcc dot gnu.org> ---
> Attached test case produces invalid results with -O2, good with -O1.

See the manual:

     Moreover, the use of type punning or aliasing to toggle the storage
     order is not supported; that is to say, a given scalar object
     cannot be accessed through distinct types that assign a different
     storage order to it.

typedef struct tIp6Addr
{
    unsigned int    s6_addr32[4];
} tIp6Addr;

struct _tBeTimNetAddr
{
    unsigned char isIPv4;
    union
    {
        unsigned int addr;
        tIp6Addr addr6;
    } BB_MSG_ENDIANNESS u;
} BB_MSG_ENDIANNESS;

_tBeTimNetAddr.u.addr6.s6_addr32[0] is accessed in little-endian and
_tBeTimNetAddr.u.addr is accessed in big-endian, but they are the same scalar.

[Bug tree-optimization/100653] usage of scalar_storage_order produces incorrect result

[jan.smets at nokia dot com]

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

--- Comment #3 from Jan Smets <jan.smets at nokia dot com> ---
Is there some way there can be warned against such invalid usages? Because
these things are really hard to see on a 'macro' level.

[Bug tree-optimization/100653] usage of scalar_storage_order produces incorrect result

[ebotcazou at gcc dot gnu.org]

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

--- Comment #4 from Eric Botcazou <ebotcazou at gcc dot gnu.org> ---
> Is there some way there can be warned against such invalid usages? Because
> these things are really hard to see on a 'macro' level.

Not at the moment, although this could probably be implemented.  But a simple
rule of thumb is to avoid using scalar_storage_order with unions at all.

[Bug tree-optimization/100653] usage of scalar_storage_order produces incorrect result

[ebotcazou at gcc dot gnu.org]

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

--- Comment #5 from Eric Botcazou <ebotcazou at gcc dot gnu.org> ---
> Not at the moment, although this could probably be implemented.  But a
> simple rule of thumb is to avoid using scalar_storage_order with unions at
> all.

I'm going to implement a simple warning for unions.

[Bug c/100653] usage of scalar_storage_order produces incorrect result

[cvs-commit at gcc dot gnu.org]

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

--- Comment #6 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Eric Botcazou <ebotcazou@gcc.gnu.org>:

https://gcc.gnu.org/g:401bd4adcfda9965363b1ac3ba7e1580f15d6883

commit r12-1072-g401bd4adcfda9965363b1ac3ba7e1580f15d6883
Author: Eric Botcazou <ebotcazou@adacore.com>
Date:   Wed May 26 19:12:05 2021 +0200

    Warn on type punning that toggles scalar storage order

    As documented in the manual, we do not support type punning that toggles
    the scalar storage order, so this adds a warning for the case of unions.

    gcc/c/
            PR c/100653
            * c-decl.c (finish_struct): Warn for a union containing an
aggregate
            field with a differing scalar storage order.
    gcc/testsuite/
            * gcc.dg/sso-13.c: New test.

[Bug c/100653] usage of scalar_storage_order produces incorrect result

[cvs-commit at gcc dot gnu.org]

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

--- Comment #7 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Eric Botcazou <ebotcazou@gcc.gnu.org>:

https://gcc.gnu.org/g:58523f23fefcef0850841e7055d75d4309f0453e

commit r12-1083-g58523f23fefcef0850841e7055d75d4309f0453e
Author: Eric Botcazou <ebotcazou@adacore.com>
Date:   Thu May 27 00:24:20 2021 +0200

    Small tweak to documentation of scalar_storage_order

    gcc/
            PR c/100653
            * doc/extend.texi (scalar_storage_order): Rephrase slightly.

[Bug c/100653] usage of scalar_storage_order produces incorrect result

[pinskia at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |george.thopas at gmail dot com

--- Comment #8 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
*** Bug 100804 has been marked as a duplicate of this bug. ***

[Bug c/100653] usage of scalar_storage_order produces incorrect result

[george.thopas at gmail dot com]

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

--- Comment #9 from George Thopas <george.thopas at gmail dot com> ---
Created attachment 51025
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=51025&action=edit
avoid eliminating fields with different endianess as equal

Short story :
Ran a bisect to find why this always works in the gcc-6 branch and has
different behaviour outside. Found out what happens (see below) and attached a
2-line patch for what I assume is missing.  Works for me but is it the 'right
thing'(tm) todo ?  

Thanks.

Long story:
I started digging into this in the hope to get a better understanding
In the last 3 years I've never seen this on the gcc-6 branch. 
Even though there's a tons of code and mixed endianess structures and 
unions validation. If I missed it, I at least needed to know at least why. 

The bisect ended up giving commit e7a3e0c653be4bd32f116dae06438896b7dc915b.
Reverting it for test purposes in gcc-7/gcc-8 just confirmed it is the 
right trigger.

There's obviously nothing wrong with that commit but at least it gave me 
a clue what to look for. To my understanding of the code :
The RPO change affects evaluation order when merging and eliminating blocks.
In the case of the example, the compiler ends up checking if both fields 
of the union are the same and can be merged/eliminated. Their both in the same
location. The code currently considers different signedness and bails out, but
not different  endianness and eliminates one depending one evaluation order. 
Whatever one that is.

[Bug c/100653] usage of scalar_storage_order produces incorrect result

[ebotcazou at gcc dot gnu.org]

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

--- Comment #10 from Eric Botcazou <ebotcazou at gcc dot gnu.org> ---
> The bisect ended up giving commit e7a3e0c653be4bd32f116dae06438896b7dc915b.
> Reverting it for test purposes in gcc-7/gcc-8 just confirmed it is the 
> right trigger.
> 
> There's obviously nothing wrong with that commit but at least it gave me 
> a clue what to look for. To my understanding of the code :
> The RPO change affects evaluation order when merging and eliminating blocks.
> In the case of the example, the compiler ends up checking if both fields 
> of the union are the same and can be merged/eliminated. Their both in the
> same location. The code currently considers different signedness and bails
> out, but not different endianness and eliminates one depending one
> evaluation order. 

That's by design, the entire implementation assumes that every scalar in memory
is accessed using the same storage order throughout the entire program.  If you
want to overcome this limitation, you need to reimplement it from scratch.