[Bug c++/103699] New: Reading or writing unaligned integers is wrongly optimized (GCC-11 and up)

[Bug c++/103699] New: Reading or writing unaligned integers is wrongly optimized (GCC-11 and up)

[kobalicek.petr at gmail dot com]

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

            Bug ID: 103699
           Summary: Reading or writing unaligned integers is wrongly
                    optimized (GCC-11 and up)
           Product: gcc
           Version: 11.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: kobalicek.petr at gmail dot com
  Target Milestone: ---

I have found a strange issue. When I use __attribute__((aligned(1)) on a type
to essentially annotate its lower alignment for memory load/store purposes, the
compiler would optimize such loads/stores out without any warnings. I have
never had a problem with this in GCC nor Clang, only GCC 11+ generates wrong
code for me.

Best illustrated in the code below [Compile with -O2 -std=c++17]

#include <stdint.h>

typedef uint32_t __attribute__((__aligned__(1))) UnalignedUInt32;
typedef uint64_t __attribute__((__aligned__(1))) UnalignedUInt64;

uint32_t byteswap32(uint32_t x) noexcept {
  return (x << 24) | (x >> 24) | ((x << 8) & 0x00FF0000u) | ((x >> 8) &
0x0000FF00);
}

uint64_t byteswap64(uint64_t x) noexcept {
  return ((x << 56) & 0xff00000000000000) |
         ((x << 40) & 0x00ff000000000000) |
         ((x << 24) & 0x0000ff0000000000) |
         ((x <<  8) & 0x000000ff00000000) |
         ((x >>  8) & 0x00000000ff000000) |
         ((x >> 24) & 0x0000000000ff0000) |
         ((x >> 40) & 0x000000000000ff00) |
         ((x >> 56) & 0x00000000000000ff);
}

static inline void writeU64be(void* p, uint64_t val) {
  static_cast<UnalignedUInt64*>(p)[0] = byteswap64(val);
}

static inline uint32_t readU32be(const void* p) noexcept {
  uint32_t x = static_cast<const UnalignedUInt32*>(p)[0];
  return byteswap32(x);
}

// Returns 0xBB
uint32_t test_1() {
    uint8_t array[16] {0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15};
    writeU64be(array + 6, 0xAABBCCDDEEFF1213);
    return array[7];
}

// Returns 0xCC
uint32_t test_2() {
    uint8_t array[16] {0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15};
    writeU64be(array + 6, 0xAABBCCDDEEFF1213);
    return array[8];
}

// Returns 0xDD
uint32_t test_3() {
    uint8_t array[16] {0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15};
    writeU64be(array + 6, 0xAABBCCDDEEFF1213);
    return array[9];
}

// Returns 0xEE
uint32_t test_4() {
    uint8_t array[16] {0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15};
    writeU64be(array + 6, 0xAABBCCDDEEFF1213);
    return array[10];
}

// Returns 0708090A - the write has no effect when read with readU32be()
uint32_t test_u32() {
    uint8_t array[16] {0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15};
    writeU64be(array + 6, 0xAABBCCDDEEFF1213);
    return readU32be(array + 7);
}

So I'm wondering, is this a correct behavior?

It seems like a bug in the optimizer to me, because when the code is dynamic
(the data is not consts) it seems to work as expected. I have found it in a
failing unit test (GCC 11 is the only compiler that fails).

Compiler Explorer: https://godbolt.org/z/9G9cx83oq

[Bug c++/103699] Reading or writing a constant unaligned value is wrongly optimized causing an incorrect result (GCC-11 and up)

[pinskia at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Last reconfirmed|                            |2021-12-14
     Ever confirmed|0                           |1
             Status|UNCONFIRMED                 |WAITING

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
On x86_64 the memory model is such that the unaligned loads are fine always.
If you don't want/need to do the load as not unaligned on x86 you need to use
volatile instead.

The testcase below gives the same results even on clang, there are no loads
left.

So you need to provide the testcase which is failing for you and on the
specific target which is failing for you.

For an example on aarch64, you might want to use -mstrict-align if you have
code which requires unaligned access to not go through the wider load/stores
(there is at least a bug there with respect to memset in GCC 11 and on the
trunk but it is in the process of being fixed).

[Bug c++/103699] Reading or writing a constant unaligned value is wrongly optimized causing an incorrect result (GCC-11 and up)

[kobalicek.petr at gmail dot com]

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

--- Comment #2 from Petr <kobalicek.petr at gmail dot com> ---
If you compile this with clang the function test_u32() will corretly return the
expected 0xBBCCDDEE and not 0x0708090A.

If you compile with older GCC, like GCC 10, the test would also return
0xBBCCDDEE. Only GCC-11 and upward return 0x0708090A.

Based on the documentation here:

https://gcc.gnu.org/onlinedocs/gcc/Common-Variable-Attributes.html#Common-Variable-Attributes

It's stated that "When used as part of a typedef, the aligned attribute can
both increase and decrease alignment, and specifying the packed attribute
generates a warning." - it explicitly allows to create a type with lower
alignment, so I don't consider this undefined behavior - in general UBSAN is
fine with this construct.

[Bug c++/103699] Reading or writing a constant unaligned value is wrongly optimized causing an incorrect result (GCC-11 and up)

[kobalicek.petr at gmail dot com]

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

--- Comment #3 from Petr <kobalicek.petr at gmail dot com> ---
BTW this almost seems like an optimizer bug, because if you compile the code
without optimizations with GCC 11 (or with -O1) it also returns the expected
value - only optimized compilation with GCC 11 returns the wrong one.

[Bug c++/103699] Reading or writing a constant unaligned value is wrongly optimized causing an incorrect result (GCC-11 and up)

[kobalicek.petr at gmail dot com]

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

--- Comment #4 from Petr <kobalicek.petr at gmail dot com> ---
Additional test case:

#include <stdio.h>
#include <stdint.h>

typedef uint32_t __attribute__((__aligned__(1))) UnalignedUInt32;
typedef uint64_t __attribute__((__aligned__(1))) UnalignedUInt64;

uint32_t byteswap32(uint32_t x) noexcept {
  return (x << 24) | (x >> 24) | ((x << 8) & 0x00FF0000u) | ((x >> 8) &
0x0000FF00);
}

uint64_t byteswap64(uint64_t x) noexcept {
  return ((x << 56) & 0xff00000000000000) |
         ((x << 40) & 0x00ff000000000000) |
         ((x << 24) & 0x0000ff0000000000) |
         ((x <<  8) & 0x000000ff00000000) |
         ((x >>  8) & 0x00000000ff000000) |
         ((x >> 24) & 0x0000000000ff0000) |
         ((x >> 40) & 0x000000000000ff00) |
         ((x >> 56) & 0x00000000000000ff);
}

static inline void writeU64be(void* p, uint64_t val) {
  static_cast<UnalignedUInt64*>(p)[0] = byteswap64(val);
}

static inline uint32_t readU32be(const void* p) noexcept {
  uint32_t x = static_cast<const UnalignedUInt32*>(p)[0];
  return byteswap32(x);
}

// Returns 0708090A
uint32_t test_u32() {
    uint8_t array[16] {0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15};
    writeU64be(array + 6, 0xAABBCCDDEEFF1213);
    return readU32be(array + 7);
}

static uint8_t array_static[16] {0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15};

int main() {
    printf("%08X\n", test_u32());

    writeU64be(array_static + 6, 0xAABBCCDDEEFF1213);
    printf("%08X\n", readU32be(array_static + 7));
    return 0;
}

It prints:
0708090A
BBCCDDEE

Clang prints:
BBCCDDEE
BBCCDDEE

[Bug c++/103699] Reading or writing a constant unaligned value is wrongly optimized causing an incorrect result (GCC-11 and up)

[pinskia at gcc dot gnu.org]

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

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

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

--- Comment #5 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Basically you have an aliasing violation.
You are writing as an uint64_t but then reading as uint32_t.
Use either -fno-strict-aliasing or add may_alias attribute to UnalignedUInt32
and UnalignedUInt64.

[Bug c++/103699] Reading or writing a constant unaligned value is wrongly optimized causing an incorrect result (GCC-11 and up)

[kobalicek.petr at gmail dot com]

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

--- Comment #6 from Petr <kobalicek.petr at gmail dot com> ---
For now I have disabled unaligned load/store optimizations in my projects when
dealing with GCC 11 and upwards.

I still think that GCC is wrong in this case regardless of strict aliasing. The
code in func_u32() is essentially creating a constant, and GCC 11+ is the only
compiler returning it wrong and also inconsistently between optimization
levels.

[Bug c++/103699] Reading or writing a constant unaligned value is wrongly optimized causing an incorrect result (GCC-11 and up)

[pinskia at gcc dot gnu.org]

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

--- Comment #7 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Petr from comment #6)
> For now I have disabled unaligned load/store optimizations in my projects
> when dealing with GCC 11 and upwards.

As I mentioned you could just add the may_alias attribute and it will work too.
You were just lucky that you only ran into this issue for GCC 11 really

> I still think that GCC is wrong in this case regardless of strict aliasing.
> The code in func_u32() is essentially creating a constant, and GCC 11+ is
> the only compiler returning it wrong and also inconsistently between
> optimization levels.

No you are still violating C/C++ aliasing rules. No matter what you think. It
is undefined behavior.
Strict aliasing is only enabled for -O2 and above which is why it might look
inconsistent. Plus since it is undefined behavior different optimizations level
will always have different behavior.

Again the following code:
    uint8_t array[16] {0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15};
    writeU64be(array + 6, 0xAABBCCDDEEFF1213);
    return readU32be(array + 7);

Which does basically:
    uint8_t array[16] {0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15};
    *(unsigned long*)(array+6) = __builtin_bswap64(0xAABBCCDDEEFF1213);
    return *(unsigned*)(array+7);

is undefined as you write to array+6 as an unsigned long but then read array+7
as an unsigned type. 

Plus aligned(1) attribute has nothing to do with the issue here:
Take:
static unsigned g(unsigned *a)
{
    return *a;
}

unsigned f(void)
{
    unsigned char array[16] {0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15};
    *(unsigned long*)(array) = __builtin_bswap64(0xAABBCCDDEEFF1213);
    return g((unsigned*)(array));
}
----- CUT ---
Compile it with -O2  -fno-early-inlining and you will same bad results as you
had got with aligned.

[Bug c++/103699] Reading or writing a constant unaligned value is wrongly optimized causing an incorrect result (GCC-11 and up)

[kobalicek.petr at gmail dot com]

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

--- Comment #8 from Petr <kobalicek.petr at gmail dot com> ---
My only problem is that A returns a different value compared to B, C, and D:

uint32_t test_u32_a() {
    char array[16] {0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15};
    writeU64be(array + 6, 0xAABBCCDDEEFF1213);
    return readU32be(array + 7);
}

uint32_t test_u32_b() {
    static char array[16] {0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15};
    writeU64be(array + 6, 0xAABBCCDDEEFF1213);
    return readU32be(array + 7);
}

uint32_t test_u32_c() {
    thread_local char array[16] {0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15};
    writeU64be(array + 6, 0xAABBCCDDEEFF1213);
    return readU32be(array + 7);
}

uint32_t test_u32_d(char array[16]) {
    writeU64be(array + 6, 0xAABBCCDDEEFF1213);
    return readU32be(array + 7);
}

And when you compile this, you would actually see that ALL functions evaluate
to a constant (because it's known what the output will be), but only in A case
the constant is different (of course because B, C, D have side effects):

test_u32_a():
        mov     eax, 117967114
        ret
test_u32_b():
        movabs  rax, 1374442237689904042
        mov     QWORD PTR test_u32_b()::array[rip+6], rax
        mov     eax, -1144201746
        ret
test_u32_c():
        movabs  rax, 1374442237689904042
        mov     QWORD PTR fs:test_u32_c()::array@tpoff+6, rax
        mov     eax, -1144201746
        ret
test_u32_d(char*):
        movabs  rax, 1374442237689904042
        mov     QWORD PTR [rdi+6], rax
        mov     eax, -1144201746
        ret

So yeah, we can talk about breaking strict aliasing here, but it's just
inconsistent. I would just expect all functions return the same value.

[Bug c++/103699] Reading or writing a constant unaligned value is wrongly optimized causing an incorrect result (GCC-11 and up)

[pinskia at gcc dot gnu.org]

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

--- Comment #9 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Petr from comment #8)
> So yeah, we can talk about breaking strict aliasing here, but it's just
> inconsistent. I would just expect all functions return the same value.

inconsistent because the behavior is undefined ....
In the case of the local array, GCC decides the store to array + 6 is not
needed and is removed.
It is inconsistent due to the inlining choices really. This is why in my
simplified example in comment #7, -fno-early-inlining is needed and all.

GCC 11+ is even more picky about strict aliasing than before really. That is
the basic thing here.

[Bug c++/103699] Reading or writing a constant unaligned value is wrongly optimized causing an incorrect result (GCC-11 and up)

[kobalicek.petr at gmail dot com]

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

--- Comment #10 from Petr <kobalicek.petr at gmail dot com> ---
Well, the problem is, that when you compile it with "-fsanitize=undefined" - it
won't report any undefined behavior, and the function would return the expected
value.

I even tried to make everything constexpr - and constexpr by definition should
never involve undefined behavior, right? But GCC 11 compiles the code with
constexpr, doesn't complain against undefined behavior, and again, returns the
wrong value.

What I miss here, from user perspective, is some kind of diagnostics. If you
remove code that is provable to have effect on the code following it, why not
to complain, at least in constexpr case?

[Bug c++/103699] Reading or writing a constant unaligned value is wrongly optimized causing an incorrect result (GCC-11 and up)

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

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

--- Comment #11 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
-fsanitize=undefined isn't an TBAA sanitizer, it can't detect aliasing
violations.
As for constexpr, you'd have to show us what exactly you've tested.
constexpr keyword on a function doesn't mean the function will be evaluated at
compile time, that happens only if it is called in constant expression.

[Bug c++/103699] Reading or writing a constant unaligned value is wrongly optimized causing an incorrect result (GCC-11 and up)

[kobalicek.petr at gmail dot com]

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

--- Comment #12 from Petr <kobalicek.petr at gmail dot com> ---
Is there a way to diagnose this? To tell GCC to report transformations that
basically cause wrong results returned?

In my code base, I have unaligned memory loads/stores abstracted, so I can
implement whatever compiler specific construct I need to make such accesses
optimized or friendly to the compiler - and this method that I have shown here,
I didn't invent it. I would even say that I only see this error in unit tests,
which can be basically constant evaluated like shown in my previous examples.

But it was kinda surprising that GCC 11+ is the only compiler that started
failing my tests, and that no analyzer basically complains about this (nor
clang static analysis, for example).

[Bug c++/103699] Reading or writing a constant unaligned value is wrongly optimized causing an incorrect result (GCC-11 and up)

[jakub at gcc dot gnu.org]

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

--- Comment #13 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
There is -Wstrict-aliasing warning, with 3 different levels from most to least
aggressive, see gcc documentation.  But that warns mostly on the casts rather
than when performing the invalid access.  There are plans for a strict aliasing
sanitizer, but it is a lot of work and hard to do right.

[Bug c++/103699] Reading or writing a constant unaligned value is wrongly optimized causing an incorrect result (GCC-11 and up)

[jakub at gcc dot gnu.org]

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

--- Comment #14 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
And, if you want those loads to be valid, gcc has may_alias attribute.

[Bug c++/103699] Reading or writing a constant unaligned value is wrongly optimized causing an incorrect result (GCC-11 and up)

[kobalicek.petr at gmail dot com]

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

--- Comment #15 from Petr <kobalicek.petr at gmail dot com> ---
Unfortunately GCC doesn't report any issues even with `-Wstrict-aliasing=1`.

BTW now I know I must use the may_alias attribute to my satisfaction, and this
is what I'm gonna do, however, from user perspective I'm not really happy as
GCC does something very silently. Personally, I would be happier if my code
doesn't compile at all than having it compiled with bugs.

[Bug c++/103699] Reading or writing a constant unaligned value is wrongly optimized causing an incorrect result (GCC-11 and up)

[pinskia at gcc dot gnu.org]

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

--- Comment #16 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
You might want to read
https://blog.regehr.org/archives/1307
https://developers.redhat.com/blog/2020/06/03/the-joys-and-perils-of-aliasing-in-c-and-c-part-2
https://developers.redhat.com/blog/2020/06/02/the-joys-and-perils-of-c-and-c-aliasing-part-1

And gcc had some warnings in the past but so many false negatives that gcc had
cut back on them. C/c++ aliasing rules are simple but using pointers and
casting to a different type are a sure way to push towards having undefined
behavior in your code.

[Bug c++/103699] Reading or writing a constant unaligned value is wrongly optimized causing an incorrect result (GCC-11 and up)

[kobalicek.petr at gmail dot com]

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

--- Comment #17 from Petr <kobalicek.petr at gmail dot com> ---
Guys thanks a lot for your feedback.

Is the may_alias annotation guaranteed to behave as expected in the future
versions of GCC too, or it's just too much UB that it's better to do unaligned
reads with memcpy?

What I like on my existing solution is that I can specify alignment, so for
example I can say that this 64-bit load is 4-byte aligned, and that hints a
compiler, I'm not sure how to hint that with memcpy. So far annotating the code
with may_alias works for me, it passes tests, but I'm kinda unsure whether this
is a good way to express unaligned memory access considering I have faced this
issue.

[Bug c++/103699] Reading or writing a constant unaligned value is wrongly optimized causing an incorrect result (GCC-11 and up)

[jakub at gcc dot gnu.org]

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

--- Comment #18 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Using memcpy is the portable way, and most compilers will optimize it
reasonably well.
For alignment assertions, you can use e.g. __builtin_assume_aligned builtin to
tell the compiler about the alignment guarantees.