Re: RFR: 8328877: [JNI] The JNI Specification needs to address the limitations of integer UTF-8 String lengths [v3]
On Tue, 3 Sep 2024 03:00:56 GMT, David Holmes wrote: >> This is the implementation of a new method added to the JNI specification. >> >> From the CSR request: >> >> The `GetStringUTFLength` function returns the length as a `jint` (`jsize`) >> value and so is limited to returning at most `Integer.MAX_VALUE`. But a Java >> string can itself consist of `Integer.MAX_VALUE` characters, each of which >> may require more than one byte to represent them in modified UTF-8 format.** >> It follows then that this function cannot return the correct answer for all >> String values and yet the specification makes no mention of this, nor of any >> possible error to report if this situation is encountered. >> >> **The modified UTF-8 format used by the VM can require up to six bytes to >> represent one unicode character, but six byte characters are stored as UTF16 >> surrogate pairs. Hence the most bytes per character is 3, and so the maximum >> length is 3*`Integer.MAX_VALUE`. With compact strings this reduces to >> 2*`Integer.MAX_VALUE`. >> >> Solution >> >> Deprecate the existing JNI `GetStringUTFLength` method noting that it may >> return a truncated length, and add a new method, JNI >> `GetStringUTFLengthAsLong` that returns the string length as a `jlong` value. >> >> --- >> >> We also add a truncation warning to `GetStringUTFLength` under -Xcheck:jni >> >> There are some incidental whitespace changes in >> `src/hotspot/os/posix/dtrace/hotspot_jni.d` along with the new method >> entries. >> >> Testing: >> - new test added >> - tiers 1-3 sanity >> >> Thanks > > David Holmes has updated the pull request incrementally with one additional > commit since the last revision: > > The JNI version update was incompete Marked as reviewed by cjplummer (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/20784#pullrequestreview-2278475330
Re: RFR: 8328877: [JNI] The JNI Specification needs to address the limitations of integer UTF-8 String lengths [v2]
On Fri, 30 Aug 2024 05:21:54 GMT, David Holmes wrote: >> This is the implementation of a new method added to the JNI specification. >> >> From the CSR request: >> >> The `GetStringUTFLength` function returns the length as a `jint` (`jsize`) >> value and so is limited to returning at most `Integer.MAX_VALUE`. But a Java >> string can itself consist of `Integer.MAX_VALUE` characters, each of which >> may require more than one byte to represent them in modified UTF-8 format.** >> It follows then that this function cannot return the correct answer for all >> String values and yet the specification makes no mention of this, nor of any >> possible error to report if this situation is encountered. >> >> **The modified UTF-8 format used by the VM can require up to six bytes to >> represent one unicode character, but six byte characters are stored as UTF16 >> surrogate pairs. Hence the most bytes per character is 3, and so the maximum >> length is 3*`Integer.MAX_VALUE`. With compact strings this reduces to >> 2*`Integer.MAX_VALUE`. >> >> Solution >> >> Deprecate the existing JNI `GetStringUTFLength` method noting that it may >> return a truncated length, and add a new method, JNI >> `GetStringUTFLengthAsLong` that returns the string length as a `jlong` value. >> >> --- >> >> We also add a truncation warning to `GetStringUTFLength` under -Xcheck:jni >> >> There are some incidental whitespace changes in >> `src/hotspot/os/posix/dtrace/hotspot_jni.d` along with the new method >> entries. >> >> Testing: >> - new test added >> - tiers 1-3 sanity >> >> Thanks > > David Holmes has updated the pull request incrementally with one additional > commit since the last revision: > > Exclude test on 32-bit Overall it looks good to me, although I don't have experience adding a new JNI API (the dtrace probes were new to me), but it seems you are following what is already in place for other functions, and the testing looks good. - Marked as reviewed by cjplummer (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/20784#pullrequestreview-2273477700
Re: RFR: 8328877: [JNI] The JNI Specification needs to address the limitations of integer UTF-8 String lengths
On Fri, 30 Aug 2024 02:07:54 GMT, David Holmes wrote: > This is the implementation of a new method added to the JNI specification. > > From the CSR request: > > The `GetStringUTFLength` function returns the length as a `jint` (`jsize`) > value and so is limited to returning at most `Integer.MAX_VALUE`. But a Java > string can itself consist of `Integer.MAX_VALUE` characters, each of which > may require more than one byte to represent them in modified UTF-8 format.** > It follows then that this function cannot return the correct answer for all > String values and yet the specification makes no mention of this, nor of any > possible error to report if this situation is encountered. > > **The modified UTF-8 format used by the VM can require up to six bytes to > represent one unicode character, but six byte characters are stored as UTF16 > surrogate pairs. Hence the most bytes per character is 3, and so the maximum > length is 3*`Integer.MAX_VALUE`. With compact strings this reduces to > 2*`Integer.MAX_VALUE`. > > Solution > > Deprecate the existing JNI `GetStringUTFLength` method noting that it may > return a truncated length, and add a new method, JNI > `GetStringUTFLengthAsLong` that returns the string length as a `jlong` value. > > --- > > We also add a truncation warning to `GetStringUTFLength` under -Xcheck:jni > > There are some incidental whitespace changes in > `src/hotspot/os/posix/dtrace/hotspot_jni.d` along with the new method entries. > > Testing: > - new test added > - tiers 1-3 sanity > > Thanks test/hotspot/jtreg/runtime/jni/checked/TestLargeUTF8Length.java line 27: > 25: * @bug 8328877 > 26: * @summary Test warning for GetStringUTFLength and functionality of > GetStringUTFLengthAsLong > 27: * @library /test/lib Shouldn't this test have: `@requires vm.bits == 64 ` - PR Review Comment: https://git.openjdk.org/jdk/pull/20784#discussion_r1737942541
Re: RFR: 8337408: Use GetTempPath2 API instead of GetTempPath [v2]
On Thu, 15 Aug 2024 22:06:14 GMT, Dhamoder Nalla wrote:
>> src/hotspot/os/windows/os_windows.cpp line 1522:
>>
>>> 1520: const char* os::get_temp_directory() {
>>> 1521: static char path_buf[MAX_PATH];
>>> 1522: if (_GetTempPath2A != nullptr) {
>>
>> Where does _GetTempPath2A get initialized?
>
> Thanks @plummercj for reviewing this PR. Fixed the missing initialization.
FYI I'm not familiar with these Windows APIs, so I probably won't give this
review approval myself. I only took a look because there was an svc file
involved (AttachProviderImpl.c). Hopefully others with a better understanding
of the changes will provide reviews.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20600#discussion_r1719063072
Re: RFR: 8337408: Use GetTempPath2 API instead of GetTempPath
On Thu, 15 Aug 2024 16:23:18 GMT, Dhamoder Nalla wrote:
> Use the GetTempPath2 APIs instead of the GetTempPath APIs in native code
> across the OpenJDK repository to retrieve the temporary directory path, as
> GetTempPath2 provides enhanced security. While GetTempPath may still function
> without errors, using GetTempPath2 reduces the risk of potential exploits for
> users.
>
>
> The code to dynamically load GetTempPath2 is duplicated due to the following
> reasons. I would appreciate any suggestions to remove the duplication where
> possible:
>
> 1. The changes span across four different folders—java.base, jdk.package,
> jdk.attach, and hotspot—with no shared code between them.
> 2. Some parts of the code use version A, while others use version W (ANSI vs.
> Unicode).
> 3. Some parts of the code are written in C others in C++.
src/hotspot/os/windows/os_windows.cpp line 1522:
> 1520: const char* os::get_temp_directory() {
> 1521: static char path_buf[MAX_PATH];
> 1522: if (_GetTempPath2A != nullptr) {
Where does _GetTempPath2A get initialized?
src/hotspot/os/windows/os_windows.cpp line 1525:
> 1523: if (_GetTempPath2A(MAX_PATH, path_buf) > 0) {
> 1524: return path_buf;
> 1525: }
Need to indent line 1524.
src/hotspot/os/windows/os_windows.cpp line 1527:
> 1525: }
> 1526: }
> 1527: else if (GetTempPath(MAX_PATH, path_buf) > 0) {
Suggestion:
} else if (GetTempPath(MAX_PATH, path_buf) > 0) {
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20600#discussion_r1718830564
PR Review Comment: https://git.openjdk.org/jdk/pull/20600#discussion_r1718831669
PR Review Comment: https://git.openjdk.org/jdk/pull/20600#discussion_r1718832664
Re: RFR: 8335684: Test ThreadCpuTime.java should pause like ThreadCpuTimeArray.java
On Thu, 4 Jul 2024 10:08:30 GMT, Kevin Walls wrote: > There are two similarly names tests. > Recently: > JDK-8335124: com/sun/management/ThreadMXBean/ThreadCpuTimeArray.java failed > with CPU time out of expected range > ...made a simple change to try and avoid noisy test failures. The same fix > should be applied here to ThreadCpuTime.java. > > Also removing an old comment about a Solaris issue. test/jdk/java/lang/management/ThreadMXBean/ThreadCpuTime.java line 239: > 237: " > ThreadUserTime = " + utime2); > 238: } > 239: */ Shouldn't this be uncommented and this bit of testing restored? It seems the only reason it was commented out is because of Solaris, which we don't need to worry about anymore. Probably best to leave this to a separate PR if you are going to restore it. - PR Review Comment: https://git.openjdk.org/jdk/pull/20025#discussion_r1669075901
Re: RFR: 8327793: Deprecate jstatd for removal [v4]
On Tue, 11 Jun 2024 18:11:55 GMT, Kevin Walls wrote: >> jstatd is an RMI server application which monitors HotSpot VMs, and provides >> an interface to the monitoring tool jstat, for use across a remote RMI >> connection. >> >> RMI is not how modern applications communicate. It is an old transport with >> long term security concerns, and configuration difficulties with firewalls. >> >> The jstatd tool should be removed. Deprecating and removing jstatd will not >> affect usage of jstat for monitoring local VMs using the Attach API. > > Kevin Walls has updated the pull request incrementally with one additional > commit since the last revision: > > Remove annotations Marked as reviewed by cjplummer (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19658#pullrequestreview-2111433619
Re: RFR: 8333827: JDK 23 RDP1 L10n resource files update [v2]
On Mon, 10 Jun 2024 21:58:29 GMT, Damon Nguyen wrote: >> This issue is responsible for updating the translations of all the >> localize(able) resources in the JDK. Primarily, the changes between JDK 22 >> RDP 1 and the integration of the JDK 23 RDP 1 L10n drop will be translated. >> >> The translation tool adjusted some definitions, which causes some changes in >> localized files where the source file had no changes. This causes some words >> being reverted from localized languages to English, and some had its >> definitions changed. >> >> Alternatively, the diffs are viewable here and was generated using Jonathan >> Gibbons' diff tool for l10n: >> https://cr.openjdk.org/~dnguyen/output2/ > > Damon Nguyen has updated the pull request incrementally with one additional > commit since the last revision: > > Remove double quotes jdk.jconsole and jdk.jdi changes look fine. - Marked as reviewed by cjplummer (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19609#pullrequestreview-208012
Re: RFR: 8333827: JDK 23 RDP1 L10n resource files update [v2]
On Mon, 10 Jun 2024 21:58:29 GMT, Damon Nguyen wrote:
>> This issue is responsible for updating the translations of all the
>> localize(able) resources in the JDK. Primarily, the changes between JDK 22
>> RDP 1 and the integration of the JDK 23 RDP 1 L10n drop will be translated.
>>
>> The translation tool adjusted some definitions, which causes some changes in
>> localized files where the source file had no changes. This causes some words
>> being reverted from localized languages to English, and some had its
>> definitions changed.
>>
>> Alternatively, the diffs are viewable here and was generated using Jonathan
>> Gibbons' diff tool for l10n:
>> https://cr.openjdk.org/~dnguyen/output2/
>
> Damon Nguyen has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Remove double quotes
src/jdk.jdi/share/classes/com/sun/tools/example/debug/tty/TTYResources_de.java
line 325:
> 323: {"Unexpected event type", "Unerwarteter Ereignistyp: {0}"},
> 324: {"unknown", "unbekannt"},
> 325: {"Unmonitoring", "Monitoring von {0} aufheben"},
The English entry is:
{"Unmonitoring", "Unmonitoring {0} "},
But the German entry now says "Monitoring". I'm sure what the original
"\u00DCberwachung" translates to, other than berwachung is monitoring. Now this
resource is partly in English, and is the incorrect English.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19609#discussion_r1633902362
Re: RFR: 8326433: Make file-local functions static in src/jdk.jdwp.agent/unix/native/libjdwp/exec_md.c [v2]
On Mon, 26 Feb 2024 22:55:06 GMT, Jiangli Zhou wrote: >> Please help review this trivial fix for resolving `ld: error: duplicate >> symbol: closeDescriptors` when static linking with both libjdwp and libjava, >> thanks. > > Jiangli Zhou has updated the pull request incrementally with two additional > commits since the last revision: > > - Address plummercj's comment and make forkedChildProcess static. > - Revert src/java.base/unix/native/libjava/childproc.c and > src/java.base/unix/native/libjava/childproc.h. Will handle those with > JDK-8326714. Looks good. - Marked as reviewed by cjplummer (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18013#pullrequestreview-1902255565
Re: RFR: 8326433: Make libjdwp and libjava closeDescriptors() as static function
On Mon, 26 Feb 2024 20:20:31 GMT, Jiangli Zhou wrote: > Please help review this trivial fix for resolving `ld: error: duplicate > symbol: closeDescriptors` when static linking with both libjdwp and libjava, > thanks. src/java.base/unix/native/libjava/childproc.h line 134: > 132: int closeSafely(int fd); > 133: int isAsciiDigit(char c); > 134: int closeDescriptors(void); It seems that most of the APIs in this file should be static. I don't think you should selectively deal with just one of them because of the conflict. Since this ends up being a more involved change, and is in a different component than the jdwp change, it should probably have a separate PR. src/jdk.jdwp.agent/unix/native/libjdwp/exec_md.c line 65: > 63: // by this function. This function returns 0 on failure > 64: // and 1 on success. > 65: static int I think you should also make forkedChildProcess static. It was added at the same time. - PR Review Comment: https://git.openjdk.org/jdk/pull/18013#discussion_r1503271560 PR Review Comment: https://git.openjdk.org/jdk/pull/18013#discussion_r1503268736
Re: RFR: 8320707: Virtual thread test updates
On Sat, 16 Dec 2023 17:25:20 GMT, Alan Bateman wrote: > A lot of test changes have accumulated in the loom repo, this includes both > new tests and updates to existing tests. Some of these updates can be brought > to the main line. This update brings over: > > - The existing tests for pinning use synchronized blocks. In preparation for > changes to allow carrier thread be released when a virtual thread parks > holding a monitor or blocks on monitorenter, these tests are changed to pin > by having a native frame on the stack. This part includes test infrastructure > to make it easy to add more tests that do operations while pinned. The tests > still test what they were originally created to test of course. > > - The test for the JFR jdk.VirtualThreadPinned event is refactored to allow > for additional cases where the event may be reported. > > - ThreadAPI is expanded to cover test for uncaught exception handling. > > - GetStackTraceWhenRunnable is refactored to not use a Selector, otherwise > this test will be invalidated when blocking selection operations release the > carrier. > > - StressStackOverflow is dialed down to run for 1m instead of 2mins. > > - The use of CountDownLatch in a number of tests that poll thread state has > been dropped to keep the tests as simple as possible. Can you explain your motivation for using AtomicBoolean with a polling loop rather than CountDownLatch(1)? I'm working on a test where I just added a CountDownLatch(1) and am wondering if I should do the same, but I'm not sure if there is something about these tests that is motivating the change. - PR Comment: https://git.openjdk.org/jdk/pull/17136#issuecomment-1861578806
Re: RFR: 8320129: "top" command during jtreg failure handler does not display CPU usage on OSX
On Fri, 1 Dec 2023 00:58:57 GMT, Leonid Mesnik wrote: > Description of problem and propsed fix from @plummercj > ' > In test/failure_handler/src/share/conf/mac.properties we have: > > process.top.app=top > process.top.args=-l 1 > > So top is run with the "-l 1" args, causing it to do one iteration and then > exit. Unfortunately the first iteration always shows all 0's for CPU usage. > If you do at least 2 iterations, you do see the proper CPU usage on the 2nd > iteration. The user just needs to know to scroll down to see it. I suggest we > start using "-l 2" unless someone has a better idea. > > BTW, for proper CPU usage you can instead look at the failure handler "ps" > output, which seems to be correct. But it is nice to have "top" produce the > correct output so all the CPU hogs are at the top of the list. > ' > > I verified that top report contains now 2 samples. Marked as reviewed by cjplummer (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/16915#pullrequestreview-1758861630
Re: RFR: 8267174: Many test files have the wrong Copyright header
On Wed, 6 Sep 2023 16:06:29 GMT, Erik Joelsson wrote: > > I wonder if this is the right thing to do for the hprof files. I believe > > they originated from some hprof tools that we no longer ship. 3rd parties > > might choose to integrate them into their own tools. > > Do you think I should revert them? I'm not sure. I think you need to consult someone with expertise in this area. - PR Comment: https://git.openjdk.org/jdk/pull/15573#issuecomment-1708757719
Re: RFR: 8267174: Many test files have the wrong Copyright header
On Tue, 5 Sep 2023 22:49:41 GMT, Erik Joelsson wrote: > There are a number of files in the `test` directory that have an incorrect > copyright header, which includes the "classpath" exception text. This patch > removes that text from all test files that I could find it in. I did this > using a combination of `sed` and `grep`. Reviewing this patch is probably > easier using the raw patch file or a suitable webrev format. > > It's my assumption that these headers were introduced by mistake as it's > quite easy to copy the wrong template when creating new files. I wonder if this is the right thing to do for the hprof files. I believe they originated from some hprof tools that we no longer ship. 3rd parties might choose to integrate them into their own tools. - PR Comment: https://git.openjdk.org/jdk/pull/15573#issuecomment-1707426607
Re: RFR: 8315097: Rename createJavaProcessBuilder [v2]
On Tue, 29 Aug 2023 09:12:51 GMT, Leo Korinth wrote: >> Rename createJavaProcessBuilder so that it is not used by mistake instead of >> createTestJvm. >> >> I have used the following sed script: `find -name "*.java" | xargs -n 1 sed >> -i -e >> "s/createJavaProcessBuilder(/createJavaProcessBuilderIgnoreTestJavaOpts(/g"` >> >> Then I have manually modified ProcessTools.java. In that file I have moved >> one version of createJavaProcessBuilder so that it is close to the other >> version. Then I have added a javadoc comment in bold telling: >> >>/** >> * Create ProcessBuilder using the java launcher from the jdk to >> * be tested. >> * >> * Please observe that you likely should use >> * createTestJvm() instead of this method because createTestJvm() >> * will add JVM options from "test.vm.opts" and "test.java.opts" >> * and this method will not do that. >> * >> * @param command Arguments to pass to the java command. >> * @return The ProcessBuilder instance representing the java command. >> */ >> >> >> I have used the name createJavaProcessBuilderIgnoreTestJavaOpts because of >> the name of Utils.prependTestJavaOpts that adds those VM flags. If you have >> a better name I could do a rename of the method. I kind of like that it is >> long and clumsy, that makes it harder to use... >> >> I have run tier 1 testing, and I have started more exhaustive testing. > > Leo Korinth has updated the pull request incrementally with one additional > commit since the last revision: > > copyright > I kind of like that it is long and clumsy, that makes it harder to use... ...but it's used in 462 files. That implies it is commonly used. Are you suggesting nearly all those uses are incorrect and eventually should be converted to `createTestJvm()`? - PR Comment: https://git.openjdk.org/jdk/pull/15452#issuecomment-1698003837
Re: RFR: JDK-8313670: Simplify shared lib name handling code in some tests [v3]
On Wed, 9 Aug 2023 11:06:04 GMT, Matthias Baesken wrote: >> There is coding e.g. in >> https://github.com/openjdk/jdk/blob/master/test/jdk/jdk/jfr/event/runtime/TestNativeLibrariesEvent.java#L72 >> that deals with shared lib naming on different OS. >> This code should be simplified. > > Matthias Baesken has updated the pull request incrementally with one > additional commit since the last revision: > > Introduce buildSharedLibraryName Needs copyright updates, but otherwise looks good. - Marked as reviewed by cjplummer (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/15151#pullrequestreview-1570274636
RFR: 8307408: Some jdk/sun/tools/jhsdb tests don't pass test JVM args to the debuggee JVM
Normally we want the test args passed to the SA debuggee. In fact for proper SA test coverage, it is more important for the test args to be passed to the debuggee than to be passed to the test or the the SA tool that the test launches. For a couple of jhsdb tests that launch jshell as the debuggee, this wasn't happening, and jshell was always launched with no extra args. Fixing this was very simple. Dealing with the unexpected fallout wasn't. I filed a number of bugs that turned up (or where otherwise exposed) once I fixed this CR. The main onces were: [JDK-8313798](https://bugs.openjdk.org/browse/JDK-8313798) [aarch64] sun/tools/jhsdb/HeapDumpTestWithActiveProcess.java sometimes times out on aarch64 [JDK-8313655](https://bugs.openjdk.org/browse/JDK-8313655) sun/tools/jhsdb/HeapDumpTestWithActiveProcess.java frequently fails with SerialGC For the first one I'm problem listing the test for now, but have a fix and will get it out for review shortly. For the second one I'm just having the test avoid the issue by not allowing jshell to be launched with SerialGC. Note also that this change caused some failures with ZGC due to an already filed CR. - Commit messages: - Pass test args to debuggee Changes: https://git.openjdk.org/jdk/pull/15168/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=15168&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8307408 Stats: 17 lines in 4 files changed: 13 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/15168.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15168/head:pull/15168 PR: https://git.openjdk.org/jdk/pull/15168
Re: RFR: JDK-8313670: Simplify shared lib name handling code in some tests
On Fri, 4 Aug 2023 09:59:41 GMT, Matthias Baesken wrote: > There is coding e.g. in > https://github.com/openjdk/jdk/blob/master/test/jdk/jdk/jfr/event/runtime/TestNativeLibrariesEvent.java#L72 > that deals with shared lib naming on different OS. > This code should be simplified. Changes requested by cjplummer (Reviewer). test/lib/jdk/test/lib/Platform.java line 376: > 374: } > 375: } > 376: The following tests could leverage this new API. Just look for calls to `Platform.sharedLibraryExt()`: test/hotspot/jtreg/runtime/signal/SigTestDriver.java test/hotspot/jtreg/vmTestbase/nsk/jvmti/NativeLibraryCopier.java test/jdk/com/sun/tools/attach/warnings/DynamicLoadWarningTest.java Perhaps a `Platform.buildSharedLibraryName()` API is worth considering. - PR Review: https://git.openjdk.org/jdk/pull/15151#pullrequestreview-1563375724 PR Review Comment: https://git.openjdk.org/jdk/pull/15151#discussion_r1284731606
Re: RFR: 8227229: Deprecate the launcher -Xdebug/-debug flags that have not done anything since Java 6 [v6]
On Wed, 19 Jul 2023 10:58:58 GMT, Jaikiran Pai wrote: >> Can I please get a review of this change which proposes to deprecate for >> removal the `-Xdebug` option and `-debug` option of the `java` command? >> This addresses https://bugs.openjdk.org/browse/JDK-8227229. >> >> As noted in the JBS issue this option is currently a no-op and has been >> there only for backward compatible since even Java 8 days. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > remove -Xdebug usage from SourceDebugExtension test Nice cleanup! Changes look good. - Marked as reviewed by cjplummer (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14918#pullrequestreview-1537863690
Re: [jdk21] RFR: 8310380: Handle problems in core-related tests on macOS when codesign tool does not work
On Fri, 30 Jun 2023 12:35:27 GMT, Matthias Baesken wrote: > 8310380: Handle problems in core-related tests on macOS when codesign tool > does not work Marked as reviewed by cjplummer (Reviewer). - PR Review: https://git.openjdk.org/jdk21/pull/87#pullrequestreview-1507621309
Re: RFR: JDK-8310380: Handle problems in core-related tests on macOS when codesign tool does not work [v4]
On Tue, 27 Jun 2023 13:45:27 GMT, Matthias Baesken wrote:
>> Currently, a number of tests fail on macOS because they miss the core file
>> (e.g. serviceability/sa/TestJmapCore.java).
>> The reason is that configure detects on some setups that codesign does not
>> work ("checking if debug mode codesign is possible... no) .
>> So adding the needed entitlement to generate cores is not done in the build.
>> This is currently not checked later in the tests.
>> But without the entitlement, a core is not generated.
>
> Matthias Baesken has updated the pull request incrementally with one
> additional commit since the last revision:
>
> small adjustments
Marked as reviewed by cjplummer (Reviewer).
-
PR Review: https://git.openjdk.org/jdk/pull/14562#pullrequestreview-1501681509
Re: RFR: JDK-8310380: Handle problems in core-related tests on macOS when codesign tool does not work [v3]
On Thu, 22 Jun 2023 11:51:15 GMT, Matthias Baesken wrote:
>> Currently, a number of tests fail on macOS because they miss the core file
>> (e.g. serviceability/sa/TestJmapCore.java).
>> The reason is that configure detects on some setups that codesign does not
>> work ("checking if debug mode codesign is possible... no) .
>> So adding the needed entitlement to generate cores is not done in the build.
>> This is currently not checked later in the tests.
>> But without the entitlement, a core is not generated.
>
> Matthias Baesken has updated the pull request incrementally with one
> additional commit since the last revision:
>
> move javaPath into caller
Changes requested by cjplummer (Reviewer).
test/lib/jdk/test/lib/Platform.java line 276:
> 274: }
> 275:
> 276: public static boolean hasPlistEntriesOSX() throws IOException {
Although I understand why you chose this API name (it mimics the form used for
`isHardenedOSX`), I don't think it is a good choice. `isHardenedOSX` is short
for "is this a hardened OSX system". That mapping does not work with
`hasPlistEntriesOSX`, which is more like "does this OSX system have plist
entries". I would suggest `hasOSXPlistEntries`.
test/lib/jdk/test/lib/util/CoreUtils.java line 154:
> 152: }
> 153: } else {
> 154: // codesign has to add entitlements using the plist, if
> this is not present we might not generate a core file
Suggestion:
// codesign has to add entitlements using the plist. If this is
not present we might not generate a core file.
-
PR Review: https://git.openjdk.org/jdk/pull/14562#pullrequestreview-1499040669
PR Review Comment: https://git.openjdk.org/jdk/pull/14562#discussion_r1242519320
PR Review Comment: https://git.openjdk.org/jdk/pull/14562#discussion_r1242520818
Re: RFR: JDK-8310380: Handle problems in core-related tests on macOS when codesign tool does not work [v3]
On Fri, 23 Jun 2023 08:37:16 GMT, Matthias Baesken wrote: > Chris are you okay with the latest rev. of this change? Sorry, I've been on vacation the past few days. I will have a look at it tomorrow. - PR Comment: https://git.openjdk.org/jdk/pull/14562#issuecomment-1606661232
Re: RFR: JDK-8310380: Handle problems in core-related tests on macOS when codesign tool does not work
On Tue, 20 Jun 2023 13:23:16 GMT, Matthias Baesken wrote:
> Currently, a number of tests fail on macOS because they miss the core file
> (e.g. serviceability/sa/TestJmapCore.java).
> The reason is that configure detects on some setups that codesign does not
> work ("checking if debug mode codesign is possible... no) .
> So adding the needed entitlement to generate cores is not done in the build.
> This is currently not checked later in the tests.
> But without the entitlement, a core is not generated.
`jdk/test/lib/TestMutuallyExclusivePlatformPredicates.java` is failing due to
the new API in Platform.java.
-
PR Comment: https://git.openjdk.org/jdk/pull/14562#issuecomment-1601655977
Re: RFR: JDK-8310380: Handle problems in core-related tests on macOS when codesign tool does not work
On Tue, 20 Jun 2023 13:23:16 GMT, Matthias Baesken wrote:
> Currently, a number of tests fail on macOS because they miss the core file
> (e.g. serviceability/sa/TestJmapCore.java).
> The reason is that configure detects on some setups that codesign does not
> work ("checking if debug mode codesign is possible... no) .
> So adding the needed entitlement to generate cores is not done in the build.
> This is currently not checked later in the tests.
> But without the entitlement, a core is not generated.
Copyrights need updating.
-
Changes requested by cjplummer (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/14562#pullrequestreview-1491822264
Re: RFR: JDK-8310380: Handle problems in core-related tests on macOS when codesign tool does not work
On Tue, 20 Jun 2023 13:23:16 GMT, Matthias Baesken wrote:
> Currently, a number of tests fail on macOS because they miss the core file
> (e.g. serviceability/sa/TestJmapCore.java).
> The reason is that configure detects on some setups that codesign does not
> work ("checking if debug mode codesign is possible... no) .
> So adding the needed entitlement to generate cores is not done in the build.
> This is currently not checked later in the tests.
> But without the entitlement, a core is not generated.
Copyrights need updating.
test/lib/jdk/test/lib/Platform.java line 264:
> 262:
> 263:
> 264: public static boolean hasPlistEntriesOSX() throws IOException {
Almost all of this is replicated from `isHardenedOSX()`. I wonder if there is a
way to do some sharing while still maintaining separate APIs. Combining them
into one API might make it harder to understand the code. Maybe a
`launchCodesign()` API that returns the BufferedReader would help.
test/lib/jdk/test/lib/Platform.java line 288:
> 286: }
> 287: }
> 288: return false;
Probably would be good to log that no Info.plist entry was found.
test/lib/jdk/test/lib/util/CoreUtils.java line 131:
> 129: return coreFileLocation; // success!
> 130: } else {
> 131: System.out.println("Core file not found, try to find a
> reason for this");
Suggestion:
System.out.println("Core file not found. Trying to find a reason
why...");
-
PR Review: https://git.openjdk.org/jdk/pull/14562#pullrequestreview-1491685114
PR Review Comment: https://git.openjdk.org/jdk/pull/14562#discussion_r1237602062
PR Review Comment: https://git.openjdk.org/jdk/pull/14562#discussion_r1237587271
PR Review Comment: https://git.openjdk.org/jdk/pull/14562#discussion_r1237590277
Re: RFR: JDK-8310191: com/sun/tools/attach/warnings/DynamicLoadWarningTest.java second failure on AIX [v2]
On Fri, 16 Jun 2023 11:59:39 GMT, Matthias Baesken wrote: >> After push of [JDK-8307478](https://bugs.openjdk.org/browse/JDK-8307478) , >> the following test started to fail on AIX : >> com/sun/tools/attach/warnings/DynamicLoadWarningTest.java ; failure output : >> >> java.lang.RuntimeException: 'WARNING: A JVM TI agent has been loaded >> dynamically' found in stderr >> at >> jdk.test.lib.process.OutputAnalyzer.stderrShouldNotContain(OutputAnalyzer.java:320) >> at >> DynamicLoadWarningTest$AppRunner.stderrShouldNotContain(DynamicLoadWarningTest.java:308) >> at >> DynamicLoadWarningTest.testLoadOneJvmtiAgent(DynamicLoadWarningTest.java:138) >> at >> java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103) >> at java.base/java.lang.reflect.Method.invoke(Method.java:580) >> >> Should be handled in a similar way to >> [JDK-8309549](https://bugs.openjdk.org/browse/JDK-8309549) . > > Matthias Baesken has updated the pull request incrementally with one > additional commit since the last revision: > > Rearrange and simplify test coding Marked as reviewed by cjplummer (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/14515#pullrequestreview-1484183655
Re: RFR: JDK-8309549: com/sun/tools/attach/warnings/DynamicLoadWarningTest.java fails on AIX
On Fri, 9 Jun 2023 13:39:26 GMT, Matthias Baesken wrote: > On AIX , new jtreg test > com/sun/tools/attach/warnings/DynamicLoadWarningTest.java always failed with > the output : > > --System.err:(294/28579)-- > STARTED DynamicLoadWarningTest::testLoadJavaAgent 'testLoadJavaAgent()' > SUCCESSFUL DynamicLoadWarningTest::testLoadJavaAgent 'testLoadJavaAgent()' > STARTED DynamicLoadWarningTest::testLoadOneJvmtiAgent '[1] > DynamicLoadWarningTest$$Lambda/0x00040020bd88@600d90bb' > org.opentest4j.AssertionFailedError: expected: <1> but was: <2> > at > org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151) > at > org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132) > at org.junit.jupiter.api.AssertEquals.failNotEqual(AssertEquals.java:197) > at org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:150) > at org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:145) > at org.junit.jupiter.api.Assertions.assertEquals(Assertions.java:528) > at > DynamicLoadWarningTest$AppRunner.stderrShouldContain(DynamicLoadWarningTest.java:298) > at > DynamicLoadWarningTest.testLoadOneJvmtiAgent(DynamicLoadWarningTest.java:125) > at > java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103) > at java.base/java.lang.reflect.Method.invoke(Method.java:580) > > Reason seems to be the different behavior of dlopen on AIX compared to e.g. > Linux > This is what I find in the manpage of AIX 7.2 or 7.3 : > https://www.ibm.com/docs/en/aix/7.2?topic=d-dlopen-subroutine > https://www.ibm.com/docs/en/aix/7.3?topic=d-dlopen-subroutine > 'If the module is already loaded, it is not loaded again, but a new, unique > value will be returned by the dlopen subroutine.' > > Sounds different to what Linux documents in the manpage: > https://man7.org/linux/man-pages/man3/dlopen.3.html > 'If the same shared object is opened again with dlopen(), the same object > handle is returned.' > > We skip this special subtest on AIX . Marked as reviewed by cjplummer (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/14393#pullrequestreview-1472750566
Re: RFR: 8309408: Thread.sleep cleanup
On Mon, 5 Jun 2023 15:10:18 GMT, Alan Bateman wrote: >> test/hotspot/jtreg/vmTestbase/nsk/monitoring/share/ThreadController.java >> line 660: >> >>> 658: expectedMethods.add(Thread.class.getName() + ".sleep"); >>> 659: expectedMethods.add(Thread.class.getName() + ".sleepNanos"); >>> 660: expectedMethods.add(Thread.class.getName() + ".sleepNanos0"); >> >> I'm surprised this test doesn't list `beforeSleep` and `afterSleep`. > >> There is one potential, pre-existing, test omission noted below. >> I'm surprised this test doesn't list `beforeSleep` and `afterSleep`. > > The monitoring/stress/thread tests will fail if they observe an unexpected > method name in the stack trace. I don't think it can happen because the tests > poll the thread state and for SleepingThread, it will sample the stack trace > when the thread state is timed-wait. The beforeSleep/afterSleep methods won't > in the stack trace when sleeping. It would be harmless to add them in that > they aren't going to cause these tests to fail but might help with any > further changes. The following commit in loom heavily modified this file with a lot of added expected methods. There are other related tests with similar changes. I'm not so sure I understand the need for so many additions, and also why expectedLength is so out of sync with the number of added method. I don't believe this commit was reviewed individually, but was just part of the overall loom review when merge into jdk. Perhaps it should be revisited. https://github.com/openjdk/loom/commit/26e66bc1a6a0dd735c8138a696809caba3e82b26 - PR Review Comment: https://git.openjdk.org/jdk/pull/14303#discussion_r1218539693
Integrated: 8309334: ProcessTools.main() does not properly set thread names when using the virtual thread wrapper
On Fri, 2 Jun 2023 21:30:46 GMT, Chris Plummer wrote: > Normally when a virtual thread wrapper is used to run a test, the main thread > is renamed to "old-m-a-i-n" and the new virtual thread that will act as the > main thread is named "main". Neither is being done by `ProcessTools.main()`. > This can cause problems for tests that expect the main thread that the test > is running in to be called "main". It is instead left unnamed. This is > causing the following 4 tests to fail: > > com/sun/jdi/JdbMethodExitTest.java > com/sun/jdi/JdbStepTest.java > com/sun/jdi/JdbStopThreadTest.java > com/sun/jdi/JdbStopThreadidTest.java > > These tests also fail due to > [JDK-8309397](https://bugs.openjdk.org/browse/JDK-8309397), which will be > fixed after this CR, and also com/sun/jdi/JdbMethodExitTest.java fails due to > [JDK-8309396](https://bugs.openjdk.org/browse/JDK-8309396), which will also > subsequently be fixed. > > Note this fix messed up one runtime test. It was expecting an exception > message to mention the "main" thread rather than "old-m-a-i-n". Loosening the > exception message matching pattern a bit solved the problem. > > Testing was done by running all of tier1 and tier5. This pull request has now been integrated. Changeset: ecb17532 Author:Chris Plummer URL: https://git.openjdk.org/jdk/commit/ecb17532dc8f3e271ad2d6550127a2253569cf9b Stats: 24 lines in 3 files changed: 5 ins; 16 del; 3 mod 8309334: ProcessTools.main() does not properly set thread names when using the virtual thread wrapper Reviewed-by: amenkov, lmesnik, sspitsyn, alanb - PR: https://git.openjdk.org/jdk/pull/14292
Re: RFR: 8309334: ProcessTools.main() does not properly set thread names when using the virtual thread wrapper [v3]
On Sat, 3 Jun 2023 21:34:16 GMT, Chris Plummer wrote: >> Normally when a virtual thread wrapper is used to run a test, the main >> thread is renamed to "old-m-a-i-n" and the new virtual thread that will act >> as the main thread is named "main". Neither is being done by >> `ProcessTools.main()`. This can cause problems for tests that expect the >> main thread that the test is running in to be called "main". It is instead >> left unnamed. This is causing the following 4 tests to fail: >> >> com/sun/jdi/JdbMethodExitTest.java >> com/sun/jdi/JdbStepTest.java >> com/sun/jdi/JdbStopThreadTest.java >> com/sun/jdi/JdbStopThreadidTest.java >> >> These tests also fail due to >> [JDK-8309397](https://bugs.openjdk.org/browse/JDK-8309397), which will be >> fixed after this CR, and also com/sun/jdi/JdbMethodExitTest.java fails due >> to [JDK-8309396](https://bugs.openjdk.org/browse/JDK-8309396), which will >> also subsequently be fixed. >> >> Note this fix messed up one runtime test. It was expecting an exception >> message to mention the "main" thread rather than "old-m-a-i-n". Loosening >> the exception message matching pattern a bit solved the problem. >> >> Testing was done by running all of tier1 and tier5. > > Chris Plummer has updated the pull request incrementally with two additional > commits since the last revision: > > - No longer need reflection to call Thread.ofVirtual().unstarted() > - Remove from problem list tests that are fixed by this PR. Thanks for the reviews Leonid, Alan, Serguei and Alex! - PR Comment: https://git.openjdk.org/jdk/pull/14292#issuecomment-1575663199
Re: RFR: 8309334: ProcessTools.main() does not properly set thread names when using the virtual thread wrapper [v3]
> Normally when a virtual thread wrapper is used to run a test, the main thread > is renamed to "old-m-a-i-n" and the new virtual thread that will act as the > main thread is named "main". Neither is being done by `ProcessTools.main()`. > This can cause problems for tests that expect the main thread that the test > is running in to be called "main". It is instead left unnamed. This is > causing the following 4 tests to fail: > > com/sun/jdi/JdbMethodExitTest.java > com/sun/jdi/JdbStepTest.java > com/sun/jdi/JdbStopThreadTest.java > com/sun/jdi/JdbStopThreadidTest.java > > These tests also fail due to > [JDK-8309397](https://bugs.openjdk.org/browse/JDK-8309397), which will be > fixed after this CR, and also com/sun/jdi/JdbMethodExitTest.java fails due to > [JDK-8309396](https://bugs.openjdk.org/browse/JDK-8309396), which will also > subsequently be fixed. > > Note this fix messed up one runtime test. It was expecting an exception > message to mention the "main" thread rather than "old-m-a-i-n". Loosening the > exception message matching pattern a bit solved the problem. > > Testing was done by running all of tier1 and tier5. Chris Plummer has updated the pull request incrementally with two additional commits since the last revision: - No longer need reflection to call Thread.ofVirtual().unstarted() - Remove from problem list tests that are fixed by this PR. - Changes: - all: https://git.openjdk.org/jdk/pull/14292/files - new: https://git.openjdk.org/jdk/pull/14292/files/de5ca147..9bcc200b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=14292&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=14292&range=01-02 Stats: 17 lines in 2 files changed: 0 ins; 16 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/14292.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14292/head:pull/14292 PR: https://git.openjdk.org/jdk/pull/14292
Re: RFR: 8309334: ProcessTools.main() does not properly set thread names when using the virtual thread wrapper [v2]
> Normally when a virtual thread wrapper is used to run a test, the main thread > is renamed to "old-m-a-i-n" and the new virtual thread that will act as the > main thread is named "main". Neither is being done by `ProcessTools.main()`. > This can cause problems for tests that expect the main thread that the test > is running in to be called "main". It is instead left unnamed. This is > causing the following 4 tests to fail: > > com/sun/jdi/JdbMethodExitTest.java > com/sun/jdi/JdbStepTest.java > com/sun/jdi/JdbStopThreadTest.java > com/sun/jdi/JdbStopThreadidTest.java > > These tests also fail due to > [JDK-8309397](https://bugs.openjdk.org/browse/JDK-8309397), which will be > fixed after this CR, and also com/sun/jdi/JdbMethodExitTest.java fails due to > [JDK-8309396](https://bugs.openjdk.org/browse/JDK-8309396), which will also > subsequently be fixed. > > Note this fix messed up one runtime test. It was expecting an exception > message to mention the "main" thread rather than "old-m-a-i-n". Loosening the > exception message matching pattern a bit solved the problem. > > Testing was done by running all of tier1 and tier5. Chris Plummer 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 two additional commits since the last revision: - Merge branch 'master' into 8309334_processtools merge - when using virtual threads, properly name the main threads - Changes: - all: https://git.openjdk.org/jdk/pull/14292/files - new: https://git.openjdk.org/jdk/pull/14292/files/e53abaf4..de5ca147 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=14292&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=14292&range=00-01 Stats: 4052 lines in 65 files changed: 3213 ins; 625 del; 214 mod Patch: https://git.openjdk.org/jdk/pull/14292.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14292/head:pull/14292 PR: https://git.openjdk.org/jdk/pull/14292
Re: RFR: 8309334: ProcessTools.main() does not properly set thread names when using the virtual thread wrapper
On Sat, 3 Jun 2023 14:07:52 GMT, Alan Bateman wrote:
>> Normally when a virtual thread wrapper is used to run a test, the main
>> thread is renamed to "old-m-a-i-n" and the new virtual thread that will act
>> as the main thread is named "main". Neither is being done by
>> `ProcessTools.main()`. This can cause problems for tests that expect the
>> main thread that the test is running in to be called "main". It is instead
>> left unnamed. This is causing the following 4 tests to fail:
>>
>> com/sun/jdi/JdbMethodExitTest.java
>> com/sun/jdi/JdbStepTest.java
>> com/sun/jdi/JdbStopThreadTest.java
>> com/sun/jdi/JdbStopThreadidTest.java
>>
>> These tests also fail due to
>> [JDK-8309397](https://bugs.openjdk.org/browse/JDK-8309397), which will be
>> fixed after this CR, and also com/sun/jdi/JdbMethodExitTest.java fails due
>> to [JDK-8309396](https://bugs.openjdk.org/browse/JDK-8309396), which will
>> also subsequently be fixed.
>>
>> Note this fix messed up one runtime test. It was expecting an exception
>> message to mention the "main" thread rather than "old-m-a-i-n". Loosening
>> the exception message matching pattern a bit solved the problem.
>>
>> Testing was done by running all of tier1 and tier5.
>
> test/lib/jdk/test/lib/process/ProcessTools.java line 899:
>
>> 897: // when main is executed in virtual thread
>> 898: MainThreadGroup tg = new MainThreadGroup();
>> 899: Thread vthread = createVirtualThread(() -> {
>
> You can replace this with `Thread.ofVirtual().unstarted(() -> {` and remove
> the helper method. I think that helper method dates from the jtreg wrapper
> solution. Better still, maybe you could use
> `Thread.ofVirtual().name("main").start(() -> {`.
Yes, I already brought this up with Leonid. I was going to file a separate bug
for it, but I can fix it with this CR if you'd like.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14292#discussion_r1215705793
RFR: 8309334: ProcessTools.main() does not properly set thread names when using the virtual thread wrapper
Normally when a virtual thread wrapper is used to run a test, the main thread is renamed to "old-m-a-i-n" and the new virtual thread that will act as the main thread is named "main". Neither is being done by `ProcessTools.main()`. This can cause problems for tests that expect the main thread that the test is running in to be called "main". It is instead left unnamed. This is causing the following 4 tests to fail: com/sun/jdi/JdbMethodExitTest.java com/sun/jdi/JdbStepTest.java com/sun/jdi/JdbStopThreadTest.java com/sun/jdi/JdbStopThreadidTest.java These tests also fail due to [JDK-8309397](https://bugs.openjdk.org/browse/JDK-8309397), which will be fixed after this CR, and also com/sun/jdi/JdbMethodExitTest.java fails due to [JDK-8309396](https://bugs.openjdk.org/browse/JDK-8309396), which will also subsequently be fixed. Note this fix messed up one runtime test. It was expecting an exception message to mention the "main" thread rather than "old-m-a-i-n". Loosening the exception message matching pattern a bit solved the problem. Testing was done by running all of tier1 and tier5. - Commit messages: - when using virtual threads, properly name the main threads Changes: https://git.openjdk.org/jdk/pull/14292/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=14292&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8309334 Stats: 11 lines in 2 files changed: 5 ins; 0 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/14292.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14292/head:pull/14292 PR: https://git.openjdk.org/jdk/pull/14292
RFR: 8309146: extend JDI StackFrame.setValue() and JDWP StackFrame.setValues minimal support for virtual threads
The JDWP spec for StackFrame.SetValue currently states: "If the thread is a virtual thread then this command can be used to set " "the value of local variables in the top-most frame when the thread is " "suspended at a breakpoint or single step event. The target VM may support " "setting local variables in other cases." The JDI spec for StackFrame.setValue() has similar wording. In [JDK-8308814](https://bugs.openjdk.org/browse/JDK-8308814) the JVMTI spec clarified support to be for a thread suspended at any event, not just a breakpoint or single step. That same clarification is needed in the JDWP and JDI specs. No implementation changes are needed. - Commit messages: - Clarify minimal suppoert for StackFrame.GetValue. Changes: https://git.openjdk.org/jdk/pull/14235/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=14235&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8309146 Stats: 5 lines in 2 files changed: 0 ins; 0 del; 5 mod Patch: https://git.openjdk.org/jdk/pull/14235.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14235/head:pull/14235 PR: https://git.openjdk.org/jdk/pull/14235
RFR: 8308237: add JDWP and JDI virtual thread support for ThreadReference.PopFrames
This is a follow-on to [JDK-8264699](https://bugs.openjdk.org/browse/JDK-8264699) which adds JVMTI PopFrames support for virtual thread. For JDWP and JDI this is mostly a spec update, although JDI needs minor changes to properly throw the correct exception. Note this PR needs JDK-8264699 in order to function properly, so there may be some GHA failures until JDK-8264699 is pushed. There are a large number of tests that can now be removed from the problem list. Also, one test needs to be modified to no longer expect OpaqueFrameException for virtual threads. It was just revereted back to it's previous form before the OpaqueFrameException support was added for virtual threads. As you can see from the problemlist update, there are quite a few tests for popFrames() support. However, there are still two coverage gaps: - There is no test for throwing NativeMethodException (even for platform threads) - There is no test case for throwing OpaqueFrameException when the virtual thread is suspended but not mounted. I may eventually add one or both tests to the PR, or I may just file separate CRs for them for now. - Commit messages: - Add virtual thread popframes support the jdwp and jdi Changes: https://git.openjdk.org/jdk/pull/14022/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=14022&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8308237 Stats: 80 lines in 6 files changed: 13 ins; 50 del; 17 mod Patch: https://git.openjdk.org/jdk/pull/14022.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14022/head:pull/14022 PR: https://git.openjdk.org/jdk/pull/14022
Re: RFR: 8307966: ProblemList java/util/concurrent/locks/Lock/OOMEInAQS.java on linux-x64
On Fri, 12 May 2023 00:05:21 GMT, Daniel D. Daugherty wrote: > A trivial fix to ProblemList java/util/concurrent/locks/Lock/OOMEInAQS.java > on linux-x64. Maybe linux-all would be better. - PR Comment: https://git.openjdk.org/jdk/pull/13946#issuecomment-1544916181
Re: RFR: 8233725: ProcessTools.startProcess() has output issues when using an OutputAnalyzer at the same time
On Fri, 21 Apr 2023 21:43:39 GMT, Leonid Mesnik wrote: > ProcessTools.startProcess() creates process and read it's output error > streams. So the any other using of corresponding Process.getInputStream() and > Process.getErrorStream() doesn't get process streams. > > This fix preserve process streams content and allow to read reuse the date. > The ByteArrayOutputStream is used as a buffer. > It stores all process output, never trying to clean date which has been read. > > The regression test has been provided with issue. > > I closed previous PR https://github.com/openjdk/jdk/pull/13560 by mistake > instead of updating it. > > I run all tests to ensure that no failures are introduced. Marked as reviewed by cjplummer (Reviewer). test/lib/jdk/test/lib/process/ProcessTools.java line 249: > 247: stdout.addOutputStream(out.getOutputStream()); > 248: stderr.addOutputStream(err.getOutputStream()); > 249: Overall this looks good, although I'm a bit unclear on how some of the underpinnings work, allowing the output to appear in these output streams, and also in the LineForwarder output (above), and for that matter, in the lineConsumer output (below). I guess there is some multiplexing of the output that I just don't grasp, but appears to be something that already worked. - PR Review: https://git.openjdk.org/jdk/pull/13594#pullrequestreview-1398781718 PR Review Comment: https://git.openjdk.org/jdk/pull/13594#discussion_r1175775335
Re: RFR: 8233725: ProcessTools.startProcess() has output issues when using an OutputAnalyzer at the same time [v2]
On Thu, 20 Apr 2023 18:40:52 GMT, Leonid Mesnik wrote:
>> ProcessTools.startProcess() creates process and read it's output error
>> streams. So the any other using of corresponding Process.getInputStream()
>> and Process.getErrorStream() doesn't get process streams.
>>
>> This fix preserve process streams content and allow to read it after process
>> completion. The another possible solution would be to throw exception when
>> user tries to read already drained streams to fail tests earlier. However it
>> complicates usage of ProcessTools.startProcess() methods.
>>
>> The regression test has been provided with issue.
>
> Leonid Mesnik has updated the pull request incrementally with one additional
> commit since the last revision:
>
> JStackStressTest.java updated.
test/jdk/sun/tools/jhsdb/JStackStressTest.java line 86:
> 84: } catch (InterruptedException e) {
> 85: }
> 86: OutputAnalyzer jshellOutput = new
> OutputAnalyzer(jShellProcess);
It's not clear to me how moving this is fixing anything.
test/jdk/sun/tools/jstatd/JstatdTest.java line 357:
> 355: assertEquals(stdout.size(), 1, "Output should contain one line");
> 356: assertTrue(stdout.get(0).startsWith("jstatd started"), "List
> should start with 'jstatd started'");
> 357: assertNotEquals(output.getExitValue(), 0,
Before your fix, was the "jstatd started" line being missed because of this bug.
test/lib/jdk/test/lib/process/ProcessTools.java line 750:
> 748: public InputStream getInputStream() {
> 749: try {
> 750: waitFor();
With this added `waitFor()` the assumption now is that the caller doesn't
intent to do incremental reads of the output as the process generates it. For
example, if the test were to send some command to the process and then want to
read the resulting output, and do this repeatedly, it won't able to use the
InputStream to do that.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13560#discussion_r1173141642
PR Review Comment: https://git.openjdk.org/jdk/pull/13560#discussion_r1173139688
PR Review Comment: https://git.openjdk.org/jdk/pull/13560#discussion_r1173146951
Re: RFR: 8305341: Alignment outside of HotSpot should be enforced by alignas instead of compiler specific attributes
On Fri, 31 Mar 2023 06:07:39 GMT, Julian Waters wrote: > C11 has been stable for a long time on all platforms, so native code can use > the standard alignas operator for alignment requirements I'm not sure what you mean by hotspot requiring a special review, but serviceability code does require two reviewers just like hotspot does. Also, you don't need to split the PR because of that. If you get one person to review the jdk changes and 2 to review the hostpot/svc changes, then you are good to go. - PR Comment: https://git.openjdk.org/jdk/pull/13258#issuecomment-1495369012
Re: RFR: 8305341: Alignment outside of HotSpot should be enforced by alignas instead of compiler specific attributes
On Fri, 31 Mar 2023 06:07:39 GMT, Julian Waters wrote: > C11 has been stable for a long time on all platforms, so native code can use > the standard alignas operator for alignment requirements > I don't think I can touch the freetype code since I think it's an external > library that was imported. HotSpot requires a special review for changes like > this, so it's not done here, but instead at #11431 (Which has dried up for a > long time now, would be great if someone finally looked at that PR...) Ok. What about the globalDefinitions_visCPP.hpp reference? - PR Comment: https://git.openjdk.org/jdk/pull/13258#issuecomment-1494864710
Re: RFR: 8305341: Alignment outside of HotSpot should be enforced by alignas instead of compiler specific attributes
On Fri, 31 Mar 2023 06:07:39 GMT, Julian Waters wrote: > C11 has been stable for a long time on all platforms, so native code can use > the standard alignas operator for alignment requirements I don't have any comments on this change in general (it's not something I've dealt with in the past), but I did notice that there are a couple of places you missed: src/hotspot/share/utilities/globalDefinitions_visCPP.hpp:119:#define ATTRIBUTE_ALIGNED(x) __declspec(align(x)) src/java.desktop/share/native/libfreetype/include/freetype/internal/ftvalid.h:82: /* __declspec(align())' in order to compile cleanly with */ src/java.desktop/share/native/libfreetype/src/smooth/ftgrays.c:484: /* __declspec(align())' in order to compile cleanly with */ For the 2nd and 3rd ones you would want to remove all of the following: #if defined( _MSC_VER ) /* Visual C++ (and Intel C++) */ /* We disable the warning `structure was padded due to */ /* __declspec(align())' in order to compile cleanly with */ /* the maximum level of warnings.*/ #pragma warning( push ) #pragma warning( disable : 4324 ) #endif /* _MSC_VER */ ... #if defined( _MSC_VER ) #pragma warning( pop ) #endif - PR Comment: https://git.openjdk.org/jdk/pull/13258#issuecomment-1492522828
Re: RFR: 8304919: Implementation of Virtual Threads [v4]
On Wed, 29 Mar 2023 08:00:36 GMT, Alan Bateman wrote: >> JEP 444 proposes to make virtual threads a permanent feature in Java 21. The >> APIs that were preview APIs in Java 19/20 are changed to permanent and their >> `@since`/equivalent are changed to 21 (as per the guidance in JEP 12). The >> JNI and JVMTI versions are bumped as this is the first change in 21 to need >> the new version number. A lot of tests are updated to drop `@enablePreview` >> and --enable-preview. >> >> There is one API change from Java 19/20, the preview API >> Thread.Builder.allowSetThreadLocals(boolean) is dropped. This requires an >> update to the JVMTI GetThreadInfo implementation to read the TCCL >> consistently. >> >> In addition, there are a small number of implementation changes to sync up >> from the loom fibers branch: >> >> - A number of stack frames are `@Hidden` to reduce noise in the stack >> traces. This exposed a few issues with the stack walker code. More >> specifically, the cases where end of a continuation falls precisely at the >> end of the batch, or where the remaining frames are hidden, weren't handled >> correctly. >> - The code to emit the JFR jdk.ThreadSleepEvent is refactored so it's in >> Thread rather than in two classes. >> - A few robustness improvements for OOME and SOE. There is more to do here, >> for future PRs. >> - New system property to print a stack trace when a virtual thread sets its >> own value of a TL. >> - ThreadPerTaskExecutor is changed to use FutureTask. >> >> Testing: tier1-6. > > Alan Bateman has updated the pull request incrementally with one additional > commit since the last revision: > > Fix ThreadSleepEvent again Serviceability changes look good. - Marked as reviewed by cjplummer (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/13203#pullrequestreview-1363917524
Re: RFR: 8304919: Implementation of Virtual Threads [v2]
On Wed, 29 Mar 2023 07:27:50 GMT, Alan Bateman wrote:
>> test/jdk/com/sun/management/ThreadMXBean/VirtualThreads.java line 143:
>>
>>> 141: long[] tids = new long[] { tid0, tid1 };
>>> 142: long[] cpuTimes = bean.getThreadCpuTime(tids);
>>> 143: if (Thread.currentThread().isVirtual()) {
>>
>> How it worked before?
>
> tid0 is the thread ID of a platform therad. tid1 is the threadID of a virtual
> thread. The only change here is allow this test run with the main wrapper
> plugin
> ([CODETOOLS-7903373](https://bugs.openjdk.org/browse/CODETOOLS-7903373)), it
> would otherwise have to be excluded for those runs.
I don't see any problemlist changes. Was this test failing when using the
wrapper because of the lack of problemlisting?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13203#discussion_r1152353646
Re: RFR: 8304919: Implementation of Virtual Threads [v4]
On Wed, 29 Mar 2023 08:00:36 GMT, Alan Bateman wrote: >> JEP 444 proposes to make virtual threads a permanent feature in Java 21. The >> APIs that were preview APIs in Java 19/20 are changed to permanent and their >> `@since`/equivalent are changed to 21 (as per the guidance in JEP 12). The >> JNI and JVMTI versions are bumped as this is the first change in 21 to need >> the new version number. A lot of tests are updated to drop `@enablePreview` >> and --enable-preview. >> >> There is one API change from Java 19/20, the preview API >> Thread.Builder.allowSetThreadLocals(boolean) is dropped. This requires an >> update to the JVMTI GetThreadInfo implementation to read the TCCL >> consistently. >> >> In addition, there are a small number of implementation changes to sync up >> from the loom fibers branch: >> >> - A number of stack frames are `@Hidden` to reduce noise in the stack >> traces. This exposed a few issues with the stack walker code. More >> specifically, the cases where end of a continuation falls precisely at the >> end of the batch, or where the remaining frames are hidden, weren't handled >> correctly. >> - The code to emit the JFR jdk.ThreadSleepEvent is refactored so it's in >> Thread rather than in two classes. >> - A few robustness improvements for OOME and SOE. There is more to do here, >> for future PRs. >> - New system property to print a stack trace when a virtual thread sets its >> own value of a TL. >> - ThreadPerTaskExecutor is changed to use FutureTask. >> >> Testing: tier1-6. > > Alan Bateman has updated the pull request incrementally with one additional > commit since the last revision: > > Fix ThreadSleepEvent again test/hotspot/jtreg/serviceability/jvmti/thread/GetFrameCount/framecnt01/framecnt01.java line 82: > 80: > 81: // this is too fragile, implementation can change at any time. > 82: checkFrames(vThread1, false, 14); Is this due to the `@hidden` being added to `Continuation.enter()` and `enter0()`? If so, since both methods are now hidden, why are there not 2 fewer frames? Was there also an additional frame added somewhere? - PR Review Comment: https://git.openjdk.org/jdk/pull/13203#discussion_r1152337485
Re: RFR: 8303814: getLastErrorString should avoid charset conversions
On Wed, 8 Mar 2023 11:30:27 GMT, Daniel Jeliński wrote:
> This patch modifies the `getLastErrorString` method to return a `jstring`.
> Thanks to that we can avoid unnecessary back and forth conversions between
> Unicode and other charsets on Windows.
>
> Other changes include:
> - the Windows implementation of `getLastErrorString` no longer checks
> `errno`. I verified all uses of the method and confirmed that `errno` is not
> used anywhere.
> - While at it, I found and fixed a few calls to
> `JNU_ThrowIOExceptionWithLastError` that were done in context where
> `LastError` was not set.
> - jdk.hotspot.agent was modified to use `JNU_ThrowByNameWithLastError` and
> `JNU_ThrowByName` instead of `getLastErrorString`; the code is expected to
> have identical behavior.
> - zip_util was modified to return static messages instead of generated ones.
> The generated messages were not observed anywhere, because they were replaced
> by a static message in ZIP_Open, which is the only method used by other
> native code.
> - `getLastErrorString` is no longer exported by libjava.
>
> Tier1-3 tests continue to pass.
>
> No new automated regression test; testing this requires installing a language
> pack that cannot be displayed in the current code page.
> Tested this manually by installing Chinese language pack on English Windows
> 11, selecting Chinese language, then checking if the message on exception
> thrown by `InetAddress.getByName("nonexistent.local");` starts with
> `"不知道这样的主机。"` (or
> `"\u4e0d\u77e5\u9053\u8fd9\u6837\u7684\u4e3b\u673a\u3002"`). Without the
> change, the exception message started with a row of question marks.
I'm approving the SA changes. Thanks for the testing.
-
Marked as reviewed by cjplummer (Reviewer).
PR: https://git.openjdk.org/jdk/pull/12922
Re: RFR: 8303814: getLastErrorString should avoid charset conversions
On Wed, 8 Mar 2023 11:30:27 GMT, Daniel Jeliński wrote:
> This patch modifies the `getLastErrorString` method to return a `jstring`.
> Thanks to that we can avoid unnecessary back and forth conversions between
> Unicode and other charsets on Windows.
>
> Other changes include:
> - the Windows implementation of `getLastErrorString` no longer checks
> `errno`. I verified all uses of the method and confirmed that `errno` is not
> used anywhere.
> - While at it, I found and fixed a few calls to
> `JNU_ThrowIOExceptionWithLastError` that were done in context where
> `LastError` was not set.
> - jdk.hotspot.agent was modified to use `JNU_ThrowByNameWithLastError` and
> `JNU_ThrowByName` instead of `getLastErrorString`; the code is expected to
> have identical behavior.
> - zip_util was modified to return static messages instead of generated ones.
> The generated messages were not observed anywhere, because they were replaced
> by a static message in ZIP_Open, which is the only method used by other
> native code.
> - `getLastErrorString` is no longer exported by libjava.
>
> Tier1-3 tests continue to pass.
>
> No new automated regression test; testing this requires installing a language
> pack that cannot be displayed in the current code page.
> Tested this manually by installing Chinese language pack on English Windows
> 11, selecting Chinese language, then checking if the message on exception
> thrown by `InetAddress.getByName("nonexistent.local");` starts with
> `"不知道这样的主机。"` (or
> `"\u4e0d\u77e5\u9053\u8fd9\u6837\u7684\u4e3b\u673a\u3002"`). Without the
> change, the exception message started with a row of question marks.
We don't have a test for the SA changes you made. The best way to test it is
with clhsdb. Run the following against a JVM pid:
`$ jhsdb clhsdb --pid `
Use "jstack -v" to get a native pc from a frame, and then try `disassemble` on
the address. It most likely will produce an exception since it can't find
hsdis, which is actually what we want to be testing in this case:
hsdb> disassemble 0x7f38b371fca0
Error: sun.jvm.hotspot.debugger.DebuggerException: hsdis-amd64.so: cannot open
shared object file: No such file or directory
You'll need to test separately on Windows and any unix platform.
-
PR: https://git.openjdk.org/jdk/pull/12922
Re: RFR: JDK-8303587 Remove VMOutOfMemoryError001 test from the problem list after 8303198
On Fri, 3 Mar 2023 16:40:41 GMT, Roger Riggs wrote: > Remove VMOutOfMemoryException001.java from the problem list, after > JDK-8303198. > > The logging of Runtime.exit interfered with out-of-memory exception handling > in this test. > Making the logging more robust in JDK-8303198 by handling exceptions restores > the conditions expected by this test. Approved and trivial. - Marked as reviewed by cjplummer (Reviewer). PR: https://git.openjdk.org/jdk/pull/12859
Re: RFR: 8303198: System and Runtime.exit() resilience to logging errors [v2]
On Wed, 1 Mar 2023 16:59:51 GMT, Roger Riggs wrote: >> Consolidate logging and handle exceptions by printing to standard error and >> ignoring the exception. >> Exceptions while logging will not interfere with Runtime.exit. > > Roger Riggs has updated the pull request incrementally with one additional > commit since the last revision: > > Add exit status to output when the logging fails with an exception. Are you going to remove VMOutOfMemoryException001.java from the problem list? - PR: https://git.openjdk.org/jdk/pull/12770
Re: RFR: 8303480: Miscellaneous fixes to mostly invisible doc comments
On Thu, 2 Mar 2023 12:03:44 GMT, Pavel Rappo wrote: > Please review this superficial documentation cleanup that was triggered by > unrelated analysis of doc comments in JDK API. > > The only effect that this multi-area PR has on the JDK API Documentation > (i.e. the observable effect on the generated HTML pages) can be summarized as > follows: > > > diff -ur build/macosx-aarch64/images/docs-before/api/serialized-form.html > build/macosx-aarch64/images/docs-after/api/serialized-form.html > --- build/macosx-aarch64/images/docs-before/api/serialized-form.html > 2023-03-02 11:47:44 > +++ build/macosx-aarch64/images/docs-after/api/serialized-form.html > 2023-03-02 11:48:45 > @@ -17084,7 +17084,7 @@ > throws href="java.base/java/io/IOException.html" title="class in > java.io">IOException, > ClassNotFoundException > readObject is called to restore the > state of the > - (@code BasicPermission} from a stream. > + BasicPermission from a stream. > > Parameters: > s - the ObjectInputStream from which data > is read > > Notes > - > > * I'm not an expert in any of the affected areas, except for jdk.javadoc, and > I was merely after misused tags. Because of that, I would appreciate reviews > from experts in other areas. > * I discovered many more issues than I included in this PR. The excluded > issues seem to occur in infrequently updated third-party code (e.g. > javax.xml), which I assume we shouldn't touch unless necessary. > * I will update copyright years after (and if) the fix had been approved, as > required. The SA changes (jdk.hotspot.agent) look fine. - Marked as reviewed by cjplummer (Reviewer). PR: https://git.openjdk.org/jdk/pull/12826
Re: [jdk20] RFR: 8300719: JDK 20 RDP2 L10n resource files update
On Tue, 24 Jan 2023 20:50:00 GMT, Damon Nguyen wrote: > Open l10n drop. Files have been updated with translated versions. Whitespace > tool has been ran on files. > All tests passed jdk.console and jdk.management.agent changes look good. - Marked as reviewed by cjplummer (Reviewer). PR: https://git.openjdk.org/jdk20/pull/116
Re: RFR: 8294321: Fix typos in files under test/jdk/java, test/jdk/jdk, test/jdk/jni [v3]
On Tue, 3 Jan 2023 19:08:59 GMT, Serguei Spitsyn wrote: > Michael, I've reviewed the changes but the > [JDK-8294321](https://bugs.openjdk.org/browse/JDK-8294321) seems to be > already resolved. So, what JBS issue are you actually trying to fix? It's closed because #11385 used it to fix some of the typos. #11385 should have used a new issue, but now instead a new issue is needed for the remaining typos. - PR: https://git.openjdk.org/jdk/pull/10029
Re: [jdk20] RFR: 8298133: JDK 20 RDP1 L10n resource files update - msgdrop 10 [v5]
On Fri, 16 Dec 2022 03:38:54 GMT, Damon Nguyen wrote: >> Open l10n drop >> All tests passed > > Damon Nguyen has updated the pull request incrementally with one additional > commit since the last revision: > > Revert old translation. Fix lang codes `src/jdk.jdi/share/classes/com/sun/tools` changes look good. - Marked as reviewed by cjplummer (Reviewer). PR: https://git.openjdk.org/jdk20/pull/35
Re: RFR: 8296546: Add @spec tags to API
On Thu, 10 Nov 2022 01:10:13 GMT, Jonathan Gibbons wrote: > Please review a "somewhat automated" change to insert `@spec` tags into doc > comments, as appropriate, to leverage the recent new javadoc feature to > generate a new page listing the references to all external specifications > listed in the `@spec` tags. > > "Somewhat automated" means that I wrote and used a temporary utility to scan > doc comments looking for HTML links to selected sites, such as `ietf.org`, > `unicode.org`, `w3.org`. These links may be in the main description of a doc > comment, or in `@see` tags. For each link, the URL is examined, and > "normalized", and inserted into the doc comment with a new `@spec` tag, > giving the link and tile for the spec. > > "Normalized" means... > * Use `https:` where possible (includes pretty much all cases) > * Use a single consistent host name for all URLs coming from the same spec > site (i.e. don't use different aliases for the same site) > * Point to the root page of a multi-page spec > * Use a consistent form of the spec, preferring HTML over plain text where > both are available (this mostly applies to IETF specs) > > In addition, a "standard" title is determined for all specs, determined > either from the content of the (main) spec page or from site index pages. > > The net effect is (or should be) that **all** the changes are to just **add** > new `@spec` tags, based on the links found in each doc comment. There should > be no other changes to the doc comments, or to the implementation of any > classes and interfaces. > > That being said, the utility I wrote does have additional abilities, to > update the links that it finds (e.g. changing to use `https:` etc,) but those > features are _not_ being used here, but could be used in followup PRs if > component teams so desired. I did notice while working on this overall > feature that many of our links do point to "outdated" pages, some with > eye-catching notices declaring that the spec has been superseded. Determining > how, when and where to update such links is beyond the scope of this PR. > > Going forward, it is to be hoped that component teams will maintain the > underlying links, and the URLs in `@spec` tags, such that if references to > external specifications are updated, this will include updating the `@spec` > tags. > > To see the effect of all these new `@spec` tags, see > http://cr.openjdk.java.net/~jjg/8296546/api.00/ > > In particular, see the new [External > Specifications](http://cr.openjdk.java.net/~jjg/8296546/api.00/external-specs.html) > page, which you can also find via the new link near the top of the > [Index](http://cr.openjdk.java.net/~jjg/8296546/api.00/index-files/index-1.html) > pages. src/jdk.jdi/share/classes/com/sun/jdi/connect/spi/Connection.java line 105: > 103: * If the length of the packet (as indictaed by the first > 104: * 4 bytes) is less than 11 bytes, or an I/O error occurs. > 105: * @spec jdwp/jdwp-spec.html Java Debug Wire Protocol http://cr.openjdk.java.net/~jjg/8296546/api.00/jdk.jdi/com/sun/jdi/connect/spi/Connection.html#readPacket() Within this javadoc page the jdwp-spec.html references are titled "JDWP Specification", but these `@spec` references are titled "Java Debug Wire Protocol". I suggest making them more consistent. There is one more case below and this same issue also applies to TransportService.java. Perhaps the title in jdwp-spec.html should be updated. I think "Java Debug Wire Protocol (JDWP) Specification" would be good. src/jdk.jdi/share/classes/com/sun/jdi/connect/spi/TransportService.java line 79: > 77: * target VM. > 78: * > 79: * @spec jdwp/jdwp-spec.html Java Debug Wire Protocol See above comment for Connection.java. src/jdk.jdi/share/classes/module-info.java line 107: > 105: * > 106: * > 107: * @spec jpda/jpda.html Java Platform Debugger Architecture http://cr.openjdk.java.net/~jjg/8296546/api.00/jdk.jdi/module-summary.html `@spec` and `@see` sections end up one right after the other with the same content, except the `@see` section has the preferred hyperlink title. Suggest you remove the `@see` section and also update `@spec` hyperlink title to include "(JPDA)", or update the actual title in the jpda.html doc so it includes "(JPDA)" in it and then rerun your tool. src/jdk.jdwp.agent/share/classes/module-info.java line 30: > 28: * > 29: * @spec jdwp/jdwp-spec.html Java Debug Wire Protocol > 30: * @spec jdwp/jdwp-transport.html Java Debug Wire Protocol Transport > Interface (jdwpTransport) http://cr.openjdk.java.net/~jjg/8296546/api.00/jdk.jdwp.agent/module-summary.html The end result here is not very clean. You have the same two specs being referred to just a few lines apart, and the hyperlink titles are not even close to be the same, even though the links are the same. Maybe the "@see" section should be removed. - PR: https://git.openjdk.org/jdk/pull/11
RFR: 8294672: Typo in description of JDWP VirtualMachine/AllThreads command
Fix typo: "and and" => "and" - Commit messages: - fix typo in VirtualMachine.AllThreads Changes: https://git.openjdk.org/jdk/pull/10895/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=10895&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8294672 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/10895.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10895/head:pull/10895 PR: https://git.openjdk.org/jdk/pull/10895
Re: RFR: 8294321: Fix typos in files under test/jdk/java, test/jdk/jdk, test/jdk/jni [v2]
On Fri, 7 Oct 2022 12:51:26 GMT, Alan Bateman wrote: >> Michael Ernst has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains six commits: >> >> - Reinstate typos in Apache code that is copied into the JDK >> - Merge ../jdk-openjdk into typos-typos >> - Remove file that was removed upstream >> - Fix inconsistency in capitalization >> - Undo change in zlip >> - Fix typos > > src/java.se/share/data/jdwp/jdwp.spec line 101: > >> 99: "> href=../../api/java.base/java/lang/Thread.html#platform-threads>platform >> thread " >> 100: "in the target VM. This includes platform threads created with >> the Thread " >> 101: "API and all native threads attached to the target VM with JNI >> code." > > The spec for the JDWP AllThreads command was significantly reworded in Java > 19 so this is where this typo crept in. We have JDK-8294672 tracking it to > fix for Java 20, maybe you should take it? Since this PR has gone stale, I'll be fixing this typo in jdwp.spec via [JDK-8294672](https://bugs.openjdk.org/browse/JDK-8294672). - PR: https://git.openjdk.org/jdk/pull/10029
Re: RFR: 8295729: Add jcheck whitespace checking for properties files [v3]
On Mon, 24 Oct 2022 19:21:07 GMT, Magnus Ihse Bursie wrote: >> Properties files is essentially source code. It should have the same >> whitespace checks as all other source code, so we don't get spurious >> trailing whitespace changes. >> >> With the new Skara jcheck, it is possible to increase the coverage of the >> whitespace checks (in the old mercurial version, this was more or less >> impossible). >> >> The only manual change is to `.jcheck/conf`. All other changes were made by >> running `find . -type f -iname "*.properties" | xargs gsed -i -e 's/[ >> \t]*$//'`. > > Magnus Ihse Bursie has updated the pull request incrementally with two > additional commits since the last revision: > > - Revert "Remove check for .properties from jcheck" > >This reverts commit c91fdaa19dc06351598bd1c0614e1af3bfa08ae2. > - Change trailing space and tab in values to unicode encoding Changes requested by cjplummer (Reviewer). src/jdk.management.agent/share/classes/jdk/internal/agent/resources/agent.properties line 27: > 25: > 26: agent.err.error = Error > 27: agent.err.exception= Exception thrown by the agent\u0020 I believe this space was just a typo and should be removed. Same for `agent.err.agentclass.failed` below and in all the other management property files. - PR: https://git.openjdk.org/jdk/pull/10792
Re: RFR: 8295729: Add jcheck whitespace checking for properties files
On Thu, 20 Oct 2022 18:38:43 GMT, Andy Goryachev wrote: > `White space following the property value is not ignored, and is treated as > part of the property value.` Although I didn't know this was in the spec, I suspected it might be the case. When looking at the jdk.management.agent changes, it looked like the extra space was actually a typo and was copied into all the localized versions (and not actually localized for unicode locales). In this case removing the white space is the right thing to do. It is in fact fixing an actual bug. - PR: https://git.openjdk.org/jdk/pull/10792
Re: RFR: 8291429: java/lang/Thread/virtual/ThreadAPI.java timed out on single core system [v2]
On Thu, 6 Oct 2022 13:39:25 GMT, Alan Bateman wrote: >> This is a test only change for two tests for virtual threads that >> hang/timeout on single core systems. The two tests involve pinning and >> require at least two carrier threads. The test lib used by these tests is >> updated to define a new method that ensures parallelism is at least a given >> value and both tests are updated to use this. There are a number of tests in >> the debugger area that may potentially use this in the future. > > Alan Bateman 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 four additional > commits since the last revision: > > - Replace try-with-resource with try-finally > - Merge > - Make close idempotent > - Initial commit Marked as reviewed by cjplummer (Reviewer). - PR: https://git.openjdk.org/jdk/pull/10562
Re: RFR: 8291429: java/lang/Thread/virtual/ThreadAPI.java timed out on single core system [v2]
On Thu, 6 Oct 2022 13:39:25 GMT, Alan Bateman wrote: >> This is a test only change for two tests for virtual threads that >> hang/timeout on single core systems. The two tests involve pinning and >> require at least two carrier threads. The test lib used by these tests is >> updated to define a new method that ensures parallelism is at least a given >> value and both tests are updated to use this. There are a number of tests in >> the debugger area that may potentially use this in the future. > > Alan Bateman 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 four additional > commits since the last revision: > > - Replace try-with-resource with try-finally > - Merge > - Make close idempotent > - Initial commit If these tests are ever run with the virtual thread wrapper, will we end up being short a carrier thread? It's unclear to me if the tests will always have at least one unpinned carrier thread to work with. - PR: https://git.openjdk.org/jdk/pull/10562
Re: RFR: 8291429: java/lang/Thread/virtual/ThreadAPI.java timed out on single core system
On Wed, 5 Oct 2022 19:58:24 GMT, Alan Bateman wrote: >> It's not a matter of whether or not the test needs to restore it. It _will_ >> restore it if there is a GC, and if this happens before the test completes, >> it could find itself without enough carrier threads. > > No, restoration requires a call to close. Maybe you are assuming that > AutoCloseable implementations setup a cleaner to do that? Yes, that was my point of confusion. I thought collection triggered calling close, but I see now when you want it to be closed, you rely on try-with-resources to call close() method. - PR: https://git.openjdk.org/jdk/pull/10562
Re: RFR: 8291429: java/lang/Thread/virtual/ThreadAPI.java timed out on single core system
On Wed, 5 Oct 2022 19:30:46 GMT, Alan Bateman wrote:
>> test/jdk/java/lang/management/ThreadMXBean/VirtualThreadDeadlocks.java line
>> 30:
>>
>>> 28: * platform and virtual threads in deadlock
>>> 29: * @enablePreview
>>> 30: * @modules java.base/java.lang:+open java.management
>>
>> Can you explain the need for this change?
>
> That is jtreg foo to open java.lang, needed to access a private field.
ok
>> test/jdk/java/lang/management/ThreadMXBean/VirtualThreadDeadlocks.java line
>> 68:
>>
>>> 66: public static void main(String[] args) throws Exception {
>>> 67: // need at least two carrier threads due to pinning
>>> 68: VThreadRunner.ensureParallelism(2);
>>
>> In this test case why is there no need to maintain a reference to the
>> returned AutoCloseable? Isn't there a chance it can be collected and the old
>> parallelism value restored while the test is running.
>
> This test doesn't need to restore it so it doesn't keep a reference and not
> an issue if it is GC'ed.
It's not a matter of whether or not the test needs to restore it. It _will_
restore it if there is a GC, and if this happens before the test completes, it
could find itself without enough carrier threads.
>> test/lib/jdk/test/lib/thread/VThreadRunner.java line 176:
>>
>>> 174: if (!closed) {
>>> 175: closed = true;
>>> 176: pool.setParallelism(parallelism);
>>
>> What is the rationale for restoring the parallelism? It's just a test. Is
>> this really necessary? Are we reusing the JVM to run other tests, and even
>> if we are does it matter?
>
> Look at the ThreadAPI test as an example. It's a TestNG with dozens of test
> methods. All but one can run on single core systems.
Ok.
-
PR: https://git.openjdk.org/jdk/pull/10562
Re: RFR: 8291429: java/lang/Thread/virtual/ThreadAPI.java timed out on single core system
On Tue, 4 Oct 2022 17:59:36 GMT, Alan Bateman wrote:
> This is a test only change for two tests for virtual threads that
> hang/timeout on single core systems. The two tests involve pinning and
> require at least two carrier threads. The test lib used by these tests is
> updated to define a new method that ensures parallelism is at least a given
> value and both tests are updated to use this. There are a number of tests in
> the debugger area that may potentially use this in the future.
test/jdk/java/lang/management/ThreadMXBean/VirtualThreadDeadlocks.java line 30:
> 28: * platform and virtual threads in deadlock
> 29: * @enablePreview
> 30: * @modules java.base/java.lang:+open java.management
Can you explain the need for this change?
test/jdk/java/lang/management/ThreadMXBean/VirtualThreadDeadlocks.java line 68:
> 66: public static void main(String[] args) throws Exception {
> 67: // need at least two carrier threads due to pinning
> 68: VThreadRunner.ensureParallelism(2);
In this test case why is there no need to maintain a reference to the returned
AutoCloseable? Isn't there a chance it can be collected and the old parallelism
value restored while the test is running.
test/lib/jdk/test/lib/thread/VThreadRunner.java line 32:
> 30:
> 31: /**
> 32: * Helper class for running tests tasks in a virtual thread.
"tests" => "test"
test/lib/jdk/test/lib/thread/VThreadRunner.java line 149:
> 147: * Ensures that the virtual thread scheduler's target parallelism is
> at least the
> 148: * given size. If the current parallelism is less than size then it
> is changed to
> 149: * size. This method returns an AutoCloseable, its close method
> restores the
" * size. This method returns an AutoCloseable whose close method..."
test/lib/jdk/test/lib/thread/VThreadRunner.java line 176:
> 174: if (!closed) {
> 175: closed = true;
> 176: pool.setParallelism(parallelism);
What is the rationale for restoring the parallelism? It's just a test. Is this
really necessary? Are we reusing the JVM to run other tests, and even if we are
does it matter?
-
PR: https://git.openjdk.org/jdk/pull/10562
Re: RFR: 8294321: Fix typos in files under test/jdk/java, test/jdk/jdk, test/jdk/jni [v2]
On Wed, 28 Sep 2022 15:32:11 GMT, Michael Ernst wrote: > The title was edited by someone other than me, as you can see from the PR > history. The PR title needs to match the CR synopsis, so update the CR first, and then update the PR. - PR: https://git.openjdk.org/jdk/pull/10029
Re: RFR: 8293613: need to properly handle and hide tmp VTMS transitions [v4]
On Fri, 23 Sep 2022 20:38:17 GMT, Serguei Spitsyn wrote: >> src/hotspot/share/prims/jvmtiThreadState.cpp line 273: >> >>> 271: >>> 272: assert(!thread->is_in_tmp_VTMS_transition(), "sanity check"); >>> 273: assert(!thread->is_in_VTMS_transition(), "VTMS_transition sanity >>> check"); >> >> Why not cover both of these with `!thread->is_in_any_VTMS_transition()`? > > It is to be clear what condition caused the assert as they are different > beasts. ok - PR: https://git.openjdk.org/jdk/pull/10321
Re: RFR: 8293613: need to properly handle and hide tmp VTMS transitions [v4]
On Thu, 22 Sep 2022 09:16:28 GMT, Serguei Spitsyn wrote: >> There are several places in VirtualThread class implementation where virtual >> threads are being mounted or unmounted, so there is a transition of the >> JavaThread identity from carrier thread to virtual thread and back. The >> execution state in such transitions is inconsistent, and so, has to be >> invisible to JVM TI agents. Such transitions are named as VTMS (virtual >> thread mount state) transitions, and there is a JVM TI mechanism which >> supports them. Besides normal VTMS transitions there are also a couple of >> performance-critical contexts (in `VirtualThread` methods: >> `scheduleUnpark()`, `cancel()` and `unpark()`) which can be named as >> temporary VTMS transitions. Execution state of such temporary VTMS >> transitions has to be also invisible to the JVM TI agent which is >> implemented in the current update. >> >> There are a couple of details of this fix to highlight: >> - A JavaThread which is in temporary VTMS transition is marked with a >> special bit `_is_in_tmp_VTMS_transition`. It is done with the native >> notification method `toggleJvmtiTmpVTMSTrans()`. >> - A couple of lambda form classes can be created in context of temporary >> VTMS transitions, so their `ClassLoad`, `ClassPrepare` and `CFLH` events are >> ignored. >> - Suspending threads in temporary transition is allowed. >> >> The fix was tested in Loom repository by using `JTREG_MAIN_WRAPPER=Virtual` >> mode of execution which forces all main threads to be run as virtual. All >> `JVM TI` and `JDI` tests were run on all platforms. It includes >> `Kitchensink` and `JCK` tests. Additionally, the fix was tested in the jdk >> 20 repository. It includes `JVM TI` and `JDI` tests and tiers 1-6. > > Serguei Spitsyn has updated the pull request incrementally with one > additional commit since the last revision: > > 1. addressed review comments from Chris; added VirtualThread.java update > from Alan Marked as reviewed by cjplummer (Reviewer). - PR: https://git.openjdk.org/jdk/pull/10321
Re: RFR: 8293613: need to properly handle and hide tmp VTMS transitions [v4]
On Fri, 23 Sep 2022 09:30:32 GMT, Serguei Spitsyn wrote:
>> src/hotspot/share/runtime/javaThread.hpp line 652:
>>
>>> 650: void set_is_in_VTMS_transition(bool val);
>>> 651: void toggle_is_in_tmp_VTMS_transition(){
>>> _is_in_tmp_VTMS_transition = !_is_in_tmp_VTMS_transition; };
>>> 652:
>>
>> My suggestion was to have the term "in VTMS transition" be inclusive of temp
>> transitions. So based on your current names I would suggest:
>>
>> - is_in_VTMS_transition -> is_in_non_tmp_VTMS_transition
>> - is_in_any_VTMS_transition -> is_in_VTMS_transition
>>
>> But that becomes a problem for `set_is_in_VTMS_transition`, which would need
>> to be renamed `set_is_in_non_tmp_VTMS_transition`, which I'm guessing you
>> don't want to do. So let's instead just hope this all goes away before
>> thinking about it any more.
>
> Thank you for sharing your suggestion.
> To be honest, I'm inclined to keep the two as simple as possible, independent
> end mutually exclusive.
> Temporary transitions have big difference comparing to normal transitions.
> They are allowed to be suspended and do not clash with VTMS disablers.
> Please, let me know if are okay with this.
>
> Unfortunately, it seems, Alan got some difficulties in getting rid of
> temporary transitions.
> I'll double check on it just to be sure I understand it correctly.
ok
-
PR: https://git.openjdk.org/jdk/pull/10321
Re: RFR: 8293613: need to properly handle and hide tmp VTMS transitions [v4]
On Thu, 22 Sep 2022 09:16:28 GMT, Serguei Spitsyn wrote: >> There are several places in VirtualThread class implementation where virtual >> threads are being mounted or unmounted, so there is a transition of the >> JavaThread identity from carrier thread to virtual thread and back. The >> execution state in such transitions is inconsistent, and so, has to be >> invisible to JVM TI agents. Such transitions are named as VTMS (virtual >> thread mount state) transitions, and there is a JVM TI mechanism which >> supports them. Besides normal VTMS transitions there are also a couple of >> performance-critical contexts (in `VirtualThread` methods: >> `scheduleUnpark()`, `cancel()` and `unpark()`) which can be named as >> temporary VTMS transitions. Execution state of such temporary VTMS >> transitions has to be also invisible to the JVM TI agent which is >> implemented in the current update. >> >> There are a couple of details of this fix to highlight: >> - A JavaThread which is in temporary VTMS transition is marked with a >> special bit `_is_in_tmp_VTMS_transition`. It is done with the native >> notification method `toggleJvmtiTmpVTMSTrans()`. >> - A couple of lambda form classes can be created in context of temporary >> VTMS transitions, so their `ClassLoad`, `ClassPrepare` and `CFLH` events are >> ignored. >> - Suspending threads in temporary transition is allowed. >> >> The fix was tested in Loom repository by using `JTREG_MAIN_WRAPPER=Virtual` >> mode of execution which forces all main threads to be run as virtual. All >> `JVM TI` and `JDI` tests were run on all platforms. It includes >> `Kitchensink` and `JCK` tests. Additionally, the fix was tested in the jdk >> 20 repository. It includes `JVM TI` and `JDI` tests and tiers 1-6. > > Serguei Spitsyn has updated the pull request incrementally with one > additional commit since the last revision: > > 1. addressed review comments from Chris; added VirtualThread.java update > from Alan src/hotspot/share/prims/jvmtiThreadState.cpp line 273: > 271: > 272: assert(!thread->is_in_tmp_VTMS_transition(), "sanity check"); > 273: assert(!thread->is_in_VTMS_transition(), "VTMS_transition sanity > check"); Why not cover both of these with `!thread->is_in_any_VTMS_transition()`? - PR: https://git.openjdk.org/jdk/pull/10321
Re: RFR: 8293592: Remove JVM_StopThread, stillborn, and related cleanup
On Fri, 23 Sep 2022 06:17:34 GMT, David Holmes wrote:
> Now that Thread.stop has been degraded to throw
> `UnsupportedOperationException` (JDK-8299610) the only direct source of async
> exceptions is from JVMTI `StopThread`. We can remove the `JVM_StopThread`
> code, remove the `stillborn` field from `java.lang.Thread` and its associated
> accesses from the VM, and we can stop special-casing `ThreadDeath` handling
> (as was done for the JDK code as part of JDK-8299610).
>
> Note that JVMTI `StopThread` can only act on a thread that is alive, so it is
> no longer possible to stop a thread before it has been started.
>
> Also note that there is a change in behaviour for JNI `ExceptionDescribe` as
> it no longer ignores `ThreadDeath` exceptions (not that it was ever specified
> to ignore them, it simply mirrored the behaviour of the default
> `UncaughtExceptionHandler` in `java.lang.ThreadGroup` - which also no longer
> ignores them so the mirroring behaviour remains the same).
>
> Testing: tiers 1-3
src/jdk.jdi/share/classes/com/sun/tools/jdi/VirtualMachineManagerImpl.java line
102:
> 100: try {
> 101: connector = connectors.next();
> 102: } catch (Exception | Error x) {
Maybe this should just catch `Throwable`, although it is unclear to me why we
would want to catch any exception here.
src/jdk.jdi/share/classes/com/sun/tools/jdi/VirtualMachineManagerImpl.java line
126:
> 124: try {
> 125: transportService = transportServices.next();
> 126: } catch (Exception | Error x) {
Another that could be just catch `Throwable`
-
PR: https://git.openjdk.org/jdk/pull/10400
Re: RFR: 8293613: need to properly handle and hide tmp VTMS transitions [v4]
On Thu, 22 Sep 2022 09:16:28 GMT, Serguei Spitsyn wrote:
>> There are several places in VirtualThread class implementation where virtual
>> threads are being mounted or unmounted, so there is a transition of the
>> JavaThread identity from carrier thread to virtual thread and back. The
>> execution state in such transitions is inconsistent, and so, has to be
>> invisible to JVM TI agents. Such transitions are named as VTMS (virtual
>> thread mount state) transitions, and there is a JVM TI mechanism which
>> supports them. Besides normal VTMS transitions there are also a couple of
>> performance-critical contexts (in `VirtualThread` methods:
>> `scheduleUnpark()`, `cancel()` and `unpark()`) which can be named as
>> temporary VTMS transitions. Execution state of such temporary VTMS
>> transitions has to be also invisible to the JVM TI agent which is
>> implemented in the current update.
>>
>> There are a couple of details of this fix to highlight:
>> - A JavaThread which is in temporary VTMS transition is marked with a
>> special bit `_is_in_tmp_VTMS_transition`. It is done with the native
>> notification method `toggleJvmtiTmpVTMSTrans()`.
>> - A couple of lambda form classes can be created in context of temporary
>> VTMS transitions, so their `ClassLoad`, `ClassPrepare` and `CFLH` events are
>> ignored.
>> - Suspending threads in temporary transition is allowed.
>>
>> The fix was tested in Loom repository by using `JTREG_MAIN_WRAPPER=Virtual`
>> mode of execution which forces all main threads to be run as virtual. All
>> `JVM TI` and `JDI` tests were run on all platforms. It includes
>> `Kitchensink` and `JCK` tests. Additionally, the fix was tested in the jdk
>> 20 repository. It includes `JVM TI` and `JDI` tests and tiers 1-6.
>
> Serguei Spitsyn has updated the pull request incrementally with one
> additional commit since the last revision:
>
> 1. addressed review comments from Chris; added VirtualThread.java update
> from Alan
src/hotspot/share/runtime/javaThread.hpp line 652:
> 650: void set_is_in_VTMS_transition(bool val);
> 651: void toggle_is_in_tmp_VTMS_transition(){
> _is_in_tmp_VTMS_transition = !_is_in_tmp_VTMS_transition; };
> 652:
My suggestion was to have the term "in VTMS transition" be inclusive of temp
transitions. So based on your current names I would suggest:
- is_in_VTMS_transition -> is_in_non_tmp_VTMS_transition
- is_in_any_VTMS_transition -> is_in_VTMS_transition
But that becomes a problem for `set_is_in_VTMS_transition`, which would need to
be renamed `set_is_in_non_tmp_VTMS_transition`, which I'm guessing you don't
want to do. So let's instead just hope this all goes away before thinking about
it any more.
-
PR: https://git.openjdk.org/jdk/pull/10321
Re: RFR: 8293613: need to properly handle and hide tmp VTMS transitions [v3]
On Tue, 20 Sep 2022 22:41:50 GMT, Serguei Spitsyn wrote: >> src/hotspot/share/runtime/javaThread.cpp line 1174: >> >>> 1172: #if INCLUDE_JVMTI >>> 1173: // Suspending a JavaThread in VTMS transition or disabling VTMS >>> transitions can cause deadlocks. >>> 1174: assert(!is_in_non_tmp_VTMS_transition(), "no suspend allowed in >>> VTMS transition"); >> >> IMHO, a non tmp VTMS transition should be considered a type of VTMS >> transition, so the assert check here does not really match the assert text. >> Also see my related comments below on naming and use of these flags and APIs. > > Thanks. Accepted. I thought I was pointing out a naming issue, but your change indicates that it was actually a bug, and it is ok to be in a tmp transition here. Is that correct? - PR: https://git.openjdk.org/jdk/pull/10321
Re: RFR: 8293613: need to properly handle and hide tmp VTMS transitions [v3]
On Tue, 20 Sep 2022 22:15:49 GMT, Serguei Spitsyn wrote:
>> src/hotspot/share/prims/jvmtiEnvBase.cpp line 713:
>>
>>> 711: if (!jt->is_in_tmp_VTMS_transition()) {
>>> 712: jvf = check_and_skip_hidden_frames(jt, jvf);
>>> 713: }
>>
>> I think this comment needs some help. It's hard to match it up with what the
>> code is doing. I think you are saying you want to avoid skipping hidden
>> frames when in transition, although if that's the case, it's not clear to me
>> why not skipping is ok.
>>
>> Also is skipping (or not skipping) ok regardless of the
>> JvmtiEnvBase::is_cthread_with_continuation() result?
>
> Thank you for reviewing and the comment, Chris.
> I agree, this part and related comment is kind of obscure.
> I'll think how to make the comment better.
> In fact, all temporary VTMS transitions do temporary switch the `JavaThread`
> identity from virtual thread to carrier. There is no need to skip frames
> because there are no real carrier thread frames at the top. Moreover, any
> attempt to skip frames which are in transition works incorrectly and gives an
> empty stack. The check `JvmtiEnvBase::is_cthread_with_continuation()` is
> needed to make sure we have a deal with a continuation. There is no need to
> skip frames otherwise.
It's still not clear to me if the result of
`JvmtiEnvBase::is_cthread_with_continuation()` impacts if you have to possibly
skip hidden frames. In other words, do you always have to do the `if` block
that follows, no matter what `JvmtiEnvBase::is_cthread_with_continuation()`
returns? If you don't, then maybe instead of a "? :" expression, an if/else
would be better. I'm not sure if the following is correct, but something like:
javaVFrame *jvf;
if (JvmtiEnvBase::is_cthread_with_continuation(jt)) {
jvf = jt->carrier_last_java_vframe(reg_map_p);
} else {
jvf - jt->last_java_vframe(reg_map_p);
// Skipping hidden frames when jt->is_in_tmp_VTMS_transition()==true
results
// in returning jvf==NULL, and so, empty stack traces for carrier threads.
if (!jt->is_in_tmp_VTMS_transition()) {
jvf = check_and_skip_hidden_frames(jt, jvf);
}
}
-
PR: https://git.openjdk.org/jdk/pull/10321
Re: RFR: 8249627: Degrade Thread.suspend and Thread.resume
On Sun, 18 Sep 2022 16:32:31 GMT, Alan Bateman wrote: > Degrade Thread.suspend/resume to throw UOE unconditionally. > > Another step in the removal of this deadlock prone mis-feature from the > user-facing API. Thread.suspend/resume have been deprecated since JDK 1.2 > (1998) and terminally deprecated since Java 14. ThreadGroup.suspend/resume > were degraded to throw UOE in Java 19. As of Java 19, Thread.suspend/resume > continues to work for platform threads but throws UOE for virtual threads. > The next step is to degrade both methods to throw UOE for all threads. A > corpus search of 19M classes in 113k JAR files found only 22 classes using > these methods so this change is unlikely to be disruptive. > > The change requires some minor adjustments to the JVM TI and JDWP > specifications, and a minor update to the JDI docs. > > Leonid Mesnik is working on > [PR10351](https://github.com/openjdk/jdk/pull/10351) to remove/replace the > last few usages of Thread.suspend/resume from the hotspot tests (most of > these can use JVMTI SuspendThread/ResumeThread). Changes look good. - Marked as reviewed by cjplummer (Reviewer). PR: https://git.openjdk.org/jdk/pull/10324
Re: RFR: 8293613: need to properly handle and hide tmp VTMS transitions [v3]
On Mon, 19 Sep 2022 19:18:57 GMT, Serguei Spitsyn wrote:
>> There are several places in VirtualThread class implementation where virtual
>> threads are being mounted or unmounted, so there is a transition of the
>> JavaThread identity from carrier thread to virtual thread and back. The
>> execution state in such transitions is inconsistent, and so, has to be
>> invisible to JVM TI agents. Such transitions are named as VTMS (virtual
>> thread mount state) transitions, and there is a JVM TI mechanism which
>> supports them. Besides normal VTMS transitions there are also a couple of
>> performance-critical contexts (in `VirtualThread` methods:
>> `scheduleUnpark()`, `cancel()` and `unpark()`) which can be named as
>> temporary VTMS transitions. Execution state of such temporary VTMS
>> transitions has to be also invisible to the JVM TI agent which is
>> implemented in the current update.
>>
>> There are a couple of details of this fix to highlight:
>> - A JavaThread which is in temporary VTMS transition is marked with a
>> special bit `_is_in_tmp_VTMS_transition`. It is done with the native
>> notification method `toggleJvmtiTmpVTMSTrans()`.
>> - A couple of lambda form classes can be created in context of temporary
>> VTMS transitions, so their `ClassLoad`, `ClassPrepare` and `CFLH` events are
>> ignored.
>> - Suspending threads in temporary transition is allowed.
>>
>> The fix was tested in Loom repository by using `JTREG_MAIN_WRAPPER=Virtual`
>> mode of execution which forces all main threads to be run as virtual. All
>> `JVM TI` and `JDI` tests were run on all platforms. It includes
>> `Kitchensink` and `JCK` tests. Additionally, the fix was tested in the jdk
>> 20 repository. It includes `JVM TI` and `JDI` tests and tiers 1-6.
>
> Serguei Spitsyn has updated the pull request incrementally with one
> additional commit since the last revision:
>
> fixed typo in VirtualThread.c
src/hotspot/share/prims/jvmtiEnvBase.cpp line 713:
> 711: if (!jt->is_in_tmp_VTMS_transition()) {
> 712: jvf = check_and_skip_hidden_frames(jt, jvf);
> 713: }
I think this comment needs some help. It's hard to match it up with what the
code is doing. I think you are saying you want to avoid skipping hidden frames
when in transition, although if that's the case, it's not clear to me why not
skipping is ok.
Also is skipping (or not skipping) ok regardless of the
JvmtiEnvBase::is_cthread_with_continuation() result?
src/hotspot/share/prims/jvmtiExport.cpp line 1055:
> 1053: if (JavaThread::current()->is_in_tmp_VTMS_transition()) {
> 1054: return false;
> 1055: }
You mentioned this in the PR description. However, it's not clear to me why
this is ok. Also, it should be commented.
Same thing for changes in JvmtiExport::post_class_load() and
JvmtiExport::post_class_prepare().
src/hotspot/share/runtime/javaThread.cpp line 1174:
> 1172: #if INCLUDE_JVMTI
> 1173: // Suspending a JavaThread in VTMS transition or disabling VTMS
> transitions can cause deadlocks.
> 1174: assert(!is_in_non_tmp_VTMS_transition(), "no suspend allowed in VTMS
> transition");
IMHO, a non tmp VTMS transition should be considered a type of VTMS transition,
so the assert check here does not really match the assert text. Also see my
related comments below on naming and use of these flags and APIs.
src/hotspot/share/runtime/javaThread.hpp line 650:
> 648: void set_is_in_VTMS_transition(bool val);
> 649: bool is_in_tmp_VTMS_transition() const { return
> _is_in_tmp_VTMS_transition; }
> 650: void toggle_is_in_tmp_VTMS_transition(){
> _is_in_tmp_VTMS_transition = !_is_in_tmp_VTMS_transition; };
The naming of the flags does not match up with the naming of the APIs, which is
confusing. An API named is_in_VTMS_transition() should return
_is_in_VTMS_transition. I think part of problem is that _is_in_VTMS_transition
is not a superset of _is_in_tmp_VTMS_transition. The flags are never both set
true, although both can be false. Maybe this should be changed to make it an
invariant that if _is_in_tmp_VTMS_transit is true, then
_is_in_tmp_VTMS_transition is true.
-
PR: https://git.openjdk.org/jdk/pull/10321
Re: RFR: 8289610: Degrade Thread.stop
On Fri, 9 Sep 2022 12:44:31 GMT, Alan Bateman wrote: > Degrade Thread.stop to throw UOE unconditionally, deprecate ThreadDeath for > removal, and remove the remaining special handling of ThreadDeath from the > JDK. > > Thread.stop is inherently unsafe and has been deprecated since JDK 1.2 (1998) > with a link to a supplementary page that explains the rationale. Some of the > API surface has already been degraded or removed: Thread.stop(Throwable) was > degraded to throw UOE in Java 8 and removed in Java 11, and ThreadGroup.stop > was degraded to throw UOE in Java 19. As of Java 19, the no-arg Thread.stop > continues to work as before for platform threads but throws UOE for virtual > threads. The next step in the glacial pace removal is the degrading of the > no-arg Thread.stop method to throw UOE for all threads. > > To keep things manageable, the change proposed here leaves JVM_StopThread in > place. A separate issue will remove it and do other cleanup/removal in the > VM. We have another JBS issue for the updates to the JLS and JVMS where > asynchronous exceptions are defined. There is also some remaining work on a > test class used by 6 jshell tests - if they aren't done in time then we will > temporarily exclude them. > > The change here has no impact on the debugger APIs (JVM TI StopThread, JDWP > ThreadReference/Stop, and JDI ThreadReference.stop). Debuggers can continue > to cause threads to throw an asynchronous exception, as might be done when > simulating code throwing an exception at some point in the code. serviceability changes look good - Marked as reviewed by cjplummer (Reviewer). PR: https://git.openjdk.org/jdk/pull/10230
RFR: 8283224: Remove THREAD_NOT_ALIVE from possible JDWP error codes
THREAD_NOT_ALIVE originates from JVMTI. However, the debug agent converts it to INVALID_THREAD before passing it on to the debug agent client (the debugger, usually JDI), so debug agent users (JDI) should never see it. Currently ThreadReference.forceEarlyReturn() is the only API that even bothers to check for it, and it throws com.sun.jdi.IllegalThreadStateException, which is the same thing it already does for INVALID_THREAD. In the JDWP spec I change the description of THREAD_NOT_ALIVE to "Not used". If you have a suggestion for better wording, please let me now. (I was thinking maybe "Unused" would be better. In the JDI ThreadReference.forceEarlyReturn implementation, I removed the code that checks for THREAD_NOT_ALIVE since it should never occur. There is no behavior change associated with this change, and there is no JDI spec update necessary. The spec already says IllegalThreadStateException means "the thread is not suspended", and not being alive implies not suspended. Note the JDWP spec for ThreadReference.ForceEarlyReturn used to mention THREAD_NOT_ALIVE as a possible error code, but it was removed as part of the loom work. - Commit messages: - Cleanup THREAD_NOT_ALIVE usage Changes: https://git.openjdk.org/jdk/pull/10189/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=10189&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8283224 Stats: 4 lines in 2 files changed: 0 ins; 3 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/10189.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10189/head:pull/10189 PR: https://git.openjdk.org/jdk/pull/10189
Re: RFR: 8292201: serviceability/sa/ClhsdbThreadContext.java fails with "'Thread "Common-Cleaner"' missing from stdout/stderr" [v2]
On Thu, 1 Sep 2022 17:20:33 GMT, Chris Plummer wrote:
>> While dumping all registers (and doing a findpc on each), the following
>> exception occurred for r8:
>>
>>
>> r8: 0x00750e4fdffc
>> Error: java.lang.ArrayIndexOutOfBoundsException: Index 4099 out of bounds
>> for length 4096
>> java.lang.ArrayIndexOutOfBoundsException: Index 4099 out of bounds for
>> length 4096
>> at jdk.hotspot.agent/sun.jvm.hotspot.debugger.Page.getLong(Page.java:182)
>> at
>> jdk.hotspot.agent/sun.jvm.hotspot.debugger.PageCache.getLong(PageCache.java:100)
>> at
>> jdk.hotspot.agent/sun.jvm.hotspot.debugger.DebuggerBase.readCInteger(DebuggerBase.java:364)
>> at
>> jdk.hotspot.agent/sun.jvm.hotspot.debugger.DebuggerBase.readAddressValue(DebuggerBase.java:462)
>> at
>> jdk.hotspot.agent/sun.jvm.hotspot.debugger.windbg.WindbgDebuggerLocal.readAddress(WindbgDebuggerLocal.java:312)
>> at
>> jdk.hotspot.agent/sun.jvm.hotspot.debugger.windbg.WindbgAddress.getAddressAt(WindbgAddress.java:71)
>> at
>> jdk.hotspot.agent/sun.jvm.hotspot.utilities.PointerFinder.find(PointerFinder.java:58)
>> at
>> jdk.hotspot.agent/sun.jvm.hotspot.runtime.JavaThread.printThreadContextOn(JavaThread.java:493)
>>
>>
>> This exception is the result of using PointerFinder ("findpc") on invalid
>> address that starts on the last 32-bit word of a page. "page" in this case
>> is referring to a page in SA's PageCache, which are always 4k in size. Note
>> findpc is frequently done using an invalid address. In this test case it is
>> being called on each x64 register, which could contain anything. findpc
>> relies on getting some sort of AddressException when this happens, and will
>> then say the address points to something that is unknown. However, in the
>> case of the address pointing to the last 32-bot word of a page, it results
>> in an ArrayIndexOutOfBoundsException when the page cache tries to read past
>> the end of the page. This is happening in Page.getLong(), which you can see
>> in the stack trace.
>>
>> The main culprit here is some weakening of the alignment checks being done.
>> The alignment check should have resulted in an UnalignedAddressException
>> long before we ever got to Page.getLong(). However, we have the following
>> override, which is allowing the unaligned address to pass the alignment
>> check.
>>
>>
>>public void checkAlignment(long address, long alignment) {
>> // Need to override default checkAlignment because we need to
>> // relax alignment constraints on Bsd/x86
>> if ( (address % alignment != 0)
>> &&(alignment != 8 || address % 4 != 0)) {
>> throw new UnalignedAddressException(
>> "Trying to read at address: "
>> + addressValueToString(address)
>> + " with alignment: " + alignment,
>> address);
>> }
>>}
>> };
>>
>>
>> This allows a pointer to a 64-bit value to only be 32-bit aligned. But
>> there's more to it than that. I modified ClhsdbFindPC.java to fetch a tid (a
>> thread address), and did a findpc on the address OR'd with 0xffc. `findpc`
>> uses PointerFinder. This forced a read of a 64-bit value that starts in the
>> last 32-bits of a page. It passed on linux-x64 but failed on windowx-x64 in
>> the same manner described in the description of this CR. The difference
>> between the two implementations is that windows relies on the default
>> implementation of DebuggerBase.readCInteger() whereas linux has an override.
>> DebuggerBase.readCInteger() does the following:
>>
>>
>> public long readCInteger(long address, long numBytes, boolean isUnsigned) {
>> utils.checkAlignment(address, numBytes);
>> if (useFastAccessors) {
>> if (isUnsigned) {
>> switch((int) numBytes) {
>> case 1: return cache.getByte(address) & 0xFF;
>> case 2: return cache.getShort(address, bigEndian) & 0x;
>> case 4: return cache.getInt(address, bigEndian) & 0xL;
>> case 8: return cache.getLong(address, bigEndian);
>> ...
>>
>>
>> There is an alignment check here, but it is the "relaxed" override shown
>> above, which allows 64-bit addresses on 32-bit boundaries. The override in
>> LinuxDebuggerLocal is:
>>
>>
>> /*
Re: RFR: 8292201: serviceability/sa/ClhsdbThreadContext.java fails with "'Thread "Common-Cleaner"' missing from stdout/stderr" [v2]
aries
> if (numBytes == 8) {
> utils.checkAlignment(address, 4);
> } else {
> utils.checkAlignment(address, numBytes);
> }
> byte[] data = readBytes(address, numBytes);
> return utils.dataToCInteger(data, isUnsigned);
>
>
> Although there is a relaxed alignment check here also, the code that reads
> from the address does not assume all the bytes are on the same page. It uses
> readBytes() intead of cache.getLong(), so it won't run into the PageCache
> issue with reading a 64-bit value that starts in the last 32-bit word of a
> page.
>
> I think the introduction of these relaxed alignment checks has a muddled
> history, probably made more muddled by ports cloning code that maybe wasn't
> necessary. Probably also initial fixes (the relaxed alignment check) seemed
> to work at first, but later the PageCache issue was discovered, leading to
> readBytes() workaround in the LinuxDebuggerLocal.readCInteger() override, but
> was not also done on other ports (so we got this CR for windows).
>
> For 64-bit support it's clear this easing of the 64-bit alignment is not
> needed, and getting rid of it would result in the proper
> UnalignedAddressException being thrown. The question is whether it is still
> needed for 32-bit x86, and if so is it needed on all ports.
>
> I can't test linux-x86, so I can't tell if it still allows 64-bit values on
> 32-bit aligned addresses, so for now I'll assume it does. So the approach
> being taken is whenever an address of a 64-bit type points to the last 32-bit
> word of a page, use readBytes() to get the 64-bit value one byte at a time.
> It still uses the page cache in the end, but it doesn't try to get all 8
> bytes from the same page. Note for 64-bit systems, the conditoin will never
> arise because of the removal of the relaxed alignment check. Instead there
> will be an UnalignedAddressException at an early point when the alignment
> check is made.
>
> One windfall of this change is now we always read 64-bit values from the page
> cache in a way that is much more efficient than reading a byte at a time.
> This has resulted in about a 25% performance improvement in a test I have
> that does a heap dump.
Chris Plummer has updated the pull request with a new target base due to a
merge or a rebase. The pull request now contains four commits:
- Merge
- Fix jcheck error
- Undo some temp test changes.
- Fix 64-bit alignment requirements
-
Changes: https://git.openjdk.org/jdk/pull/10090/files
Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=10090&range=01
Stats: 227 lines in 11 files changed: 52 ins; 154 del; 21 mod
Patch: https://git.openjdk.org/jdk/pull/10090.diff
Fetch: git fetch https://git.openjdk.org/jdk pull/10090/head:pull/10090
PR: https://git.openjdk.org/jdk/pull/10090
RFR: 8292201: serviceability/sa/ClhsdbThreadContext.java fails with "'Thread "Common-Cleaner"' missing from stdout/stderr"
While dumping all registers (and doing a findpc on each), the following
exception occurred for r8:
r8: 0x00750e4fdffc
Error: java.lang.ArrayIndexOutOfBoundsException: Index 4099 out of bounds for
length 4096
java.lang.ArrayIndexOutOfBoundsException: Index 4099 out of bounds for length
4096
at jdk.hotspot.agent/sun.jvm.hotspot.debugger.Page.getLong(Page.java:182)
at
jdk.hotspot.agent/sun.jvm.hotspot.debugger.PageCache.getLong(PageCache.java:100)
at
jdk.hotspot.agent/sun.jvm.hotspot.debugger.DebuggerBase.readCInteger(DebuggerBase.java:364)
at
jdk.hotspot.agent/sun.jvm.hotspot.debugger.DebuggerBase.readAddressValue(DebuggerBase.java:462)
at
jdk.hotspot.agent/sun.jvm.hotspot.debugger.windbg.WindbgDebuggerLocal.readAddress(WindbgDebuggerLocal.java:312)
at
jdk.hotspot.agent/sun.jvm.hotspot.debugger.windbg.WindbgAddress.getAddressAt(WindbgAddress.java:71)
at
jdk.hotspot.agent/sun.jvm.hotspot.utilities.PointerFinder.find(PointerFinder.java:58)
at
jdk.hotspot.agent/sun.jvm.hotspot.runtime.JavaThread.printThreadContextOn(JavaThread.java:493)
This exception is the result of using PointerFinder ("findpc") on invalid
address that starts on the last 32-bit word of a page. "page" in this case is
referring to a page in SA's PageCache, which are always 4k in size. Note findpc
is frequently done using an invalid address. In this test case it is being
called on each x64 register, which could contain anything. findpc relies on
getting some sort of AddressException when this happens, and will then say the
address points to something that is unknown. However, in the case of the
address pointing to the last 32-bot word of a page, it results in an
ArrayIndexOutOfBoundsException when the page cache tries to read past the end
of the page. This is happening in Page.getLong(), which you can see in the
stack trace.
The main culprit here is some weakening of the alignment checks being done. The
alignment check should have resulted in an UnalignedAddressException long
before we ever got to Page.getLong(). However, we have the following override,
which is allowing the unaligned address to pass the alignment check.
public void checkAlignment(long address, long alignment) {
// Need to override default checkAlignment because we need to
// relax alignment constraints on Bsd/x86
if ( (address % alignment != 0)
&&(alignment != 8 || address % 4 != 0)) {
throw new UnalignedAddressException(
"Trying to read at address: "
+ addressValueToString(address)
+ " with alignment: " + alignment,
address);
}
}
};
This allows a pointer to a 64-bit value to only be 32-bit aligned. But there's
more to it than that. I modified ClhsdbFindPC.java to fetch a tid (a thread
address), and did a findpc on the address OR'd with 0xffc. `findpc` uses
PointerFinder. This forced a read of a 64-bit value that starts in the last
32-bits of a page. It passed on linux-x64 but failed on windowx-x64 in the same
manner described in the description of this CR. The difference between the two
implementations is that windows relies on the default implementation of
DebuggerBase.readCInteger() whereas linux has an override.
DebuggerBase.readCInteger() does the following:
public long readCInteger(long address, long numBytes, boolean isUnsigned) {
utils.checkAlignment(address, numBytes);
if (useFastAccessors) {
if (isUnsigned) {
switch((int) numBytes) {
case 1: return cache.getByte(address) & 0xFF;
case 2: return cache.getShort(address, bigEndian) & 0x;
case 4: return cache.getInt(address, bigEndian) & 0xL;
case 8: return cache.getLong(address, bigEndian);
...
There is an alignment check here, but it is the "relaxed" override shown above,
which allows 64-bit addresses on 32-bit boundaries. The override in
LinuxDebuggerLocal is:
/** Need to override this to relax alignment checks on x86. */
public long readCInteger(long address, long numBytes, boolean isUnsigned)
throws UnmappedAddressException, UnalignedAddressException {
// Only slightly relaxed semantics -- this is a hack, but is
// necessary on x86 where it seems the compiler is
// putting some global 64-bit data on 32-bit boundaries
if (numBytes == 8) {
utils.checkAlignment(address, 4);
} else {
utils.checkAlignment(address, numBytes);
}
byte[] data = readBytes(address, numBytes);
return utils.dataToCInteger(data, isUnsigned);
Although there is a relaxed alignment check here also, the code that reads from
the address does not assume all the bytes are on the same page. It uses
readBytes() intead of cache.getLong(), so it won't run into the PageCache issue
with reading a 64-bit value that starts in the last
Re: RFR: 8291081: Some sun/tools/jstatd/TestJstatd* tests fail with "Not a percentage\: 68.31\: expected true, was false"
On Mon, 8 Aug 2022 19:52:47 GMT, Leonid Mesnik wrote: > The test should use the same locales in all processes, the default language > should work fine. Can you explain why setting the locale on the test but not the subprocess is causing the failure? - PR: https://git.openjdk.org/jdk/pull/9798
Re: RFR: 8290846: sun/tools/jstatd/JstatdTest* tests should use VM options
On Thu, 21 Jul 2022 22:33:36 GMT, Leonid Mesnik wrote: > Propagate test.vm.opts/test.java.opts to tested process. Also, accept the > output of non-generation (ZGC) GC as valid. Marked as reviewed by cjplummer (Reviewer). - PR: https://git.openjdk.org/jdk/pull/9604
Re: RFR: 8290846: sun/tools/jstatd/JstatdTest* tests should use VM options
On Wed, 27 Jul 2022 21:39:47 GMT, Leonid Mesnik wrote: >> I meant shouldn't we see ZGC failures before your changes. Otherwise I don't >> understand why this change is needed. > > Before my changes test just silently ignored any GC setting and always use G1. Ah, ok. That makes sense now. - PR: https://git.openjdk.org/jdk/pull/9604
Re: RFR: 8290846: sun/tools/jstatd/JstatdTest* tests should use VM options
On Fri, 22 Jul 2022 03:10:11 GMT, Leonid Mesnik wrote: >> So shouldn't we have ZGC test failures then? > > No, test passes with ZGC. I meant shouldn't we see ZGC failures before your changes. Otherwise I don't understand why this change is needed. - PR: https://git.openjdk.org/jdk/pull/9604
Re: RFR: 8290846: sun/tools/jstatd/JstatdTest* tests should use VM options
On Fri, 22 Jul 2022 00:03:26 GMT, Leonid Mesnik wrote: >> test/jdk/sun/tools/jstatd/JstatGCUtilParser.java line 48: >> >>> 46: S0(GcStatisticsType.PERCENTAGE_OR_DASH), >>> 47: S1(GcStatisticsType.PERCENTAGE_OR_DASH), >>> 48: E(GcStatisticsType.PERCENTAGE_OR_DASH), >> >> I don't see jstat tests in the zgc problem list. Is this fixing known test >> failures? > > Yes, ZGC reports dash for eden and survivor spaces. So shouldn't we have ZGC test failures then? - PR: https://git.openjdk.org/jdk/pull/9604
Re: RFR: 8290846: sun/tools/jstatd/JstatdTest* tests should use VM options
On Thu, 21 Jul 2022 22:33:36 GMT, Leonid Mesnik wrote:
> Propagate test.vm.opts/test.java.opts to tested process. Also, accept the
> output of non-generation (ZGC) GC as valid.
test/jdk/sun/tools/jstatd/JstatGCUtilParser.java line 48:
> 46: S0(GcStatisticsType.PERCENTAGE_OR_DASH),
> 47: S1(GcStatisticsType.PERCENTAGE_OR_DASH),
> 48: E(GcStatisticsType.PERCENTAGE_OR_DASH),
I don't see jstat tests in the zgc problem list. Is this fixing known test
failures?
test/jdk/sun/tools/jstatd/JstatdTest.java line 131:
> 129: private OutputAnalyzer runJps() throws Exception {
> 130: JDKToolLauncher launcher =
> JDKToolLauncher.createUsingTestJDK("jps");
> 131:
> launcher.addVMArgs(Utils.getFilteredTestJavaOpts("-XX:+UsePerfData"));
I assume this is removed because jps only requires that the target JVMs be run
with UsePerfData, but not jps itself.
-
PR: https://git.openjdk.org/jdk/pull/9604
Re: RFR: 8289768: Clean up unused code [v3]
On Fri, 8 Jul 2022 07:08:46 GMT, Daniel Jeliński wrote: >> This patch removes many unused variables and one unused label reported by >> the compilers when relevant warnings are enabled. >> >> The unused code was found by compiling after removing `unused` from the list >> of disabled warnings for >> [gcc](https://github.com/openjdk/jdk/blob/master/make/autoconf/flags-cflags.m4#L190) >> and >> [clang](https://github.com/openjdk/jdk/blob/master/make/autoconf/flags-cflags.m4#L203), >> and enabling >> [C4189](https://docs.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-4-c4189?view=msvc-170) >> MSVC warning. >> >> I only removed variables that were uninitialized or initialized without side >> effects. I verified that the removed variables were not used in any >> `#ifdef`'d code. I checked that the changed code still compiles on Windows, >> Linux and Mac, both in release and debug versions. > > Daniel Jeliński has updated the pull request incrementally with two > additional commits since the last revision: > > - Update copyright > - Remove more unused variables Marked as reviewed by cjplummer (Reviewer). - PR: https://git.openjdk.org/jdk/pull/9383
Re: RFR: 8289768: Clean up unused code [v2]
On Wed, 6 Jul 2022 05:32:29 GMT, Daniel Jeliński wrote: >> This patch removes many unused variables and one unused label reported by >> the compilers when relevant warnings are enabled. >> >> The unused code was found by compiling after removing `unused` from the list >> of disabled warnings for >> [gcc](https://github.com/openjdk/jdk/blob/master/make/autoconf/flags-cflags.m4#L190) >> and >> [clang](https://github.com/openjdk/jdk/blob/master/make/autoconf/flags-cflags.m4#L203), >> and enabling >> [C4189](https://docs.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-4-c4189?view=msvc-170) >> MSVC warning. >> >> I only removed variables that were uninitialized or initialized without side >> effects. I verified that the removed variables were not used in any >> `#ifdef`'d code. I checked that the changed code still compiles on Windows, >> Linux and Mac, both in release and debug versions. > > Daniel Jeliński 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 four additional > commits since the last revision: > > - Merge remote-tracking branch 'origin' into unused-variables > - Remove unused variables (windows) > - Remove unused variables (macos) > - Remove unused variables (linux) Are you going to update copyrights? I reviewed src/jdk.hotspot.agent, src/jdk.jdi/, src/jdk.jdwp.agent, and src/jdk.management. Looks good other than copyrights and the suggestion I made for additional cleanup in symtab.c. src/jdk.hotspot.agent/linux/native/libsaproc/symtab.c line 308: > 306: ELF_SHDR* shbuf = NULL; > 307: ELF_SHDR* cursct = NULL; > 308: ELF_PHDR* phbuf = NULL; phbuf is also essentially unused. The only reference is to free it if non-null, but it's always NULL, so that code can be removed too. - Changes requested by cjplummer (Reviewer). PR: https://git.openjdk.org/jdk/pull/9383
