RFR: 8363620: AArch64: reimplement emit_static_call_stub()

[Fei Gao]

On Fri, 28 Nov 2025 15:29:10 GMT, Andrew Haley <aph at openjdk.org> wrote:

> > I’ve updated the patch with a new commit. In the meantime, we also found that patching `isb` to `nop` can work the same as patching `isb` to `b .+4`.
> 
> Please don't. The slow path via an indirect jump to the static call stub is not important. It makes no sense to use the Arm memory model in such a subtle way merely to avoid a perfectly-predicted rare branch. The time taken to understand such trickery by reviewers and future maintainers is not justified by the minuscule performance gain.
> 
> "Debugging is twice as hard as writing the code in the first place. Therefore, if you write the code as cleverly as possible, you are, by definition, not smart enough to debug it." - Brian Kernighan

@theRealAph Thanks for your valuable input!

I’m trying to clarify how I understand your idea of patching `isb` to `b .+4`. I agree that the correctness of removing the `isb` in the stub is quite subtle, and the change to `set_destination_mt_safe` is rather obscure. Because of this, I initially assumed that the motivation for patching `isb` to `b .+4` was to avoid introducing a tricky change to `set_destination_mt_safe`, thereby keeping the PR self-contained. Based on that understanding, I thought: if the main reason for patching `isb` to `b .+4` is to avoid additional complexity, and patching `isb` to `nop` can provide the same effect, then why not patch it to `nop` instead? That’s what led me to propose the new commit.

However, now I’m a bit confused, and I think I may not have fully understood your original intent behind patching `isb` to `b .+4`. In your original idea, were you suggesting a change like this:

diff --git a/src/hotspot/cpu/aarch64/compiledIC_aarch64.cpp b/src/hotspot/cpu/aarch64/compiledIC_aarch64.cpp
index 6fe3315014b..9bf29ff2b55 100644
--- a/src/hotspot/cpu/aarch64/compiledIC_aarch64.cpp
+++ b/src/hotspot/cpu/aarch64/compiledIC_aarch64.cpp
@@ -100,6 +100,11 @@ void CompiledDirectCall::set_to_interpreted(const methodHandle& callee, address
   method_holder->set_data((intptr_t)callee());
   MacroAssembler::pd_patch_instruction(method_holder->next_instruction_address(), entry);
   ICache::invalidate_range(stub, to_interp_stub_size());
+  // Patch 'isb' to 'b .+4'.
+  CodeBuffer stub_first_instruction(stub, Assembler::instruction_size);
+  Assembler assembler(&stub_first_instruction);
+  assembler.b(assembler.pc() + 4);
+
   // Update jump to call.
   set_destination_mt_safe(stub);
 }
@@ -109,6 +114,10 @@ void CompiledDirectCall::set_stub_to_clean(static_stub_Relocation* static_stub)
   address stub = static_stub->addr();
   assert(stub != nullptr, "stub not found");
   assert(CompiledICLocker::is_safe(stub), "mt unsafe call");
+  // Patch 'b .+4' to 'isb'.
+  CodeBuffer stub_first_instruction(stub, Assembler::instruction_size);
+  Assembler assembler(&stub_first_instruction);
+  assembler.isb();
   // Creation also verifies the object.
   NativeMovConstReg* method_holder
     = nativeMovConstReg_at(stub + NativeInstruction::instruction_size);


Thank you for your time!

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

PR Comment: https://git.openjdk.org/jdk/pull/26638#issuecomment-3590049824