Skip to content

8326936: RISC-V: Shenandoah GC crashes due to incorrect atomic memory operations #18039

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 6 commits into from
Closed

8326936: RISC-V: Shenandoah GC crashes due to incorrect atomic memory operations #18039

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7ff141a
Fix a bug about GCC atomic built-in function.
MaxXSoft Feb 28, 2024
4b894b0
Remove unnecessary sign-extension.
MaxXSoft Feb 29, 2024
48e3895
Fix static assert.
MaxXSoft Feb 29, 2024
94194f7
Add comments, change `flag` to `rc_temp`.
MaxXSoft Mar 1, 2024
e1a49d8
Fix value types, macro conditions and comments.
MaxXSoft Mar 5, 2024
dff9923
Add explicit type cast for `old_value`.
MaxXSoft Mar 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 49 additions & 0 deletions src/hotspot/os_cpu/linux_riscv/atomic_linux_riscv.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,12 @@
#define FULL_COMPILER_ATOMIC_SUPPORT
#endif

#if defined(__clang_major__)
#define CORRECT_COMPILER_ATOMIC_SUPPORT
#elif defined(__GNUC__) && (__riscv_xlen <= 32 || __GNUC__ > 13)
#define CORRECT_COMPILER_ATOMIC_SUPPORT
#endif

Copy link
Member

@RealFYang RealFYang Mar 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't make sense to me to have this CORRECT_COMPILER_ATOMIC_SUPPORT macro.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

template<size_t byte_size>
struct Atomic::PlatformAdd {
template<typename D, typename I>
Expand Down Expand Up @@ -114,6 +120,44 @@ inline T Atomic::PlatformCmpxchg<1>::operator()(T volatile* dest __attribute__((
}
#endif

#ifndef CORRECT_COMPILER_ATOMIC_SUPPORT
// The implementation of `__atomic_compare_exchange` lacks sign extensions
// in GCC 13 and lower when using with 32-bit unsigned integers on RV64,
// so we should implement it manually.
// GCC bug: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114130.
// See also JDK-8326936.
template<>
template<typename T>
inline T Atomic::PlatformCmpxchg<4>::operator()(T volatile* dest __attribute__((unused)),
Copy link
Member

@RealFYang RealFYang Mar 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a code comment about why we should have this defined? Better to add a link to the related GCC bug.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

T compare_value,
T exchange_value,
atomic_memory_order order) const {
STATIC_ASSERT(4 == sizeof(T));

int32_t old_value;
uint64_t rc_temp;

if (order != memory_order_relaxed) {
FULL_MEM_BARRIER;
}

__asm__ __volatile__ (
"1: lr.w %0, %2 \n\t"
" bne %0, %3, 2f \n\t"
" sc.w %1, %4, %2 \n\t"
" bnez %1, 1b \n\t"
"2: \n\t"
: /*%0*/"=&r" (old_value), /*%1*/"=&r" (rc_temp), /*%2*/"+A" (*dest)
: /*%3*/"r" ((int64_t)(int32_t)compare_value), /*%4*/"r" (exchange_value)
: "memory" );

if (order != memory_order_relaxed) {
FULL_MEM_BARRIER;
}
return (T)old_value;
}
#endif

template<size_t byte_size>
template<typename T>
inline T Atomic::PlatformXchg<byte_size>::operator()(T volatile* dest,
Expand Down Expand Up @@ -151,6 +195,10 @@ inline T Atomic::PlatformCmpxchg<byte_size>::operator()(T volatile* dest __attri
STATIC_ASSERT(byte_size >= 4);
#endif

#ifndef CORRECT_COMPILER_ATOMIC_SUPPORT
STATIC_ASSERT(byte_size != 4);
#endif

STATIC_ASSERT(byte_size == sizeof(T));
if (order != memory_order_relaxed) {
FULL_MEM_BARRIER;
Expand Down Expand Up @@ -187,5 +235,6 @@ struct Atomic::PlatformOrderedStore<byte_size, RELEASE_X_FENCE>
};

#undef FULL_COMPILER_ATOMIC_SUPPORT
#undef CORRECT_COMPILER_ATOMIC_SUPPORT

#endif // OS_CPU_LINUX_RISCV_ATOMIC_LINUX_RISCV_HPP