RFR: 8358821: patch_verified_entry causes problems, use nmethod entry barriers instead [v4]

[Amit Kumar]

On Mon, 16 Jun 2025 15:04:23 GMT, Amit Kumar <amitkumar at openjdk.org> wrote:

>> Just FYI, s390 build is broken with this change: 
>> 
>> 
>> #
>> # A fatal error has been detected by the Java Runtime Environment:
>> #
>> #  Internal Error (/home/amit/jdk/src/hotspot/share/gc/shared/barrierSetNMethod.cpp:196), pid=1779086, tid=1779117
>> #  assert(!nm->is_osr_method() || may_enter) failed: OSR nmethods should always be entrant after migration
>> #
>> # JRE version: OpenJDK Runtime Environment (26.0) (fastdebug build 26-internal-adhoc.amit.jdk)
>> # Java VM: OpenJDK 64-Bit Server VM (fastdebug 26-internal-adhoc.amit.jdk, mixed mode, tiered, compressed oops, compressed class ptrs, g1 gc, linux-s390x)
>> # Problematic frame:
>> # V  [libjvm.so+0x40b196]  BarrierSetNMethod::nmethod_stub_entry_barrier(unsigned char**)+0x15e
>> #
>> # Core dump will be written. Default location: Core dumps may be processed with "/lib/systemd/systemd-coredump %P %u %g %s %t 9223372036854775808 %h %d" (or dumping to /home/amit/jdk/make/core.1779086)
>> #
>> # If you would like to submit a bug report, please visit:
>> #   https://bugreport.java.com/bugreport/crash.jsp
>> #
>> 
>> 
>> stack trace: 
>> 
>> Stack: [0x000003ff9e580000,0x000003ff9e680000],  sp=0x000003ff9e67b068,  free space=1004k
>> Native frames: (J=compiled Java code, j=interpreted, Vv=VM code, C=native code)
>> V  [libjvm.so+0x40b196]  BarrierSetNMethod::nmethod_stub_entry_barrier(unsigned char**)+0x15e  (barrierSetNMethod.cpp:196)
>> v  ~StubRoutines::method_entry_barrier 0x000003ff9050cd18
>> J 282% c2 sun.nio.fs.UnixPath.initOffsets()V java.base (189 bytes) @ 0x000003ff90c4f0c8 [0x000003ff90c4f080+0x0000000000000048]
>> j  sun.nio.fs.UnixPath.getFileName()Lsun/nio/fs/UnixPath;+1 java.base
>> j  sun.nio.fs.UnixFileSystemProvider.isHidden(Ljava/nio/file/Path;)Z+6 java.base
>> j  java.nio.file.Files.isHidden(Ljava/nio/file/Path;)Z+5 java.base
>> j  jdk.internal.module.ModulePath.isHidden(Ljava/nio/file/Path;)Z+1 java.base
>> j  jdk.internal.module.ModulePath.lambda$explodedPackages$0(Ljava/nio/file/Path;Ljava/nio/file/attribute/BasicFileAttributes;)Z+11 java.base
>> j  jdk.internal.module.ModulePath$$Lambda+0x00000000a105cbe0.test(Ljava/lang/Object;Ljava/lang/Object;)Z+12 java.base
>> j  java.nio.file.Files.lambda$find$0(Ljava/util/function/BiPredicate;Ljava/nio/file/FileTreeWalker$Event;)Z+9 java.base
>> j  java.nio.file.Files$$Lambda+0x00000000a10646c0.test(Ljava/lang/Object;)Z+8 java.base
>> ....
>
>> @offamitkumar: The problem is probably the initialization to -1: [`z_cfi(Z_R0_scratch, /* to be patched */ -1);`.](https://github.com/openjdk/jdk/blob/9d060574e5dbd13e634f00d749d0108ceff1fae8/src/hotspot/cpu/s390/gc/shared/barrierSetAssembler_s390.cpp#L183) Should be 0.
> 
> Thank you Martin for the suggestion. 
> 
> @dean-long would you please add this diff, fixing s390x build. I ran tier1 test with fastdebug, test are clean; 
> 
> 
> diff --git a/src/hotspot/cpu/s390/gc/shared/barrierSetAssembler_s390.cpp b/src/hotspot/cpu/s390/gc/shared/barrierSetAssembler_s390.cpp
> index e78906708af..2d663061aec 100644
> --- a/src/hotspot/cpu/s390/gc/shared/barrierSetAssembler_s390.cpp
> +++ b/src/hotspot/cpu/s390/gc/shared/barrierSetAssembler_s390.cpp
> @@ -180,7 +180,7 @@ void BarrierSetAssembler::nmethod_entry_barrier(MacroAssembler* masm) {
>      __ z_lg(Z_R0_scratch, in_bytes(bs_nm->thread_disarmed_guard_value_offset()), Z_thread); // 6 bytes
>  
>      // Compare to current patched value:
> -    __ z_cfi(Z_R0_scratch, /* to be patched */ -1); // 6 bytes (2 + 4 byte imm val)
> +    __ z_cfi(Z_R0_scratch, /* to be patched */ 0); // 6 bytes (2 + 4 byte imm val)
>  
>      // Conditional Jump
>      __ z_larl(Z_R14, (Assembler::instr_len((unsigned long)LARL_ZOPC) + Assembler::instr_len((unsigned long)BCR_ZOPC)) / 2); // 6 bytes
> diff --git a/src/hotspot/cpu/s390/stubGenerator_s390.cpp b/src/hotspot/cpu/s390/stubGenerator_s390.cpp
> index d3f6540a3ea..bb1d9ce6037 100644
> --- a/src/hotspot/cpu/s390/stubGenerator_s390.cpp
> +++ b/src/hotspot/cpu/s390/stubGenerator_s390.cpp
> @@ -3197,7 +3197,7 @@ class StubGenerator: public StubCodeGenerator {
>  
>      // VM-Call: BarrierSetNMethod::nmethod_stub_entry_barrier(address* return_address_ptr)
>      __ call_VM_leaf(CAST_FROM_FN_PTR(address, BarrierSetNMethod::nmethod_stub_entry_barrier));
> -    __ z_ltr(Z_R0_scratch, Z_RET);
> +    __ z_ltr(Z_RET, Z_RET);
>  
>      // VM-Call Epilogue
>      __ restore_volatile_regs(Z_SP, frame::z_abi_160_size, true, false);

> Thanks @offamitkumar. Could you explain the `__ z_ltr(Z_R0_scratch, Z_RET);` change, for my curiosity? Thanks.

`ltr` instruction stands for "load and test" (32 bit). Initially we were loading the value from `Z_RET` to `Z_R0_scratch` and then it will be compared against 0. But in this case there is no requirement of loading the value in Z_R0, as it's not being used further. So we can load the value again in `Z_RET` and the compare it against 0. There is nothing wrong in previous solution, it's just killing Z_R0 for nothing.

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

PR Comment: https://git.openjdk.org/jdk/pull/25764#issuecomment-2978824615