Re: RFR: 8326716: JVMTI spec: clarify what nullptr means for C/C++ developers [v4]
> The following RFE was fixed recently: > [8324680](https://bugs.openjdk.org/browse/JDK-8324680): Replace NULL with > nullptr in JVMTI generated code > > It replaced all the `NULL`'s in the generated spec with`nullptr`. JVMTI > agents can be developed in C or C++. > This update is to make it clear that `nullptr` is C programming language > `null` pointer. > > I think we do not need a CSR for this fix. > > Testing: N/A (not needed) Serguei Spitsyn has updated the pull request incrementally with one additional commit since the last revision: review: more null pointer corrections - Changes: - all: https://git.openjdk.org/jdk/pull/19257/files - new: https://git.openjdk.org/jdk/pull/19257/files/4e1c48a1..48ba8f5d Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19257&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19257&range=02-03 Stats: 50 lines in 1 file changed: 0 ins; 0 del; 50 mod Patch: https://git.openjdk.org/jdk/pull/19257.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19257/head:pull/19257 PR: https://git.openjdk.org/jdk/pull/19257
Re: RFR: 8326716: JVMTI spec: clarify what nullptr means for C/C++ developers [v3]
On Fri, 31 May 2024 01:41:17 GMT, Serguei Spitsyn wrote: >> The following RFE was fixed recently: >> [8324680](https://bugs.openjdk.org/browse/JDK-8324680): Replace NULL with >> nullptr in JVMTI generated code >> >> It replaced all the `NULL`'s in the generated spec with`nullptr`. JVMTI >> agents can be developed in C or C++. >> This update is to make it clear that `nullptr` is C programming language >> `null` pointer. >> >> I think we do not need a CSR for this fix. >> >> Testing: N/A (not needed) > > Serguei Spitsyn has updated the pull request incrementally with one > additional commit since the last revision: > > review: replace nullptr with null pointer in the docs Thanks, David. I've done one more attempt to correct it. Please, let me know if it is wrong. - PR Comment: https://git.openjdk.org/jdk/pull/19257#issuecomment-2141435705
RFR: 8333130: MakeJAR2.sh uses hard-coded JDK version
Can I please get a review of this test-only change which addresses https://bugs.openjdk.org/browse/JDK-8333130? There are a couple of tests `NativeMethodPrefixApp` and `RetransformApp` under `test/jdk/java/lang/instrument/` which launch the app/test with a `-javaagent:` pointing to a test specific jar. The test, launched with that `javaagent`, then verifies the test specific details. In their current form, in order to construct the agent `.jar` and make it available to the jtreg test that's launched, they `@run` a jtreg `shell` test. This `shell` test uses the `MakeJAR2.sh` script to generate the jar file. The contents of the script is relatively straightforward - it compiles (using `javac`) some boot classpath classes, some agent specific classes and then generates a jar file with the agent classes and a `MANIFEST.MF` which points to the boot classpath classes along with test specific manifest attributes. In a recent PR the `MakeJAR2.sh` script was updated to pass `--enable-preview --release 23` since one of the existing agent class was updated to use the ClassFile API PreviewFeature (https://github.com/openjdk/jdk/pull/13009). This generated agent jar then is passed as `-javaagent:` to the subsequent `@run main` test which does the actual testing. So essentially the `shell` test which uses the `MakeJAR2.sh` is merely there to create a jar file that's needed by the actual test. Within the JDK we have several tests which compile other classes and generate jar files using in-process test libraries and the `java.util.spi.ToolProvider` implementations. These 2 tests too can be updated to use a similar technique, to completely get rid of the `MakeJAR2.sh`. Removing the `MakeJAR2.sh` will simplify the ability to pass around value for `--release` when using `--enable-preview`. The commit in this PR updates these 2 tests to use `@run driver` which then compiles relevant classes (maintaining the semantics of `MakeJAR2.sh`) and generates the agent jar file and then finally launching the test process. As part of the update, I did not retain the `@author` tag from the 2 test definitions, since it's my understanding that we no longer use those. I will add them back if we should retain them. The tests continue to pass locally with this change. I've also triggered tier testing in our CI for this change. - Commit messages: - 8333130: MakeJAR2.sh uses hard-coded JDK version Changes: https://git.openjdk.org/jdk/pull/19495/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=19495&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8333130 Stats: 409 lines in 5 files changed: 265 ins; 131 del; 13 mod Patch: https://git.openjdk.org/jdk/pull/19495.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19495/head:pull/19495 PR: https://git.openjdk.org/jdk/pull/19495
Re: RFR: 8333130: MakeJAR2.sh uses hard-coded JDK version
On Fri, 31 May 2024 09:01:27 GMT, Jaikiran Pai wrote: > Can I please get a review of this test-only change which addresses > https://bugs.openjdk.org/browse/JDK-8333130? > > There are a couple of tests `NativeMethodPrefixApp` and `RetransformApp` > under `test/jdk/java/lang/instrument/` which launch the app/test with a > `-javaagent:` pointing to a test specific jar. The test, launched with > that `javaagent`, then verifies the test specific details. > > In their current form, in order to construct the agent `.jar` and make it > available to the jtreg test that's launched, they `@run` a jtreg `shell` > test. This `shell` test uses the `MakeJAR2.sh` script to generate the jar > file. The contents of the script is relatively straightforward - it compiles > (using `javac`) some boot classpath classes, some agent specific classes and > then generates a jar file with the agent classes and a `MANIFEST.MF` which > points to the boot classpath classes along with test specific manifest > attributes. In a recent PR the `MakeJAR2.sh` script was updated to pass > `--enable-preview --release 23` since one of the existing agent class was > updated to use the ClassFile API PreviewFeature > (https://github.com/openjdk/jdk/pull/13009). > > This generated agent jar then is passed as `-javaagent:` to the subsequent > `@run main` test which does the actual testing. > > So essentially the `shell` test which uses the `MakeJAR2.sh` is merely there > to create a jar file that's needed by the actual test. Within the JDK we have > several tests which compile other classes and generate jar files using > in-process test libraries and the `java.util.spi.ToolProvider` > implementations. These 2 tests too can be updated to use a similar technique, > to completely get rid of the `MakeJAR2.sh`. Removing the `MakeJAR2.sh` will > simplify the ability to pass around value for `--release` when using > `--enable-preview`. > > The commit in this PR updates these 2 tests to use `@run driver` which then > compiles relevant classes (maintaining the semantics of `MakeJAR2.sh`) and > generates the agent jar file and then finally launching the test process. As > part of the update, I did not retain the `@author` tag from the 2 test > definitions, since it's my understanding that we no longer use those. I will > add them back if we should retain them. > > The tests continue to pass locally with this change. I've also triggered tier > testing in our CI for this change. Changes look reasonable. Great to see the removal of the need to execute a shell script as part of the test run - Marked as reviewed by lancea (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19495#pullrequestreview-2090554946
Re: RFR: 8333130: MakeJAR2.sh uses hard-coded JDK version [v2]
> Can I please get a review of this test-only change which addresses > https://bugs.openjdk.org/browse/JDK-8333130? > > There are a couple of tests `NativeMethodPrefixApp` and `RetransformApp` > under `test/jdk/java/lang/instrument/` which launch the app/test with a > `-javaagent:` pointing to a test specific jar. The test, launched with > that `javaagent`, then verifies the test specific details. > > In their current form, in order to construct the agent `.jar` and make it > available to the jtreg test that's launched, they `@run` a jtreg `shell` > test. This `shell` test uses the `MakeJAR2.sh` script to generate the jar > file. The contents of the script is relatively straightforward - it compiles > (using `javac`) some boot classpath classes, some agent specific classes and > then generates a jar file with the agent classes and a `MANIFEST.MF` which > points to the boot classpath classes along with test specific manifest > attributes. In a recent PR the `MakeJAR2.sh` script was updated to pass > `--enable-preview --release 23` since one of the existing agent class was > updated to use the ClassFile API PreviewFeature > (https://github.com/openjdk/jdk/pull/13009). > > This generated agent jar then is passed as `-javaagent:` to the subsequent > `@run main` test which does the actual testing. > > So essentially the `shell` test which uses the `MakeJAR2.sh` is merely there > to create a jar file that's needed by the actual test. Within the JDK we have > several tests which compile other classes and generate jar files using > in-process test libraries and the `java.util.spi.ToolProvider` > implementations. These 2 tests too can be updated to use a similar technique, > to completely get rid of the `MakeJAR2.sh`. Removing the `MakeJAR2.sh` will > simplify the ability to pass around value for `--release` when using > `--enable-preview`. > > The commit in this PR updates these 2 tests to use `@run driver` which then > compiles relevant classes (maintaining the semantics of `MakeJAR2.sh`) and > generates the agent jar file and then finally launching the test process. As > part of the update, I did not retain the `@author` tag from the 2 test > definitions, since it's my understanding that we no longer use those. I will > add them back if we should retain them. > > The tests continue to pass locally with this change. I've also triggered tier > testing in our CI for this change. Jaikiran Pai has updated the pull request incrementally with one additional commit since the last revision: fix Boot-Class-Path value for Windows - Changes: - all: https://git.openjdk.org/jdk/pull/19495/files - new: https://git.openjdk.org/jdk/pull/19495/files/a792b964..88997f4f Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19495&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19495&range=00-01 Stats: 10 lines in 2 files changed: 4 ins; 0 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/19495.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19495/head:pull/19495 PR: https://git.openjdk.org/jdk/pull/19495
Re: RFR: 8330846: Add stacks of mounted virtual threads to the HotSpot thread dump
On Thu, 30 May 2024 16:39:03 GMT, Alan Bateman wrote: >> Print the stack traces of mounted virtual threads when calling `jcmd >> Thread.print`. > > Thanks for take this one. Here's the result with the changes in 1a75277e. > > "ForkJoinPool-1-worker-1" #25 [33795] daemon prio=5 os_prio=31 cpu=46574.42ms > elapsed=47.15s tid=0x7f81670d1a00 [0x7e9a4000] >Carrying virtual thread #24 > at jdk.internal.vm.Continuation.run(java.base/Continuation.java:262) > at > java.lang.VirtualThread.runContinuation(java.base/VirtualThread.java:283) > at > java.lang.VirtualThread$$Lambda/0x0001220b2868.run(java.base/Unknown > Source) > at > java.util.concurrent.ForkJoinTask$RunnableExecuteAction.compute(java.base/ForkJoinTask.java:1726) > at > java.util.concurrent.ForkJoinTask$RunnableExecuteAction.compute(java.base/ForkJoinTask.java:1717) > at > java.util.concurrent.ForkJoinTask$InterruptibleTask.exec(java.base/ForkJoinTask.java:1641) > at > java.util.concurrent.ForkJoinTask.doExec(java.base/ForkJoinTask.java:507) > at > java.util.concurrent.ForkJoinPool$WorkQueue.topLevelExec(java.base/ForkJoinPool.java:1455) > at > java.util.concurrent.ForkJoinPool.runWorker(java.base/ForkJoinPool.java:2031) > at > java.util.concurrent.ForkJoinWorkerThread.run(java.base/ForkJoinWorkerThread.java:189) >Carrying virtual thread #24 > at Main.lambda$main$0(Main.java:7) > at java.lang.VirtualThread.run(java.base/VirtualThread.java:381) > > > Note that the line "Carrying virtual thread #24" is printed twice. Also it's > not immediately clear that there are two stack traces. > > You'll likely get different opinions on how mounted virtual threads should be > presented. A few things to try > - indent the stack trace of the mounted virtual thread > - list the mounted virtual threads at the end Thanks for your comments @AlanBateman and @dholmes-ora ! - The format proposed by @dholmes-ora definitely makes sense and I'll of course drop the duplicated "Carrying" statement. - Regarding using `JavaThread::print_vthread_stack_on`, I agree that it'd be good to use it instead of creating a new way of printing stack traces. However `print_vthread_stack_on` does not have the `const` modifier so it cannot be called directly from `Thread.print_on`. Making `print_vthread_stack_on` more strict, seems to be fairly complicated and I don't know if I'd be OK with relaxing the `const` constraint in `Thread.print_on` for the purpose of reusing `print_vthread_stack_on` - PR Comment: https://git.openjdk.org/jdk/pull/19482#issuecomment-2141946811
RFR: 8333353: Delete extra empty line in CodeBlob.java
Hi all, This trivial fix, delete the extra empty line before `getOopMaps` function in `src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/code/CodeBlob.java` file. No risk. - Commit messages: - 853: Delete extra empty line in CodeBlob.java Changes: https://git.openjdk.org/jdk/pull/19499/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=19499&range=00 Issue: https://bugs.openjdk.org/browse/JDK-853 Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/19499.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19499/head:pull/19499 PR: https://git.openjdk.org/jdk/pull/19499
Re: RFR: 8330846: Add stacks of mounted virtual threads to the HotSpot thread dump
On Fri, 31 May 2024 02:07:25 GMT, David Holmes wrote: >> Print the stack traces of mounted virtual threads when calling `jcmd >> Thread.print`. > > I'd probably give preference to the stack of the virtual thread, as the stack > of the carrier when a vthread is mounted is generally quite uninteresting: > > "ForkJoinPool-1-worker-1" #25 [33795] daemon prio=5 os_prio=31 cpu=46574.42ms > elapsed=47.15s tid=0x7f81670d1a00 [0x7e9a4000] >Carrying virtual thread #24 > at Main.lambda$main$0(Main.java:7) > at java.lang.VirtualThread.run(java.base/VirtualThread.java:381) > at jdk.internal.vm.Continuation.run(java.base/Continuation.java:262) > at > java.lang.VirtualThread.runContinuation(java.base/VirtualThread.java:283) > at > java.lang.VirtualThread$$Lambda/0x0001220b2868.run(java.base/Unknown > Source) > at > java.util.concurrent.ForkJoinTask$RunnableExecuteAction.compute(java.base/ForkJoinTask.java:1726) > at > java.util.concurrent.ForkJoinTask$RunnableExecuteAction.compute(java.base/ForkJoinTask.java:1717) > at > java.util.concurrent.ForkJoinTask$InterruptibleTask.exec(java.base/ForkJoinTask.java:1641) > at > java.util.concurrent.ForkJoinTask.doExec(java.base/ForkJoinTask.java:507) > at > java.util.concurrent.ForkJoinPool$WorkQueue.topLevelExec(java.base/ForkJoinPool.java:1455) > at > java.util.concurrent.ForkJoinPool.runWorker(java.base/ForkJoinPool.java:2031) > at > java.util.concurrent.ForkJoinWorkerThread.run(java.base/ForkJoinWorkerThread.java:189) > * The format proposed by @dholmes-ora definitely makes sense You will different get different opinions on how it is presented. Can you also try putting a new section that lists the mounted virtual threads and their stack trace. If the virtual thread has a name then it can be shown too. - PR Comment: https://git.openjdk.org/jdk/pull/19482#issuecomment-2142000785
Stepping in debugger switches to interpretation mode
Dear Sir/Madam, I encountered a problem while debugging the code. I am attaching the reproducer to this email in the* Main.java file*. When running it with the debugger without stepping, the application runs in less than a second (see jdb output in the *jdb_run.txt *file). However, after performing a single step, the application is running in interpretation mode, becoming very slow (see jdb output in the *jdb_step.txt* file). I assume running in the interpreter mode, as I see *InterpreterRuntime::post_method_exit* calls in the profiler. Could you please help me figure out what causes the application to run in the interpreter mode? Is this a bug or an expected behavior? Are there any ways to work around this issue? Best regards, Maksim Zuev Software developer at JetBrains Initializing jdb ... > stop at Main:7 Deferring breakpoint Main:7. It will be set after the class is loaded. > run run Main Set uncaught java.lang.Throwable Set deferred uncaught java.lang.Throwable > VM Started: Set deferred breakpoint Main:7 Breakpoint hit: "thread=main", Main.main(), line=7 bci=0 7int x = 1; main[1] cont > 3cc8b2e70ead788fba06f607b827bd8dcb06c6b3b234578b1200b793c75ef999 173ms The application exited Main.java Description: Binary data Initializing jdb ... > stop at Main:7 Deferring breakpoint Main:7. It will be set after the class is loaded. > run run Main Set uncaught java.lang.Throwable Set deferred uncaught java.lang.Throwable > VM Started: Set deferred breakpoint Main:7 Breakpoint hit: "thread=main", Main.main(), line=7 bci=0 7int x = 1; main[1] next > Step completed: "thread=main", Main.main(), line=9 bci=2 9long start = System.currentTimeMillis(); main[1] cont > 3cc8b2e70ead788fba06f607b827bd8dcb06c6b3b234578b1200b793c75ef999 51212ms The application exited
Re: Stepping in debugger switches to interpretation mode
On Fri, 2024-05-31 at 14:44 +0200, Maksim Zuev wrote: > Dear Sir/Madam, > > I encountered a problem while debugging the code. I am attaching the > reproducer to this email in the Main.java file. > > When running it with the debugger without stepping, the application > runs in less than a second (see jdb output in the jdb_run.txt file). > However, after performing a single step, the application is running > in interpretation mode, becoming very slow (see jdb output in the > jdb_step.txt file). > > I assume running in the interpreter mode, as I > see InterpreterRuntime::post_method_exit > calls in the profiler. > > Could you please help me figure out what causes the application to > run in the interpreter mode? Is this a bug or an expected behavior? > Are there any ways to work around this issue? I think this is: https://bugs.openjdk.org/browse/JDK-8229012 Thanks, Severin
Re: RFR: 8333301: Remove static builds using --enable-static-build
On Thu, 30 May 2024 19:14:43 GMT, Magnus Ihse Bursie wrote: > The original way of building static libraries in the JDK was to use the > configure argument --enable-static-build, which set the value of the make > variable STATIC_BUILD. (Note that this is not the same as the source code > definition STATIC_BUILD, which will be set by other means as well.) > > This method only ever worked on macOS, and has long since stopped working. It > was originally introduced for the Mobile Project, but I've now learn that not > even they use it anymore. > > It just adds clutter to the build system, and should be removed. Looks OK to me. - Marked as reviewed by sgehwolf (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19487#pullrequestreview-2091001402
Re: RFR: 8332785: Replace naked uses of UseSharedSpaces with CDSConfig::is_using_archive [v2]
> Hi folks, > > This PR addresses [8332785](https://bugs.openjdk.org/browse/JDK-8332785) > replacing all naked uses for ```UseSharedSpaces``` with > ```CDSConfig::is_using_archive```. > > Testing: > - [x] Tier 1 with GHA. > > Thanks, > Sonia Sonia Zaldana Calles has updated the pull request incrementally with one additional commit since the last revision: Updating copyright headers and unnecessary CDSConfig:: - Changes: - all: https://git.openjdk.org/jdk/pull/19463/files - new: https://git.openjdk.org/jdk/pull/19463/files/f3e6b17c..ba4c0032 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19463&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19463&range=00-01 Stats: 15 lines in 15 files changed: 0 ins; 0 del; 15 mod Patch: https://git.openjdk.org/jdk/pull/19463.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19463/head:pull/19463 PR: https://git.openjdk.org/jdk/pull/19463
Re: RFR: 8333353: Delete extra empty line in CodeBlob.java
On Fri, 31 May 2024 12:29:59 GMT, SendaoYan wrote: > Hi all, > This trivial fix, delete the extra empty line before `getOopMaps` function > in `src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/code/CodeBlob.java` > file. > No risk. The GHA test runner report a failure `serviceability/jvmti/ObjectMonitorUsage/ObjectMonitorUsage.java`, which has been recorded in [JDK-8332923](https://bugs.openjdk.org/browse/JDK-8332923), unralated to this issue. - PR Comment: https://git.openjdk.org/jdk/pull/19499#issuecomment-2142586291
Re: Stepping in debugger switches to interpretation mode
On 5/31/24 7:25 AM, Severin Gehwolf wrote: On Fri, 2024-05-31 at 14:44 +0200, Maksim Zuev wrote: Dear Sir/Madam, I encountered a problem while debugging the code. I am attaching the reproducer to this email in the Main.java file. When running it with the debugger without stepping, the application runs in less than a second (see jdb output in the jdb_run.txt file). However, after performing a single step, the application is running in interpretation mode, becoming very slow (see jdb output in the jdb_step.txt file). I assume running in the interpreter mode, as I see InterpreterRuntime::post_method_exit calls in the profiler. Could you please help me figure out what causes the application to run in the interpreter mode? Is this a bug or an expected behavior? Are there any ways to work around this issue? I think this is: https://bugs.openjdk.org/browse/JDK-8229012 Thanks, Severin Yes, that seems likely. It should resolve itself once you exit the method you were single stepping in. But as mentioned in the bug, if you single step in the main method of a thread, that thread will forever run interpreted. Chris
Re: RFR: 8333353: Delete extra empty line in CodeBlob.java
On Fri, 31 May 2024 12:29:59 GMT, SendaoYan wrote: > Hi all, > This trivial fix, delete the extra empty line before `getOopMaps` function > in `src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/code/CodeBlob.java` > file. > No risk. Approved and trivial. - Marked as reviewed by cjplummer (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19499#pullrequestreview-2091269784
Re: Stepping in debugger switches to interpretation mode
On Fri, 2024-05-31 at 14:44 +0200, Maksim Zuev wrote: > Dear Sir/Madam, > > I encountered a problem while debugging the code. I am attaching the > reproducer to this email in the Main.java file. > > When running it with the debugger without stepping, the application > runs in less than a second (see jdb output in the jdb_run.txt file). > However, after performing a single step, the application is running > in interpretation mode, becoming very slow (see jdb output in the > jdb_step.txt file). > > I assume running in the interpreter mode, as I > see InterpreterRuntime::post_method_exit > calls in the profiler. > > Could you please help me figure out what causes the application to > run in the interpreter mode? Is this a bug or an expected behavior? > Are there any ways to work around this issue? There is also a RHEL ticket, maybe more people can comment in the RHEL tracker, than on the JDK ticket: https://issues.redhat.com/browse/RHEL-3498 There is more related discussion too. Best regards, Simeon On Fri, 31 May 2024 at 18:49, Severin Gehwolf wrote: > On Fri, 2024-05-31 at 14:44 +0200, Maksim Zuev wrote: > > Dear Sir/Madam, > > > > I encountered a problem while debugging the code. I am attaching the > > reproducer to this email in the Main.java file. > > > > When running it with the debugger without stepping, the application > > runs in less than a second (see jdb output in the jdb_run.txt file). > > However, after performing a single step, the application is running > > in interpretation mode, becoming very slow (see jdb output in the > > jdb_step.txt file). > > > > I assume running in the interpreter mode, as I > > see InterpreterRuntime::post_method_exit > > calls in the profiler. > > > > Could you please help me figure out what causes the application to > > run in the interpreter mode? Is this a bug or an expected behavior? > > Are there any ways to work around this issue? > > I think this is: > https://bugs.openjdk.org/browse/JDK-8229012 > > Thanks, > Severin > >
Re: RFR: 8333353: Delete extra empty line in CodeBlob.java
On Fri, 31 May 2024 16:30:55 GMT, Chris Plummer wrote: > Approved and trivial. Thanks for the review and approved. - PR Comment: https://git.openjdk.org/jdk/pull/19499#issuecomment-2142650762
Re: RFR: 8333301: Remove static builds using --enable-static-build
On Thu, 30 May 2024 19:14:43 GMT, Magnus Ihse Bursie wrote: > The original way of building static libraries in the JDK was to use the > configure argument --enable-static-build, which set the value of the make > variable STATIC_BUILD. (Note that this is not the same as the source code > definition STATIC_BUILD, which will be set by other means as well.) > > This method only ever worked on macOS, and has long since stopped working. It > was originally introduced for the Mobile Project, but I've now learn that not > even they use it anymore. > > It just adds clutter to the build system, and should be removed. Marked as reviewed by erikj (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19487#pullrequestreview-2091321250
Re: RFR: 8333130: MakeJAR2.sh uses hard-coded JDK version [v2]
On Fri, 31 May 2024 12:01:14 GMT, Jaikiran Pai wrote: >> Can I please get a review of this test-only change which addresses >> https://bugs.openjdk.org/browse/JDK-8333130? >> >> There are a couple of tests `NativeMethodPrefixApp` and `RetransformApp` >> under `test/jdk/java/lang/instrument/` which launch the app/test with a >> `-javaagent:` pointing to a test specific jar. The test, launched with >> that `javaagent`, then verifies the test specific details. >> >> In their current form, in order to construct the agent `.jar` and make it >> available to the jtreg test that's launched, they `@run` a jtreg `shell` >> test. This `shell` test uses the `MakeJAR2.sh` script to generate the jar >> file. The contents of the script is relatively straightforward - it compiles >> (using `javac`) some boot classpath classes, some agent specific classes and >> then generates a jar file with the agent classes and a `MANIFEST.MF` which >> points to the boot classpath classes along with test specific manifest >> attributes. In a recent PR the `MakeJAR2.sh` script was updated to pass >> `--enable-preview --release 23` since one of the existing agent class was >> updated to use the ClassFile API PreviewFeature >> (https://github.com/openjdk/jdk/pull/13009). >> >> This generated agent jar then is passed as `-javaagent:` to the subsequent >> `@run main` test which does the actual testing. >> >> So essentially the `shell` test which uses the `MakeJAR2.sh` is merely there >> to create a jar file that's needed by the actual test. Within the JDK we >> have several tests which compile other classes and generate jar files using >> in-process test libraries and the `java.util.spi.ToolProvider` >> implementations. These 2 tests too can be updated to use a similar >> technique, to completely get rid of the `MakeJAR2.sh`. Removing the >> `MakeJAR2.sh` will simplify the ability to pass around value for `--release` >> when using `--enable-preview`. >> >> The commit in this PR updates these 2 tests to use `@run driver` which then >> compiles relevant classes (maintaining the semantics of `MakeJAR2.sh`) and >> generates the agent jar file and then finally launching the test process. As >> part of the update, I did not retain the `@author` tag from the 2 test >> definitions, since it's my understanding that we no longer use those. I will >> add them back if we should retain them. >> >> The tests continue to pass locally with this change. I've also triggered >> tier testing in our CI for this change. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > fix Boot-Class-Path value for Windows Marked as reviewed by darcy (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19495#pullrequestreview-2091336086
Integrated: 8320215: HeapDumper can use DumpWriter buffer during merge
On Fri, 19 Apr 2024 00:10:12 GMT, Alex Menkov wrote: > The fix updates HeapMerger to use writer buffer (no need to copy memory, also > writer buffer is 1MB instead of 4KB). > Additionally fixed small issue in FileWriter (looks like `ssize_t` instead of > `size_t` is a typo, the argument should be unsigned) > > Testing: all HeapDump-related tests on Oracle supported platforms This pull request has now been integrated. Changeset: e4fbb15c Author:Alex Menkov URL: https://git.openjdk.org/jdk/commit/e4fbb15c6a7b18f1ec66176080404818d3871194 Stats: 22 lines in 3 files changed: 9 ins; 4 del; 9 mod 8320215: HeapDumper can use DumpWriter buffer during merge Reviewed-by: sspitsyn, yyang - PR: https://git.openjdk.org/jdk/pull/18850
Re: RFR: 8333130: MakeJAR2.sh uses hard-coded JDK version [v2]
On Fri, 31 May 2024 12:01:14 GMT, Jaikiran Pai wrote:
>> Can I please get a review of this test-only change which addresses
>> https://bugs.openjdk.org/browse/JDK-8333130?
>>
>> There are a couple of tests `NativeMethodPrefixApp` and `RetransformApp`
>> under `test/jdk/java/lang/instrument/` which launch the app/test with a
>> `-javaagent:` pointing to a test specific jar. The test, launched with
>> that `javaagent`, then verifies the test specific details.
>>
>> In their current form, in order to construct the agent `.jar` and make it
>> available to the jtreg test that's launched, they `@run` a jtreg `shell`
>> test. This `shell` test uses the `MakeJAR2.sh` script to generate the jar
>> file. The contents of the script is relatively straightforward - it compiles
>> (using `javac`) some boot classpath classes, some agent specific classes and
>> then generates a jar file with the agent classes and a `MANIFEST.MF` which
>> points to the boot classpath classes along with test specific manifest
>> attributes. In a recent PR the `MakeJAR2.sh` script was updated to pass
>> `--enable-preview --release 23` since one of the existing agent class was
>> updated to use the ClassFile API PreviewFeature
>> (https://github.com/openjdk/jdk/pull/13009).
>>
>> This generated agent jar then is passed as `-javaagent:` to the subsequent
>> `@run main` test which does the actual testing.
>>
>> So essentially the `shell` test which uses the `MakeJAR2.sh` is merely there
>> to create a jar file that's needed by the actual test. Within the JDK we
>> have several tests which compile other classes and generate jar files using
>> in-process test libraries and the `java.util.spi.ToolProvider`
>> implementations. These 2 tests too can be updated to use a similar
>> technique, to completely get rid of the `MakeJAR2.sh`. Removing the
>> `MakeJAR2.sh` will simplify the ability to pass around value for `--release`
>> when using `--enable-preview`.
>>
>> The commit in this PR updates these 2 tests to use `@run driver` which then
>> compiles relevant classes (maintaining the semantics of `MakeJAR2.sh`) and
>> generates the agent jar file and then finally launching the test process. As
>> part of the update, I did not retain the `@author` tag from the 2 test
>> definitions, since it's my understanding that we no longer use those. I will
>> add them back if we should retain them.
>>
>> The tests continue to pass locally with this change. I've also triggered
>> tier testing in our CI for this change.
>
> Jaikiran Pai has updated the pull request incrementally with one additional
> commit since the last revision:
>
> fix Boot-Class-Path value for Windows
Changes requested by lmesnik (Reviewer).
test/jdk/java/lang/instrument/NativeMethodPrefixApp.java line 168:
> 166:
> 167: private static void launchApp(final Path agentJar) throws Exception {
> 168: final OutputAnalyzer oa = ProcessTools.executeLimitedTestJava(
Is there any reason the test uses 'executeLimitedTestJava' and not
'executeTestJava'?
It might make sense to run it with different VM flags.
-
PR Review: https://git.openjdk.org/jdk/pull/19495#pullrequestreview-2091382349
PR Review Comment: https://git.openjdk.org/jdk/pull/19495#discussion_r1622751369
Re: RFR: 8333353: Delete extra empty line in CodeBlob.java
On Fri, 31 May 2024 12:29:59 GMT, SendaoYan wrote: > Hi all, > This trivial fix, delete the extra empty line before `getOopMaps` function > in `src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/code/CodeBlob.java` > file. > No risk. Marked as reviewed by lmesnik (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19499#pullrequestreview-2091383870
RFR: 8307824: Clean up Finalizable.java and finalize terminology in vmTestbase/nsk/share
The fix removes finalization cleanup from vmTestbase. The last to classes that use it are: DebugeeBinder and SocketIOPipe. The DebugeeBinder is used in jdi and jdwp tests and is always linked with debuggee process. So the DebugeeProcess.waitFor() is the good place to close binder and free all it's resources. The SocketIOPipe is used directly in AOD tests where it should be closed after test execution. The OPipe (child of SocketIOPipe) also used in jdi and jdwp tests where it is connected directly in tests. However is also connected with debuggee and could be closed in DebugeeProcess.waitFor(). The VMOutOfMemoryException001 test is fixed to release some memory after throwing OOME so Sytem.exit() could complete successfully. Previously some memory freed during VM shutdown hook. I verified that cleanup printed that corresponding 'close' method has been already called before VM shutdown phase for debugger process. Additionally, run all vmTestbase tests to verify there are no failures, - Commit messages: - removed finalization Changes: https://git.openjdk.org/jdk/pull/19505/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=19505&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8307824 Stats: 291 lines in 10 files changed: 14 ins; 271 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/19505.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19505/head:pull/19505 PR: https://git.openjdk.org/jdk/pull/19505
Re: RFR: 8307824: Clean up Finalizable.java and finalize terminology in vmTestbase/nsk/share
On Fri, 31 May 2024 18:22:47 GMT, Leonid Mesnik wrote:
> The fix removes finalization cleanup from vmTestbase.
> The last to classes that use it are: DebugeeBinder and SocketIOPipe.
> The DebugeeBinder is used in jdi and jdwp tests and is always linked with
> debuggee process. So the DebugeeProcess.waitFor() is the good place to close
> binder and free all it's resources.
> The SocketIOPipe is used directly in AOD tests where it should be closed
> after test execution.
>
> The OPipe (child of SocketIOPipe) also used in jdi and jdwp tests where it is
> connected directly in tests. However is also connected with debuggee and
> could be closed in DebugeeProcess.waitFor().
>
> The VMOutOfMemoryException001 test is fixed to release some memory after
> throwing OOME so Sytem.exit() could complete successfully. Previously some
> memory freed during VM shutdown hook.
>
> I verified that cleanup printed that corresponding 'close' method has been
> already called before VM shutdown phase for debugger process.
> Additionally, run all vmTestbase tests to verify there are no failures,
test/hotspot/jtreg/vmTestbase/nsk/share/aod/DummyTargetApplication.java line 68:
> 66: if ((signal == null) ||
> !signal.equals(AODTestRunner.SIGNAL_FINISH))
> 67: throw new TestBug("Unexpected signal: '" + signal + "'");
> 68: pipe.close();
Exceptions can be thrown before you get here.
test/hotspot/jtreg/vmTestbase/nsk/share/jpda/DebugeeProcess.java line 215:
> 213: if (binder != null) {
> 214: binder.close();
> 215: }
Won't this be skipped if there is an exception during `waitForDebugee` or
`waitForRedirectors`?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19505#discussion_r1622898244
PR Review Comment: https://git.openjdk.org/jdk/pull/19505#discussion_r1622896237
Re: RFR: 8332923: ObjectMonitorUsage.java failed with unexpected waiter_count [v3]
On Thu, 30 May 2024 06:36:09 GMT, SendaoYan wrote: >> SendaoYan has updated the pull request incrementally with one additional >> commit since the last revision: >> >> change from java_lang_VirtualThread::is_instance(thread_oop) to >> hread_oop->is_a(vmClasses::BaseVirtualThread_klass()) in >> Threads::get_pending_threads() > > The GHA test runner report a intermittent failure > `ToolTabSnippetTest.testCleaningCompletionTODO(): failure`, which has been > recorded in [JDK-8287078](https://bugs.openjdk.org/browse/JDK-8287078), I > think it's unrelated to this PR. @sendaoYan The serviceability fixes require two reviews. Please, wait for a second reviewer before integration. - PR Comment: https://git.openjdk.org/jdk/pull/19405#issuecomment-2142964196
Re: RFR: 8333130: MakeJAR2.sh uses hard-coded JDK version [v2]
On Fri, 31 May 2024 12:01:14 GMT, Jaikiran Pai wrote: >> Can I please get a review of this test-only change which addresses >> https://bugs.openjdk.org/browse/JDK-8333130? >> >> There are a couple of tests `NativeMethodPrefixApp` and `RetransformApp` >> under `test/jdk/java/lang/instrument/` which launch the app/test with a >> `-javaagent:` pointing to a test specific jar. The test, launched with >> that `javaagent`, then verifies the test specific details. >> >> In their current form, in order to construct the agent `.jar` and make it >> available to the jtreg test that's launched, they `@run` a jtreg `shell` >> test. This `shell` test uses the `MakeJAR2.sh` script to generate the jar >> file. The contents of the script is relatively straightforward - it compiles >> (using `javac`) some boot classpath classes, some agent specific classes and >> then generates a jar file with the agent classes and a `MANIFEST.MF` which >> points to the boot classpath classes along with test specific manifest >> attributes. In a recent PR the `MakeJAR2.sh` script was updated to pass >> `--enable-preview --release 23` since one of the existing agent class was >> updated to use the ClassFile API PreviewFeature >> (https://github.com/openjdk/jdk/pull/13009). >> >> This generated agent jar then is passed as `-javaagent:` to the subsequent >> `@run main` test which does the actual testing. >> >> So essentially the `shell` test which uses the `MakeJAR2.sh` is merely there >> to create a jar file that's needed by the actual test. Within the JDK we >> have several tests which compile other classes and generate jar files using >> in-process test libraries and the `java.util.spi.ToolProvider` >> implementations. These 2 tests too can be updated to use a similar >> technique, to completely get rid of the `MakeJAR2.sh`. Removing the >> `MakeJAR2.sh` will simplify the ability to pass around value for `--release` >> when using `--enable-preview`. >> >> The commit in this PR updates these 2 tests to use `@run driver` which then >> compiles relevant classes (maintaining the semantics of `MakeJAR2.sh`) and >> generates the agent jar file and then finally launching the test process. As >> part of the update, I did not retain the `@author` tag from the 2 test >> definitions, since it's my understanding that we no longer use those. I will >> add them back if we should retain them. >> >> The tests continue to pass locally with this change. I've also triggered >> tier testing in our CI for this change. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > fix Boot-Class-Path value for Windows Ping @sspitsyn - who handles JLI/JPLIS reviews for the Serviceability team? - PR Comment: https://git.openjdk.org/jdk/pull/19495#issuecomment-2142976174
Re: RFR: 8333130: MakeJAR2.sh uses hard-coded JDK version [v2]
On Fri, 31 May 2024 12:01:14 GMT, Jaikiran Pai wrote:
>> Can I please get a review of this test-only change which addresses
>> https://bugs.openjdk.org/browse/JDK-8333130?
>>
>> There are a couple of tests `NativeMethodPrefixApp` and `RetransformApp`
>> under `test/jdk/java/lang/instrument/` which launch the app/test with a
>> `-javaagent:` pointing to a test specific jar. The test, launched with
>> that `javaagent`, then verifies the test specific details.
>>
>> In their current form, in order to construct the agent `.jar` and make it
>> available to the jtreg test that's launched, they `@run` a jtreg `shell`
>> test. This `shell` test uses the `MakeJAR2.sh` script to generate the jar
>> file. The contents of the script is relatively straightforward - it compiles
>> (using `javac`) some boot classpath classes, some agent specific classes and
>> then generates a jar file with the agent classes and a `MANIFEST.MF` which
>> points to the boot classpath classes along with test specific manifest
>> attributes. In a recent PR the `MakeJAR2.sh` script was updated to pass
>> `--enable-preview --release 23` since one of the existing agent class was
>> updated to use the ClassFile API PreviewFeature
>> (https://github.com/openjdk/jdk/pull/13009).
>>
>> This generated agent jar then is passed as `-javaagent:` to the subsequent
>> `@run main` test which does the actual testing.
>>
>> So essentially the `shell` test which uses the `MakeJAR2.sh` is merely there
>> to create a jar file that's needed by the actual test. Within the JDK we
>> have several tests which compile other classes and generate jar files using
>> in-process test libraries and the `java.util.spi.ToolProvider`
>> implementations. These 2 tests too can be updated to use a similar
>> technique, to completely get rid of the `MakeJAR2.sh`. Removing the
>> `MakeJAR2.sh` will simplify the ability to pass around value for `--release`
>> when using `--enable-preview`.
>>
>> The commit in this PR updates these 2 tests to use `@run driver` which then
>> compiles relevant classes (maintaining the semantics of `MakeJAR2.sh`) and
>> generates the agent jar file and then finally launching the test process. As
>> part of the update, I did not retain the `@author` tag from the 2 test
>> definitions, since it's my understanding that we no longer use those. I will
>> add them back if we should retain them.
>>
>> The tests continue to pass locally with this change. I've also triggered
>> tier testing in our CI for this change.
>
> Jaikiran Pai has updated the pull request incrementally with one additional
> commit since the last revision:
>
> fix Boot-Class-Path value for Windows
The fix looks over-complicated.
I think it can be done by something like:
- `@build NativeMethodPrefixAgent bootreporter.StringIdCallback
bootreporter.StringIdCallbackReporter` (maybe `@compile` to specify `--release`)
- use jdk.test.lib.helpers.ClassFileInstaller:
ClassFileInstaller.writeJar("NativeMethodPrefixAgent.jar",
ClassFileInstaller.Manifest.fromString(manifest),
"NativeMethodPrefixAgent",
"asmlib.Instrumentor",
"bootreporter.StringIdCallback",
"bootreporter.StringIdCallbackReporter");
-
PR Comment: https://git.openjdk.org/jdk/pull/19495#issuecomment-2143117731
Re: RFR: 8311177: Switching to interpreter only mode in carrier thread can lead to crashes [v3]
> Please, review the following `interp-only` issue related to carrier threads. > There are 3 problems fixed here: > - The `EnterInterpOnlyModeClosure::do_threads` is taking the > `JvmtiThreadState` with the `jt->jvmti_thread_state()` which is incorrect > when we have a deal with a carrier thread. The target state is known at the > point when the `HandshakeClosure` is set, so the fix is to pass it as a > constructor parameter. > - The `state->is_pending_interp_only_mode())` was processed at mounts only > but it has to be processed for unmounts as well. > - The test > `test/hotspot/jtreg/serviceability/jvmti/vthread/MethodExitTest/libMethodExitTest.cpp` > has a wrong assumption that there can't be `MethodExit` event on the carrier > thread when the function `breakpoint_hit1` is being executed. However, it can > happen if the virtual thread gets unmounted. > > The fix also includes new test case `vthread/CarrierThreadEventNotification` > developed by Patricio. > > Testing: > - Ran new test case locally > - Ran mach5 tiers 1-6 Serguei Spitsyn has updated the pull request incrementally with one additional commit since the last revision: review: refactored def and use of process_pending_interp_only() - Changes: - all: https://git.openjdk.org/jdk/pull/19438/files - new: https://git.openjdk.org/jdk/pull/19438/files/2f75975f..19e4d8fa Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19438&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19438&range=01-02 Stats: 36 lines in 4 files changed: 16 ins; 18 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/19438.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19438/head:pull/19438 PR: https://git.openjdk.org/jdk/pull/19438
Re: RFR: 8311177: Switching to interpreter only mode in carrier thread can lead to crashes [v2]
On Thu, 30 May 2024 18:59:10 GMT, Patricio Chilano Mateo wrote: >> Serguei Spitsyn has updated the pull request incrementally with one >> additional commit since the last revision: >> >> review: addressed nits in new test > > src/hotspot/share/prims/jvmtiThreadState.cpp line 674: > >> 672: } >> 673: // enable interp_only_mode for carrier thread if it has pending bit >> 674: process_pending_interp_only(thread); > > So for the last unmount case we will call this before doing the JVMTI state > rebinding, but shouldn't it be called after it in VTMS_vthread_end? Actually > why not moving this call inside rebind_to_jvmti_thread_state_of()? Thank you for the comment! I was also thinking about placing it to the `rebind_to_jvmti_thread_state_of()`. I've made and pushed this change now. - PR Review Comment: https://git.openjdk.org/jdk/pull/19438#discussion_r1623046562
Re: RFR: 8333130: MakeJAR2.sh uses hard-coded JDK version [v2]
On Fri, 31 May 2024 21:04:41 GMT, Daniel D. Daugherty wrote: > Ping @sspitsyn - who handles JLI/JPLIS reviews for the Serviceability team? Sorry for the latency. Will start reviewing it today. Also, I see Alex is reviewing it. - PR Comment: https://git.openjdk.org/jdk/pull/19495#issuecomment-2143136005
Re: RFR: 8311177: Switching to interpreter only mode in carrier thread can lead to crashes [v3]
On Fri, 31 May 2024 23:55:20 GMT, Serguei Spitsyn wrote:
>> Please, review the following `interp-only` issue related to carrier threads.
>> There are 3 problems fixed here:
>> - The `EnterInterpOnlyModeClosure::do_threads` is taking the
>> `JvmtiThreadState` with the `jt->jvmti_thread_state()` which is incorrect
>> when we have a deal with a carrier thread. The target state is known at the
>> point when the `HandshakeClosure` is set, so the fix is to pass it as a
>> constructor parameter.
>> - The `state->is_pending_interp_only_mode())` was processed at mounts only
>> but it has to be processed for unmounts as well.
>> - The test
>> `test/hotspot/jtreg/serviceability/jvmti/vthread/MethodExitTest/libMethodExitTest.cpp`
>> has a wrong assumption that there can't be `MethodExit` event on the
>> carrier thread when the function `breakpoint_hit1` is being executed.
>> However, it can happen if the virtual thread gets unmounted.
>>
>> The fix also includes new test case
>> `vthread/CarrierThreadEventNotification` developed by Patricio.
>>
>> Testing:
>> - Ran new test case locally
>> - Ran mach5 tiers 1-6
>
> Serguei Spitsyn has updated the pull request incrementally with one
> additional commit since the last revision:
>
> review: refactored def and use of process_pending_interp_only()
test/hotspot/jtreg/serviceability/jvmti/vthread/CarrierThreadEventNotification/libCarrierThreadEventNotification.cpp
line 40:
> 38:
> 39: static const char* CTHREAD_NAME_START = "ForkJoinPool";
> 40: static const size_t CTHREAD_NAME_START_LEN = (int)strlen("ForkJoinPool");
`(int)` cast is not needed
test/hotspot/jtreg/serviceability/jvmti/vthread/CarrierThreadEventNotification/libCarrierThreadEventNotification.cpp
line 58:
> 56: cthreads[ct_cnt++] = jni->NewGlobalRef(thread);
> 57: }
> 58: deallocate(jvmti, jni, (void*)tname);
cast to `void*` is not needed
test/hotspot/jtreg/serviceability/jvmti/vthread/CarrierThreadEventNotification/libCarrierThreadEventNotification.cpp
line 96:
> 94: }
> 95: jvmtiError err = jvmti->Deallocate((unsigned char*)carrier_threads);
> 96: check_jvmti_status(jni, err, "deallocate: error in JVMTI Deallocate
> call");
replace with `deallocate(jvmti, jni, carrier_threads);` ?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19438#discussion_r1623060427
PR Review Comment: https://git.openjdk.org/jdk/pull/19438#discussion_r1623061692
PR Review Comment: https://git.openjdk.org/jdk/pull/19438#discussion_r1623061890
Re: RFR: 8311177: Switching to interpreter only mode in carrier thread can lead to crashes [v3]
On Thu, 30 May 2024 02:41:39 GMT, Serguei Spitsyn wrote: >> test/hotspot/jtreg/serviceability/jvmti/vthread/MethodExitTest/libMethodExitTest.cpp >> line 201: >> >>> 199: >>> 200: // need to reset this value after the breakpoint_hit1 >>> 201: received_method_exit_event = JNI_FALSE; >> >> There was a loom-dev email thread regarding this last year. Seems related. I >> had concluded that the way the test was written that no MethodExit event >> should have been received. I'm not sure if I missed something in my analysis >> or if this failure is a result of your changes: >> >> https://mail.openjdk.org/pipermail/loom-dev/2023-August/006059.html >> https://mail.openjdk.org/pipermail/loom-dev/2023-September/006170.html > > Thank you for the comment and links to the discussion. In fact, I've observed > the MethodExit events really posted between the breakpoint hits: `hit1` and > `hit2`. The first one is at the return from the `unmount()` method. I was not > able to prove why they should not be expected. I'm not sure I follow the test logic. Its summary says "Verifies that MethodExit events are delivered on both carrier and virtual threads", but now it just ignores MethodExit requested for carrier thread in breakpoint_hit1. Then there is no sense to request the event on carrier thread. Per the test summary I'd expect the test should test MethodExit for carrier thread, but then java part needs to force unmount - PR Review Comment: https://git.openjdk.org/jdk/pull/19438#discussion_r1623073345
Re: RFR: 8333130: MakeJAR2.sh uses hard-coded JDK version [v2]
On Fri, 31 May 2024 12:01:14 GMT, Jaikiran Pai wrote: >> Can I please get a review of this test-only change which addresses >> https://bugs.openjdk.org/browse/JDK-8333130? >> >> There are a couple of tests `NativeMethodPrefixApp` and `RetransformApp` >> under `test/jdk/java/lang/instrument/` which launch the app/test with a >> `-javaagent:` pointing to a test specific jar. The test, launched with >> that `javaagent`, then verifies the test specific details. >> >> In their current form, in order to construct the agent `.jar` and make it >> available to the jtreg test that's launched, they `@run` a jtreg `shell` >> test. This `shell` test uses the `MakeJAR2.sh` script to generate the jar >> file. The contents of the script is relatively straightforward - it compiles >> (using `javac`) some boot classpath classes, some agent specific classes and >> then generates a jar file with the agent classes and a `MANIFEST.MF` which >> points to the boot classpath classes along with test specific manifest >> attributes. In a recent PR the `MakeJAR2.sh` script was updated to pass >> `--enable-preview --release 23` since one of the existing agent class was >> updated to use the ClassFile API PreviewFeature >> (https://github.com/openjdk/jdk/pull/13009). >> >> This generated agent jar then is passed as `-javaagent:` to the subsequent >> `@run main` test which does the actual testing. >> >> So essentially the `shell` test which uses the `MakeJAR2.sh` is merely there >> to create a jar file that's needed by the actual test. Within the JDK we >> have several tests which compile other classes and generate jar files using >> in-process test libraries and the `java.util.spi.ToolProvider` >> implementations. These 2 tests too can be updated to use a similar >> technique, to completely get rid of the `MakeJAR2.sh`. Removing the >> `MakeJAR2.sh` will simplify the ability to pass around value for `--release` >> when using `--enable-preview`. >> >> The commit in this PR updates these 2 tests to use `@run driver` which then >> compiles relevant classes (maintaining the semantics of `MakeJAR2.sh`) and >> generates the agent jar file and then finally launching the test process. As >> part of the update, I did not retain the `@author` tag from the 2 test >> definitions, since it's my understanding that we no longer use those. I will >> add them back if we should retain them. >> >> The tests continue to pass locally with this change. I've also triggered >> tier testing in our CI for this change. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > fix Boot-Class-Path value for Windows Hello Alex, what you suggest seems interesting and like you note much more simpler. I'll try it out and if that will work fine with the use of `--release` for the `asmlib/Instrumentor.java`. - PR Comment: https://git.openjdk.org/jdk/pull/19495#issuecomment-2143194801
Re: RFR: 8333130: MakeJAR2.sh uses hard-coded JDK version [v2]
On Sat, 1 Jun 2024 01:18:35 GMT, Jaikiran Pai wrote: > Hello Alex, what you suggest seems interesting and like you note much more > simpler. I'll try it out and if that will work fine with the use of > `--release` for the `asmlib/Instrumentor.java`. Actually I think you don't need to specify `--release` if you use jtreg "@enablePreview" tag, so "@build" should work fine - PR Comment: https://git.openjdk.org/jdk/pull/19495#issuecomment-2143215730
Re: RFR: 8332923: ObjectMonitorUsage.java failed with unexpected waiter_count [v3]
On Thu, 30 May 2024 06:36:09 GMT, SendaoYan wrote: >> SendaoYan has updated the pull request incrementally with one additional >> commit since the last revision: >> >> change from java_lang_VirtualThread::is_instance(thread_oop) to >> hread_oop->is_a(vmClasses::BaseVirtualThread_klass()) in >> Threads::get_pending_threads() > > The GHA test runner report a intermittent failure > `ToolTabSnippetTest.testCleaningCompletionTODO(): failure`, which has been > recorded in [JDK-8287078](https://bugs.openjdk.org/browse/JDK-8287078), I > think it's unrelated to this PR. > @sendaoYan The serviceability fixes require two reviews. Please, wait for a > second reviewer before integration. Okey. - PR Comment: https://git.openjdk.org/jdk/pull/19405#issuecomment-2143276984
Re: RFR: 8332923: ObjectMonitorUsage.java failed with unexpected waiter_count [v3]
On Thu, 30 May 2024 01:13:20 GMT, SendaoYan wrote: >> Hi all, >> ObjectMonitorUsage.java failed with `unexpected waiter_count` after >> [JDK-8328083](https://bugs.openjdk.org/browse/JDK-8328083) on linux x86_32. >> There are two changes in this PR: >> 1. In `JvmtiEnvBase::get_object_monitor_usage` function, change from >> `java_lang_VirtualThread::is_instance(thread_oop)` to >> `thread_oop->is_a(vmClasses::BaseVirtualThread_klass())`to support the >> alternative implementation. >> 2. In `Threads::get_pending_threads(ThreadsList *, int, address)` function >> of threads.cpp file, change from >> `java_lang_VirtualThread::is_instance(thread_oop)` to >> `thread_oop->is_a(vmClasses::BaseVirtualThread_klass())`to support the >> alternative implementation. This modified function only used by >> `JvmtiEnvBase::get_object_monitor_usage(JavaThread*, jobject, >> jvmtiMonitorUsage*)`, so the risk of the modified on threads.cpp file is low. >> >> >> >> Additional testing: >> - [x] linux x86_32 run all testcases in serviceability/jvmti, all testcases >> run successed expect >> `serviceability/jvmti/vthread/GetThreadState/GetThreadStateTest.java#default` >> run failed. This test also run failed before this PR, which has been >> recorded in [JDK-8333140](https://bugs.openjdk.org/browse/JDK-8333140) >> - [x] linux x86_64 run all testcases in serviceability/jvmti, all testcases >> run successed. > > SendaoYan has updated the pull request incrementally with one additional > commit since the last revision: > > change from java_lang_VirtualThread::is_instance(thread_oop) to > hread_oop->is_a(vmClasses::BaseVirtualThread_klass()) in > Threads::get_pending_threads() Marked as reviewed by alanb (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19405#pullrequestreview-2092010428
