On Tue, 29 Oct 2024 01:59:35 GMT, Dean Long <dl...@openjdk.org> wrote:
>> Patricio Chilano Mateo has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Fix comment in VThreadWaitReenter
>
> src/hotspot/cpu/aarch64/templateInterpreterGenerator_aarch64.cpp line 1351:
>
>> 1349: // set result handler
>> 1350: __ mov(result_handler, r0);
>> 1351: __ str(r0, Address(rfp,
>> frame::interpreter_frame_result_handler_offset * wordSize));
>
> I'm guessing this is here because preemption doesn't save/restore registers,
> even callee-saved registers, so we need to save this somewhere. I think this
> deserves a comment.
Added comment.
> src/hotspot/cpu/aarch64/templateInterpreterGenerator_aarch64.cpp line 1509:
>
>> 1507: Label no_oop;
>> 1508: __ adr(t,
>> ExternalAddress(AbstractInterpreter::result_handler(T_OBJECT)));
>> 1509: __ ldr(result_handler, Address(rfp,
>> frame::interpreter_frame_result_handler_offset*wordSize));
>
> We only need this when preempted, right? So could this be moved into the
> block above, where we call restore_after_resume()?
Moved.
> src/hotspot/cpu/x86/c1_Runtime1_x86.cpp line 643:
>
>> 641: uint Runtime1::runtime_blob_current_thread_offset(frame f) {
>> 642: #ifdef _LP64
>> 643: return r15_off / 2;
>
> I think using r15_offset_in_bytes() would be less confusing.
I copied the same comments the other platforms have to make it more clear.
> src/hotspot/cpu/x86/interp_masm_x86.cpp line 359:
>
>> 357: push_cont_fastpath();
>> 358:
>> 359: // Make VM call. In case of preemption set last_pc to the one we want
>> to resume to.
>
> From the comment, it sounds like we want to set last_pc to resume_pc, but I
> don't see that happening. The push/pop of rscratch1 doesn't seem to be doing
> anything.
Method `MacroAssembler::call_VM_helper()` expects the current value at the top
of the stack to be the last_java_pc. There is comment on that method explaining
it:
https://github.com/openjdk/jdk/blob/60364ef0010bde2933c22bf581ff8b3700c4afd6/src/hotspot/cpu/x86/macroAssembler_x86.cpp#L1658
> src/hotspot/share/c1/c1_Runtime1.hpp line 138:
>
>> 136: static void initialize_pd();
>> 137:
>> 138: static uint runtime_blob_current_thread_offset(frame f);
>
> I think this returns an offset in wordSize units, but it's not documented.
> In some places we always return an offset in bytes and let the caller convert.
Added comment.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1821591515
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1821593810
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1821592920
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1821593351
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1821591930
