Re: RFR: 8308745: ObjArrayKlass::allocate_objArray_klass may call into java while holding a lock

[David Holmes]

On Tue, 5 Mar 2024 23:13:30 GMT, Dean Long  wrote:

>  I'm wondering if your new recursive lock class could use the existing 
> ObjectMonitor.

There has been a drive to remove `ObjectLocker` from the C++ code due to the 
negative impact on Loom. Also not sure what existing `ObjectMonitor` you are 
referring to. ??

-

PR Comment: https://git.openjdk.org/jdk/pull/17739#issuecomment-1980207637

Re: RFR: 8325187: JVMTI GetThreadState says virtual thread is JVMTI_THREAD_STATE_INTERRUPTED when it no longer is [v3]

[Serguei Spitsyn]

> Please, review this fix correcting the JVMTI  `RawMonitorWait()` 
> implementation.
> The `RawMonitorWait()` is using the the  `jt->is_interrupted(true)` to update 
> the interrupt status of the interrupted waiting thread.  The issue is that 
> when it calls `jt->is_interrupted(true)` to fetch and clear the `interrupt 
> status` of the virtual thread, this information is not propagated to the 
> `java.lang.Thread` instance.
> In the `VirtualThread` class implementation the `interrupt status` for 
> virtual thread is stored for both virtual and carrier threads. It is because 
> the implementation of object monitors for virtual threads is based on pinning 
> virtual threads, and so, always operates on carrier thread. The fix is to 
> clear the interrupt status for both virtual and carrier  `java.lang.Thread` 
> instances.
> 
> Testing:
>  - tested with new test 
> `hotspot/jtreg/serviceability/jvmti/vthread/InterruptRawMonitor` which is 
> passed with the fix and failed without it
>  - ran mach5 tiers 1-6

Serguei Spitsyn has updated the pull request incrementally with one additional 
commit since the last revision:

  review: improved sync in new test InterruptRawMonitor

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/18093/files
  - new: https://git.openjdk.org/jdk/pull/18093/files/b365ede4..1e72452b

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=18093=02
 - incr: https://webrevs.openjdk.org/?repo=jdk=18093=01-02

  Stats: 40 lines in 2 files changed: 20 ins; 7 del; 13 mod
  Patch: https://git.openjdk.org/jdk/pull/18093.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/18093/head:pull/18093

PR: https://git.openjdk.org/jdk/pull/18093

Re: RFR: 8256314: JVM TI GetCurrentContendedMonitor is implemented incorrectly [v7]

[Serguei Spitsyn]

> The implementation of the JVM TI `GetCurrentContendedMonitor()` does not 
> match the spec. It can sometimes return an incorrect information about the 
> contended monitor. Such a behavior does not match the function spec. 
> With this update the `GetCurrentContendedMonitor()` is returning the monitor 
> only when the specified thread is waiting to enter or re-enter the monitor, 
> and the monitor is not returned when the specified thread is waiting in the 
> `java.lang.Object.wait` to be notified.
> 
> The implementations of the JDWP `ThreadReference.CurrentContendedMonitor` 
> command and JDI `ThreadReference.currentContendedMonitor()` method are based 
> and depend on this JVMTI function. The JDWP command and the JDI method were 
> specified incorrectly and had incorrect behavior. The fix slightly corrects 
> both the JDWP and JDI specs. The JDWP and JDI implementations have been also 
> fixed because they use this JVM TI update. Please, see and review the related 
> CSR and Release-Note.
> 
> CSR: [8326024](https://bugs.openjdk.org/browse/JDK-8326024): JVM TI 
> GetCurrentContendedMonitor is implemented incorrectly
> RN:   [8326038](https://bugs.openjdk.org/browse/JDK-8326038): Release Note: 
> JVM TI GetCurrentContendedMonitor is implemented incorrectly
> 
> Testing:
>  - tested with the mach5 tiers 1-6

Serguei Spitsyn has updated the pull request incrementally with one additional 
commit since the last revision:

  review: updated test desciption for two modified tests

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/17944/files
  - new: https://git.openjdk.org/jdk/pull/17944/files/9b69af50..289f6ff7

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=17944=06
 - incr: https://webrevs.openjdk.org/?repo=jdk=17944=05-06

  Stats: 5 lines in 2 files changed: 2 ins; 0 del; 3 mod
  Patch: https://git.openjdk.org/jdk/pull/17944.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/17944/head:pull/17944

PR: https://git.openjdk.org/jdk/pull/17944

Re: RFR: 8256314: JVM TI GetCurrentContendedMonitor is implemented incorrectly [v6]

[Serguei Spitsyn]

On Tue, 5 Mar 2024 18:39:43 GMT, Leonid Mesnik  wrote:

>> Serguei Spitsyn has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   review: added new internal function 
>> JvmtiEnvBase::get_thread_or_vthread_state
>
> test/hotspot/jtreg/vmTestbase/nsk/jdi/ObjectReference/waitingThreads/waitingthreads004a.java
>  line 106:
> 
>> 104: display("entered and notifyAll: synchronized 
>> (lockingObject) {}");
>> 105: lockingObject.notifyAll();
>> 106: 
> 
> Please update test documentation in TestDescription. line:
>  - An object with threads waiting in Object.wait(long) method.
>  should be updated/or another one added.

Thanks, updated `TestDescription.java` now.

> test/hotspot/jtreg/vmTestbase/nsk/jdwp/ThreadReference/CurrentContendedMonitor/curcontmonitor001a.java
>  line 88:
> 
>> 86: // ensure that tested thread is waiting for monitor object
>> 87: synchronized (TestedClass.thread.monitor) {
>> 88: TestedClass.thread.monitor.notifyAll();
> 
> You need to update test documentation in TestDescription.java to explicitly 
> say that test not "waiting" but exit from wait and waiting for monitor 
> (contended).

Thanks, updated `TestDescription.java` now.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/17944#discussion_r1513663234
PR Review Comment: https://git.openjdk.org/jdk/pull/17944#discussion_r1513663121

Re: RFR: 8308745: ObjArrayKlass::allocate_objArray_klass may call into java while holding a lock

[Dean Long]

On Tue, 6 Feb 2024 22:59:04 GMT, Coleen Phillimore  wrote:

> This change creates a new sort of native recursive lock that can be held 
> during JNI and Java calls, which can be used for synchronization while 
> creating objArrayKlasses at runtime.
> 
> Passes tier1-7.

Is the caller always a JavaThread?  I'm wondering if your new recursive lock 
class could use the existing ObjectMonitor.  I thought David asked the same 
question, but I can't find it.

-

PR Comment: https://git.openjdk.org/jdk/pull/17739#issuecomment-1979796523

Re: RFR: 8325187: JVMTI GetThreadState says virtual thread is JVMTI_THREAD_STATE_INTERRUPTED when it no longer is [v2]

[David Holmes]

On Tue, 5 Mar 2024 19:57:39 GMT, Serguei Spitsyn  wrote:

> The behavior of ObjectMonitor::wait and RawMonitorWait is different.
The Object.wait() throws the InterruptedException if it was interrupted. The 
RawMonitorWait clears the thread interrupt status and returns the error code 
JVMTI_ERROR_INTERRUPT.

That is not a significant/relevant difference as far as I can see. They both 
call `thread->is_interrupted(true)`.

> For Object.wait, the clearing of the interrupt status is done in the Java 
> code, 

Okay so that is where the carrier and virtual thread states get back in sync, 
and that is what is missing in the `RawMonitorWait` case. The proposed 
fix/change to `is_interrupted` is what threw me as that is the wrong place to 
make a change because it affects both cases. What is missing is an upcall from 
`RawMonitorWait` to some Java code to do the same job as the `Object.wait` Java 
code does.

-

PR Comment: https://git.openjdk.org/jdk/pull/18093#issuecomment-1979794228

Re: RFR: 8324868: debug agent does not properly handle interrupts of a virtual thread [v2]

[Chris Plummer]

On Wed, 28 Feb 2024 22:10:04 GMT, Chris Plummer  wrote:

>> Fix numerous debug agent issues and testing deficiencies related to 
>> Thread.interrupt(). As a first read you should look at the description in 
>> the CR. More details in first comment below.
>> 
>> Also, this PR is dependent on the fix for 
>> [JDK-8325187](https://bugs.openjdk.org/browse/JDK-8325187). The test might 
>> generate GHA failures in the meantime.
>> 
>> Tested with all svc tier1, tier2, and tier5 tests.
>
> Chris Plummer has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   review feed back. fix up some comments.

Can I get a 2nd review please? Thank you!

-

PR Comment: https://git.openjdk.org/jdk/pull/17989#issuecomment-1979718522

Re: RFR: 8325532: serviceability/dcmd/compiler/PerfMapTest.java leaves created files in the /tmp dir.

[Chris Plummer]

On Tue, 5 Mar 2024 01:48:49 GMT, Jaikiran Pai  wrote:

>> Hi Jai. I think that solves all the functional requirements, but now we've 
>> turned two easily understood lines of code (calling run() and 
>> deleteIfExists()) into what you have above. I'm not so sure it is worth it. 
>> I refer back to Leonid's comment above on this topic:
>> 
>>> I thought about execution of Files.deleteIfExists(path) in the case of test 
>>> fails, but decided that it is not so important. Also, test always might 
>>> fail with crash when no finally block is executed. So I am fine with 
>>> current way.
>
> Hello Chris,
>> Hi Jai. I think that solves all the functional requirements, but now we've 
>> turned two easily understood lines of code (calling run() and 
>> deleteIfExists()) into what you have above. 
> 
> I agree. 
> 
>> I'm not so sure it is worth it. I refer back to Leonid's comment above on 
>> this topic:
>> 
>> > I thought about execution of Files.deleteIfExists(path) in the case of 
>> > test fails, but decided that it is not so important. Also, test always 
>> > might fail with crash when no finally block is executed. So I am fine with 
>> > current way.
> 
> What Leonid suggests looks fine to me and keeps things simple.

Ok. I think then I'll just push this PR in its current form.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/17992#discussion_r1513556350

Re: RFR: 8325187: JVMTI GetThreadState says virtual thread is JVMTI_THREAD_STATE_INTERRUPTED when it no longer is [v2]

[Chris Plummer]

On Tue, 5 Mar 2024 19:57:39 GMT, Serguei Spitsyn  wrote:

> The Object.wait() throws the InterruptedException if it was interrupted.

And clears the interrupted status, just like RawMonitorWait.

-

PR Comment: https://git.openjdk.org/jdk/pull/18093#issuecomment-1979670426

Re: RFR: 8325187: JVMTI GetThreadState says virtual thread is JVMTI_THREAD_STATE_INTERRUPTED when it no longer is [v2]

[Serguei Spitsyn]

On Tue, 5 Mar 2024 07:47:19 GMT, Alan Bateman  wrote:

>> Thank you for sharing this, Chris. It sounds like we need to introduce a 
>> mechanism to temporarily hide the JVMTI events. The question is if it is 
>> worth the complexity we add with it, especially if it is used just in a 
>> couple of cases.
>
>> If we do it with a Java upcall to the `VirtualThread.getAndClearInterrupt()` 
>> then we also have to skip the JVMTI events, AFAIK, as it is not a good idea 
>> to post the JVMTI events recursively. 
> 
> I meant it would need to use ObjectLocker to hold the lock in the VM when 
> changing both fields. It should only need to do this for RawMonitorWait at 
> this time. For Object.wait, the clearing of the interrupt status is done in 
> the Java code, something that will change once Object.wait changes to 
> freeze/unmount when not pinned.

Got it. Thank you, Alan.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18093#discussion_r1513425638

Re: RFR: 8325187: JVMTI GetThreadState says virtual thread is JVMTI_THREAD_STATE_INTERRUPTED when it no longer is [v2]

[Serguei Spitsyn]

On Tue, 5 Mar 2024 07:07:27 GMT, David Holmes  wrote:

> I have to say that I don't understand how the behaviour of `RawMonitorWait` 
> is any different to `ObjectMonitor::wait` when it comes to the use of the 
> is_interrupted(true). ??? Is it simply that because we are in native code and 
> we can immediately query the actual thread state that we can observe when the 
> carrier and virtual thread states are transiently different?

The behavior of `ObjectMonitor::wait` and `RawMonitorWait` is different.
The `Object.wait()` throws the `InterruptedException` if it was interrupted. 
The `RawMonitorWait` clears the thread `interrupt status` and returns the error 
code `JVMTI_ERROR_INTERRUPT`.

Also, I guess, Alan partially answered this question here:
> For `Object.wait`, the clearing of the interrupt status is done in the Java 
> code, something that will change once `Object.wait` changes to 
> `freeze/unmount` when not pinned.

-

PR Comment: https://git.openjdk.org/jdk/pull/18093#issuecomment-1979528696

Re: RFR: 8296244: Alternate implementation of user-based authorization Subject APIs that doesn’t depend on Security Manager APIs [v6]

[Weijun Wang]

> This code change adds an alternative implementation of user-based 
> authorization `Subject` APIs that doesn't depend on Security Manager APIs. 
> Depending on if the Security Manager is allowed, the methods store the 
> current subject differently. See the spec change in the `Subject.java` file 
> for details. When the Security Manager APIs are finally removed in a future 
> release, this new implementation will be only implementation for these 
> methods.
> 
> One major change in the new implementation is that `Subject.getSubject` 
> always throws an `UnsupportedOperationException` since it has an 
> `AccessControlContext` argument but the current subject is no longer 
> associated with an `AccessControlContext` object.
> 
> Now it's the time to migrate from the `getSubject` and `doAs` methods to 
> `current` and `callAs`. If the user application is simply calling 
> `getSubject(AccessController.getContext())`, then switching to `current()` 
> would work. If the `AccessControlContext` argument is retrieved from an 
> earlier `getContext()` call and the associated subject might be different 
> from that of the current `AccessControlContext`, then instead of storing the 
> previous `AccessControlContext` object and passing it into `getSubject` to 
> get the "previous" subject, the application should store the `current()` 
> return value directly.

Weijun Wang has updated the pull request incrementally with one additional 
commit since the last revision:

  revert changes to MBeanServerFileAccessController.java

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/17472/files
  - new: https://git.openjdk.org/jdk/pull/17472/files/2b55b171..80810b54

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=17472=05
 - incr: https://webrevs.openjdk.org/?repo=jdk=17472=04-05

  Stats: 14 lines in 1 file changed: 0 ins; 5 del; 9 mod
  Patch: https://git.openjdk.org/jdk/pull/17472.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/17472/head:pull/17472

PR: https://git.openjdk.org/jdk/pull/17472

Re: RFR: 8296244: Alternate implementation of user-based authorization Subject APIs that doesn’t depend on Security Manager APIs [v3]

[Weijun Wang]

On Tue, 5 Mar 2024 16:49:01 GMT, Kevin Walls  wrote:

>> Do you know where the subject is set? If it's set by a `doAs` call then it 
>> will co-operate with `current()` no matter if SM is allowed. I tried to 
>> search in the whole module and cannot find a `doAs` call. If it is also 
>> through `SubjectDomainCombiner` then it only works with SM.
>
> Subject is stored in the RMIConnectionImpl: 
> src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnectionImpl.java
> 
> (That is complicated by SubjectDelegation, which we deprecated for removal.  
> I have the PR out to remove it:
> https://github.com/openjdk/jdk/pull/18025 )
> 
> makeClient in RMIJRMPServerImpl creates RMIConnectionImpl
> 
> ..and RMIServerImpl.java has a doNewClient method calling that.  This is what 
> takes a Credentials Object and deals withJMXAuthenticator to get an 
> authenticated Subject.  None of this requires the SM.

I see that in `RMIConnectionImpl.java` it is stored inside an 
`AccessControlContext`, and there are `doPrivileged` calls on this ACC to pass 
the subject into an action. So, even if there might be no SM but the subject 
will never be bound to a thread using a scoped value.

I’ll revert the change then, and this code must have SM allowed to run 
correctly. If user runs it with SM disabled, at least they will see an UOE 
instead of letting `current()` silently returns a null.

Ultimately, if we want it working after SM, we should update 
`RMIConnectionImpl` and rewrite the ACC-related code to using `Subject.callAs`.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/17472#discussion_r1513410552

Re: RFR: 8308745: ObjArrayKlass::allocate_objArray_klass may call into java while holding a lock

[Coleen Phillimore]

On Tue, 13 Feb 2024 02:07:54 GMT, Coleen Phillimore  wrote:

>> src/hotspot/share/oops/arrayKlass.cpp line 141:
>> 
>>> 139:   ObjArrayKlass::allocate_objArray_klass(class_loader_data(), 
>>> dim + 1, this, CHECK_NULL);
>>> 140:   // use 'release' to pair with lock-free load
>>> 141:   release_set_higher_dimension(ak);
>> 
>> Why has this code changed? I only expected to see the lock changed.
>
> The assert is dumb, leftover from when we didn't have C++ types (only 
> klassOop).  Of course it's an objArrayKlass, that's its type!  The higher 
> dimension should be set in the constructor of ObjArrayKlass.  Every version 
> of this change, I move this assignment there.

Changing this like this makes it more similar to the InstanceKlass::array_klass 
function in structure, and there was unnecessary { } scopes in both and an 
unnecessary ResourceMark.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1487058776

Re: RFR: 8308745: ObjArrayKlass::allocate_objArray_klass may call into java while holding a lock

[Coleen Phillimore]

On Thu, 8 Feb 2024 21:30:48 GMT, David Holmes  wrote:

>> This change creates a new sort of native recursive lock that can be held 
>> during JNI and Java calls, which can be used for synchronization while 
>> creating objArrayKlasses at runtime.
>> 
>> Passes tier1-4.
>
> src/hotspot/share/classfile/classLoaderData.cpp line 412:
> 
>> 410:   // To call this, one must have the MultiArray_lock held, but the 
>> _klasses list still has lock free reads.
>> 411:   assert_locked_or_safepoint(MultiArray_lock);
>> 412: 
> 
> Do we no longer hold the lock or are you just missing the API to assert it is 
> held?

We no longer hold the lock in the callers to iterate through CLD::_klasses 
list.  It was never needed.  CLD::_klasses has iterators that are lock free 
(atomics)

> src/hotspot/share/oops/arrayKlass.cpp line 141:
> 
>> 139:   ObjArrayKlass::allocate_objArray_klass(class_loader_data(), 
>> dim + 1, this, CHECK_NULL);
>> 140:   // use 'release' to pair with lock-free load
>> 141:   release_set_higher_dimension(ak);
> 
> Why has this code changed? I only expected to see the lock changed.

The assert is dumb, leftover from when we didn't have C++ types (only 
klassOop).  Of course it's an objArrayKlass, that's its type!  The higher 
dimension should be set in the constructor of ObjArrayKlass.  Every version of 
this change, I move this assignment there.

> src/hotspot/share/prims/jvmtiExport.cpp line 3151:
> 
>> 3149:   if (MultiArray_lock->owner() == thread) {
>> 3150: return false;
>> 3151:   }
> 
> So the recursive nature of the lock now means we don't have to bail out here 
> - right?

Yes.  If it were only the JNI upcall that was broken while holding the 
MultiArray_lock, I think I could have used this same approach.  Unfortunately, 
its also throwing OOM for metaspace and for Java heap :(

> src/hotspot/share/prims/jvmtiGetLoadedClasses.cpp line 107:
> 
>> 105: // To get a consistent list of classes we need MultiArray_lock to 
>> ensure
>> 106: // array classes aren't created.
>> 107: MutexLocker ma(MultiArray_lock);
> 
> Unclear why this is no longer the case.

CLD::_klasses list is iterated through lock free with atomics. Older code used 
to iterate through the system dictionary which only had InstanceKlasses and 
then used to iterate through the arrays in InstanceKlass::_array_klasses.  
Which required the MultiArray_lock protecting it.  There used to be an 
array_klasses_do() function.   Now all the klasses are in the CLD::_klasses 
list and traversed atomically.

> src/hotspot/share/runtime/recursiveLock.cpp line 45:
> 
>> 43: _recursions++;
>> 44: assert(_recursions == 1, "should be");
>> 45: Atomic::release_store(&_owner, current); // necessary?
> 
> No release necessary. The only thing written since the sem.wait is recursions 
> and it is only  updated or needed by the owning thread.

ok thanks.

> src/hotspot/share/runtime/recursiveLock.cpp line 54:
> 
>> 52:   _recursions--;
>> 53:   if (_recursions == 0) {
>> 54: Atomic::release_store(&_owner, (Thread*)nullptr); // necessary?
> 
> No release necessary.

ok, thanks.

> src/hotspot/share/runtime/recursiveLock.hpp line 33:
> 
>> 31: // There are also no checks that the recursive lock is not held when 
>> going to Java or to JNI, like
>> 32: // other JVM mutexes have.  This should be used only for cases where the 
>> alternatives with all the
>> 33: // nice safety features don't work.
> 
> Mention that it does interact with safepoints properly for JavaThreads

Added a comment.

> src/hotspot/share/runtime/recursiveLock.hpp line 45:
> 
>> 43:   void unlock(Thread* current);
>> 44: };
>> 45: 
> 
> Should expose a `holdsLock` method to allow use in assertions.

Done.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1487056926
PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1487058194
PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1487060684
PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1487062202
PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1487063071
PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1487063156
PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1492431178
PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1492430934

Re: RFR: 8308745: ObjArrayKlass::allocate_objArray_klass may call into java while holding a lock

[David Holmes]

On Tue, 6 Feb 2024 22:59:04 GMT, Coleen Phillimore  wrote:

> This change creates a new sort of native recursive lock that can be held 
> during JNI and Java calls, which can be used for synchronization while 
> creating objArrayKlasses at runtime.
> 
> Passes tier1-4.

src/hotspot/share/classfile/classLoaderData.cpp line 412:

> 410:   // To call this, one must have the MultiArray_lock held, but the 
> _klasses list still has lock free reads.
> 411:   assert_locked_or_safepoint(MultiArray_lock);
> 412: 

Do we no longer hold the lock or are you just missing the API to assert it is 
held?

src/hotspot/share/oops/arrayKlass.cpp line 141:

> 139:   ObjArrayKlass::allocate_objArray_klass(class_loader_data(), 
> dim + 1, this, CHECK_NULL);
> 140:   // use 'release' to pair with lock-free load
> 141:   release_set_higher_dimension(ak);

Why has this code changed? I only expected to see the lock changed.

src/hotspot/share/prims/jvmtiExport.cpp line 3151:

> 3149:   if (MultiArray_lock->owner() == thread) {
> 3150: return false;
> 3151:   }

So the recursive nature of the lock now means we don't have to bail out here - 
right?

src/hotspot/share/prims/jvmtiGetLoadedClasses.cpp line 107:

> 105: // To get a consistent list of classes we need MultiArray_lock to 
> ensure
> 106: // array classes aren't created.
> 107: MutexLocker ma(MultiArray_lock);

Unclear why this is no longer the case.

src/hotspot/share/runtime/recursiveLock.cpp line 45:

> 43: _recursions++;
> 44: assert(_recursions == 1, "should be");
> 45: Atomic::release_store(&_owner, current); // necessary?

No release necessary. The only thing written since the sem.wait is recursions 
and it is only  updated or needed by the owning thread.

src/hotspot/share/runtime/recursiveLock.cpp line 54:

> 52:   _recursions--;
> 53:   if (_recursions == 0) {
> 54: Atomic::release_store(&_owner, (Thread*)nullptr); // necessary?

No release necessary.

src/hotspot/share/runtime/recursiveLock.hpp line 33:

> 31: // There are also no checks that the recursive lock is not held when 
> going to Java or to JNI, like
> 32: // other JVM mutexes have.  This should be used only for cases where the 
> alternatives with all the
> 33: // nice safety features don't work.

Mention that it does interact with safepoints properly for JavaThreads

src/hotspot/share/runtime/recursiveLock.hpp line 45:

> 43:   void unlock(Thread* current);
> 44: };
> 45: 

Should expose a `holdsLock` method to allow use in assertions.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1483613181
PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1483614823
PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1483623614
PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1483624460
PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1483608746
PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1483609712
PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1483610648
PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1483612492

Re: RFR: 8308745: ObjArrayKlass::allocate_objArray_klass may call into java while holding a lock

[Coleen Phillimore]

On Tue, 6 Feb 2024 22:59:04 GMT, Coleen Phillimore  wrote:

> This change creates a new sort of native recursive lock that can be held 
> during JNI and Java calls, which can be used for synchronization while 
> creating objArrayKlasses at runtime.
> 
> Passes tier1-4.

Thanks for looking at this WIP.

-

PR Review: https://git.openjdk.org/jdk/pull/17739#pullrequestreview-1876761135

RFR: 8308745: ObjArrayKlass::allocate_objArray_klass may call into java while holding a lock

[Coleen Phillimore]

This change creates a new sort of native recursive lock that can be held during 
JNI and Java calls, which can be used for synchronization while creating 
objArrayKlasses at runtime.

Passes tier1-4.

-

Commit messages:
 - 8308745: ObjArrayKlass::allocate_objArray_klass may call into java while 
holding a lock

Changes: https://git.openjdk.org/jdk/pull/17739/files
 Webrev: https://webrevs.openjdk.org/?repo=jdk=17739=00
  Issue: https://bugs.openjdk.org/browse/JDK-8308745
  Stats: 167 lines in 11 files changed: 87 ins; 47 del; 33 mod
  Patch: https://git.openjdk.org/jdk/pull/17739.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/17739/head:pull/17739

PR: https://git.openjdk.org/jdk/pull/17739

Re: RFR: 8325187: JVMTI GetThreadState says virtual thread is JVMTI_THREAD_STATE_INTERRUPTED when it no longer is [v2]

[Serguei Spitsyn]

On Tue, 5 Mar 2024 18:16:03 GMT, Leonid Mesnik  wrote:

>> Serguei Spitsyn has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   review: addressed a couple of comments on new test
>
> test/hotspot/jtreg/serviceability/jvmti/vthread/InterruptRawMonitor/InterruptRawMonitor.java
>  line 51:
> 
>> 49: System.out.println(thread);
>> 50: thread.start();
>> 51: Thread.sleep(2000);
> 
> I think it would be better to check 'thread' status or the same monitor  to 
> sync instead of sleep.  The sleep is always looks suspect, especially when 
> intermittent failure happens. 
> Also, it helps to save 2 seconds.

Will implement this suggestion, thanks.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18093#discussion_r1513397834

Re: AttachListener bsd differences

[Laurence Cable]

looking at the "attacher" code in jdk.attach it certainly does not 
appear to attempt to "attach" via cwd, so I would say that the 
"attachee" code to check this is redundant, unless there is a backwards 
compatibility/interoperability issue that requires this where

an early JDK "attacher" might attempt to rendezvous via cwd...

Rgds

- Larry

p.s also "littering" arbitrary directories with .attach files seems 
"rude" :)



On 3/4/24 9:38 PM, David Holmes wrote:

Hi Sonia,

Adding serviceability and Alan Bateman ...

On 5/03/2024 2:35 am, Sonia Zaldana Calles wrote:

Hi folks,


I’m working on JDK-8324683 [0].


I’m hoping to unify the code for attachListener_.cpp for posix 
platforms.



While reviewing linux against bsd, I noted a difference in 
AttachListener::is_init_trigger().



Both linux and AIX implementations, first try “dir>/.attach” and failing that, attempt “/tmp/.attach”.  
[1][2]



On the other hand, bsd doesn’t attempt “/tmp/.attach” [3].


I think you meant it doesn't attempt /.attach



I have been trying to do a bit of digging to see if this disparity 
was intentional but I have found that this change precedes the MacOS 
port, so I’m having trouble coming to a conclusion.


Looking at the history here:

https://hg.openjdk.org/macosx-port/macosx-port/hotspot/log/407770c31c18/src/os/bsd/vm/attachListener_bsd.cpp 



The very first load of the file did check the current directory too:

https://hg.openjdk.org/macosx-port/macosx-port/hotspot/rev/be94a5de4d32

but that was removed in the very next upload (though they never fixed 
the comment preceding the code):


https://hg.openjdk.org/macosx-port/macosx-port/hotspot/rev/9f1dd0b1d28c

To know why you would have to ask the person that did it I'm afraid.

I was wondering if perhaps anyone on this list could shed some light 
on this matter and whether it would be acceptable to do the alternate 
path on BSD too.


The init trigger mechanism was added by Alan under:

https://bugs.openjdk.org/browse/JDK-6272307

I get the sense that having the well-known file in the working 
directory was actually a bit of an anachronism. That may be why the 
macOS folk didn't bother with it (speculation on my part). AIX simply 
copied the Linux code.


Whether it would be okay/harmless/safe to have macOS/BSD also check 
there I really can't say. I'm not sure if we have code that would ever 
write the file to the target cwd these days? Hopefully Alan or current 
serviceability folk can say more.


Cheers,
David
-



Thanks for your help,

Sonia



[0] https://bugs.openjdk.org/browse/JDK-8324683 



[1] 
https://github.com/openjdk/jdk/blob/master/src/hotspot/os/linux/attachListener_linux.cpp#L521C3-L529C4 
 



[2] 
https://github.com/openjdk/jdk/blob/master/src/hotspot/os/aix/attachListener_aix.cpp#L551C2-L559C4 
 



[3] 
https://github.com/openjdk/jdk/blob/master/src/hotspot/os/bsd/attachListener_bsd.cpp#L520C2-L522C4 
 



--
Sonia Zaldaña Calles
Software Engineer, OpenJDK
Red Hat

Re: RFR: 8256314: JVM TI GetCurrentContendedMonitor is implemented incorrectly [v6]

[Leonid Mesnik]

On Tue, 5 Mar 2024 06:18:56 GMT, Serguei Spitsyn  wrote:

>> The implementation of the JVM TI `GetCurrentContendedMonitor()` does not 
>> match the spec. It can sometimes return an incorrect information about the 
>> contended monitor. Such a behavior does not match the function spec. 
>> With this update the `GetCurrentContendedMonitor()` is returning the monitor 
>> only when the specified thread is waiting to enter or re-enter the monitor, 
>> and the monitor is not returned when the specified thread is waiting in the 
>> `java.lang.Object.wait` to be notified.
>> 
>> The implementations of the JDWP `ThreadReference.CurrentContendedMonitor` 
>> command and JDI `ThreadReference.currentContendedMonitor()` method are based 
>> and depend on this JVMTI function. The JDWP command and the JDI method were 
>> specified incorrectly and had incorrect behavior. The fix slightly corrects 
>> both the JDWP and JDI specs. The JDWP and JDI implementations have been also 
>> fixed because they use this JVM TI update. Please, see and review the 
>> related CSR and Release-Note.
>> 
>> CSR: [8326024](https://bugs.openjdk.org/browse/JDK-8326024): JVM TI 
>> GetCurrentContendedMonitor is implemented incorrectly
>> RN:   [8326038](https://bugs.openjdk.org/browse/JDK-8326038): Release Note: 
>> JVM TI GetCurrentContendedMonitor is implemented incorrectly
>> 
>> Testing:
>>  - tested with the mach5 tiers 1-6
>
> Serguei Spitsyn has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   review: added new internal function 
> JvmtiEnvBase::get_thread_or_vthread_state

The test changes look good, need just small doc update.
The jvmti test  
serviceability/jvmti/thread/GetCurrentContendedMonitor/contmon01/contmon01.java 
is correct and don't check wait monitors.
I wonder if it makes sense to add test which verify that thread waiting in 
Object.wait() is not included into the result.

test/hotspot/jtreg/vmTestbase/nsk/jdi/ObjectReference/waitingThreads/waitingthreads004a.java
 line 106:

> 104: display("entered and notifyAll: synchronized 
> (lockingObject) {}");
> 105: lockingObject.notifyAll();
> 106: 

Please update test documentation in TestDescription. line:
 - An object with threads waiting in Object.wait(long) method.
 should be updated/or another one added.

test/hotspot/jtreg/vmTestbase/nsk/jdwp/ThreadReference/CurrentContendedMonitor/curcontmonitor001a.java
 line 88:

> 86: // ensure that tested thread is waiting for monitor object
> 87: synchronized (TestedClass.thread.monitor) {
> 88: TestedClass.thread.monitor.notifyAll();

You need to update test documentation in TestDescription.java to explicitly say 
that test not "waiting" but exit from wait and waiting for monitor (contended).

-

Marked as reviewed by lmesnik (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/17944#pullrequestreview-1917844453
PR Review Comment: https://git.openjdk.org/jdk/pull/17944#discussion_r1513326686
PR Review Comment: https://git.openjdk.org/jdk/pull/17944#discussion_r1513323999

Re: RFR: 8325187: JVMTI GetThreadState says virtual thread is JVMTI_THREAD_STATE_INTERRUPTED when it no longer is [v2]

[Leonid Mesnik]

On Tue, 5 Mar 2024 06:16:04 GMT, Serguei Spitsyn  wrote:

>> Please, review this fix correcting the JVMTI  `RawMonitorWait()` 
>> implementation.
>> The `RawMonitorWait()` is using the the  `jt->is_interrupted(true)` to 
>> update the interrupt status of the interrupted waiting thread.  The issue is 
>> that when it calls `jt->is_interrupted(true)` to fetch and clear the 
>> `interrupt status` of the virtual thread, this information is not propagated 
>> to the `java.lang.Thread` instance.
>> In the `VirtualThread` class implementation the `interrupt status` for 
>> virtual thread is stored for both virtual and carrier threads. It is because 
>> the implementation of object monitors for virtual threads is based on 
>> pinning virtual threads, and so, always operates on carrier thread. The fix 
>> is to clear the interrupt status for both virtual and carrier  
>> `java.lang.Thread` instances.
>> 
>> Testing:
>>  - tested with new test 
>> `hotspot/jtreg/serviceability/jvmti/vthread/InterruptRawMonitor` which is 
>> passed with the fix and failed without it
>>  - ran mach5 tiers 1-6
>
> Serguei Spitsyn has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   review: addressed a couple of comments on new test

Changes requested by lmesnik (Reviewer).

test/hotspot/jtreg/serviceability/jvmti/vthread/InterruptRawMonitor/InterruptRawMonitor.java
 line 51:

> 49: System.out.println(thread);
> 50: thread.start();
> 51: Thread.sleep(2000);

I think it would be better to check 'thread' status or the same monitor  to 
sync instead of sleep.  The sleep is always looks suspect, especially when 
intermittent failure happens. 
Also, it helps to save 2 seconds.

-

PR Review: https://git.openjdk.org/jdk/pull/18093#pullrequestreview-1917793041
PR Review Comment: https://git.openjdk.org/jdk/pull/18093#discussion_r1513292890

Re: RFR: 8296244: Alternate implementation of user-based authorization Subject APIs that doesn’t depend on Security Manager APIs [v3]

[Kevin Walls]

On Tue, 5 Mar 2024 14:44:29 GMT, Weijun Wang  wrote:

>> Right, this does not depend on the SM.   All we need to do is get the 
>> Subject.
>> This method implements the basic monitor (readonly) and control (readwrite) 
>> access.
>> accessMap maps identity String to Access, and the checkAccess() method here 
>> will check the Subject by using of its Principal names as keys in that map.
>
> Do you know where the subject is set? If it's set by a `doAs` call then it 
> will co-operate with `current()` no matter if SM is allowed. I tried to 
> search in the whole module and cannot find a `doAs` call. If it is also 
> through `SubjectDomainCombiner` then it only works with SM.

Subject is stored in the RMIConnectionImpl: 
src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnectionImpl.java

(That is complicated by SubjectDelegation, which we deprecated for removal.  I 
have the PR out to remove it:
https://github.com/openjdk/jdk/pull/18025 )

makeClient in RMIJRMPServerImpl creates RMIConnectionImpl

..and RMIServerImpl.java has a doNewClient method calling that.  This is what 
takes a Credentials Object and deals withJMXAuthenticator to get an 
authenticated Subject.  None of this requires the SM.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/17472#discussion_r1513164360

Re: RFR: 8296244: Alternate implementation of user-based authorization Subject APIs that doesn’t depend on Security Manager APIs [v5]

[Weijun Wang]

> This code change adds an alternative implementation of user-based 
> authorization `Subject` APIs that doesn't depend on Security Manager APIs. 
> Depending on if the Security Manager is allowed, the methods store the 
> current subject differently. See the spec change in the `Subject.java` file 
> for details. When the Security Manager APIs are finally removed in a future 
> release, this new implementation will be only implementation for these 
> methods.
> 
> One major change in the new implementation is that `Subject.getSubject` 
> always throws an `UnsupportedOperationException` since it has an 
> `AccessControlContext` argument but the current subject is no longer 
> associated with an `AccessControlContext` object.
> 
> Now it's the time to migrate from the `getSubject` and `doAs` methods to 
> `current` and `callAs`. If the user application is simply calling 
> `getSubject(AccessController.getContext())`, then switching to `current()` 
> would work. If the `AccessControlContext` argument is retrieved from an 
> earlier `getContext()` call and the associated subject might be different 
> from that of the current `AccessControlContext`, then instead of storing the 
> previous `AccessControlContext` object and passing it into `getSubject` to 
> get the "previous" subject, the application should store the `current()` 
> return value directly.

Weijun Wang has updated the pull request with a new target base due to a merge 
or a rebase. The incremental webrev excludes the unrelated changes brought in 
by the merge/rebase. The pull request contains eight additional commits since 
the last revision:

 - Merge branch 'master' into 8296244
 - revert some test changes, spec change for subject
 - fix MBeanServerFileAccessController, more test in SM
 - JMX needs SM
 - Resolve Alan's comments
 - remove exe bits
 - remove x bit
 - the patch

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/17472/files
  - new: https://git.openjdk.org/jdk/pull/17472/files/e57f7250..2b55b171

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=17472=04
 - incr: https://webrevs.openjdk.org/?repo=jdk=17472=03-04

  Stats: 97308 lines in 3235 files changed: 46912 ins; 26097 del; 24299 mod
  Patch: https://git.openjdk.org/jdk/pull/17472.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/17472/head:pull/17472

PR: https://git.openjdk.org/jdk/pull/17472

Re: RFR: 8296244: Alternate implementation of user-based authorization Subject APIs that doesn’t depend on Security Manager APIs [v3]

[Weijun Wang]

On Tue, 5 Mar 2024 11:36:53 GMT, Kevin Walls  wrote:

>> I think we need @kevinjwalls or @dfuch to help advise on this.
>
> Right, this does not depend on the SM.   All we need to do is get the Subject.
> This method implements the basic monitor (readonly) and control (readwrite) 
> access.
> accessMap maps identity String to Access, and the checkAccess() method here 
> will check the Subject by using of its Principal names as keys in that map.

Do you know where the subject is set? If it's set by a `doAs` call then it will 
co-operate with `current()` no matter if SM is allowed. I tried to search in 
the whole module and cannot find a `doAs` call. If it is also through 
`SubjectDomainCombiner` then it only works with SM.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/17472#discussion_r1512951092

Re: RFR: 8318026: jcmd should provide access to low-level JVM debug information [v4]

[Thomas Stuefe]

On Mon, 4 Mar 2024 22:02:53 GMT, Kevin Walls  wrote:

>> Kevin Walls has updated the pull request incrementally with three additional 
>> commits since the last revision:
>> 
>>  - Usage correction
>>  - Help to clarify this is VM inspection.  Comment to relate source to 
>> debug.cpp.
>>  - jcheck trailing whitespace
>
> Hi Thomas @tstuefe -
> 
> Security concerns certainly needed some thought. We have remote access to 
> DiagnosticCommands over JMX.  I don't see a particular need for that for this 
> command.  I think it was keeping the DCmd flagged as hidden that hides it 
> from the list of dcmds available in that way.  So we have the attach api 
> controls using local user identity, as we do for everything else.
> 
> Yes, "events" is not that useful, I should remove it.  I was taking the 
> useful parts of debug.cpp, and although the events were in VM.info, I had 
> missed that since your JDK-8224600, VM.events is its own command now!
> 
> Thread.print with stacks, vs "VM.debug threads" which is just a thread list, 
> so you have e.g. JavaThread*, name, native id, stack range.
> 
> "find": Usage at the moment, would likely be MessageBoxOnError, and a native 
> debugger to get the native stacktrace and parameters that include an object 
> reference you care about.  It might be other jcmds if e.g. events capture a 
> relevant problematic pointer, but likely it involves a native debugger.
> Using jcmd behaves better, showing the output where you run jcmd, not in the 
> VM's current output as the debug.cpp utils do.
> Native debugger syntax to call abritrary functions can be awkward, 
> particularly on Windows, so the jcmd should be a better experience.
> 
> This might be more compelling in post-mortem usage.  Am working on that.  
> i.e. jcmd on a core file.  But I am saying it offers some value today.
> 
> The class/method finders, I've heard some enthusiasm for their inclusion.  We 
> don't want to encourage overlap but yes we do have some overlapping jcmds.
> 
> 
> This is an umbrella but I don't think it's vague. VMDebugDcmd is for 
> inspecting VM state.  It's inspired by debug.cpp utilities, does not need to 
> implement all of them, but does aim to make them more accessible (I will 
> assert that they are not well known, which is hard to prove.)
> 
> Do we have a problem with jcmd feature creep?  If anything has crept too far 
> it can be addressed. Looks like the DCmd classes have approximately doubled 
> since jdk8u but this looks like growth in a good way.
> 
> Thanks
> Kevin

Hi Kevin, @kevinjwalls ,

thank you for your explanations. Please find answers inline.

> Security concerns certainly needed some thought. We have remote access to 
> DiagnosticCommands over JMX. I don't see a particular need for that for this 
> command. I think it was keeping the DCmd flagged as hidden that hides it from 
> the list of dcmds available in that way. So we have the attach api controls 
> using local user identity, as we do for everything else.
> 
> Yes, "events" is not that useful, I should remove it. I was taking the useful 
> parts of debug.cpp, and although the events were in VM.info, I had missed 
> that since your JDK-8224600, VM.events is its own command now!
> 
> Thread.print with stacks, vs "VM.debug threads" which is just a thread list, 
> so you have e.g. JavaThread*, name, native id, stack range.
> 
> "find": Usage at the moment, would likely be MessageBoxOnError, and a native 
> debugger to get the native stacktrace and parameters that include an object 
> reference you care about. It might be other jcmds if e.g. events capture a 
> relevant problematic pointer, but likely it involves a native debugger. Using 
> jcmd behaves better, showing the output where you run jcmd, not in the VM's 
> current output as the debug.cpp utils do. Native debugger syntax to call 
> abritrary functions can be awkward, particularly on Windows, so the jcmd 
> should be a better experience.

I remain sceptic here, because in my experience, once you start poking at the 
JVM innards at this level, I guess you will be quickly at the limit of what 
this command can do for you and need to attach a debugger anyway.

Could this be an own command, e,g, `VM.inspect`, and possibly limited to debug 
VMs? Do we really need this feature in production?

> 
> This might be more compelling in post-mortem usage. Am working on that. i.e. 
> jcmd on a core file. But I am saying it offers some value today.

Don't we have jhsdb for that?

> 
> The class/method finders, I've heard some enthusiasm for their inclusion. We 
> don't want to encourage overlap but yes we do have some overlapping jcmds.

Yeah, I can see this being useful.

> 
> This is an umbrella but I don't think it's vague. VMDebugDcmd is for 
> inspecting VM state. It's inspired by debug.cpp utilities, does not need to 
> implement all of them, but does aim to make them more accessible (I will 
> assert that they are not well known, which is hard to prove.)
> 
> Do we have 

Re: RFR: 8296244: Alternate implementation of user-based authorization Subject APIs that doesn’t depend on Security Manager APIs [v3]

[Kevin Walls]

On Mon, 4 Mar 2024 19:57:25 GMT, Sean Mullan  wrote:

>> I was not exactly sure if we will support this functionality when there is 
>> no SM. The class name has `AccessControler` and the method names use 
>> `checkAccess`, but they actually do not always depend on security manager.
>
> I think we need @kevinjwalls or @dfuch to help advise on this.

Right, this does not depend on the SM.   All we need to do is get the Subject.
This method implements the basic monitor (readonly) and control (readwrite) 
access.
accessMap maps identity String to Access, and the checkAccess() method here 
will check the Subject by using of its Principal names as keys in that map.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/17472#discussion_r1512676642

Re: RFR: 8318026: jcmd should provide access to low-level JVM debug information [v7]

[Kevin Walls]

> Introduce the jcmd "VM.debug" to implement access to a useful set of the 
> established debug.cpp utilities, with "jcmd PID VM.debug subcommand ...".
> 
> Not recommended for live production use.  Calling these "debug" utilities, 
> and not including them in the jcmd help output, is to remind us they are not 
> general customer-facing tools.

Kevin Walls has updated the pull request incrementally with one additional 
commit since the last revision:

  Test update

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/17655/files
  - new: https://git.openjdk.org/jdk/pull/17655/files/98a46d09..3f566649

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=17655=06
 - incr: https://webrevs.openjdk.org/?repo=jdk=17655=05-06

  Stats: 7 lines in 1 file changed: 0 ins; 4 del; 3 mod
  Patch: https://git.openjdk.org/jdk/pull/17655.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/17655/head:pull/17655

PR: https://git.openjdk.org/jdk/pull/17655

Re: RFR: 8318026: jcmd should provide access to low-level JVM debug information [v6]

[Kevin Walls]

> Introduce the jcmd "VM.debug" to implement access to a useful set of the 
> established debug.cpp utilities, with "jcmd PID VM.debug subcommand ...".
> 
> Not recommended for live production use.  Calling these "debug" utilities, 
> and not including them in the jcmd help output, is to remind us they are not 
> general customer-facing tools.

Kevin Walls has updated the pull request incrementally with one additional 
commit since the last revision:

  Show description if unknown subcommand.

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/17655/files
  - new: https://git.openjdk.org/jdk/pull/17655/files/6ba1c909..98a46d09

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=17655=05
 - incr: https://webrevs.openjdk.org/?repo=jdk=17655=04-05

  Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod
  Patch: https://git.openjdk.org/jdk/pull/17655.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/17655/head:pull/17655

PR: https://git.openjdk.org/jdk/pull/17655

Re: RFR: JDK-8320005 : Allow loading of shared objects with .a extension on AIX [v28]

[Magnus Ihse Bursie]

On Tue, 5 Mar 2024 08:48:15 GMT, Martin Doerr  wrote:

>> Suchismith Roy has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   indendt
>
> jdk21u is only open for critical backports and requires special approval. 
> Please backport it to jdk21u-dev.
> What about jdk22u?
> `/backport` commands should be used in commits, not in closed PRs.
> Not sure if it works without Committer privileges.

@TheRealMDoerr FYI, `/backport` works in PRs as well as commits since some time 
ago... In fact, I would recommend it as the better way to create a backport, 
now that it exists.

-

PR Comment: https://git.openjdk.org/jdk/pull/16604#issuecomment-1978482994

Re: RFR: 8318026: jcmd should provide access to low-level JVM debug information [v5]

[Kevin Walls]

> Introduce the jcmd "VM.debug" to implement access to a useful set of the 
> established debug.cpp utilities, with "jcmd PID VM.debug subcommand ...".
> 
> Not recommended for live production use.  Calling these "debug" utilities, 
> and not including them in the jcmd help output, is to remind us they are not 
> general customer-facing tools.

Kevin Walls has updated the pull request incrementally with one additional 
commit since the last revision:

  Remove unnecessary 'events' subcommand.

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/17655/files
  - new: https://git.openjdk.org/jdk/pull/17655/files/1d1e20e2..6ba1c909

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=17655=04
 - incr: https://webrevs.openjdk.org/?repo=jdk=17655=03-04

  Stats: 4 lines in 2 files changed: 0 ins; 2 del; 2 mod
  Patch: https://git.openjdk.org/jdk/pull/17655.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/17655/head:pull/17655

PR: https://git.openjdk.org/jdk/pull/17655

Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v22]

[Serguei Spitsyn]

On Tue, 5 Mar 2024 08:53:42 GMT, Serguei Spitsyn  wrote:

>> src/hotspot/share/prims/jvmtiEnvBase.cpp line 1507:
>> 
>>> 1505: nWait = 0;
>>> 1506: for (ObjectWaiter* waiter = mon->first_waiter();
>>> 1507:  waiter != nullptr && (nWait == 0 || waiter != 
>>> mon->first_waiter());
>> 
>> Sorry I do not understand the logic of this line. `waiters` is just a 
>> linked-list of `ObjectWaiter`s so all we need to do is traverse it and count 
>> the number of elements.
>
> The loop is endless without this extra condition, so we are getting a test 
> execution timeout.
> The `waiters` seems to be `circular doubly linked list` as we can see below:
> 
> inline void ObjectMonitor::AddWaiter(ObjectWaiter* node) {
>   assert(node != nullptr, "should not add null node");
>   assert(node->_prev == nullptr, "node already in list");
>   assert(node->_next == nullptr, "node already in list");
>   // put node at end of queue (circular doubly linked list)
>   if (_WaitSet == nullptr) {
> _WaitSet = node;
> node->_prev = node;
> node->_next = node;
>   } else {
> ObjectWaiter* head = _WaitSet;
> ObjectWaiter* tail = head->_prev;
> assert(tail->_next == head, "invariant check");
> tail->_next = node;
> head->_prev = node;
> node->_next = head;
> node->_prev = tail;
>   }
> }
> 
> I'll make sure nothing is missed and check the old code again.

Okay. Now I see why the old code did not have endless loops.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1512409266

Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v22]

[Serguei Spitsyn]

On Tue, 5 Mar 2024 06:30:20 GMT, David Holmes  wrote:

>> Serguei Spitsyn has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   review: addressed more comments on the fix and new test
>
> src/hotspot/share/prims/jvmtiEnvBase.cpp line 1507:
> 
>> 1505: nWait = 0;
>> 1506: for (ObjectWaiter* waiter = mon->first_waiter();
>> 1507:  waiter != nullptr && (nWait == 0 || waiter != 
>> mon->first_waiter());
> 
> Sorry I do not understand the logic of this line. `waiters` is just a 
> linked-list of `ObjectWaiter`s so all we need to do is traverse it and count 
> the number of elements.

The loop is endless without this extra condition, so we are getting a test 
execution timeout.
The `waiters` seems to be `circular doubly linked list` as we can see below:

inline void ObjectMonitor::AddWaiter(ObjectWaiter* node) {
  assert(node != nullptr, "should not add null node");
  assert(node->_prev == nullptr, "node already in list");
  assert(node->_next == nullptr, "node already in list");
  // put node at end of queue (circular doubly linked list)
  if (_WaitSet == nullptr) {
_WaitSet = node;
node->_prev = node;
node->_next = node;
  } else {
ObjectWaiter* head = _WaitSet;
ObjectWaiter* tail = head->_prev;
assert(tail->_next == head, "invariant check");
tail->_next = node;
head->_prev = node;
node->_next = head;
node->_prev = tail;
  }
}

I'll make sure nothing is missed and check the old code again.

> test/hotspot/jtreg/serviceability/jvmti/ObjectMonitorUsage/ObjectMonitorUsage.java
>  line 36:
> 
>> 34:  *   - unowned object without any waiting threads
>> 35:  *   - unowned object with threads waiting to be notified
>> 36:  *   - owned object without any waiting threads
> 
> You could say "threads waiting" in all cases - it looks odd to reverse it for 
> two cases.

Thanks, updated.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1512386413
PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1512391923

Re: RFR: JDK-8320005 : Allow loading of shared objects with .a extension on AIX [v28]

[Martin Doerr]

On Tue, 27 Feb 2024 08:31:15 GMT, Suchismith Roy  wrote:

>> J2SE agent does not start and throws error when it tries to find the shared 
>> library ibm_16_am.
>> After searching for ibm_16_am.so ,the jvm agent throws and error as dll_load 
>> fails.It fails to identify the shared library ibm_16_am.a shared archive 
>> file on AIX.
>> Hence we are providing a function which will additionally search for .a file 
>> on AIX ,when the search for .so file fails.
>
> Suchismith Roy has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   indendt

jdk21u is only open for critical backports and requires special approval. 
Please backport it to jdk21u-dev. What about jdk22u?

-

PR Comment: https://git.openjdk.org/jdk/pull/16604#issuecomment-1978239031