RFR: 8326936: RISC-V: Shenandoah GC crashes due to incorrect atomic memory operations [v3]

[MaxXing]

On Fri, 1 Mar 2024 01:56:19 GMT, Fei Yang <fyang at openjdk.org> wrote:

>> MaxXing has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Fix static assert.
>
> src/hotspot/os_cpu/linux_riscv/atomic_linux_riscv.hpp line 47:
> 
>> 45: #define CORRECT_COMPILER_ATOMIC_SUPPORT
>> 46: #endif
>> 47: 
> 
> It doesn't make sense to me to have this `CORRECT_COMPILER_ATOMIC_SUPPORT` macro.

Reason for adding this macro:

* `__atomic_compare_exchange` is correct in Clang, we can directly use it with `uint32_t` for potential optimization opportunities.
* If this bug has been fixed in a later version of GCC, we can enable this macro for it, and leave the macro disabled for other GCC versions.

> src/hotspot/os_cpu/linux_riscv/atomic_linux_riscv.hpp line 126:
> 
>> 124: template<>
>> 125: template<typename T>
>> 126: inline T Atomic::PlatformCmpxchg<4>::operator()(T volatile* dest __attribute__((unused)),
> 
> Can you add a code comment about why we should have this defined? Better to add a link to the related GCC bug.

Updated.

> src/hotspot/os_cpu/linux_riscv/atomic_linux_riscv.hpp line 133:
> 
>> 131: 
>> 132:   T old_value;
>> 133:   long flag;
> 
> To be consistent with the preceding `Atomic::PlatformCmpxchg<1>`, I would suggest change `long flag;` into `uint64_t rc_temp;`

Updated.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/18039#discussion_r1508730139
PR Review Comment: https://git.openjdk.org/jdk/pull/18039#discussion_r1508730361
PR Review Comment: https://git.openjdk.org/jdk/pull/18039#discussion_r1508730823