Re: RFR: JDK-8315575: Retransform of record class with record component annotation fails with CFE [v5]

[Serguei Spitsyn]

On Wed, 20 Mar 2024 03:25:39 GMT, Serguei Spitsyn  wrote:

>> Alex Menkov has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Updated the fix and the test for multiple annotations
>
> test/jdk/java/lang/instrument/RetransformRecordAnnotation.java line 107:
> 
>> 105: newClassBytes = classBytes;
>> 106: fInst.retransformClasses(targetClass);
>> 107: assertTrue(targetClass.getName() + " was not seen by 
>> transform()", seenClassBytes != null);
> 
> Nit: I guess, the `targetClassName` was intended to be used in place of 
> `targetClass.getName()`.

The `retransform()` method returns an array of bytes. There is also a comment 
about it at the line 101. But the result has never been checked (please, see 
lines: 116, 123, 129, 135). This creates some confusion and questions why the 
return value is needed.
The parameter `null` at lines 123, 129, 135 is also confusing and need some 
explanation.
As I read the code, the method  `retransform()` is saving the parameter 
`classBytes` in the `newClassBytes` field. The field `newClassBytes` value is 
then returned by the `transform()` method.
But the `ClassFileTransformer.html#transform` states this:
`Returns: a well-formed class file buffer (the result of the transform), or 
null if no transform is performed`.

So that, when the `null` is returned by the `transform()` then it means there 
was no real transform.
Could you comment on this? Do I miss anything here?

Please, see: 
[ClassFileTransformer.html#transform](https://docs.oracle.com/en/java/javase/21/docs/api/java.instrument/java/lang/instrument/ClassFileTransformer.html#transform(java.lang.Module,java.lang.ClassLoader,java.lang.String,java.lang.Class,java.security.ProtectionDomain,byte%5B%5D))

Also, the comment at line 122 is not needed at this form:

// Ensure retransformation does not fail with ClassFormatError.

Sorry, I was not clear when I've asked to add a comment at this point. I wanted 
a clarification about what does passing the `null` mean in terms of 
transformation.
My understanding is that there is no real transformation when the `null` is 
returned but the `Instrumentation` mechanism is still working and can fail and 
we want to test/verify it.  So, passing a Null-transformer is enough for our 
testing. Is it correct?
Please, keep in mind that few people have some knowledge about the 
re-transformation details and would appreciate any clarification here.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18161#discussion_r1531502998

Re: RFR: JDK-8315575: Retransform of record class with record component annotation fails with CFE [v5]

[Serguei Spitsyn]

On Tue, 19 Mar 2024 21:57:38 GMT, Alex Menkov  wrote:

>> RecordComponent class has _attributes_count field.
>> The only user of the field is JvmtiClassFileReconstituter. Incorrect value 
>> of the field causes producing incorrect data for Record attribute.
>> Parsing Record attribute ClassFileParser skips unknown attributes and may 
>> skip RuntimeInvisibleAnnotations/RuntimeInvisibleTypeAnnotations.
>> Also annotations can be changed (added/removed) by class redefinition.
>> The fix removes attributes_count from RecordComponent; 
>> JvmtiClassFileReconstituter calculates correct attributes_count generating 
>> class bytes.
>> 
>> Testing: 
>> - tier1,tier2,hs-tier5-svc;
>>  - redefineClasses/retransformClasses tests:
>>- test/jdk/java/lang/instrument
>>- test/hotspot/jtreg/serviceability/jvmti/RedefineClasses
>>- test/hotspot/jtreg/vmTestbase/nsk/jvmti/RedefineClasses
>>- test/hotspot/jtreg/vmTestbase/nsk/jvmti/RetransformClasses
>
> Alex Menkov has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Updated the fix and the test for multiple annotations

test/jdk/java/lang/instrument/RetransformRecordAnnotation.java line 107:

> 105: newClassBytes = classBytes;
> 106: fInst.retransformClasses(targetClass);
> 107: assertTrue(targetClass.getName() + " was not seen by 
> transform()", seenClassBytes != null);

Nit: I guess, the `targetClassName` was intended to be used in place of 
`targetClass.getName()`.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18161#discussion_r1531472856

Re: RFR: 8328285: GetOwnedMonitorInfo functions should use JvmtiHandshake [v2]

[Patricio Chilano Mateo]

On Tue, 19 Mar 2024 00:10:32 GMT, Serguei Spitsyn  wrote:

>> The `JvmtiHandshake` and `JvmtiUnitedHandshakeClosure` classes were 
>> introduced in the JDK 22 to unify/simplify the JVM TI functions supporting 
>> implementation of the virtual threads. This enhancement is to refactor JVM 
>> TI functions `GetOwnedMonitorInfo` and `GetOwnedMonitorStackDepthInfo` on 
>> the base of `JvmtiHandshake and `JvmtiUnitedHandshakeClosure` classes.
>> 
>> Testing:
>>  - Ran mach5 tiers 1-6
>
> Serguei Spitsyn has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   review: correct one comment

Looks good to me.

src/hotspot/share/prims/jvmtiEnv.cpp line 1368:

> 1366:   if (java_thread != nullptr) {
> 1367: Handle thread_handle(calling_thread, thread_oop);
> 1368: EscapeBarrier eb(true, calling_thread, java_thread);

I see that now we will execute the EscapeBarrier code for the vthread case too. 
We actually had to disable that for virtual threads since it doesn't work 
(8285739 and 8305625). But we only addressed the case when targeting all 
threads and not the single thread one. We would need to add the same check in 
EscapeBarrier::deoptimize_objects(int d1, int d2) to skip a thread with mounted 
continuation.

-

PR Review: https://git.openjdk.org/jdk/pull/18332#pullrequestreview-1947846211
PR Review Comment: https://git.openjdk.org/jdk/pull/18332#discussion_r1531444319

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

[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 11 additional commits since the 
last revision:

 - years and comments
 - Merge branch 'master' into 8296244
 - revert changes to MBeanServerFileAccessController.java
 - 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
 - ... and 1 more: https://git.openjdk.org/jdk/compare/bb164794...dfa22af0

-

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

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

  Stats: 384142 lines in 1493 files changed: 18191 ins; 81433 del; 284518 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: 8327990: [macosx-aarch64] Various tests fail with -XX:+AssertWXAtThreadSync [v3]

[David Holmes]

On Mon, 18 Mar 2024 13:14:41 GMT, Richard Reingruber  wrote:

>> This pr changes  `JfrJvmtiAgent::retransform_classes()` and 
>> `jfr_set_enabled` to switch to `WXWrite` before transitioning to the vm.
>> 
>> Testing:
>> make test TEST=jdk/jfr/event/runtime/TestClassLoadEvent.java 
>> TEST_VM_OPTS=-XX:+AssertWXAtThreadSync
>> make test TEST=compiler/intrinsics/klass/CastNullCheckDroppingsTest.java 
>> TEST_VM_OPTS=-XX:+AssertWXAtThreadSync
>> 
>> More tests are pending.
>
> Richard Reingruber 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 three additional 
> commits since the last revision:
> 
>  - Merge branch 'master' into 8327990_jfr_enters_vm_without_wxwrite
>  - Set WXWrite at more locations
>  - Switch to WXWrite before entering the vm

Not ideal but it fixes a real problem.

-

Marked as reviewed by dholmes (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/18238#pullrequestreview-1947550278

Re: RFR: 8327704: Update nsk/jdi tests to use driver instead of othervm [v3]

[Serguei Spitsyn]

On Sat, 9 Mar 2024 05:27:43 GMT, Leonid Mesnik  wrote:

>> vmtestbase nsk/jdi tests run 2 processes: debugger and debugee.
>> There is not need to start  debugger in the separate process for each test. 
>> Also, no need to run it with "-Xcomp" because the main goal is to test 
>> debugee behavior with different VM flags.
>> This fix updates tests to run debugger as driver to optimize execution time.
>> The fix also eliminates System.exit() which is not compatible with 
>> driver/agentvm mode and update shared classes to correctly add classpath 
>> when running debugee.
>> There were few tests which still execute using othervm mode. They require 
>> some specific classpath/settings. 
>> Tested by running all tests, with '-Xcomp' and virtual thread test factory.
>
> Leonid Mesnik has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   reverted failing tests.

Looks okay. Is not this fix trivial? :)

-

Marked as reviewed by sspitsyn (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/18174#pullrequestreview-1947544867

Re: RFR: 8327704: Update nsk/jdi tests to use driver instead of othervm [v3]

[Leonid Mesnik]

On Tue, 19 Mar 2024 20:41:56 GMT, Chris Plummer  wrote:

>> Leonid Mesnik has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   reverted failing tests.
>
> test/hotspot/jtreg/vmTestbase/nsk/share/jdi/Binder.java line 757:
> 
>> 755: }
>> 756: 
>> 757: if (classPath != null && !vmArgs.contains("-cp") && 
>> !vmArgs.contains("-classpath")) {
> 
> How does this change relate to using driver instead of othervm?

The classpaths are different for tests executed in agentvm and othervm.
As I see from logs jtreg uses CLASSPATH in othervm mode so debugee also 
inherits it. While agentvm load classed from 'test.class.path'.
It is not harm to use classpath 'test.class.path' in othervm mode.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18174#discussion_r1531188332

Re: RFR: 8327704: Update nsk/jdi tests to use driver instead of othervm [v3]

[Leonid Mesnik]

On Sat, 9 Mar 2024 05:27:43 GMT, Leonid Mesnik  wrote:

>> vmtestbase nsk/jdi tests run 2 processes: debugger and debugee.
>> There is not need to start  debugger in the separate process for each test. 
>> Also, no need to run it with "-Xcomp" because the main goal is to test 
>> debugee behavior with different VM flags.
>> This fix updates tests to run debugger as driver to optimize execution time.
>> The fix also eliminates System.exit() which is not compatible with 
>> driver/agentvm mode and update shared classes to correctly add classpath 
>> when running debugee.
>> There were few tests which still execute using othervm mode. They require 
>> some specific classpath/settings. 
>> Tested by running all tests, with '-Xcomp' and virtual thread test factory.
>
> Leonid Mesnik has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   reverted failing tests.

They also updated just to throw RuntimeException as other tests.

On Tue, Mar 19, 2024 at 2:20 PM Chris Plummer ***@***.***>
wrote:

> So no changes were made to any of these tests?
>
> —
> Reply to this email directly, view it on GitHub
> , or
> unsubscribe
> 
> .
> You are receiving this because you were mentioned.Message ID:
> ***@***.***>
>

-

PR Comment: https://git.openjdk.org/jdk/pull/18174#issuecomment-2008214565

Re: RFR: JDK-8315575: Retransform of record class with record component annotation fails with CFE [v5]

[Alex Menkov]

> RecordComponent class has _attributes_count field.
> The only user of the field is JvmtiClassFileReconstituter. Incorrect value of 
> the field causes producing incorrect data for Record attribute.
> Parsing Record attribute ClassFileParser skips unknown attributes and may 
> skip RuntimeInvisibleAnnotations/RuntimeInvisibleTypeAnnotations.
> Also annotations can be changed (added/removed) by class redefinition.
> The fix removes attributes_count from RecordComponent; 
> JvmtiClassFileReconstituter calculates correct attributes_count generating 
> class bytes.
> 
> Testing: 
> - tier1,tier2,hs-tier5-svc;
>  - redefineClasses/retransformClasses tests:
>- test/jdk/java/lang/instrument
>- test/hotspot/jtreg/serviceability/jvmti/RedefineClasses
>- test/hotspot/jtreg/vmTestbase/nsk/jvmti/RedefineClasses
>- test/hotspot/jtreg/vmTestbase/nsk/jvmti/RetransformClasses

Alex Menkov has updated the pull request incrementally with one additional 
commit since the last revision:

  Updated the fix and the test for multiple annotations

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/18161/files
  - new: https://git.openjdk.org/jdk/pull/18161/files/671c71e1..9a53d4ca

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

  Stats: 82 lines in 2 files changed: 58 ins; 11 del; 13 mod
  Patch: https://git.openjdk.org/jdk/pull/18161.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/18161/head:pull/18161

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

Re: RFR: 8327704: Update nsk/jdi tests to use driver instead of othervm [v3]

[Chris Plummer]

On Sat, 9 Mar 2024 05:27:43 GMT, Leonid Mesnik  wrote:

>> vmtestbase nsk/jdi tests run 2 processes: debugger and debugee.
>> There is not need to start  debugger in the separate process for each test. 
>> Also, no need to run it with "-Xcomp" because the main goal is to test 
>> debugee behavior with different VM flags.
>> This fix updates tests to run debugger as driver to optimize execution time.
>> The fix also eliminates System.exit() which is not compatible with 
>> driver/agentvm mode and update shared classes to correctly add classpath 
>> when running debugee.
>> There were few tests which still execute using othervm mode. They require 
>> some specific classpath/settings. 
>> Tested by running all tests, with '-Xcomp' and virtual thread test factory.
>
> Leonid Mesnik has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   reverted failing tests.

So no changes were made to any of these tests?

-

PR Comment: https://git.openjdk.org/jdk/pull/18174#issuecomment-2008154319

Re: RFR: 8327704: Update nsk/jdi tests to use driver instead of othervm [v3]

[Leonid Mesnik]

On Tue, 19 Mar 2024 20:42:34 GMT, Chris Plummer  wrote:

>> Leonid Mesnik has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   reverted failing tests.
>
> test/hotspot/jtreg/vmTestbase/nsk/share/jdi/ClassExclusionFilterTest.java 
> line 43:
> 
>> 41: if (result != 0) {
>> 42: throw new RuntimeException("TEST FAILED with result " + 
>> result);
>> 43: }
> 
> Why do we have tests under jdi/share?

It is not a test, but helper class with not the best name.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18174#discussion_r1531123676

Re: RFR: 8327704: Update nsk/jdi tests to use driver instead of othervm [v3]

[Leonid Mesnik]

On Sat, 9 Mar 2024 05:27:43 GMT, Leonid Mesnik  wrote:

>> vmtestbase nsk/jdi tests run 2 processes: debugger and debugee.
>> There is not need to start  debugger in the separate process for each test. 
>> Also, no need to run it with "-Xcomp" because the main goal is to test 
>> debugee behavior with different VM flags.
>> This fix updates tests to run debugger as driver to optimize execution time.
>> The fix also eliminates System.exit() which is not compatible with 
>> driver/agentvm mode and update shared classes to correctly add classpath 
>> when running debugee.
>> There were few tests which still execute using othervm mode. They require 
>> some specific classpath/settings. 
>> Tested by running all tests, with '-Xcomp' and virtual thread test factory.
>
> Leonid Mesnik has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   reverted failing tests.

There are othervm/native tests. I am not sure if they could be just converted.
MonitorContendedEnteredRequest/addClassFilter_ClassName/TestDescription.java:64:
 * @run main/othervm/native
MonitorContendedEnteredRequest/MonitorContendedEnteredRequest001/TestDescription.java:54:
 * @run main/othervm/native
MonitorContendedEnteredRequest/MonitorContendedEnteredRequest002/TestDescription.java:54:
 * @run main/othervm/native
MonitorContendedEnteredRequest/addClassFilter_ReferenceType/TestDescription.java:62:
 * @run main/othervm/native
MonitorContendedEnteredRequest/addClassExclusionFilter/TestDescription.java:63: 
* @run main/othervm/native
MonitorContendedEnteredRequest/addInstanceFilter/TestDescription.java:63: * 
@run main/othervm/native
MonitorContendedEnteredRequest/addThreadFilter/TestDescription.java:63: * @run 
main/othervm/native
MonitorContendedEnterRequest/addClassFilter_ClassName/TestDescription.java:64: 
* @run main/othervm/native
MonitorContendedEnterRequest/addClassFilter_ReferenceType/TestDescription.java:62:
 * @run main/othervm/native
MonitorContendedEnterRequest/MonitorContendedEnterRequest001/TestDescription.java:54:
 * @run main/othervm/native
MonitorContendedEnterRequest/addClassExclusionFilter/TestDescription.java:63: * 
@run main/othervm/native
MonitorContendedEnterRequest/addInstanceFilter/TestDescription.java:63: * @run 
main/othervm/native
MonitorContendedEnterRequest/addThreadFilter/TestDescription.java:65: * @run 
main/othervm/native
MonitorContendedEnterRequest/MonitorContendedEnterRequest002/TestDescription.java:54:
 * @run main/othervm/native
MethodExitEvent/returnValue/returnValue004/returnValue004.java:50: * @run 
main/othervm/native
ThreadReference/forceEarlyReturn/forceEarlyReturn005/forceEarlyReturn005.java:54:
 * @run main/othervm/native
ThreadReference/forceEarlyReturn/forceEarlyReturn004/forceEarlyReturn004.java:42:
 * @run main/othervm/native
ThreadReference/ownedMonitorsAndFrames/ownedMonitorsAndFrames004/ownedMonitorsAndFrames004.java:48:
 * @run main/othervm/native
ThreadReference/ownedMonitorsAndFrames/ownedMonitorsAndFrames009/ownedMonitorsAndFrames009.java:47:
 * @run main/othervm/native
ThreadReference/ownedMonitorsAndFrames/ownedMonitorsAndFrames002/ownedMonitorsAndFrames002.java:52:
 * @run main/othervm/native
ThreadReference/ownedMonitorsAndFrames/ownedMonitorsAndFrames007/ownedMonitorsAndFrames007.java:54:
 * @run main/othervm/native
ThreadReference/ownedMonitorsAndFrames/ownedMonitorsAndFrames008/TestDescription.java:57:
 * @run main/othervm/native
ThreadReference/ownedMonitorsAndFrames/ownedMonitorsAndFrames006/ownedMonitorsAndFrames006.java:51:
 * @run main/othervm/native
ThreadReference/ownedMonitorsAndFrames/ownedMonitorsAndFrames003/ownedMonitorsAndFrames003.java:51:
 * @run main/othervm/native
ThreadReference/ownedMonitorsAndFrames/ownedMonitorsAndFrames005/ownedMonitorsAndFrames005.java:48:
 * @run main/othervm/native
ReferenceType/instances/instances001/instances001.java:62: * @run 
main/othervm/native
ReferenceType/instances/instances004/TestDescription.java:49: * @run 
main/othervm/native
ReferenceType/instances/instances002/instances002.java:44: * @run 
main/othervm/native
ReferenceType/instances/instances005/instances005.java:44: * @run 
main/othervm/native
ReferenceType/instances/instances003/instances003.java:48: * @run 
main/othervm/native
ObjectReference/referringObjects/referringObjects003/referringObjects003.java:60:
 * @run main/othervm/native
ObjectReference/referringObjects/referringObjects002/referringObjects002.java:61:
 * @run main/othervm/native
ObjectReference/referringObjects/referringObjects001/referringObjects001.java:60:
 * @run main/othervm/native
ObjectReference/referringObjects/referringObjects004/referringObjects004.java:44:
 * @run main/othervm/native
stress/MonitorEvents/MonitorEvents001/TestDescription.java:62: * @run 
main/othervm/native
stress/MonitorEvents/MonitorEvents002/TestDescription.java:62: * @run 
main/othervm/native
stress/serial/ownedMonitorsAndFrames002/TestDescription.java:61: * @run 
main/othervm/native

Re: RFR: 8327704: Update nsk/jdi tests to use driver instead of othervm [v3]

[Chris Plummer]

On Sat, 9 Mar 2024 05:27:43 GMT, Leonid Mesnik  wrote:

>> vmtestbase nsk/jdi tests run 2 processes: debugger and debugee.
>> There is not need to start  debugger in the separate process for each test. 
>> Also, no need to run it with "-Xcomp" because the main goal is to test 
>> debugee behavior with different VM flags.
>> This fix updates tests to run debugger as driver to optimize execution time.
>> The fix also eliminates System.exit() which is not compatible with 
>> driver/agentvm mode and update shared classes to correctly add classpath 
>> when running debugee.
>> There were few tests which still execute using othervm mode. They require 
>> some specific classpath/settings. 
>> Tested by running all tests, with '-Xcomp' and virtual thread test factory.
>
> Leonid Mesnik has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   reverted failing tests.

>  There were few tests which still execute using othervm mode. They require 
> some specific classpath/settings.

Which ones? They are hard to find among the 2000+ files changed.

test/hotspot/jtreg/vmTestbase/nsk/share/jdi/Binder.java line 757:

> 755: }
> 756: 
> 757: if (classPath != null && !vmArgs.contains("-cp") && 
> !vmArgs.contains("-classpath")) {

How does this change relate to using driver instead of othervm?

test/hotspot/jtreg/vmTestbase/nsk/share/jdi/ClassExclusionFilterTest.java line 
43:

> 41: if (result != 0) {
> 42: throw new RuntimeException("TEST FAILED with result " + 
> result);
> 43: }

Why do we have tests under jdi/share?

-

PR Comment: https://git.openjdk.org/jdk/pull/18174#issuecomment-2008097455
PR Review Comment: https://git.openjdk.org/jdk/pull/18174#discussion_r1531083329
PR Review Comment: https://git.openjdk.org/jdk/pull/18174#discussion_r1531084477

Re: RFR: 8327990: [macosx-aarch64] Various tests fail with -XX:+AssertWXAtThreadSync [v3]

[Andrew Haley]

On Mon, 18 Mar 2024 13:14:41 GMT, Richard Reingruber  wrote:

>> This pr changes  `JfrJvmtiAgent::retransform_classes()` and 
>> `jfr_set_enabled` to switch to `WXWrite` before transitioning to the vm.
>> 
>> Testing:
>> make test TEST=jdk/jfr/event/runtime/TestClassLoadEvent.java 
>> TEST_VM_OPTS=-XX:+AssertWXAtThreadSync
>> make test TEST=compiler/intrinsics/klass/CastNullCheckDroppingsTest.java 
>> TEST_VM_OPTS=-XX:+AssertWXAtThreadSync
>> 
>> More tests are pending.
>
> Richard Reingruber 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 three additional 
> commits since the last revision:
> 
>  - Merge branch 'master' into 8327990_jfr_enters_vm_without_wxwrite
>  - Set WXWrite at more locations
>  - Switch to WXWrite before entering the vm

Marked as reviewed by aph (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/18238#pullrequestreview-1947320660

Re: RFR: 8327990: [macosx-aarch64] Various tests fail with -XX:+AssertWXAtThreadSync [v3]

[Andrew Haley]

On Tue, 19 Mar 2024 15:17:50 GMT, Thomas Stuefe  wrote:

> > Not necessarily. It may well remove some transitions from paths that don't 
> > need it, but if you move the state change too low down the call chain you 
> > could end up transitioning much more often in code that does need it e.g. 
> > if a transitioning method is called in a loop.
> 
> Not if you do the switching lazily. The first iteration would switch to the 
> needed state; subsequent iterations would not do anything since the state 
> already matches. Unless you interleave writes and execs, but then you would 
> need the state changes anyway.

Exactly. You do the transition when it's needed.

-

PR Comment: https://git.openjdk.org/jdk/pull/18238#issuecomment-2008070652

Re: RFR: JDK-8327769: jcmd GC.heap_dump without options should write to location given by -XX:HeapDumpPath, if set [v8]

[Chris Plummer]

On Tue, 19 Mar 2024 10:16:42 GMT, Matthias Baesken  wrote:

>>> If -XX:HeapDumpPath is specified, then it is used as the default
>> 
>> No, the filename set with jcmd GC:heamp_dump  filename   has priority. So we 
>> should better keep the current description.
>
> So should I also use  HeapDumpGzipLevel  the same way as HeapDumpPath ? Tehn 
> we have to change the text in globals.hpp for HeapDumpGzipLevel as well 
> because it mentions only the HeapDumpOnOutOfmemoryError case and not the jcmd 
> case .

I actually mentioned  HeapDumpGzipLevel as another reason not to make this 
change. I'm still not seeing it's value relative to the documentation headaches 
it causes.

@ansteiner commented that:

> This is really helpful from a support point of view.

I'd like to understand how. It seems to me that the person using the jcmd has a 
lot more interest in where the file ends up than the person launching the JVM, 
and can always specify the location with the jcmd. It seems odd to me that the 
jcmd user would want to rely on HeapDumpPath when they can just specify the 
path with the jcmd and know for sure where the file is going to end up.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18190#discussion_r1531043688

Re: RFR: 8327990: [macosx-aarch64] Various tests fail with -XX:+AssertWXAtThreadSync [v3]

[Tobias Holenstein]

On Mon, 18 Mar 2024 13:14:41 GMT, Richard Reingruber  wrote:

>> This pr changes  `JfrJvmtiAgent::retransform_classes()` and 
>> `jfr_set_enabled` to switch to `WXWrite` before transitioning to the vm.
>> 
>> Testing:
>> make test TEST=jdk/jfr/event/runtime/TestClassLoadEvent.java 
>> TEST_VM_OPTS=-XX:+AssertWXAtThreadSync
>> make test TEST=compiler/intrinsics/klass/CastNullCheckDroppingsTest.java 
>> TEST_VM_OPTS=-XX:+AssertWXAtThreadSync
>> 
>> More tests are pending.
>
> Richard Reingruber 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 three additional 
> commits since the last revision:
> 
>  - Merge branch 'master' into 8327990_jfr_enters_vm_without_wxwrite
>  - Set WXWrite at more locations
>  - Switch to WXWrite before entering the vm

Marked as reviewed by tholenstein (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/18238#pullrequestreview-1947241693

Re: RFR: 8327990: [macosx-aarch64] Various tests fail with -XX:+AssertWXAtThreadSync [v3]

[Martin Doerr]

On Mon, 18 Mar 2024 13:14:41 GMT, Richard Reingruber  wrote:

>> This pr changes  `JfrJvmtiAgent::retransform_classes()` and 
>> `jfr_set_enabled` to switch to `WXWrite` before transitioning to the vm.
>> 
>> Testing:
>> make test TEST=jdk/jfr/event/runtime/TestClassLoadEvent.java 
>> TEST_VM_OPTS=-XX:+AssertWXAtThreadSync
>> make test TEST=compiler/intrinsics/klass/CastNullCheckDroppingsTest.java 
>> TEST_VM_OPTS=-XX:+AssertWXAtThreadSync
>> 
>> More tests are pending.
>
> Richard Reingruber 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 three additional 
> commits since the last revision:
> 
>  - Merge branch 'master' into 8327990_jfr_enters_vm_without_wxwrite
>  - Set WXWrite at more locations
>  - Switch to WXWrite before entering the vm

+1

-

Marked as reviewed by mdoerr (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/18238#pullrequestreview-1947160377

Re: RFR: 8327990: [macosx-aarch64] Various tests fail with -XX:+AssertWXAtThreadSync [v3]

[Thomas Stuefe]

On Mon, 18 Mar 2024 13:14:41 GMT, Richard Reingruber  wrote:

>> This pr changes  `JfrJvmtiAgent::retransform_classes()` and 
>> `jfr_set_enabled` to switch to `WXWrite` before transitioning to the vm.
>> 
>> Testing:
>> make test TEST=jdk/jfr/event/runtime/TestClassLoadEvent.java 
>> TEST_VM_OPTS=-XX:+AssertWXAtThreadSync
>> make test TEST=compiler/intrinsics/klass/CastNullCheckDroppingsTest.java 
>> TEST_VM_OPTS=-XX:+AssertWXAtThreadSync
>> 
>> More tests are pending.
>
> Richard Reingruber 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 three additional 
> commits since the last revision:
> 
>  - Merge branch 'master' into 8327990_jfr_enters_vm_without_wxwrite
>  - Set WXWrite at more locations
>  - Switch to WXWrite before entering the vm

I think this patch makes sense, and does not compete with a long-term solution.

-

Marked as reviewed by stuefe (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/18238#pullrequestreview-1947069985

Re: RFR: JDK-8327474 Review use of java.io.tmpdir in jdk tests [v2]

[Bill Huang]

> This task addresses an essential aspect of our testing infrastructure: the 
> proper handling and cleanup of temporary files and socket files created 
> during test execution. The motivation behind these changes is to prevent the 
> accumulation of unnecessary files in the default temporary directory, which 
> can affect the system's storage and potentially influence subsequent test 
> runs.
> 
> Our review identified that several tests create temporary files or socket 
> files without ensuring their removal post-execution. 
> - Direct calls to java.io.File.createTempFile and 
> java.nio.file.Files.createTempFile without adequate cleanup.
> - Tests using NIO socket channels with StandardProtocolFamily.UNIX, not 
> explicitly removing socket files post-use.

Bill Huang has updated the pull request incrementally with one additional 
commit since the last revision:

  Implemented review comments

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/18352/files
  - new: https://git.openjdk.org/jdk/pull/18352/files/8472c31f..620f9259

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

  Stats: 136 lines in 5 files changed: 36 ins; 13 del; 87 mod
  Patch: https://git.openjdk.org/jdk/pull/18352.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/18352/head:pull/18352

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

Re: RFR: 8327990: [macosx-aarch64] Various tests fail with -XX:+AssertWXAtThreadSync [v3]

[Tobias Holenstein]

On Wed, 13 Mar 2024 13:53:46 GMT, Richard Reingruber  wrote:

>> src/hotspot/share/jfr/instrumentation/jfrJvmtiAgent.cpp line 160:
>> 
>>> 158:   ResourceMark rm(THREAD);
>>> 159:   // WXWrite is needed before entering the vm below and in callee 
>>> methods.
>>> 160:   MACOS_AARCH64_ONLY(ThreadWXEnable __wx(WXWrite, THREAD));
>> 
>> I understand you placed this here to cover the transition inside 
>> `create_classes_array` and the immediate one at line 170, but doesn't this 
>> risk having the wrong WX state for code in between?
>
> I've asked this myself (after making the change).
> Being in `WXWrite` mode would be wrong if the thread would execute 
> dynamically generated code. There's not too much happening outside the scope 
> of the `ThreadInVMfromNative` instances. I see jni calls 
> (`GetObjectArrayElement`, `ExceptionOccurred`) and a jvmti call 
> (`RetransformClasses`) but these are safe because the callees enter the vm 
> right away. We even avoid switching to `WXWrite` and back there.
> So I thought it would be ok to coarsen the WXMode switching.
> But maybe it's still better to avoid any risk especially since there's likely 
> no performance effect.

Or could the  `ThreadInVMfromNative tvmfn(THREAD);` in 
`check_exception_and_log` be move out to `JfrJvmtiAgent::retransform_classes`?  
And then only use one `ThreadInVMfromNative` in 
`JfrJvmtiAgent::retransform_classes`

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18238#discussion_r1530831165

Re: RFR: 8327990: [macosx-aarch64] Various tests fail with -XX:+AssertWXAtThreadSync [v3]

[Tobias Holenstein]

On Tue, 19 Mar 2024 17:08:52 GMT, Richard Reingruber  wrote:

> I see it differently. This PR is just a simple attempt to get clean test runs 
> with AssertWXAtThreadSync (after fixing an actual crash 
> https://bugs.openjdk.org/browse/JDK-8327036). While the violating locations 
> in this PR might be unlikely to produce actual crashes I think it is 
> worthwhile to have clean testing with AssertWXAtThreadSync because this will 
> help prevent regressions that are more likely.

I agree. Fixing the current state with this PR makes sense to me. Changing the 
logic of W^X will take more time and discussion. So from my point of view this 
PR is ready and should be integrated. If no-one disagrees I will approve

-

PR Comment: https://git.openjdk.org/jdk/pull/18238#issuecomment-2007742855

Re: RFR: 8327990: [macosx-aarch64] Various tests fail with -XX:+AssertWXAtThreadSync [v3]

[Richard Reingruber]

On Tue, 19 Mar 2024 12:17:22 GMT, David Holmes  wrote:

> But all this discussion suggests to me that this PR is not really worth 
> pursuing at this time - IIUC no actual failures are observed other than those 
> pertaining to AssertWXAtThreadSync and that flag will be gone if we do decide 
> to be more fine-grained about WX management.

I see it differently. This PR is just a simple attempt to get clean test runs 
with AssertWXAtThreadSync (after fixing an actual crash 
https://bugs.openjdk.org/browse/JDK-8327036). While the violating locations in 
this PR might be unlikely to produce actual crashes I think it is worthwhile to 
have clean testing with AssertWXAtThreadSync because this will help prevent 
regressions that are more likely.

Beyond the trivial fixes of this PR I'm very much in favor of further 
enhancements as the aforementioned https://bugs.openjdk.org/browse/JDK-8307817.
My recommendation would be to remove as much non-constant data from the code 
cache as possible.

-

PR Comment: https://git.openjdk.org/jdk/pull/18238#issuecomment-2007709981

Re: RFD: Remove Hotspot-internal MBeans from sun.management

[mandy . chung]

Would a PR to remove these APIs be welcome?



Good with me.

Mandy

On 3/19/24 9:41 AM, Eirik Bjørsnøs wrote:


Hi,

Last September, Volker shared the observation that we have 
Hotspot-internal MBeans in sun.management which are strongly 
encapsulated and not used internally by OpenJDK besides their unit tests:


https://www.mail-archive.com/core-libs-dev@openjdk.org/msg19878.html 



A summary of the email thread:

Mandy pointed out:

We added these HotSpot internal MBeans in JDK 5 to expose the
internal metrics.  Most of these internal metrics are exposed via
jstat tool too.   We didn't receive much feedback regarding these
HotSpot internal MBeans.    Removing them is fine and good cleanup
effort.


Alan made a similar point:

It's left over from experiments on exposing some internal metrics,
I think during JDK 5. It's code that should probably have been
removed a long time ago.


Kirk P raised a concern:

It would be a shame to lose these metrics because many of them
have been very
useful over time and some would be even more useful with some
modifications.

To which Mandy responded:

What we're referring to is to remove sun.management.Hotspot*, the
internal MBeans which are never exposed and registered in the
platform MBeanServer.   The internal metrics in HotSpot VM should
be retained as they are exposed through other ways like jstat, GC
logs, etc.


The email thread seems to have ended here without further action taken.

My interpretation of the above is that we have a consensus that these 
Hotspot-internal MBeans can be removed. Since I was not part of the 
initial discussion and some time has passed, I'd like some 
confirmation that my interpretation is correct.


Would a PR to remove these APIs be welcome?

(This would remove HotspotInternalMBean, HotspotMemoryMBean, 
HotspotRuntimeMBean, HotspotThreadMBean, with associated 
implementation, factory methods, tests and probably also some native 
code in libmanagement. Details can be discussed in a PR)


Cheers,
Eirik.

RFD: Remove Hotspot-internal MBeans from sun.management

[Eirik Bjørsnøs]

Hi,

Last September, Volker shared the observation that we have Hotspot-internal
MBeans in sun.management which are strongly encapsulated and not used
internally by OpenJDK besides their unit tests:

https://www.mail-archive.com/core-libs-dev@openjdk.org/msg19878.html

A summary of the email thread:

Mandy pointed out:

We added these HotSpot internal MBeans in JDK 5 to expose the internal
> metrics.  Most of these internal metrics are exposed via jstat tool too.
> We didn't receive much feedback regarding these HotSpot internal MBeans.
>  Removing them is fine and good cleanup effort.


Alan made a similar point:

It's left over from experiments on exposing some internal metrics, I think
> during JDK 5. It's code that should probably have been removed a long time
> ago.


Kirk P raised a concern:

It would be a shame to lose these metrics because many of them have been
> very
> useful over time and some would be even more useful with some
> modifications.
>

To which Mandy responded:

What we're referring to is to remove sun.management.Hotspot*, the internal
> MBeans which are never exposed and registered in the platform MBeanServer.
>   The internal metrics in HotSpot VM should be retained as they are exposed
> through other ways like jstat, GC logs, etc.


The email thread seems to have ended here without further action taken.

My interpretation of the above is that we have a consensus that these
Hotspot-internal MBeans can be removed. Since I was not part of the initial
discussion and some time has passed, I'd like some confirmation that my
interpretation is correct.

Would a PR to remove these APIs be welcome?

(This would remove HotspotInternalMBean, HotspotMemoryMBean,
HotspotRuntimeMBean, HotspotThreadMBean, with associated implementation,
factory methods, tests and probably also some native code in libmanagement.
Details can be discussed in a PR)

Cheers,
Eirik.

Integrated: 8328341: Remove deprecated per-thread compiler stats in sun.management

[Eirik Bjørsnøs]

On Mon, 18 Mar 2024 09:42:13 GMT, Eirik Bjørsnøs  wrote:

> Please review this cleanup PR which removes per-thread compiler stats from 
> `sun.management`
> 
> This removes:
> 
> * The deprecated interface method 
> `HotspotCompilationMBean.getCompilerThreadStats()` along with the 
> implementation method in `HotspotCompilation`
> * The class returned by these methods, `CompilerThreadStat`
> * The nested class `HotspotCompilation.CompilerThreadInfo` which now falls 
> out of use
> * The field `HotspotCompilation.threads`, along with its initialization in 
> `initCompilerCounters`
> 
> This was initially discussed here: 
> https://mail.openjdk.org/pipermail/serviceability-dev/2024-March/054589.html
> 
> Testing and verification: As this is purely a removal and cleanup PR of 
> unused code, no updates are made on the testing side. I have verified that 
> the string `getCompilerThreadStats` is not found in OpenJDK after this PR.

This pull request has now been integrated.

Changeset: 9214a62f
Author:Eirik Bjørsnøs 
URL:   
https://git.openjdk.org/jdk/commit/9214a62f26917162b3ff2144a1f3f9cde3b808fa
Stats: 149 lines in 3 files changed: 0 ins; 147 del; 2 mod

8328341: Remove deprecated per-thread compiler stats in sun.management

Reviewed-by: kevinw

-

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

Re: RFR: 8328341: Remove deprecated per-thread compiler stats in sun.management

[Eirik Bjørsnøs]

On Mon, 18 Mar 2024 09:42:13 GMT, Eirik Bjørsnøs  wrote:

> Please review this cleanup PR which removes per-thread compiler stats from 
> `sun.management`
> 
> This removes:
> 
> * The deprecated interface method 
> `HotspotCompilationMBean.getCompilerThreadStats()` along with the 
> implementation method in `HotspotCompilation`
> * The class returned by these methods, `CompilerThreadStat`
> * The nested class `HotspotCompilation.CompilerThreadInfo` which now falls 
> out of use
> * The field `HotspotCompilation.threads`, along with its initialization in 
> `initCompilerCounters`
> 
> This was initially discussed here: 
> https://mail.openjdk.org/pipermail/serviceability-dev/2024-March/054589.html
> 
> Testing and verification: As this is purely a removal and cleanup PR of 
> unused code, no updates are made on the testing side. I have verified that 
> the string `getCompilerThreadStats` is not found in OpenJDK after this PR.

Thanks for reviewing, Kevin!

-

PR Comment: https://git.openjdk.org/jdk/pull/18344#issuecomment-2007634465

Re: RFR: 8327990: [macosx-aarch64] Various tests fail with -XX:+AssertWXAtThreadSync [v3]

[Thomas Stuefe]

On Tue, 19 Mar 2024 12:17:22 GMT, David Holmes  wrote:

>> Instead, could we tag code that needs one or the other, keep track of the 
>> current WX state in thread-local memory, and flip WX only when we know we 
>> need to? 

The first part we already do.

I wonder wheter we could - at least as workaround for if we missed a spot - do 
wx switching as a reaction to a SIBBUS related to WX violation in code cache. 
Switch state around, return from signal handler and retry operation.

(Edit: tested it, does not seem to work. I guess when the SIGBUS is triggered 
in the kernel thread WX state had already been processed somehow).

> > That's very odd. The example there doesn't even involve MAP_JIT memory, so 
> > what does it have to do with WX?
> 
> @theRealAph that is the mystery we hope will be resolved once we know the 
> nature of the underlying OS bug. Somehow switching to exec mode 
> fixes/works-around the issue. I can imagine a missing conditional to check if 
> the region is MAP_JIT.
> 
> > Changing WX at VM state transitions is a form of temporal coupling, a 
> > classic design smell that has caused problems for decades.
> 
> The original introducers of WXEnable made the decision that the VM should be 
> in WRITE mode unless it needs EXEC. That is the state we are presently trying 
> to achieve with this change. If that original design choice is wrong then ...
> 
> > Instead, could we tag code that needs one or the other, keep track of the 
> > current WX state in thread-local memory, and flip WX only when we know we 
> > need to?
> 
> And I've asked about this every time a missing WXEnable has had to be added. 
> We seem to be generically able to describe what kind of code needs which 
> mode, but we seem to struggle to pin it down. Though that is what 
> https://bugs.openjdk.org/browse/JDK-8307817 is looking at doing.
> 
> > That'd (by definition) reduce the number of transitions to the minimum if 
> > we were through.
> 
> Not necessarily. It may well remove some transitions from paths that don't 
> need it, but if you move the state change too low down the call chain you 
> could end up transitioning much more often in code that does need it e.g. if 
> a transitioning method is called in a loop. 

Not if you do the switching lazily. The first iteration would switch to the 
needed state; subsequent iterations would not do anything since the state 
already matches. Unless you interleave writes and execs, but then you would 
need the state changes anyway.

-

PR Comment: https://git.openjdk.org/jdk/pull/18238#issuecomment-2007469410

Re: RFR: 8327990: [macosx-aarch64] Various tests fail with -XX:+AssertWXAtThreadSync [v3]

[Matthias Baesken]

On Tue, 19 Mar 2024 12:17:22 GMT, David Holmes  wrote:

>  IIUC no actual failures are observed other than those pertaining to 
> AssertWXAtThreadSync

We saw sporadic crashes in our jtreg (maybe also jck?) runs; only **_later_** 
we enabled AssertWXAtThreadSync for more checking.

-

PR Comment: https://git.openjdk.org/jdk/pull/18238#issuecomment-2007159560

Re: RFR: 8327990: [macosx-aarch64] Various tests fail with -XX:+AssertWXAtThreadSync [v3]

[David Holmes]

On Tue, 19 Mar 2024 09:30:52 GMT, Andrew Haley  wrote:

> That's very odd. The example there doesn't even involve MAP_JIT memory, so 
> what does it have to do with WX?

@theRealAph that is the mystery we hope will be resolved once we know the 
nature of the underlying OS bug. Somehow switching to exec mode 
fixes/works-around the issue. I can imagine a missing conditional to check if 
the region is MAP_JIT.

> Changing WX at VM state transitions is a form of temporal coupling, a classic 
> design smell that has caused problems for decades.

The original introducers of WXEnable made the decision that the VM should be in 
WRITE mode unless it needs EXEC. That is the state we are presently trying to 
achieve with this change. If that original design choice is wrong then ...

> Instead, could we tag code that needs one or the other, keep track of the 
> current WX state in thread-local memory, and flip WX only when we know we 
> need to?

And I've asked about this every time a missing WXEnable has had to be added. We 
seem to be generically able to  describe what kind of code needs which mode, 
but we seem to struggle to pin it down. Though that is what 
https://bugs.openjdk.org/browse/JDK-8307817 is looking at doing.

> That'd (by definition) reduce the number of transitions to the minimum if we 
> were through.

Not necessarily. It may well remove some transitions from paths that don't need 
it, but if you move the state change too low down the call chain you could end 
up transitioning much more often in code that does need it e.g. if a 
transitioning method is called in a loop. We need to optimise the actual call 
paths as well as identify specific methods.

But all this discussion suggests to me that this PR is not really worth 
pursuing at this time - IIUC no actual failures are observed other than those 
pertaining to `AssertWXAtThreadSync` and that flag will be gone if we do decide 
to be more fine-grained about WX management.

-

PR Comment: https://git.openjdk.org/jdk/pull/18238#issuecomment-2007029830

Re: RFR: JDK-8327769: jcmd GC.heap_dump without options should write to location given by -XX:HeapDumpPath, if set [v8]

[Matthias Baesken]

On Tue, 19 Mar 2024 10:13:16 GMT, Matthias Baesken  wrote:

>> So far we only use the gzip level setting from jcmd, not HeapDumpGzipLevel .
>> See the `level` variable in  `HeapDumpDCmd::execute` . Yeah you are probably 
>> right we should make it consistent.
>
>> If -XX:HeapDumpPath is specified, then it is used as the default
> 
> No, the filename set with jcmd GC:heamp_dump  filename   has priority. So we 
> should better keep the current description.

So should I also use  HeapDumpGzipLevel  the same way as HeapDumpPath ? Tehn we 
have to change the text in globals.hpp for HeapDumpGzipLevel as well because it 
mentions only the HeapDumpOnOutOfmemoryError case and not the jcmd case .

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18190#discussion_r1530082635

Re: RFR: JDK-8327769: jcmd GC.heap_dump without options should write to location given by -XX:HeapDumpPath, if set [v8]

[Matthias Baesken]

On Sat, 16 Mar 2024 12:50:05 GMT, Matthias Baesken  wrote:

>> src/hotspot/share/services/diagnosticCommand.cpp line 475:
>> 
>>> 473: HeapDumpDCmd::HeapDumpDCmd(outputStream* output, bool heap) :
>>> 474:DCmdWithParser(output, heap),
>>> 475:   _filename("filename","Name of the dump file", "STRING", false, "if 
>>> no filename was specified, but -XX:HeapDumpPath=hdp is set, path hdp is 
>>> taken"),
>> 
>> This seems clumsy, but I'm having a hard time coming up with something 
>> better.
>> 
>> "the filename specified by -XX:HeapDumpPath, if specified"
>> "If -XX:HeapDumpPath is specified, then it is used as the default"
>> 
>> Makes me wonder if this approach is wrong since it is hard to document 
>> clearly. Maybe we should go back to not having the jcmd use HeapDumpPath. I 
>> understand the argument for having the jcmd use the HeapDumpPath setting, 
>> but it might not be worth the documentation confusion it introduces. You can 
>> argue that HeapDumpPath really is just intended to help support 
>> HeapDumpOnOutOfMemoryError. Does the jcmd also use HeapDumpGzipLevel? I'm 
>> not sure, but we should be consistent with the application of HeapDumpPath 
>> and HeapDumpGzipLevel to the jcmd.
>
> So far we only use the gzip level setting from jcmd, not HeapDumpGzipLevel .
> See the `level` variable in  `HeapDumpDCmd::execute` . Yeah you are probably 
> right we should make it consistent.

> If -XX:HeapDumpPath is specified, then it is used as the default

No, the filename set with jcmd GC:heamp_dump  filename   has priority. So we 
should better keep the current description.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18190#discussion_r1530077879

Re: RFR: 8327990: [macosx-aarch64] Various tests fail with -XX:+AssertWXAtThreadSync [v3]

[Andrew Haley]

On Tue, 19 Mar 2024 04:37:41 GMT, David Holmes  wrote:

> That said we have had a lot of people looking at the overall WX state 
> management logic in the past week or so due to 
> https://bugs.openjdk.org/browse/JDK-8327860. The workaround there requires us 
> to be in EXEC mode

That's very odd. The example there doesn't even involve MAP_JIT memory, so what 
does it have to do with WX?

> and there are a number of folk who are questioning why "we" chose WRITE mode 
> as the default with a switch to EXEC, instead of EXEC as the default with a 
> switch to WRITE.
> But whichever way that goes I think the VM state transitions are the places 
> to enforce that choice.

Hmm. Changing WX at VM state transitions is a form of temporal coupling, a 
classic design smell that has caused problems for decades. It's causing 
problems for us now. Instead, could we tag code that needs one or the other, 
keep track of the current WX state in thread-local memory, and flip WX only 
when we know we need to? That'd (by definition) reduce the number of 
transitions to the minimum if we were through.

-

PR Comment: https://git.openjdk.org/jdk/pull/18238#issuecomment-2006498201

Integrated: 8325187: JVMTI GetThreadState says virtual thread is JVMTI_THREAD_STATE_INTERRUPTED when it no longer is

[Serguei Spitsyn]

On Fri, 1 Mar 2024 23:26:48 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

This pull request has now been integrated.

Changeset: 6eea5d67
Author:Serguei Spitsyn 
URL:   
https://git.openjdk.org/jdk/commit/6eea5d675566adca3fca88639008c6c0221450a4
Stats: 209 lines in 7 files changed: 205 ins; 1 del; 3 mod

8325187: JVMTI GetThreadState says virtual thread is 
JVMTI_THREAD_STATE_INTERRUPTED when it no longer is

Reviewed-by: dholmes, alanb

-

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

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

[Serguei Spitsyn]

On Mon, 18 Mar 2024 23:45:29 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: rename is_interrupted() to get_and_clear_interrupted(); update 
> comment

David and Alan, thank you for review!

-

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

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

[Alan Bateman]

On Mon, 18 Mar 2024 23:45:29 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: rename is_interrupted() to get_and_clear_interrupted(); update 
> comment

I don't have any more comments either, the src changes looks okay. I have not 
reviewed the test.

-

Marked as reviewed by alanb (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/18093#pullrequestreview-1945459563

Re: RFR: 8328341: Remove deprecated per-thread compiler stats in sun.management

[Kevin Walls]

On Mon, 18 Mar 2024 09:42:13 GMT, Eirik Bjørsnøs  wrote:

> Please review this cleanup PR which removes per-thread compiler stats from 
> `sun.management`
> 
> This removes:
> 
> * The deprecated interface method 
> `HotspotCompilationMBean.getCompilerThreadStats()` along with the 
> implementation method in `HotspotCompilation`
> * The class returned by these methods, `CompilerThreadStat`
> * The nested class `HotspotCompilation.CompilerThreadInfo` which now falls 
> out of use
> * The field `HotspotCompilation.threads`, along with its initialization in 
> `initCompilerCounters`
> 
> This was initially discussed here: 
> https://mail.openjdk.org/pipermail/serviceability-dev/2024-March/054589.html
> 
> Testing and verification: As this is purely a removal and cleanup PR of 
> unused code, no updates are made on the testing side. I have verified that 
> the string `getCompilerThreadStats` is not found in OpenJDK after this PR.

Yes, looks good.
No problems with testst in test/jdk/javax/management or test/jdk/sun/management.

-

Marked as reviewed by kevinw (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/18344#pullrequestreview-1945428269