[Bug target/92462] [arm32] -ftree-pre makes a variable to be wrongly hoisted out

[wilco at gcc dot gnu.org]

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

Wilco <wilco at gcc dot gnu.org> changed:

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

--- Comment #1 from Wilco <wilco at gcc dot gnu.org> ---
(In reply to Aleksei Voitylov from comment #0)
> Created attachment 47212 [details]
> reduced testcase from the openjdk sources
> 
> While compiling openjdk sources I bumped into a bug: when -ftree-pre is
> enabled the optimizer hoists out reload of a variable which subsequently
> leads to the infinite loop situation.
> 
> Below is the relevant piece of code and "new_value" is the variable that
> gets hoisted out.
> 
> template<typename T>
> inline T Atomic::CmpxchgByteUsingInt::operator()(T exchange_value,
>         T volatile* dest,
>         T compare_value,
>         atomic_memory_order order) const {
>     printf ("Atomic::CmpxchgByteUsingInt::operator: 1: %d, 2: %d\n",
> exchange_value, compare_value);
>     uint8_t canon_exchange_value = exchange_value;
>     uint8_t canon_compare_value = compare_value;
>     volatile uint32_t* aligned_dest
>         = reinterpret_cast<volatile uint32_t*>(align_down(dest,
> sizeof(uint32_t)));
>     size_t offset = (uintptr_t)dest  - (uintptr_t)aligned_dest;
>     uint32_t cur = *aligned_dest;
>     uint8_t* cur_as_bytes = reinterpret_cast<uint8_t*>(&cur);
>     cur_as_bytes[offset] = canon_compare_value;
>     do {
>         uint32_t new_value = cur;
>         reinterpret_cast<uint8_t*>(&new_value)[offset] =
> canon_exchange_value;
>         printf ("Atomic::CmpxchgByteUsingInt::operator2: 1: %d, 2: %d\n",
> new_value, cur);
>         uint32_t res = cmpxchg(new_value, aligned_dest, cur, order);
>         if (res == cur) break;
>         cur = res;
>     } while (cur_as_bytes[offset] == canon_compare_value);
>     return PrimitiveConversions::cast<T>(cur_as_bytes[offset]);
> }
> 
> $ g++ -O1 -ftree-pre t.cpp
> $ ./a.out
> Atomic::CmpxchgByteUsingInt::operator: 1: 0, 2: 1
> Atomic::CmpxchgByteUsingInt::operator2: 1: 0, 2: 0
> 
> $ g++ -O1 t.cpp
> $ ./a.out
> Atomic::CmpxchgByteUsingInt::operator: 1: 0, 2: 1
> Atomic::CmpxchgByteUsingInt::operator2: 1: 0, 2: 256
> 
> Below is the assembler of the loop for the correct version:
> 
> .L7:
>         ldr     r4, [sp]
>         str     r4, [sp, #4]
>         strb    r7, [r5, #-4]
>         mov     r2, r4
>         ldr     r1, [sp, #4]
>         mov     r0, r6
>         bl      printf
>         cbz     r4, .L6
>         movs    r3, #0
>         str     r3, [sp]
>         ldrb    r3, [r8]        @ zero_extendqisi2
>         cmp     r3, #1
>         beq     .L7
> 
> and for the incorrect one:
> 
> .L7:
>         str     r4, [sp, #4]
>         strb    r8, [r6]
>         mov     r2, r4
>         ldr     r1, [sp, #4]
>         mov     r0, r5
>         bl      printf
>         cbz     r4, .L6
>         movs    r4, #0
>         str     r4, [sp]
>         ldrb    r3, [r7]        @ zero_extendqisi2
>         cmp     r3, #1
>         beq     .L7

There are serious aliasing bugs in the source - GCC is quite correct in
assuming that cur and cur_as_bytes[offset] never alias (obviously) and even
optimize away the cmpxchg (no idea why, that appears wrong).

Even if you fix the aliasing bugs, it won't emulate a byte-oriented cmpxchg
correctly, there are bugs in the logic too.

So I suggest to go back to the drawing board - you can't hack your own atomic
operations and just hope for the best. GCC supports a standard set of atomic
operations for a good reason!