RFR: 8361449: RISC-V: Code cleanup for native call [v2]

Feilong Jiang fjiang at openjdk.org
Sat Jul 12 08:33:39 UTC 2025


On Mon, 7 Jul 2025 03:05:25 GMT, Dingli Zhang <dzhang at openjdk.org> wrote:

>> Hi, please consider this code cleanup change for native call.
>> 
>> This removes the address parameter for NativeCall::reloc_destination and NativeFarCall::reloc_destination.
>> This also removes several unnecessary code blob related runtime checks turning them into assertions.
>> 
>> ### Testing
>> * [x]  hs:tier1 - hs:tier3 tested with linux-riscv64 fastdebug build
>
> Dingli Zhang has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Remove outdated comments

src/hotspot/cpu/riscv/nativeInst_riscv.cpp line 104:

> 102: 
> 103:   CodeBlob *code = CodeCache::find_blob(call_addr);
> 104:   assert(code != nullptr && code->is_nmethod(), "nmethod expected");

The `if (code != nullptr && code->is_nmethod())` statement indicates that `code->is_nmethod()` may not always be true. Do you think we should use assert here? Looks like the new assert logic is inconsistent with the original code.

src/hotspot/cpu/riscv/nativeInst_riscv.cpp line 105:

> 103:   CodeBlob *code = CodeCache::find_blob(call_addr);
> 104:   assert(code != nullptr && code->is_nmethod(), "nmethod expected");
> 105:   nmethod* nm = code->as_nmethod();

`nm` is only used for `get_trampoline_for(call_addr, nm)`, maybe we can just use `code->as_nmethod()` directly instead of making a new variable.

src/hotspot/cpu/riscv/nativeInst_riscv.cpp line 156:

> 154:   if (code->is_nmethod()) {
> 155:     nmethod* nm = code->as_nmethod();
> 156:     stub_addr = trampoline_stub_Relocation::get_trampoline_for(call_addr, nm);

Same here
Suggestion:

    stub_addr = trampoline_stub_Relocation::get_trampoline_for(call_addr,  code->as_nmethod());

src/hotspot/cpu/riscv/relocInfo_riscv.cpp line 76:

> 74: address Relocation::pd_call_destination(address orig_addr) {
> 75:   assert(is_call(), "should be a call here");
> 76:   if (orig_addr == nullptr) {

IIUC, it is synchronized with [JDK-8321509](https://bugs.openjdk.org/browse/JDK-8321509)? Should we add `USE_TRAMPOLINE_STUB_FIX_OWNER` and the other stuff?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/26150#discussion_r2202426556
PR Review Comment: https://git.openjdk.org/jdk/pull/26150#discussion_r2202428678
PR Review Comment: https://git.openjdk.org/jdk/pull/26150#discussion_r2202431146
PR Review Comment: https://git.openjdk.org/jdk/pull/26150#discussion_r2202440479


More information about the hotspot-compiler-dev mailing list