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

MaxXSoft · 2024-02-28T09:55:37Z

With compressed oops enabled, Shenandoah GC crashes in the concurrent marking phase due to some incorrect atomic memory operations. This resulted in the failure of multiple related tests, including gc/shenandoah/TestSmallHeap.java, gc/metaspace/TestMetaspacePerfCounters.java#Shenandoah-64, and so on, tested on XuanTie C910 and QEMU.

This failure is related to a GCC bug we recently discovered: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114130.

In detail, there's a pattern in method ShenandoahHeap::conc_update_with_forwarded:

if (in_collection_set(obj)) {
  // Dereferences `obj` without explicit null check.
  oop fwd = ShenandoahBarrierSet::resolve_forwarded_not_null(obj);
  // Then calls atomic built-in.
  atomic_update_oop(fwd, p, obj);
}

atomic_update_oop then compresses obj into a 32-bit word and calls the built-in __atomic_compare_exchange. The latter produces incorrect assembly that comparing this unsigned 32-bit word with the sign-extended result of lr.w instructions.

This bug can be bypassed by adding an explicit null check (like if (obj && in_collection_set(obj))), or adding compiler flag -fno-delete-null-pointer-checks.

In previous commits, JDK-8316186 removed RISC-V's PlatformCmpxchg<4> but left the flag -fno-delete-null-pointer-checks enabled. Then JDK-8316893 removed the flag and made the bug visible. This patch adds PlatformCmpxchg<4> back.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8326936: RISC-V: Shenandoah GC crashes due to incorrect atomic memory operations (Bug - P4)

Reviewers

Fei Yang (@RealFYang - Reviewer)
Robbin Ehn (@robehn - Reviewer) ⚠️ Review applies to e1a49d85

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/18039/head:pull/18039
$ git checkout pull/18039

Update a local copy of the PR:
$ git checkout pull/18039
$ git pull https://git.openjdk.org/jdk.git pull/18039/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 18039

View PR using the GUI difftool:
$ git pr show -t 18039

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/18039.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2024-02-28T09:56:50Z

👋 Welcome back MaxXSoft! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2024-02-28T09:57:47Z

@MaxXSoft The following label will be automatically applied to this pull request:

hotspot-runtime

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2024-02-28T10:01:18Z

Webrevs

RealFYang · 2024-02-28T11:25:19Z

Hi, I don't witness such an issue when running those regression tests with jdk binaries built with native GCC-12 on Licheepi-4a or Unmatched boards. Do you know why? Does that mean this will only trigger for jdk binaries built with GCC-13? Guess @robehn who owns https://bugs.openjdk.org/browse/JDK-8316186 might also be interested in this.

MaxXSoft · 2024-02-28T12:34:46Z

Hello, @RealFYang !

I can reproduce this crash using the following environment and configuration:

Linux fedora-riscv 6.1.22
gcc (GCC) 13.1.1 20230426 (Red Hat 13.1.1-1)
Release build. This is important since fastdebug enables CHECK_UNHANDLED_OOPS macro, causing an explicit null check before every oop dereference.

This bug produces a bad libjvm.so without any sext.w instructions in function _ZN14ShenandoahHeap26conc_update_with_forwardedI9narrowOopEEvPT_:

0000000000ab3200 <_ZN14ShenandoahHeap26conc_update_with_forwardedI9narrowOopEEvPT_>:
  ...
  ab3250:	02a88463          	beq	a7,a0,ab3278 <_ZN14ShenandoahHeap26conc_update_with_forwardedI9narrowOopEEvPT_+0x78>
  ab3254:	0ff0000f          	fence
  ab3258:	00c7d7b3          	srl	a5,a5,a2
  ab325c:	00c75733          	srl	a4,a4,a2
  ab3260:	1005a6af          	lr.w	a3,(a1)
  ab3264:	00f69563          	bne	a3,a5,ab326e <_ZN14ShenandoahHeap26conc_update_with_forwardedI9narrowOopEEvPT_+0x6e>
  ab3268:	18e5a62f          	sc.w	a2,a4,(a1)
  ab326c:	fa75                	bnez	a2,ab3260 <_ZN14ShenandoahHeap26conc_update_with_forwardedI9narrowOopEEvPT_+0x60>
  ab326e:	0ff0000f          	fence
  ab3272:	6422                	ld	s0,8(sp)
  ...

With this patch, the dump of libjvm.so is:

0000000000ab2ec0 <_ZN14ShenandoahHeap26conc_update_with_forwardedI9narrowOopEEvPT_>:
  ...
  ab2f10:	02a88863          	beq	a7,a0,ab2f40 <_ZN14ShenandoahHeap26conc_update_with_forwardedI9narrowOopEEvPT_+0x80>
  ab2f14:	00c7d7b3          	srl	a5,a5,a2
  ab2f18:	00c75733          	srl	a4,a4,a2
  ab2f1c:	2781                	sext.w	a5,a5
  ab2f1e:	2701                	sext.w	a4,a4
  ab2f20:	0ff0000f          	fence
  ab2f24:	0007861b          	sext.w	a2,a5
  ab2f28:	1005a6af          	lr.w	a3,(a1)
  ab2f2c:	00c69563          	bne	a3,a2,ab2f36 <_ZN14ShenandoahHeap26conc_update_with_forwardedI9narrowOopEEvPT_+0x76>
  ab2f30:	18e5a62f          	sc.w	a2,a4,(a1)
  ab2f34:	fa65                	bnez	a2,ab2f24 <_ZN14ShenandoahHeap26conc_update_with_forwardedI9narrowOopEEvPT_+0x64>
  ab2f36:	0ff0000f          	fence
  ab2f3a:	6422                	ld	s0,8(sp)
  ...

(The base commit of jdk is 4dd6c44)

The good and bad libjvm.so file: libjvm.so.tar.gz

I haven't tried other GCC versions yet.

robehn · 2024-02-28T18:06:49Z

Hi, on vacation, back next week.

In this case old_value is unsigned, but in the assembly we loaded a sign extended value.
Return value thus seems always singed extended, as we plainly return old_value.
As the compiler can't know it should zero extend an 'uint32_t' on return.
So this does not look correct to me?

Anyhow can't we just use a int32_t when comparing, and cast that back to the 'uint32_t' on return? (and still use _atomic...)

Sorry not really available, as I said back next week.
Thanks for finding!

MaxXSoft · 2024-02-29T03:49:15Z

Hello, @robehn !

Thanks for your reply and suggestion. I tried this before, and this workaround does not work. That's why I reported this bug to GCC, it's so weird.

In this case old_value is unsigned, but in the assembly we loaded a sign extended value.
Return value thus seems always singed extended, as we plainly return old_value.
As the compiler can't know it should zero extend an 'uint32_t' on return.
So this does not look correct to me?

According to what I observed, it's correct. RISC-V calling convention says:

When passed in registers or on the stack, integer scalars narrower than XLEN bits are widened according to the sign of their type up to 32 bits, then sign-extended to XLEN bits.

And then it says:

Values are returned in the same manner as a first named argument of the same type would be passed.

So the return value should be sign-extended, even if it's unsigned (according to my understand). Here's a simple example: https://godbolt.org/z/n6vfoYPPx. GCC generates slliw for the unsigned return value.

Anyhow can't we just use a int32_t when comparing, and cast that back to the 'uint32_t' on return? (and still use _atomic...)

Seems we can't. Check this example for more details: https://godbolt.org/z/cv5ejTfxh. I wrote both the inline assembly version and the built-in version CAS, and tested them on the same code snippet. The inline assembly version works well due to the ABI, but the built-in version is still buggy.

I also tried to build JDK with built-in atomic with a unsigned to signed conversion as the above example, and unsurprisingly it failed on Shenandoah-related tests.

In addition I removed the redundant sext.w in PlatformCmpxchg<4>, it works well. Dump of libjvm.so:

  ab2d04:	02a88663          	beq	a7,a0,ab2d30 <_ZN14ShenandoahHeap26conc_update_with_forwardedI9narrowOopEEvPT_+0x7c>
  ab2d08:	00c7d7b3          	srl	a5,a5,a2
  ab2d0c:	00c75733          	srl	a4,a4,a2
  ab2d10:	2781                	sext.w	a5,a5
  ab2d12:	2701                	sext.w	a4,a4
  ab2d14:	0ff0000f          	fence
  ab2d18:	1005a6af          	lr.w	a3,(a1)
  ab2d1c:	00f69563          	bne	a3,a5,ab2d26 <_ZN14ShenandoahHeap26conc_update_with_forwardedI9narrowOopEEvPT_+0x72>
  ab2d20:	18e5a62f          	sc.w	a2,a4,(a1)
  ab2d24:	fa75                	bnez	a2,ab2d18 <_ZN14ShenandoahHeap26conc_update_with_forwardedI9narrowOopEEvPT_+0x64>
  ab2d26:	0ff0000f          	fence
  ab2d2a:	6422                	ld	s0,8(sp)

zifeihan · 2024-02-29T06:40:19Z

Hi, When I use this patch, it doesn't compile, and reports the following error msg:

/home/zifeihan/jdk/src/hotspot/os_cpu/linux_riscv/atomic_linux_riscv.hpp:190:27: note: the comparison reduces to ‘(1 >= 8)’
  190 |   STATIC_ASSERT(byte_size >= 8);
/home/zifeihan/jdk/src/hotspot/share/utilities/debug.hpp:289:44: note: in definition of macro ‘STATIC_ASSERT’
   ... (rest of output omitted)
* For target hotspot_variant-server_libjvm_objs_arraycopynode.o:
In file included from /home/zifeihan/jdk/src/hotspot/share/memory/allocation.hpp:29,
                 from /home/zifeihan/jdk/src/hotspot/share/classfile/classLoaderData.hpp:28,
                 from /home/zifeihan/jdk/src/hotspot/share/precompiled/precompiled.hpp:34:
/home/zifeihan/jdk/src/hotspot/os_cpu/linux_riscv/atomic_linux_riscv.hpp: In instantiation of ‘T Atomic::PlatformCmpxchg<byte_size>::operator()(volatile T*, T, T, atomic_memory_order) const [with T = unsigned char; long unsigned int byte_size = 1]’:
/home/zifeihan/jdk/src/hotspot/share/runtime/atomic.hpp:1026:40:   required from ‘T Atomic::CmpxchgImpl<T, T, T, typename std::enable_if<std::is_integral<_Tp>::value, void>::type>::operator()(volatile T*, T, T, atomic_memory_order) const [with T = unsigned char; typename std::enable_if<std::is_integral<_Tp>::value, void>::type = void]’
/home/zifeihan/jdk/src/hotspot/share/runtime/atomic.hpp:1002:32:   required from ‘static D Atomic::cmpxchg(volatile D*, U, T, atomic_memory_order) [with D = unsigned char; U = unsigned char; T = unsigned char]’
/home/zifeihan/jdk/src/hotspot/share/oops/methodData.hpp:216:46:   required from here
/home/zifeihan/jdk/src/hotspot/os_cpu/linux_riscv/atomic_linux_riscv.hpp:190:27: error: static assertion failed: byte_size >= 8
  190 |   STATIC_ASSERT(byte_size >= 8);
/home/zifeihan/jdk/src/hotspot/share/utilities/debug.hpp:289:44: note: in definition of macro ‘STATIC_ASSERT’
  289 | #define STATIC_ASSERT(Cond) static_assert((Cond), #Cond)
      |                                            ^~~~
/home/zifeihan/jdk/src/hotspot/os_cpu/linux_riscv/atomic_linux_riscv.hpp:190:27: note: the comparison reduces to ‘(1 >= 8)’
  190 |   STATIC_ASSERT(byte_size >= 8);
/home/zifeihan/jdk/src/hotspot/share/utilities/debug.hpp:289:44: note: in definition of macro ‘STATIC_ASSERT’
   ... (rest of output omitted)
* For target hotspot_variant-server_libjvm_objs_assembler.o:
In file included from /home/zifeihan/jdk/src/hotspot/share/memory/allocation.hpp:29,
                 from /home/zifeihan/jdk/src/hotspot/share/classfile/classLoaderData.hpp:28,
                 from /home/zifeihan/jdk/src/hotspot/share/precompiled/precompiled.hpp:34:
/home/zifeihan/jdk/src/hotspot/os_cpu/linux_riscv/atomic_linux_riscv.hpp: In instantiation of ‘T Atomic::PlatformCmpxchg<byte_size>::operator()(volatile T*, T, T, atomic_memory_order) const [with T = unsigned char; long unsigned int byte_size = 1]’:
/home/zifeihan/jdk/src/hotspot/share/runtime/atomic.hpp:1026:40:   required from ‘T Atomic::CmpxchgImpl<T, T, T, typename std::enable_if<std::is_integral<_Tp>::value, void>::type>::operator()(volatile T*, T, T, atomic_memory_order) const [with T = unsigned char; typename std::enable_if<std::is_integral<_Tp>::value, void>::type = void]’
/home/zifeihan/jdk/src/hotspot/share/runtime/atomic.hpp:1002:32:   required from ‘static D Atomic::cmpxchg(volatile D*, U, T, atomic_memory_order) [with D = unsigned char; U = unsigned char; T = unsigned char]’
/home/zifeihan/jdk/src/hotspot/share/runtime/atomic.hpp:750:38:   required from ‘T Atomic::PrefetchBitopsUsingCmpxchg::bitop(volatile T*, atomic_memory_order, Op) const [with T = unsigned char; Op = Atomic::PrefetchBitopsUsingCmpxchg::fetch_then_or<unsigned char>(volatile unsigned char*, unsigned char, atomic_memory_order) const::<lambda(unsigned char)>]’
/home/zifeihan/jdk/src/hotspot/share/runtime/atomic.hpp:763:17:   required from ‘T Atomic::PrefetchBitopsUsingCmpxchg::fetch_then_or(volatile T*, T, atomic_memory_order) const [with T = unsigned char]’
/home/zifeihan/jdk/src/hotspot/share/runtime/atomic.hpp:194:53:   required from ‘static T Atomic::fetch_then_or(volatile T*, T, atomic_memory_order) [with T = unsigned char]’
/home/zifeihan/jdk/src/hotspot/share/oops/instanceKlassFlags.hpp:126:58:   required from here
/home/zifeihan/jdk/src/hotspot/os_cpu/linux_riscv/atomic_linux_riscv.hpp:190:27: error: static assertion failed: byte_size >= 8
  190 |   STATIC_ASSERT(byte_size >= 8);
/home/zifeihan/jdk/src/hotspot/share/utilities/debug.hpp:289:44: note: in definition of macro ‘STATIC_ASSERT’
  289 | #define STATIC_ASSERT(Cond) static_assert((Cond), #Cond)
      |                                            ^~~~
   ... (rest of output omitted)

* All command lines available in /home/zifeihan/jdk/build/linux-riscv64-server-release/make-support/failure-logs.
=== End of repeated output ===

No indication of failed target found.
HELP: Try searching the build log for '] Error'.
HELP: Run 'make doctor' to diagnose build problems.

make[1]: *** [/home/zifeihan/jdk/make/Init.gmk:323: main] Error 2
make: *** [/home/zifeihan/jdk/make/Init.gmk:189: default] Error 2

gcc version:

[zifeihan@fedora-riscv jdk]$ gcc -v
Using built-in specs.
COLLECT_GCC=gcc
COLLECT_LTO_WRAPPER=/usr/libexec/gcc/riscv64-redhat-linux/13/lto-wrapper
Target: riscv64-redhat-linux
Configured with: ../configure --enable-bootstrap --enable-languages=c,c++,fortran,objc,obj-c++,ada,go,d,m2,lto --prefix=/usr --mandir=/usr/share/man --infodir=/usr/share/info --with-bugurl=http://bugzilla.redhat.com/bugzilla --enable-shared --enable-threads=posix --enable-checking=release --enable-multilib --with-system-zlib --enable-__cxa_atexit --disable-libunwind-exceptions --enable-gnu-unique-object --enable-linker-build-id --with-gcc-major-version-only --enable-libstdcxx-backtrace --with-libstdcxx-zoneinfo=/usr/share/zoneinfo --with-linker-hash-style=gnu --enable-plugin --enable-initfini-array --with-isl=/builddir/build/BUILD/gcc-13.2.1-20230728/obj-riscv64-redhat-linux/isl-install --enable-gnu-indirect-function --with-arch=rv64gc --with-abi=lp64d --with-multilib-list=lp64d --build=riscv64-redhat-linux --with-build-config=bootstrap-lto --enable-link-serialization=1
Thread model: posix
Supported LTO compression algorithms: zlib zstd
gcc version 13.2.1 20230728 (Red Hat 13.2.1-1) (GCC)
```

MaxXSoft · 2024-02-29T07:10:17Z

@zifeihan

Sorry for that, I fixed this assertion in the latest commit. Now it should compile. Please let me know if not.

zifeihan · 2024-02-29T13:22:42Z

@zifeihan

Sorry for that, I fixed this assertion in the latest commit. Now it should compile. Please let me know if not.

@MaxXSoft Thanks for fix that. I tried the latest version and everything works well with the build. And the tier1 test is working fine.

MaxXSoft · 2024-02-29T14:19:29Z

@MaxXSoft Thanks for fix that. I tried the latest version and everything works well with the build. And the tier1 test is working fine.

@zifeihan Happy to hear that. Thanks for your testing!

RealFYang

Hi, I have several minor comments. BTW: I am curious about why GCC old versions like GCC-11/12 would work. Can you take a look? Thanks.

RealFYang · 2024-03-01T01:56:19Z

src/hotspot/os_cpu/linux_riscv/atomic_linux_riscv.hpp

+#elif defined(__GNUC__) && __riscv_xlen <= 32
+#define CORRECT_COMPILER_ATOMIC_SUPPORT
+#endif
+


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.

RealFYang · 2024-03-01T01:58:27Z

src/hotspot/os_cpu/linux_riscv/atomic_linux_riscv.hpp

+#ifndef CORRECT_COMPILER_ATOMIC_SUPPORT
+template<>
+template<typename T>
+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.

RealFYang · 2024-03-01T02:01:32Z

src/hotspot/os_cpu/linux_riscv/atomic_linux_riscv.hpp

+  STATIC_ASSERT(4 == sizeof(T));
+
+  T old_value;
+  long flag;


To be consistent with the preceding Atomic::PlatformCmpxchg<1>, I would suggest change long flag; into uint64_t rc_temp;

MaxXSoft · 2024-03-01T09:43:59Z

@RealFYang Thanks for your comments!

BTW: I am curious about why GCC old versions like GCC-11/12 would work. Can you take a look? Thanks.

I'm trying to figure this out. Are you building in release mode? fastdebug enables CHECK_UNHANDLED_OOPS macro, which adds explicit null checks before every oop dereferences, so the bug will not appear.

RealFYang · 2024-03-01T13:36:01Z

@RealFYang Thanks for your comments!

BTW: I am curious about why GCC old versions like GCC-11/12 would work. Can you take a look? Thanks.

I'm trying to figure this out. Are you building in release mode? fastdebug enables CHECK_UNHANDLED_OOPS macro, which adds explicit null checks before every oop dereferences, so the bug will not appear.

Yeah, I am talking about linux-riscv64 release build built with GCC-11/12.

robehn · 2024-03-04T07:43:03Z

Hey,

So the return value should be sign-extended, even if it's unsigned (according to my understand). Here's a simple example: https://godbolt.org/z/n6vfoYPPx. GCC generates slliw for the unsigned return value.

This method should, at least often, be inlined, so old_value will not be passed as a return value.
And someone may add additional code in the same method.
Now it seems like the compiler always sign extends every word from inline asm, but I don't think that is a guarantee?
I was told always pass registers to inline asm and not types.
So I suggest having the correct sign extended word type, e.g. int32_t, as old_value.

Anyhow can't we just use a int32_t when comparing, and cast that back to the 'uint32_t' on return? (and still use _atomic...)

Seems we can't. Check this example for more details: https://godbolt.org/z/cv5ejTfxh. I wrote both the inline assembly version and the built-in version CAS, and tested them on the same code snippet. The inline assembly version works well due to the ABI, but the built-in version is still buggy.

Yes, I don't find a good way to force it. It possible using volatile but it involves storing and load from stack.

Thanks, Robbin

MaxXSoft · 2024-03-05T02:53:54Z

Yeah, I am talking about linux-riscv64 release build built with GCC-11/12.

@RealFYang

Hello! I tried to build linux-riscv64 release with GCC 12, the test result shows that it works. I also checked the dump of libjvm.so, sext.w does appear in it:

  a8d268:	c609                	beqz	a2,a8d272 <_ZN14ShenandoahHeap26conc_update_with_forwardedI9narrowOopEEvPT_+0x60>
  a8d26a:	00a7d7b3          	srl	a5,a5,a0
  a8d26e:	0007869b          	sext.w	a3,a5
  a8d272:	0ff0000f          	fence
  a8d276:	00a75733          	srl	a4,a4,a0
  a8d27a:	1005a7af          	lr.w	a5,(a1)
  a8d27e:	00d79563          	bne	a5,a3,a8d288 <_ZN14ShenandoahHeap26conc_update_with_forwardedI9narrowOopEEvPT_+0x76>
  a8d282:	18e5a62f          	sc.w	a2,a4,(a1)
  a8d286:	fa75                	bnez	a2,a8d27a <_ZN14ShenandoahHeap26conc_update_with_forwardedI9narrowOopEEvPT_+0x68>
  a8d288:	0ff0000f          	fence
  a8d28c:	6422                	ld	s0,8(sp)

But it seems that other optimizations interfere with the generated code. If someone changes the source code, the sign extension will disappear due to the GCC bug. Check out this short example simplified from the code of ShenandoahHeap::conc_update_with_forwarded<narrowOop>: https://godbolt.org/z/TY84KKx5f.

So we still need this patch on GCC 11/12.

MaxXSoft · 2024-03-05T03:44:01Z

Hi @robehn , Thanks for your suggestions!

Now it seems like the compiler always sign extends every word from inline asm, but I don't think that is a guarantee?
I was told always pass registers to inline asm and not types.
So I suggest having the correct sign extended word type, e.g. int32_t, as old_value.

You are right. I tried the inline assembly version CAS on Clang (although we don't enable this patch for Clang), and the parameter don't seem to be sign-extended unless we force it: https://godbolt.org/z/fM8n9GMcz.

I added an explicit type cast for compare_value, and changed the type of old_value to int32_t in the latest commit.

robehn

Thank you!

I played a bit inline asm and it's actually handled pretty horrible with regards to zero/sign extensions. It seems like in some cases it is not possible to get the compiler to do the correct thing.

openjdk · 2024-03-05T06:52:36Z

⚠️ @MaxXSoft the full name on your profile does not match the author name in this pull requests' HEAD commit. If this pull request gets integrated then the author name from this pull requests' HEAD commit will be used for the resulting commit. If you wish to push a new commit with a different author name, then please run the following commands in a local repository of your personal fork:

$ git checkout fix-riscv-atomic
$ git commit --author='Preferred Full Name <you@example.com>' --allow-empty -m 'Update full name'
$ git push

openjdk · 2024-03-05T06:52:37Z

@MaxXSoft This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

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

Reviewed-by: fyang, rehn

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 70 new commits pushed to the master branch:

560cf59: 8327287: Remove unused FLSVerifyDictionary debug option
fec51d4: 8327130: Serial: Remove Generation::record_spaces_top
e9adceb: 8327208: Remove unused method java.util.jar.Manifest.make72Safe
d6f2a17: 8325881: Require minimum gcc version 10
0b95909: 8327224: G1: comment in G1BarrierSetC2::post_barrier() refers to nonexistent new_deferred_store_barrier()
c589555: 8325095: C2: bailout message broken: ResourceArea allocated string used after free
b7540df: 8327007: javax/swing/JSpinner/8008657/bug8008657.java fails
e1b661f: 8319900: Recursive lightweight locking: riscv64 implementation
045eea1: 8327057: Parallel: Refactor ParMarkBitMap::iterate
f615ac4: 8309622: Re-examine the cache mechanism in BaseLocale
... and 60 more: https://git.openjdk.org/jdk/compare/4dd6c44cbdb0b5957414fa87b6c559fa4d6f2fa8...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@RealFYang, @robehn) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

RealFYang · 2024-03-05T06:59:53Z

src/hotspot/os_cpu/linux_riscv/atomic_linux_riscv.hpp

+  if (order != memory_order_relaxed) {
+    FULL_MEM_BARRIER;
+  }
+  return old_value;


Should we do an explicit type conversion here? Like: return (T)old_value;

MaxXSoft · 2024-03-05T09:16:24Z

@RealFYang @robehn Thanks for your review!

/integrate

openjdk · 2024-03-05T09:16:38Z

@MaxXSoft
Your change (at version dff9923) is now ready to be sponsored by a Committer.

RealFYang

LGTM. Thanks. The condition for defining macro CORRECT_COMPILER_ATOMIC_SUPPORT needs to be improved when the gcc bugfix is backported to gcc branches 11, 12 and 13 [1].

[1] https://gcc.gnu.org/pipermail/gcc-patches/2024-February/646822.html

RealFYang · 2024-03-05T11:41:41Z

/sponsor

openjdk · 2024-03-05T11:42:38Z

Going to push as commit a089ed2.
Since your change was applied there have been 70 commits pushed to the master branch:

560cf59: 8327287: Remove unused FLSVerifyDictionary debug option
fec51d4: 8327130: Serial: Remove Generation::record_spaces_top
e9adceb: 8327208: Remove unused method java.util.jar.Manifest.make72Safe
d6f2a17: 8325881: Require minimum gcc version 10
0b95909: 8327224: G1: comment in G1BarrierSetC2::post_barrier() refers to nonexistent new_deferred_store_barrier()
c589555: 8325095: C2: bailout message broken: ResourceArea allocated string used after free
b7540df: 8327007: javax/swing/JSpinner/8008657/bug8008657.java fails
e1b661f: 8319900: Recursive lightweight locking: riscv64 implementation
045eea1: 8327057: Parallel: Refactor ParMarkBitMap::iterate
f615ac4: 8309622: Re-examine the cache mechanism in BaseLocale
... and 60 more: https://git.openjdk.org/jdk/compare/4dd6c44cbdb0b5957414fa87b6c559fa4d6f2fa8...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-03-05T11:42:47Z

@RealFYang @MaxXSoft Pushed as commit a089ed2.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

MaxXSoft · 2024-03-05T11:57:28Z

/backport jdk22u

openjdk · 2024-03-05T11:58:05Z

@MaxXSoft To use the /backport command, you need to be in the OpenJDK census and your GitHub account needs to be linked with your OpenJDK username (how to associate your GitHub account with your OpenJDK username).

zifeihan · 2024-03-07T08:38:51Z

/backport jdk22u

openjdk · 2024-03-07T08:40:10Z

@zifeihan the backport was successfully created on the branch backport-zifeihan-a089ed2b in my personal fork of openjdk/jdk22u. To create a pull request with this backport targeting openjdk/jdk22u:master, just click the following link:

➡️ Create pull request

The title of the pull request is automatically filled in correctly and below you find a suggestion for the pull request body:

Hi all,

This pull request contains a backport of commit a089ed2b from the openjdk/jdk repository.

The commit being backported was authored by MaxXSoft on 5 Mar 2024 and was reviewed by Fei Yang and Robbin Ehn.

Thanks!

If you need to update the source branch of the pull then run the following commands in a local clone of your personal fork of openjdk/jdk22u:

$ git fetch https://github.com/openjdk-bots/jdk22u.git backport-zifeihan-a089ed2b:backport-zifeihan-a089ed2b
$ git checkout backport-zifeihan-a089ed2b
# make changes
$ git add paths/to/changed/files
$ git commit --message 'Describe additional changes made'
$ git push https://github.com/openjdk-bots/jdk22u.git backport-zifeihan-a089ed2b

Fix a bug about GCC atomic built-in function.

7ff141a

openjdk bot added the rfr Pull request is ready for review label Feb 28, 2024

openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label Feb 28, 2024

Remove unnecessary sign-extension.

4b894b0

Fix static assert.

48e3895

RealFYang suggested changes Mar 1, 2024

View reviewed changes

Add comments, change flag to rc_temp.

94194f7

Fix value types, macro conditions and comments.

e1a49d8

robehn approved these changes Mar 5, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Mar 5, 2024

RealFYang reviewed Mar 5, 2024

View reviewed changes

Add explicit type cast for old_value.

dff9923

openjdk bot added the sponsor Pull request is ready to be sponsored label Mar 5, 2024

RealFYang approved these changes Mar 5, 2024

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Mar 5, 2024

openjdk bot closed this Mar 5, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Mar 5, 2024

MaxXSoft deleted the fix-riscv-atomic branch March 5, 2024 11:56

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

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

Conversation

MaxXSoft commented Feb 28, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Feb 28, 2024

openjdk bot commented Feb 28, 2024

mlbridge bot commented Feb 28, 2024 • edited Loading

Webrevs

RealFYang commented Feb 28, 2024 • edited Loading

MaxXSoft commented Feb 28, 2024 • edited Loading

robehn commented Feb 28, 2024

MaxXSoft commented Feb 29, 2024 • edited Loading

zifeihan commented Feb 29, 2024

MaxXSoft commented Feb 29, 2024

zifeihan commented Feb 29, 2024

MaxXSoft commented Feb 29, 2024

RealFYang left a comment • edited Loading

Choose a reason for hiding this comment

RealFYang Mar 1, 2024 • edited Loading

Choose a reason for hiding this comment

MaxXSoft Mar 1, 2024

Choose a reason for hiding this comment

RealFYang Mar 1, 2024 • edited Loading

Choose a reason for hiding this comment

MaxXSoft Mar 1, 2024

Choose a reason for hiding this comment

RealFYang Mar 1, 2024

Choose a reason for hiding this comment

MaxXSoft Mar 1, 2024

Choose a reason for hiding this comment

MaxXSoft commented Mar 1, 2024

RealFYang commented Mar 1, 2024

robehn commented Mar 4, 2024

MaxXSoft commented Mar 5, 2024

MaxXSoft commented Mar 5, 2024 • edited Loading

robehn left a comment

Choose a reason for hiding this comment

openjdk bot commented Mar 5, 2024

openjdk bot commented Mar 5, 2024 • edited Loading

RealFYang Mar 5, 2024

Choose a reason for hiding this comment

MaxXSoft Mar 5, 2024

Choose a reason for hiding this comment

MaxXSoft commented Mar 5, 2024

openjdk bot commented Mar 5, 2024

RealFYang left a comment

Choose a reason for hiding this comment

RealFYang commented Mar 5, 2024

openjdk bot commented Mar 5, 2024

openjdk bot commented Mar 5, 2024

MaxXSoft commented Mar 5, 2024

openjdk bot commented Mar 5, 2024

zifeihan commented Mar 7, 2024

openjdk bot commented Mar 7, 2024

MaxXSoft commented Feb 28, 2024 •

edited by openjdk bot

Loading

mlbridge bot commented Feb 28, 2024 •

edited

Loading

RealFYang commented Feb 28, 2024 •

edited

Loading

MaxXSoft commented Feb 28, 2024 •

edited

Loading

MaxXSoft commented Feb 29, 2024 •

edited

Loading

RealFYang left a comment •

edited

Loading

RealFYang Mar 1, 2024 •

edited

Loading

RealFYang Mar 1, 2024 •

edited

Loading

MaxXSoft commented Mar 5, 2024 •

edited

Loading

openjdk bot commented Mar 5, 2024 •

edited

Loading