RFR: 8316694: Implement relocation of nmethod within CodeCache [v24]

[Dean Long]

On Tue, 17 Jun 2025 01:37:22 GMT, Chad Rakoczy <duke at openjdk.org> wrote:

>> src/hotspot/cpu/aarch64/relocInfo_aarch64.cpp line 89:
>> 
>>> 87:       x = trampoline;
>>> 88:     }
>>> 89:     call->set_destination(x);
>> 
>> I think I see what you're doing here, but it doesn't look right. At the very least it's a trap for maintainers, who don't expect the destination address to be discarded if the call doesn't reach.
>> 
>> When the call doesn't reach, I believe you're fixing up an internal call to point to its target in the new copy of the code. But this isn't right when calls are PC relative, is it? In that case it makes more sense to leave the call instruction alone rather than rewrite it.
>
> This code specifically is for calls that were close enough to have an immediate offset before relocation but are now too far and require a trampoline. I agree this is probably not the best location for it and it should be moved up in the call stack (probably in `CallRelocation::fix_relocation_after_move`).
> 
> The code that fixes up the internal calls is [here](https://github.com/chadrako/jdk/blob/4e80e35959829ecf1579efc65b9525b2aff2be1f/src/hotspot/share/code/relocInfo.cpp#L380-L385). The issue is that the calls are PC relative but the logic in `CallRelocation::fix_relocation_after_move` changes the offset to point to the old nmethod’s code assuming it is an external routine. So it is incorrectly updating the offset when it needs to stay the same.

Isn't this logic only required because of Graal (JDK-8358096)?  For HotSpot, there should always be a trampoline if one is needed [1], even taking into account possible nmethod relocation, because MacroAssembler::trampoline_call() uses is_always_within_branch_range(), which makes its decision based only on the codecache size, boundaries, and the target, not the address of the call site.
Further, my understanding is that the Graal vs HotSpot issue is because Graal uses the A64 ISA "hard" limit reachability limit, while HotSpot uses a "soft" limit thanks to this constant with a lower value for debug:

  static const uint64_t branch_range = NOT_DEBUG(128 * M) DEBUG_ONLY(2 * M);

[1] By "needed" here, I mean the hard A64 ISA limit.  It might be useful to have a version of reachable_from_branch_at() that used the hard limit when working with JVMCI-generate code, because ca

> So it is incorrectly updating the offset when it needs to stay the same.

I don't understand what exactly is going with that, but after working on JDK-8321509, I wouldn't be surprised if there were assumptions that break when trying relocate, and you indeed might need to move the logic up a level, or even take into account what type of relocation it is, or the fact that the source is a finalized nmethod and not a BufferBlob.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/23573#discussion_r2151256939