On Thu, 26 Feb 2026 08:39:20 GMT, Dean Long <[email protected]> wrote:
>> Anton Artemov has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - 8302745: Don't touch ARM code. >> - 8302745: Addressed reviewer's comments. > > Hi Anton. Sorry, maybe I'm missing something, but this looks wrong. My > understanding is that the aarch64 interpreter frame goes through the > following stages: > 1. new frame, sp includes max_stack and 16 byte alignment, sp <= esp > 2. call to interpreter, r19=sp snapshot, sp bumped to esp - extra_locals > Depending on size of caller max_stack and callee extra_locals, adjusted > sp can now be either less or greater than > r19, so we can't reliably assert any relationship between the two > 3. method handle invoke, esp += 1 for membername > As far as I can tell, this PR changes the meaning of unextended_sp be an > aligned version of esp. > Using this value on method return means SP no longer includes worst case > stack size based on max_stack, > so growing esp can now exceed SP. But we should have asserts for that, so I > probably got something wrong. Hi Dean @dean-long, I have looked a bit more into possible issues and came to conclusion that growing `esp` can exceed (i.e. be below) the `sp` in the mainline code. Look for instance at `TemplateInterpreterGenerator::generate_native_entry()` for aarch64. The stack pointer there isn't set up the normal way, i.e. without taking into account `max_stack`. Instead: https://github.com/openjdk/jdk/blob/bad59c55d98b2ef3b4a810e767577093b44764cd/src/hotspot/cpu/aarch64/templateInterpreterGenerator_aarch64.cpp#L1430 So, there are 4 expected pushes. But in fact, there are 5. After the safepoint_poll if slow path is taken, there are two consecutive pushes, and after the 2nd one `esp` will be below the `sp`. The asserts I referred to in the previous comment fail in the mainline because of this. Same on my branch. Maybe I made a mistake somewhere, but it looks like it is not considered to be a problem on the mainline. Regarding the change I propose and its possible side-effect of `esp` exceeding `sp`, in `TemplateInterpreterGenerator::generate_return_entry_for()`, `TemplateInterpreterGenerator::generate_deopt_entry_for()`, `Interpreter::_remove_activation_preserving_args_entry` and `Interpreter::_throw_exception_entry` there is a call to `restore_sp_after_call`, where `sp `is restored back from a value stored at `frame::interpreter_frame_extended_sp_offset` inside of the frame. As far as I can tell, the value written to that offset contains the worst case stack size. It is true though that a raw `r19` value is used to restore sp in other places, but if I get it right, they all are intrinsic entry generation methods. Is it a concern there? ------------- PR Comment: https://git.openjdk.org/jdk/pull/29744#issuecomment-4031453180
