On Mon, 21 Oct 2024 06:38:28 GMT, Axel Boldt-Christmas <abold...@openjdk.org>
wrote:
>> Patricio Chilano Mateo has updated the pull request incrementally with six
>> additional commits since the last revision:
>>
>> - Fix comments in objectMonitor.hpp
>> - Move frame::saved_thread_address() to platform dependent files
>> - Fix typo in jvmtiExport.cpp
>> - remove usage of frame::metadata_words in possibly_adjust_frame()
>> - Fix comments in c2 locking paths
>> - Revert and simplify changes to c1_Runtime1 on aarch64 and riscv
>
> src/hotspot/cpu/aarch64/c1_Runtime1_aarch64.cpp line 231:
>
>> 229:
>> 230: StubFrame::~StubFrame() {
>> 231: __ epilogue(_use_pop_on_epilogue);
>
> Can we not hook the `_use_pop_on_epilogue` into `return_state_t`, simplify
> the constructors and keep the old should_not_reach_here guard for stubs which
> should not return?
> e.g.
> ```C++
> enum return_state_t {
> does_not_return, requires_return, requires_pop_epilogue_return
> };
>
> StubFrame::~StubFrame() {
> if (_return_state == does_not_return) {
> __ should_not_reach_here();
> } else {
> __ epilogue(_return_state == requires_pop_epilogue_return);
> }
> }
Yes, that's much better. I changed it in both aarch64 and riscv.
> src/hotspot/cpu/aarch64/c2_MacroAssembler_aarch64.cpp line 115:
>
>> 113: // The object's monitor m is unlocked iff m->owner == nullptr,
>> 114: // otherwise m->owner may contain a thread id, a stack address for
>> LM_LEGACY,
>> 115: // or the ANONYMOUS_OWNER constant for LM_LIGHTWEIGHT.
>
> Comment seems out of place in `LockingMode != LM_LIGHTWEIGHT` code.
I removed this comment about what other values might be stored in _owner since
we don't need to handle those cases here.
> src/hotspot/cpu/aarch64/c2_MacroAssembler_aarch64.cpp line 380:
>
>> 378: lea(t2_owner_addr, owner_address);
>> 379:
>> 380: // CAS owner (null => current thread id).
>
> I think we should be more careful when and where we talk about thread id and
> lock id respectively. Given that `switchToCarrierThread` switches the thread,
> but not the lock id. We should probably define and talk about the lock id
> when it comes to locking, as saying thread id may be incorrect.
>
> Then there is also the different thread ids, the OS level one, and the java
> level one. (But not sure how to reconcile this without causing confusion)
Fixed the comments to refer to _lock_id. Even without the switchToCarrierThread
case I think that's the correct thing to do.
> src/hotspot/cpu/aarch64/continuationFreezeThaw_aarch64.inline.hpp line 313:
>
>> 311:
>> 312: log_develop_trace(continuations, preempt)("adjusted sp for c2
>> runtime stub, initial sp: " INTPTR_FORMAT " final sp: " INTPTR_FORMAT
>> 313: " fp: " INTPTR_FORMAT,
>> p2i(sp + frame::metadata_words), p2i(sp), sp[-2]);
>
> Is there a reason for the mix of `2` and `frame::metadata_words`?
>
> Maybe this could be
> ```C++
> intptr_t* const unadjusted_sp = sp;
> sp -= frame::metadata_words;
> sp[-2] = unadjusted_sp[-2];
> sp[-1] = unadjusted_sp[-1];
>
> log_develop_trace(continuations, preempt)("adjusted sp for c2 runtime
> stub, initial sp: " INTPTR_FORMAT " final sp: " INTPTR_FORMAT
> " fp: " INTPTR_FORMAT,
> p2i(unadjusted_sp), p2i(sp), sp[-2]);
I removed the use of frame::metadata_words from the log statement instead to
make it consistent, since we would still implicitly be assuming metadata_words
it's 2 words when we do the copying. We could use a memcpy and refer to
metadata_words, but I think it is clear this way since we are explicitly
talking about the 2 extra words missing from the runtime frame as the comment
explains.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1809745804
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1809746249
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1809746397
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1809747046
