On Mon, 24 Apr 2023 22:51:10 GMT, Daniel D. Daugherty <dcu...@openjdk.org>
wrote:
>> Roman Kennke has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Remove unnecessary comments
>
> src/hotspot/cpu/arm/macroAssembler_arm.cpp line 1803:
>
>> 1801: #ifdef ASSERT
>> 1802: // Poison scratch regs
>> 1803: POISON_REGS((~savemask), t1, t2, t3, 0x10000001);
>
> Should this poison value be: 0x20000002
Why?
> src/hotspot/share/logging/logTag.hpp line 80:
>
>> 78: LOG_TAG(exceptions) \
>> 79: LOG_TAG(exit) \
>> 80: LOG_TAG(fastlock2) \
>
> So why 'fastlock2'? Where's 'fastlock1'? Or 'fastlock'?
It's currently only used in the arm port. I'm changing it to 'fastlock', ok?
> src/hotspot/share/runtime/arguments.cpp line 1994:
>
>> 1992: if (UseHeavyMonitors) {
>> 1993: FLAG_SET_CMDLINE(LockingMode, LM_MONITOR);
>> 1994: }
>
> HotSpot option processing has a general rule of last setting wins.
> With L1992-1994 here, I think there might be a problem with a cmd
> line that specifies:
>
> -XX:+UseHeavyMonitors -XX:LockingMode=1
>
> I think that the resulting value of `LockingMode` will be `LM_MONITOR`
> instead of `LM_LEGACY`. Granted mixing uses of `UseHeavyMonitors`
> with `LockingMode` is just asking for trouble...
I added a check that rejects conflicting +UseHeavyMonitors and LockingMode=X
flags on the cmd line. UseHeavyMonitors is already deprecated, and with the new
LockingMode flag should be removed asap (in a follow-up PR).
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/10907#discussion_r1178244295
PR Review Comment: https://git.openjdk.org/jdk/pull/10907#discussion_r1178253708
PR Review Comment: https://git.openjdk.org/jdk/pull/10907#discussion_r1178260893
