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

[Chad Rakoczy]

On Tue, 17 Jun 2025 04:08:18 GMT, Dean Long <dlong at openjdk.org> wrote:

>> 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.
>
> I took another look, and I think the problem with CallRelocation::fix_relocation_after_move() is the ambiguity of "destination" for a trampoline call.  There is the near instruction destination inside the same nmethod, and there is the effective final far destination contained in the trampoline stub.  For nmethod to nmethod relocation, I think you want a customized version of this function instead of trying to use it as-is.  The customized version could continue to use the near destination, but fixed up with new_addr_for, or maybe simpler would be to use the final far destination, which is always going to be outside the nmethod if you clear inline caches first, which I think you would need to do, otherwise recursive self calls to the Java method would not get relocated correctly.

> Isn't this logic only required because of Graal (JDK-8358096)?

I think it is needed for HotSpot as well. Just because a trampoline was generated for a call does mean it is being used. We still need this check in the event that there is a direct call that no longer reaches.

> For nmethod to nmethod relocation, I think you want a customized version of this function instead of trying to use it as-is.

I moved this logic to [CallRelocation::fix_relocation_after_move()](https://github.com/chadrako/jdk/blob/03bfce8779a0f9c9a2c276fc6cf084698e6725d7/src/hotspot/share/code/relocInfo.cpp#L373-L402). It first checks if the destination is within the source and also check if the call is too far.

> otherwise recursive self calls to the Java method would not get relocated correctly

What do you mean by this? I don't see how recursive calls would behave differently. There is a check for intra-nmethod calls which I think would cover this case

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

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