RFR: 8321509: False positive in get_trampoline fast path causes crash [v2]

[Dean Long]

On Tue, 25 Jun 2024 15:43:15 GMT, Vladimir Kozlov <kvn at openjdk.org> wrote:

>> Dean Long has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   rename to USE_TRAMPOLINE_STUB_FIX_OWNER
>
> src/hotspot/cpu/aarch64/nativeInst_aarch64.cpp line 58:
> 
>> 56:   if (destination == addr) {
>> 57:     return destination;
>> 58:   }
> 
> This needs comment.

Actually, I don't think I need this anymore.  We should never hit it for an nmethod, and during expansion, I will introduce raw_destination() for the assert in pd_fix_owner_after_move().

> src/hotspot/cpu/aarch64/nativeInst_aarch64.cpp line 110:
> 
>> 108:   CodeBlob *code = CodeCache::find_blob(call_addr);
>> 109:   assert(code != nullptr, "Could not find the containing code blob");
>> 110:   if (!code->is_nmethod()) return nullptr;
> 
> Is it because trampolines are not used by not nmethods? Should we have an assert here about that?

Previously, get_trampoline() was called on CodeBuffer/CodeBlob during expansion, but now it is only called by set_destination_mt_safe, so it should be OK to assert that it is an nmethod.

> src/hotspot/cpu/aarch64/nativeInst_aarch64.cpp line 113:
> 
>> 111:   nmethod* nm = code->as_nmethod();
>> 112: 
>> 113:   address bl_destination = instruction_address() + displacement();
> 
> Can you use `call_addr + displacement()` here?

Sure.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/19796#discussion_r1653544344
PR Review Comment: https://git.openjdk.org/jdk/pull/19796#discussion_r1653541506
PR Review Comment: https://git.openjdk.org/jdk/pull/19796#discussion_r1653544571