RFR: 8321509: False positive in get_trampoline fast path causes crash

[Dean Long]

On Tue, 9 Jul 2024 17:01:43 GMT, Evgeny Astigeevich <eastigeevich at openjdk.org> wrote:

>> AArch64 binds some trampoline call-sites early, thanks to its is_always_within_branch_range() check. This allows a false positive match with a trampoline stub during code buffer expansion in rare situations.  To fix this, this PR makes the following changes:
>> 
>> 1. Do not call get_trampoline() in Relocation::pd_call_destination or pd_set_call_destination, as they use the destination cannot be trusted during fixup.
>> 2. Restrict NativeCall::get_trampoline() to only operate on nmethods, not CodeBuffers (or BufferBlob)
>> 3. Fixup trampoline stub "owners" (call sites) as late as possible, in new trampoline_stub_Relocation::pd_fix_owner_after_move(), and only if destination is an nmethod.
>> 4. Avoid calling NativeCall::set_destination_mt_safe() during CodeBuffer fixup, which allows assert_lock to also go away
>> 5. Detect self-calls in NativeCall::destination() to avoid unnecessary call to find_blob()
>> 6. Add NativeCall fast paths for pd_call_destination/pd_set_call_destination
>
>> AArch64 binds some trampoline call-sites early, thanks to its is_always_within_branch_range() check. This allows a false positive match with a trampoline stub during code buffer expansion in rare situations.
> 
> I am looking at the fix. I am not sure it is a proper fix.
> `is_always_within_branch_range()` returns a value based on a static configuration of CodeCache. The returned value stays correct independent from CodeBuffer expansion.
> I think the problem might be that `CodeBuffer::relocate_code_to` assumes the  destination gets the final layout. So using it for CodeBuffer which is not finalized might be a problem. I have had issues with the function when I tried to implement relocation of nmethods to different places of CodeCache.

To answer @eastig, it is true that the branch target is correct in the source buffer during expansion.  However, because the target is PC-relative, it can be temporarily incorrect when copied to the destination buffer.  That is the problem with trusting the target from the destination buffer during expansion/copying.

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

PR Comment: https://git.openjdk.org/jdk/pull/19796#issuecomment-2218681802