Re: RFR: 8284642: Unexpected behavior of -XX:MaxDirectMemorySize=0 [v3]
On Mon, 25 Apr 2022 14:26:17 GMT, Harold Seigel wrote: >> Please review this small fix for JDK-8284642. The fix was tested by running >> Mach5 tiers 1-2 on Linux, Mac OS, and Windows, and Mach5 tiers 3-5 on Linux >> x64. Additionally, the modified test and the test in the bug report were >> run locally. >> >> Thanks, Harold > > Harold Seigel has updated the pull request incrementally with one additional > commit since the last revision: > > Fix globals.hpp text Thanks for the reviews! - PR: https://git.openjdk.java.net/jdk/pull/8222
Integrated: 8284642: Unexpected behavior of -XX:MaxDirectMemorySize=0
On Wed, 13 Apr 2022 12:24:46 GMT, Harold Seigel wrote: > Please review this small fix for JDK-8284642. The fix was tested by running > Mach5 tiers 1-2 on Linux, Mac OS, and Windows, and Mach5 tiers 3-5 on Linux > x64. Additionally, the modified test and the test in the bug report were run > locally. > > Thanks, Harold This pull request has now been integrated. Changeset: 975a060a Author:Harold Seigel URL: https://git.openjdk.java.net/jdk/commit/975a060ade6e11b021222ae7f7a2de0d0c041308 Stats: 4 lines in 2 files changed: 1 ins; 0 del; 3 mod 8284642: Unexpected behavior of -XX:MaxDirectMemorySize=0 Reviewed-by: stuefe, dholmes - PR: https://git.openjdk.java.net/jdk/pull/8222
Re: RFR: 8284642: Unexpected behavior of -XX:MaxDirectMemorySize=0 [v2]
On Fri, 22 Apr 2022 18:14:27 GMT, Harold Seigel wrote: >> Please review this small fix for JDK-8284642. The fix was tested by running >> Mach5 tiers 1-2 on Linux, Mac OS, and Windows, and Mach5 tiers 3-5 on Linux >> x64. Additionally, the modified test and the test in the bug report were >> run locally. >> >> Thanks, Harold > > Harold Seigel has updated the pull request incrementally with one additional > commit since the last revision: > > restore source, fix man page Hi David, Thanks for looking at this and for the suggested text. Please review this latest commit to fix the text in globals.hpp. Thanks, Harold - PR: https://git.openjdk.java.net/jdk/pull/8222
Re: RFR: 8284642: Unexpected behavior of -XX:MaxDirectMemorySize=0 [v3]
> Please review this small fix for JDK-8284642. The fix was tested by running > Mach5 tiers 1-2 on Linux, Mac OS, and Windows, and Mach5 tiers 3-5 on Linux > x64. Additionally, the modified test and the test in the bug report were run > locally. > > Thanks, Harold Harold Seigel has updated the pull request incrementally with one additional commit since the last revision: Fix globals.hpp text - Changes: - all: https://git.openjdk.java.net/jdk/pull/8222/files - new: https://git.openjdk.java.net/jdk/pull/8222/files/5fdb220a..22185a6d Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8222&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8222&range=01-02 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8222.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8222/head:pull/8222 PR: https://git.openjdk.java.net/jdk/pull/8222
Re: RFR: 8284642: Unexpected behavior of -XX:MaxDirectMemorySize=0 [v2]
> Please review this small fix for JDK-8284642. The fix was tested by running > Mach5 tiers 1-2 on Linux, Mac OS, and Windows, and Mach5 tiers 3-5 on Linux > x64. Additionally, the modified test and the test in the bug report were run > locally. > > Thanks, Harold Harold Seigel has updated the pull request incrementally with one additional commit since the last revision: restore source, fix man page - Changes: - all: https://git.openjdk.java.net/jdk/pull/8222/files - new: https://git.openjdk.java.net/jdk/pull/8222/files/c178c602..5fdb220a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8222&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8222&range=00-01 Stats: 9 lines in 3 files changed: 0 ins; 0 del; 9 mod Patch: https://git.openjdk.java.net/jdk/pull/8222.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8222/head:pull/8222 PR: https://git.openjdk.java.net/jdk/pull/8222
Re: RFR: 8284642: Unexpected behavior of -XX:MaxDirectMemorySize=0
On Wed, 13 Apr 2022 12:24:46 GMT, Harold Seigel wrote: > Please review this small fix for JDK-8284642. The fix was tested by running > Mach5 tiers 1-2 on Linux, Mac OS, and Windows, and Mach5 tiers 3-5 on Linux > x64. Additionally, the modified test and the test in the bug report were run > locally. > > Thanks, Harold Please review the updated man page change. Thanks, Harold - PR: https://git.openjdk.java.net/jdk/pull/8222
Re: RFR: 8284642: Unexpected behavior of -XX:MaxDirectMemorySize=0
On Wed, 13 Apr 2022 12:24:46 GMT, Harold Seigel wrote: > Please review this small fix for JDK-8284642. The fix was tested by running > Mach5 tiers 1-2 on Linux, Mac OS, and Windows, and Mach5 tiers 3-5 on Linux > x64. Additionally, the modified test and the test in the bug report were run > locally. > > Thanks, Harold Should no changes be made to the code, instead just remove "the size is set to 0, meaning that" from the description of MaxDirectMemorySize? -XX:MaxDirectMemorySize=size Sets the maximum total size (in bytes) of the java.nio package, direct-buffer allocations. Append the letter k or K to indicate kilobytes, m or M to indicate megabytes, or g or G to indicate gigabytes. By default, **the size is set to 0, meaning that** the JVM chooses the size for NIO direct-buffer allocations automatically. - PR: https://git.openjdk.java.net/jdk/pull/8222
RFR: 8284642: Unexpected behavior of -XX:MaxDirectMemorySize=0
Please review this small fix for JDK-8284642. The fix was tested by running Mach5 tiers 1-2 on Linux, Mac OS, and Windows, and Mach5 tiers 3-5 on Linux x64. Additionally, the modified test and the test in the bug report were run locally. Thanks, Harold - Commit messages: - 8284642: Unexpected behavior of -XX:MaxDirectMemorySize=0 Changes: https://git.openjdk.java.net/jdk/pull/8222/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=8222&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8284642 Stats: 7 lines in 2 files changed: 0 ins; 0 del; 7 mod Patch: https://git.openjdk.java.net/jdk/pull/8222.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8222/head:pull/8222 PR: https://git.openjdk.java.net/jdk/pull/8222
RFR: 8277481: Obsolete seldom used CDS flags
Please review this change to obsolete deprecated CDS options UseSharedSpaces, RequireSharedSpaces, DynamicDumpSharedSpaces, and DumpSharedSpaces. The change was tested by running Mach5 tiers 1-2 on Linux, Mac OS, and Windows and Mach5 tiers 3-5 on Linux x64 and Windows x64. The use of UseSharedSpaces in ps_core_common.c was tested on Mac OS x64 by temporarily removing serviceability/sa/ClhsdbPmap.java#core from the problem list. Thanks! Harold - Commit messages: - fix typo - 8277481: Obsolete seldom used CDS flags Changes: https://git.openjdk.java.net/jdk/pull/6800/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=6800&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8277481 Stats: 151 lines in 13 files changed: 22 ins; 94 del; 35 mod Patch: https://git.openjdk.java.net/jdk/pull/6800.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6800/head:pull/6800 PR: https://git.openjdk.java.net/jdk/pull/6800
Re: RFR: 8275735: [linux] Remove deprecated Metrics api (kernel memory limit)
On Thu, 28 Oct 2021 13:03:56 GMT, Severin Gehwolf wrote: > Please review this change to remove some API which no longer works as > expected as recent OCI runtimes start to drop support for `--kernel-memory` > switch. See the bug for references. This part of the API is not present in > hotspot code. > > Testing: Container tests (cgroup v1) on Linux x86_64 (all pass) The changes look good. Thanks for doing this. Harold - Marked as reviewed by hseigel (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/6156
Re: RFR: 8272614: Unused parameters in MethodHandleNatives linking methods
On Tue, 19 Oct 2021 17:12:16 GMT, Harold Seigel wrote: > Please review this small fix for JDK-8272614 to remove the unused indexInCP > argument to linkCallSite() and linkDynamicConstant(). The fix was tested > with Mach5 tiers 1-2 on Linux, Mac OS, and Windows, and Mach5 tiers 3-6 on > Linux x64. > > Thanks, Harold Thanks David and Lois! - PR: https://git.openjdk.java.net/jdk/pull/6021
Integrated: 8272614: Unused parameters in MethodHandleNatives linking methods
On Tue, 19 Oct 2021 17:12:16 GMT, Harold Seigel wrote: > Please review this small fix for JDK-8272614 to remove the unused indexInCP > argument to linkCallSite() and linkDynamicConstant(). The fix was tested > with Mach5 tiers 1-2 on Linux, Mac OS, and Windows, and Mach5 tiers 3-6 on > Linux x64. > > Thanks, Harold This pull request has now been integrated. Changeset: bbc60611 Author:Harold Seigel URL: https://git.openjdk.java.net/jdk/commit/bbc606117fcd8b48fc8f830c50cf7eb573da1c4c Stats: 8 lines in 3 files changed: 0 ins; 3 del; 5 mod 8272614: Unused parameters in MethodHandleNatives linking methods Reviewed-by: dholmes, lfoltan - PR: https://git.openjdk.java.net/jdk/pull/6021
RFR: 8272614: Unused parameters in MethodHandleNatives linking methods
Please review this small fix for JDK-8272614 to remove the unused indexInCP argument to linkCallSite() and linkDynamicConstant(). The fix was tested with Mach5 tiers 1-2 on Linux, Mac OS, and Windows, and Mach5 tiers 3-6 on Linux x64. Thanks, Harold - Commit messages: - 8272614: Unused parameters in MethodHandleNatives linking methods Changes: https://git.openjdk.java.net/jdk/pull/6021/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=6021&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8272614 Stats: 8 lines in 3 files changed: 0 ins; 3 del; 5 mod Patch: https://git.openjdk.java.net/jdk/pull/6021.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6021/head:pull/6021 PR: https://git.openjdk.java.net/jdk/pull/6021
Re: RFR: 8272124: Cgroup v1 initialization causes NullPointerException when cgroup path contains colon [v4]
On Wed, 18 Aug 2021 13:04:46 GMT, Harold Seigel wrote: >> Please review this small fix for JDK-8272124. The fix puts a limit of 3 >> when splitting self cgroup lines by ':' so that Cgroup paths won't get >> truncated if they contain embedded ':'s. For example, an entry of >> "11:memory:/user.sli:ce" in a /proc/self/cgroup file will now result in a >> Cgroup path of "/user.sli:ce" instead of "/user.sli". >> >> The fix was tested with Mach5 tiers 1 and 2, and Mach5 tiers 3-5 on Linux >> x64 and Linux aarch64. >> >> Thanks, Harold > > Harold Seigel has updated the pull request incrementally with one additional > commit since the last revision: > > Fix test typo and error Thanks Severin and Misha for your reviews and improvements for this fix! - PR: https://git.openjdk.java.net/jdk/pull/5127
Integrated: 8272124: Cgroup v1 initialization causes NullPointerException when cgroup path contains colon
On Mon, 16 Aug 2021 17:25:57 GMT, Harold Seigel wrote: > Please review this small fix for JDK-8272124. The fix puts a limit of 3 when > splitting self cgroup lines by ':' so that Cgroup paths won't get truncated > if they contain embedded ':'s. For example, an entry of > "11:memory:/user.sli:ce" in a /proc/self/cgroup file will now result in a > Cgroup path of "/user.sli:ce" instead of "/user.sli". > > The fix was tested with Mach5 tiers 1 and 2, and Mach5 tiers 3-5 on Linux x64 > and Linux aarch64. > > Thanks, Harold This pull request has now been integrated. Changeset: 4d6593ce Author:Harold Seigel URL: https://git.openjdk.java.net/jdk/commit/4d6593ce0243457e7431a5990957a8f880e0a3fb Stats: 57 lines in 2 files changed: 54 ins; 0 del; 3 mod 8272124: Cgroup v1 initialization causes NullPointerException when cgroup path contains colon Reviewed-by: mseledtsov, sgehwolf - PR: https://git.openjdk.java.net/jdk/pull/5127
Re: RFR: 8272124: Cgroup v1 initialization causes NullPointerException when cgroup path contains colon [v4]
> Please review this small fix for JDK-8272124. The fix puts a limit of 3 when > splitting self cgroup lines by ':' so that Cgroup paths won't get truncated > if they contain embedded ':'s. For example, an entry of > "11:memory:/user.sli:ce" in a /proc/self/cgroup file will now result in a > Cgroup path of "/user.sli:ce" instead of "/user.sli". > > The fix was tested with Mach5 tiers 1 and 2, and Mach5 tiers 3-5 on Linux x64 > and Linux aarch64. > > Thanks, Harold Harold Seigel has updated the pull request incrementally with one additional commit since the last revision: Fix test typo and error - Changes: - all: https://git.openjdk.java.net/jdk/pull/5127/files - new: https://git.openjdk.java.net/jdk/pull/5127/files/fcc8c908..f339fe14 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5127&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5127&range=02-03 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/5127.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5127/head:pull/5127 PR: https://git.openjdk.java.net/jdk/pull/5127
Re: RFR: 8272124: Cgroup v1 initialization causes NullPointerException when cgroup path contains colon [v3]
On Wed, 18 Aug 2021 12:25:45 GMT, Harold Seigel wrote: >> Please review this small fix for JDK-8272124. The fix puts a limit of 3 >> when splitting self cgroup lines by ':' so that Cgroup paths won't get >> truncated if they contain embedded ':'s. For example, an entry of >> "11:memory:/user.sli:ce" in a /proc/self/cgroup file will now result in a >> Cgroup path of "/user.sli:ce" instead of "/user.sli". >> >> The fix was tested with Mach5 tiers 1 and 2, and Mach5 tiers 3-5 on Linux >> x64 and Linux aarch64. >> >> Thanks, Harold > > Harold Seigel has updated the pull request incrementally with one additional > commit since the last revision: > > add mountinfo containing colongs to test Thanks for finding those issues. Please review the latest iteration of this fix. Thanks, Harold - PR: https://git.openjdk.java.net/jdk/pull/5127
Re: RFR: 8272124: Cgroup v1 initialization causes NullPointerException when path contains colon [v2]
On Tue, 17 Aug 2021 17:39:49 GMT, Harold Seigel wrote: >> Please review this small fix for JDK-8272124. The fix puts a limit of 3 >> when splitting self cgroup lines by ':' so that Cgroup paths won't get >> truncated if they contain embedded ':'s. For example, an entry of >> "11:memory:/user.sli:ce" in a /proc/self/cgroup file will now result in a >> Cgroup path of "/user.sli:ce" instead of "/user.sli". >> >> The fix was tested with Mach5 tiers 1 and 2, and Mach5 tiers 3-5 on Linux >> x64 and Linux aarch64. >> >> Thanks, Harold > > Harold Seigel has updated the pull request incrementally with one additional > commit since the last revision: > > add test case, comments, and other small changes Please review this update that modifies the new test case to have a mountinfo entry that contains multiple ":" characters. Thanks, Harold - PR: https://git.openjdk.java.net/jdk/pull/5127
Re: RFR: 8272124: Cgroup v1 initialization causes NullPointerException when path contains colon [v3]
> Please review this small fix for JDK-8272124. The fix puts a limit of 3 when > splitting self cgroup lines by ':' so that Cgroup paths won't get truncated > if they contain embedded ':'s. For example, an entry of > "11:memory:/user.sli:ce" in a /proc/self/cgroup file will now result in a > Cgroup path of "/user.sli:ce" instead of "/user.sli". > > The fix was tested with Mach5 tiers 1 and 2, and Mach5 tiers 3-5 on Linux x64 > and Linux aarch64. > > Thanks, Harold Harold Seigel has updated the pull request incrementally with one additional commit since the last revision: add mountinfo containing colongs to test - Changes: - all: https://git.openjdk.java.net/jdk/pull/5127/files - new: https://git.openjdk.java.net/jdk/pull/5127/files/5fa37e97..fcc8c908 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5127&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5127&range=01-02 Stats: 19 lines in 1 file changed: 18 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/5127.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5127/head:pull/5127 PR: https://git.openjdk.java.net/jdk/pull/5127
Re: RFR: 8272398: Update DockerTestUtils.buildJdkDockerImage() [v2]
On Tue, 17 Aug 2021 14:58:48 GMT, Mikhailo Seledtsov wrote: >> Please review this change that updates the buildJdkDockerImage() test >> library API. >> >> This work originated while working on "8195809: [TESTBUG] jps and jcmd -l >> support for containers is not tested". >> The initial intent was to extend the buildJdkDockerImage() API of >> DockerTestUtils to accept custom Dockerfile content. >> As I analyzed the usage of buildJdkDockerImage() I realized that: >> - 2nd argument "dockerfile" is always the same: "Dockerfile-BasicTest" >> its use has been obsolete for some time, in favor of Dockerfile >> generated by DockerTestUtils >> - 3rd argument "buildDirName" is also always the same: "jdk-docker" >> >> Hence I thought it would be a good idea to simplify this API and make it >> up-to-date. >> >> Also, since the method signature is being updated, I thought it would be a >> good idea to also change the name to use more generic container terminology: >> buildJdkDockerImage() --> buildJdkContainerImage() > > Mikhailo Seledtsov has updated the pull request incrementally with one > additional commit since the last revision: > > Addressing review feedback Other than that one obsolete comment the changes look good. Thanks, Harold test/lib/jdk/test/lib/containers/docker/DockerTestUtils.java line 175: > 173: /** > 174: * Build a container image based on given Dockerfile and image build > directory. > 175: * This comment looks wrong because there is no longer a given Dockerfile. - Marked as reviewed by hseigel (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5134
Re: RFR: 8272124: Cgroup v1 initialization causes NullPointerException when path contains colon
On Mon, 16 Aug 2021 17:25:57 GMT, Harold Seigel wrote: > Please review this small fix for JDK-8272124. The fix puts a limit of 3 when > splitting self cgroup lines by ':' so that Cgroup paths won't get truncated > if they contain embedded ':'s. For example, an entry of > "11:memory:/user.sli:ce" in a /proc/self/cgroup file will now result in a > Cgroup path of "/user.sli:ce" instead of "/user.sli". > > The fix was tested with Mach5 tiers 1 and 2, and Mach5 tiers 3-5 on Linux x64 > and Linux aarch64. > > Thanks, Harold Thanks Misha and Severin for looking at this change! Please review this updated commit that tries to address Severin's comments. A new test case was added to TestCgroupSubsystemFactory.java for the multiple ':'s case and comments were added to CgroupSubsystemFactory.java. The ".filter(tokens -> (tokens.length >= 3))" code was removed but can be restored if need be. Thanks, Harold - PR: https://git.openjdk.java.net/jdk/pull/5127
Re: RFR: 8272124: Cgroup v1 initialization causes NullPointerException when path contains colon [v2]
> Please review this small fix for JDK-8272124. The fix puts a limit of 3 when > splitting self cgroup lines by ':' so that Cgroup paths won't get truncated > if they contain embedded ':'s. For example, an entry of > "11:memory:/user.sli:ce" in a /proc/self/cgroup file will now result in a > Cgroup path of "/user.sli:ce" instead of "/user.sli". > > The fix was tested with Mach5 tiers 1 and 2, and Mach5 tiers 3-5 on Linux x64 > and Linux aarch64. > > Thanks, Harold Harold Seigel has updated the pull request incrementally with one additional commit since the last revision: add test case, comments, and other small changes - Changes: - all: https://git.openjdk.java.net/jdk/pull/5127/files - new: https://git.openjdk.java.net/jdk/pull/5127/files/918df2b4..5fa37e97 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5127&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5127&range=00-01 Stats: 41 lines in 2 files changed: 36 ins; 0 del; 5 mod Patch: https://git.openjdk.java.net/jdk/pull/5127.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5127/head:pull/5127 PR: https://git.openjdk.java.net/jdk/pull/5127
RFR: 8272124: Cgroup v1 initialization causes NullPointerException when path contains colon
Please review this small fix for JDK-8272124. The fix puts a limit of 3 when splitting self cgroup lines by ':' so that Cgroup paths won't get truncated if they contain embedded ':'s. For example, an entry of "11:memory:/user.sli:ce" in a /proc/self/cgroup file will now result in a Cgroup path of "/user.sli:ce" instead of "/user.sli". The fix was tested with Mach5 tiers 1 and 2, and Mach5 tiers 3-5 on Linux x64 and Linux aarch64. Thanks, Harold - Commit messages: - 8272124: Cgroup v1 initialization causes NullPointerException when path contains colon Changes: https://git.openjdk.java.net/jdk/pull/5127/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=5127&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8272124 Stats: 3 lines in 2 files changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/5127.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5127/head:pull/5127 PR: https://git.openjdk.java.net/jdk/pull/5127
Re: RFR: 8266851: Implement JEP 403: Strongly Encapsulate JDK Internals [v4]
On Tue, 18 May 2021 23:01:05 GMT, Mark Reinhold wrote:
>> Please review this implementation of JEP 403
>> (https://openjdk.java.net/jeps/403).
>> Alan Bateman is the original author of almost all of it. Passes tiers 1-3
>> on
>> {linux,macos,windows}-x64 and {linux,macos}-aarch64.
>
> Mark Reinhold has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fix up IllegalAccessTest
Hotspot changes look good.
Thanks, Harold
-
Marked as reviewed by hseigel (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/4015
Re: RFR: 8266851: Implement JEP 403: Strongly Encapsulate JDK Internals
On Thu, 13 May 2021 17:14:36 GMT, Mark Reinhold wrote:
> Please review this implementation of JEP 403
> (https://openjdk.java.net/jeps/403).
> Alan Bateman is the original author of almost all of it. Passes tiers 1-3 on
> {linux,macos,windows}-x64 and {linux,macos}-aarch64.
src/hotspot/share/runtime/arguments.cpp line 2434:
> 2432: // -agentlib and -agentpath
> 2433: } else if (match_option(option, "-agentlib:", &tail) ||
> 2434: (is_absolute_path = match_option(option, "-agentpath:",
> &tail))) {
I think jdk.module.illegalAccess can also be removed from lines 1332 and 2064
of arguments.cpp.
Thanks, Harold
-
PR: https://git.openjdk.java.net/jdk/pull/4015
Re: RFR: 8243287: Removal of Unsafe::defineAnonymousClass [v5]
On Thu, 13 May 2021 12:31:44 GMT, Harold Seigel wrote: >> Please review this large change to remove Unsafe::defineAnonymousClass(). >> The change removes dAC relevant code and changes a lot of tests. Many of >> the changed tests need renaming. I hope to do this in a follow up RFE. >> Some of the tests were modified to use hidden classes, others were deleted >> because either similar hidden classes tests already exist or they tested dAC >> specific functionality, such as host classes. >> >> This change was tested with Mach5 tiers 1-2 on Linux, Mac OS, and Windows, >> and Mach5 tiers 3-7 on Linux x64. >> >> Thanks, Harold > > Harold Seigel has updated the pull request incrementally with one additional > commit since the last revision: > > fix Weak hidden comment Thanks Ioi, Alan, Mandy, and David for reviewing this big change! - PR: https://git.openjdk.java.net/jdk/pull/3974
Integrated: 8243287: Removal of Unsafe::defineAnonymousClass
On Tue, 11 May 2021 12:50:31 GMT, Harold Seigel wrote: > Please review this large change to remove Unsafe::defineAnonymousClass(). > The change removes dAC relevant code and changes a lot of tests. Many of the > changed tests need renaming. I hope to do this in a follow up RFE. Some of > the tests were modified to use hidden classes, others were deleted because > either similar hidden classes tests already exist or they tested dAC specific > functionality, such as host classes. > > This change was tested with Mach5 tiers 1-2 on Linux, Mac OS, and Windows, > and Mach5 tiers 3-7 on Linux x64. > > Thanks, Harold This pull request has now been integrated. Changeset: e14b0268 Author:Harold Seigel URL: https://git.openjdk.java.net/jdk/commit/e14b0268411bba8eb01bf6c477cc8743a53ffd1c Stats: 3754 lines in 122 files changed: 75 ins; 3426 del; 253 mod 8243287: Removal of Unsafe::defineAnonymousClass Reviewed-by: iklam, mchung, alanb, dholmes - PR: https://git.openjdk.java.net/jdk/pull/3974
Re: RFR: 8243287: Removal of Unsafe::defineAnonymousClass [v5]
On Thu, 13 May 2021 07:19:03 GMT, David Holmes wrote: >> Harold Seigel has updated the pull request incrementally with one additional >> commit since the last revision: >> >> fix Weak hidden comment > > src/hotspot/share/oops/constantPool.hpp line 493: > >> 491: // object into a CONSTANT_String entry of an unsafe anonymous class. >> 492: // Methods internally created for method handles may also >> 493: // use pseudo-strings to link themselves to related metaobjects. > > Is this comment wrong? Are psuedo-strings not used by anything now? Thanks for looking at this. I discussed pseudo strings with Coleen and we didn't find any use of them besides unsafe.DefineAnonymousClass. - PR: https://git.openjdk.java.net/jdk/pull/3974
Re: RFR: 8243287: Removal of Unsafe::defineAnonymousClass [v4]
On Wed, 12 May 2021 22:30:30 GMT, Mandy Chung wrote:
>> Harold Seigel has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> test changes and small fixes
>
> src/hotspot/share/classfile/classLoaderData.cpp line 299:
>
>> 297: }
>> 298:
>> 299: // Weak hidden classes have their own ClassLoaderData that is marked to
>> keep alive
>
> `s/Weak/Non-strong/`
fixed. thanks for finding it.
> test/hotspot/jtreg/runtime/HiddenClasses/StressHiddenClasses.java line 39:
>
>> 37: import jdk.test.lib.compiler.InMemoryJavaCompiler;
>> 38:
>> 39: public class StressHiddenClasses {
>
> Since `StressClassLoadingTest` is revised to test hidden class, this test is
> a subset of it.
> I think this can be removed as JDK-8265694 suggests.
Thanks Mandy. I will remove the test as the fix for JDK-8265694 after this
change is pushed.
-
PR: https://git.openjdk.java.net/jdk/pull/3974
Re: RFR: 8243287: Removal of Unsafe::defineAnonymousClass [v5]
> Please review this large change to remove Unsafe::defineAnonymousClass(). > The change removes dAC relevant code and changes a lot of tests. Many of the > changed tests need renaming. I hope to do this in a follow up RFE. Some of > the tests were modified to use hidden classes, others were deleted because > either similar hidden classes tests already exist or they tested dAC specific > functionality, such as host classes. > > This change was tested with Mach5 tiers 1-2 on Linux, Mac OS, and Windows, > and Mach5 tiers 3-7 on Linux x64. > > Thanks, Harold Harold Seigel has updated the pull request incrementally with one additional commit since the last revision: fix Weak hidden comment - Changes: - all: https://git.openjdk.java.net/jdk/pull/3974/files - new: https://git.openjdk.java.net/jdk/pull/3974/files/5246dd76..38675761 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3974&range=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3974&range=03-04 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/3974.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3974/head:pull/3974 PR: https://git.openjdk.java.net/jdk/pull/3974
Re: RFR: 8243287: Removal of Unsafe::defineAnonymousClass [v3]
On Tue, 11 May 2021 17:07:35 GMT, Ioi Lam wrote: >> Harold Seigel has updated the pull request incrementally with one additional >> commit since the last revision: >> >> fix GetModuleTest.java > > src/hotspot/share/oops/instanceMirrorKlass.inline.hpp line 65: > >> 63: // so when handling the java mirror for the class we need to >> make sure its class >> 64: // loader data is claimed, this is done by calling do_cld >> explicitly. >> 65: // For non-string hidden classes the call to do_cld is made when >> the class > > Typo: non-strong fixed, thanks for finding this. - PR: https://git.openjdk.java.net/jdk/pull/3974
Re: RFR: 8243287: Removal of Unsafe::defineAnonymousClass [v3]
On Tue, 11 May 2021 20:49:46 GMT, Mandy Chung wrote: >> Harold Seigel has updated the pull request incrementally with one additional >> commit since the last revision: >> >> fix GetModuleTest.java > > src/jdk.internal.vm.ci/share/classes/jdk.vm.ci.meta/src/jdk/vm/ci/meta/MetaUtil.java > line 53: > >> 51: return simpleName; >> 52: } >> 53: // Must be a local class > > This file should not be changed. It refers to the Java language anonymous > class (not VM anonymous class). Changes have been restored. - PR: https://git.openjdk.java.net/jdk/pull/3974
Re: RFR: 8243287: Removal of Unsafe::defineAnonymousClass [v3]
On Tue, 11 May 2021 14:13:49 GMT, Harold Seigel wrote: >> Please review this large change to remove Unsafe::defineAnonymousClass(). >> The change removes dAC relevant code and changes a lot of tests. Many of >> the changed tests need renaming. I hope to do this in a follow up RFE. >> Some of the tests were modified to use hidden classes, others were deleted >> because either similar hidden classes tests already exist or they tested dAC >> specific functionality, such as host classes. >> >> This change was tested with Mach5 tiers 1-2 on Linux, Mac OS, and Windows, >> and Mach5 tiers 3-7 on Linux x64. >> >> Thanks, Harold > > Harold Seigel has updated the pull request incrementally with one additional > commit since the last revision: > > fix GetModuleTest.java Thanks Alan, Ioi, and Mandy for looking at this. The latest changes should address the issues that you found. Harold - PR: https://git.openjdk.java.net/jdk/pull/3974
Re: RFR: 8243287: Removal of Unsafe::defineAnonymousClass [v4]
> Please review this large change to remove Unsafe::defineAnonymousClass(). > The change removes dAC relevant code and changes a lot of tests. Many of the > changed tests need renaming. I hope to do this in a follow up RFE. Some of > the tests were modified to use hidden classes, others were deleted because > either similar hidden classes tests already exist or they tested dAC specific > functionality, such as host classes. > > This change was tested with Mach5 tiers 1-2 on Linux, Mac OS, and Windows, > and Mach5 tiers 3-7 on Linux x64. > > Thanks, Harold Harold Seigel has updated the pull request incrementally with one additional commit since the last revision: test changes and small fixes - Changes: - all: https://git.openjdk.java.net/jdk/pull/3974/files - new: https://git.openjdk.java.net/jdk/pull/3974/files/874a1603..5246dd76 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3974&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3974&range=02-03 Stats: 286 lines in 10 files changed: 22 ins; 256 del; 8 mod Patch: https://git.openjdk.java.net/jdk/pull/3974.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3974/head:pull/3974 PR: https://git.openjdk.java.net/jdk/pull/3974
Re: RFR: 8243287: Removal of Unsafe::defineAnonymousClass [v3]
On Tue, 11 May 2021 13:41:53 GMT, Alan Bateman wrote:
>> test/jdk/java/lang/Class/GetModuleTest.java line 42:
>>
>>> 40: import static org.testng.Assert.*;
>>> 41:
>>> 42: public class GetModuleTest {
>>
>> testGetModuleOnVMAnonymousClass is the only test here that uses ASM so you
>> can remove the imports as part of the removal.
>
> Can you check test/jdkjava/lang/Class/attributes/ClassAttributesTest.java? It
> may minimally need a comment to be updated. That's the only additional test
> that I could find in test/jdk.
Hi Alan,
Thanks for find this. I removed the unneeded imports and @modules from
GetModulesTest.java and updated the comment in ClassAttributesTest.java.
Thanks, Harold
-
PR: https://git.openjdk.java.net/jdk/pull/3974
Re: RFR: 8243287: Removal of Unsafe::defineAnonymousClass
Hi Brian, Thanks for looking at this. The JDK no longer creates unsafe anon classes. So, those tests could only find an unsafe anonymous class if they explicitly created one. In which case, the tests would need to call Unsafe.defineAnonymousClass(). And, hopefully, those tests have been handled in this webrev. If there are dead tests then they probably died when the JDK stopped creating unsafe anon classes. Note that none of them showed up as failures during regression testing. Harold On 5/11/2021 9:20 AM, Brian Goetz wrote: There may be some JDK code that checks for anon classes by comparing the name to see if it contains a slash, especially tests, but which don’t say “anonymous”. Did you do a search for these idioms too, which are now dead tests? Sent from my iPad On May 11, 2021, at 8:59 AM, Harold Seigel wrote: Please review this large change to remove Unsafe::defineAnonymousClass(). The change removes dAC relevant code and changes a lot of tests. Many of the changed tests need renaming. I hope to do this in a follow up RFE. Some of the tests were modified to use hidden classes, others were deleted because either similar hidden classes tests already exist or they tested dAC specific functionality, such as host classes. This change was tested with Mach5 tiers 1-2 on Linux, Mac OS, and Windows, and Mach5 tiers 3-7 on Linux x64. Thanks, Harold - Commit messages: - 8243287: Removal of Unsafe::defineAnonymousClass Changes: https://git.openjdk.java.net/jdk/pull/3974/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=3974&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8243287 Stats: 3516 lines in 116 files changed: 69 ins; 3181 del; 266 mod Patch: https://git.openjdk.java.net/jdk/pull/3974.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3974/head:pull/3974 PR: https://git.openjdk.java.net/jdk/pull/3974
Re: RFR: 8243287: Removal of Unsafe::defineAnonymousClass [v3]
> Please review this large change to remove Unsafe::defineAnonymousClass(). > The change removes dAC relevant code and changes a lot of tests. Many of the > changed tests need renaming. I hope to do this in a follow up RFE. Some of > the tests were modified to use hidden classes, others were deleted because > either similar hidden classes tests already exist or they tested dAC specific > functionality, such as host classes. > > This change was tested with Mach5 tiers 1-2 on Linux, Mac OS, and Windows, > and Mach5 tiers 3-7 on Linux x64. > > Thanks, Harold Harold Seigel has updated the pull request incrementally with one additional commit since the last revision: fix GetModuleTest.java - Changes: - all: https://git.openjdk.java.net/jdk/pull/3974/files - new: https://git.openjdk.java.net/jdk/pull/3974/files/35c6634c..874a1603 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3974&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3974&range=01-02 Stats: 3 lines in 1 file changed: 0 ins; 2 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/3974.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3974/head:pull/3974 PR: https://git.openjdk.java.net/jdk/pull/3974
Re: RFR: 8243287: Removal of Unsafe::defineAnonymousClass [v2]
> Please review this large change to remove Unsafe::defineAnonymousClass(). > The change removes dAC relevant code and changes a lot of tests. Many of the > changed tests need renaming. I hope to do this in a follow up RFE. Some of > the tests were modified to use hidden classes, others were deleted because > either similar hidden classes tests already exist or they tested dAC specific > functionality, such as host classes. > > This change was tested with Mach5 tiers 1-2 on Linux, Mac OS, and Windows, > and Mach5 tiers 3-7 on Linux x64. > > Thanks, Harold Harold Seigel has updated the pull request incrementally with one additional commit since the last revision: test fixes - Changes: - all: https://git.openjdk.java.net/jdk/pull/3974/files - new: https://git.openjdk.java.net/jdk/pull/3974/files/233a4725..35c6634c Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3974&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3974&range=00-01 Stats: 5 lines in 2 files changed: 0 ins; 3 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/3974.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3974/head:pull/3974 PR: https://git.openjdk.java.net/jdk/pull/3974
RFR: 8243287: Removal of Unsafe::defineAnonymousClass
Please review this large change to remove Unsafe::defineAnonymousClass(). The change removes dAC relevant code and changes a lot of tests. Many of the changed tests need renaming. I hope to do this in a follow up RFE. Some of the tests were modified to use hidden classes, others were deleted because either similar hidden classes tests already exist or they tested dAC specific functionality, such as host classes. This change was tested with Mach5 tiers 1-2 on Linux, Mac OS, and Windows, and Mach5 tiers 3-7 on Linux x64. Thanks, Harold - Commit messages: - 8243287: Removal of Unsafe::defineAnonymousClass Changes: https://git.openjdk.java.net/jdk/pull/3974/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=3974&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8243287 Stats: 3516 lines in 116 files changed: 69 ins; 3181 del; 266 mod Patch: https://git.openjdk.java.net/jdk/pull/3974.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3974/head:pull/3974 PR: https://git.openjdk.java.net/jdk/pull/3974
Re: RFR: 8265111: ProblemList java/util/concurrent/locks/Lock/TimedAcquireLeak.java on macosx-aarch64
On Tue, 13 Apr 2021 06:55:33 GMT, Mikael Vidstedt wrote: > Let's problem list java/util/concurrent/locks/Lock/TimedAcquireLeak.java (a > tier1 test) until JDK-8262897 is fixed. Looks good and trivial. Harold - Marked as reviewed by hseigel (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/3452
Re: RFR: 8262379: Add regression test for JDK-8257746
On Thu, 25 Feb 2021 16:27:20 GMT, Severin Gehwolf wrote: > Fails prior the patch of JDK-8257746, passes after. As expected. > > Thoughts? The new tests looks good. Thanks for adding it. Harold - Marked as reviewed by hseigel (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/2725
Re: RFR: 8254001: [Metrics] Enhance parsing of cgroup interface files for version detection [v4]
On Tue, 9 Feb 2021 13:31:25 GMT, Severin Gehwolf wrote: >> This is an enhancement which solves two issues: >> >> 1. Multiple reads of relevant cgroup interface files. Now interface files >> are only read once per file (just like Hotspot). >> 2. Proxies creation of the impl specific subsystem via `determineType()` as >> before, but now reads all relevant interface files: `/proc/cgroups`, >> `/proc/self/mountinfo` and `/proc/self/cgroup`. Once read it passes the >> parsed information to the impl specific subsystem classes for instantiation. >> This allows for more flexibility of testing as interface files can be mocked >> and, thus, more cases can be tested that way without having access to these >> specific systems. For example, proper regression tests for JDK-8217766 and >> JDK-8253435 have been added now with this in place. >> >> * [x] Tested on Linux x86_64 on cgroups v1 and cgroups v2. Container tests >> pass. > > Severin Gehwolf 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 seven additional > commits since the last revision: > > - Fix jcheck > - Add documentation and reduce code running in the critical section > - Add some documentation > - Merge branch 'master' into jdk-8254001-enhance-file-parsing-java-metrics > - Merge branch 'master' into jdk-8254001-enhance-file-parsing-java-metrics > - Merge branch 'master' into jdk-8254001-enhance-file-parsing-java-metrics > - 8254001: [Metrics] Enhance parsing of cgroup interface files for version > detection Hi Severin, Thanks for doing this! Sorry for taking so long to review this change. The change looks good. Before pushing it, could you add a comment explaining what the code in lines 185-194 of CgroupSubsystemFactory.java is doing? Also, please don't overwrite the fix for JDK-8257746. Thanks again! Harold - Marked as reviewed by hseigel (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1393
Re: RFR: 8261604: ProblemList jdk/dynalink/TypeConverterFactoryMemoryLeakTest.java
On Thu, 11 Feb 2021 17:55:13 GMT, Daniel D. Daugherty wrote: > A trivial fix to ProblemList > jdk/dynalink/TypeConverterFactoryMemoryLeakTest.java > in order to reduce noise in the JDK17 CI. Looks good and trivial. Thanks, Harold - Marked as reviewed by hseigel (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/2531
Re: RFR: 8257746: Regression introduced with JDK-8250984 - memory might be null in some machines
On Wed, 27 Jan 2021 21:10:16 GMT, Poonam Bajaj wrote:
> Please review this simple change that adds null checks for memory in
> CgroupV1Subsystem.java.
>
> Problem: After the backport of JDK-8250984, there are places where
> memory.isSwapEnabled() is called. For example:
>
> public long getMemoryAndSwapFailCount() {
> if (!memory.isSwapEnabled()) {
> return getMemoryFailCount();
> }
> return SubSystem.getLongValue(memory, "memory.memsw.failcnt");
> }
>
> But memory could be Null on some machines that have cgroup entries for CPU
> but not for memory. This would cause a NullPointerException when memory is
> accessed.
>
> Fix: Add null checks for memory.
Changes look good! Thanks for doing this.
Harold
-
Marked as reviewed by hseigel (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/2269
Re: [jdk16] RFR: 8257596: Clarify trusted final fields for record classes [v2]
On Fri, 11 Dec 2020 19:29:09 GMT, Mandy Chung wrote: >> This is a follow-up on JDK-8255342 that removes non-specified JVM checks on >> classes with RecordComponents attributes. >> >> See the discussion at >> https://mail.openjdk.java.net/pipermail/amber-spec-experts/2020-December/002670.html >> >> That fixes trusting final fields of records to align with the JLS definition >> of a record class. `InstanceKlass::is_record` is fixed to check a record >> class must be final and a direct subclass of `java.lang.Record` with the >> presence of the `Record` attribute. There is no change to JVM class file >> validation. I also propose clearly define: >> - `JVM_IsRecord` returns true if the given class is a record i.e. final and >> direct subclass of `java.lang.Record` with `Record` attribute >> - `JVM_GetRecordComponents `returns an `RecordComponents` array if `Record` >> attribute is present; otherwise, returns NULL. This does not check if it's a >> record class or not. So it may return non-null on a non-record class if it >> has `Record` attribute. So `JVM_GetRecordComponents` returns the content of >> the classfile. >> >> tier1-tier3 and jck-runtime:vm and jck-runtime:lang tests all passed. > > Mandy Chung has updated the pull request incrementally with one additional > commit since the last revision: > > Update comments in jvm.cpp per Harold's feedback Looks Good! - Marked as reviewed by hseigel (Reviewer). PR: https://git.openjdk.java.net/jdk16/pull/14
Re: [jdk16] RFR: 8257596: Clarify trusted final fields for record classes
On Fri, 11 Dec 2020 17:58:33 GMT, Mandy Chung wrote: > This is a follow-up on JDK-8255342 that removes non-specified JVM checks on > classes with RecordComponents attributes. > > See the discussion at > https://mail.openjdk.java.net/pipermail/amber-spec-experts/2020-December/002670.html > > That fixes trusting final fields of records to align with the JLS definition > of a record class. `InstanceKlass::is_record` is fixed to check a record > class must be final and a direct subclass of `java.lang.Record` with the > presence of the `Record` attribute. There is no change to JVM class file > validation. I also propose clearly define: > - `JVM_IsRecord` returns true if the given class is a record i.e. final and > direct subclass of `java.lang.Record` with `Record` attribute > - `JVM_GetRecordComponents `returns an `RecordComponents` array if `Record` > attribute is present; otherwise, returns NULL. This does not check if it's a > record class or not. So it may return non-null on a non-record class if it > has `Record` attribute. So `JVM_GetRecordComponents` returns the content of > the classfile. > > tier1-tier3 and jck-runtime:vm and jck-runtime:lang tests all passed. Hi Mandy, The changes look good. Thanks for running the tests and changing the attribute name in the comments. Could you also change the two comments that you added to jvm.c with these comments: Comment for JVM_IsRecord(): // A class is a record if and only if it is final and a direct subclass of // java.lang.Record and has a Record attribute; otherwise, it is not a record. Comment for JVM_GetRecordComponents(): // Returns an array containing the components of the Record attribute, // or NULL if the attribute is not present. // // Note that this function returns the components of the Record attribute // even if the class is not a Record. Thanks, Harold - Marked as reviewed by hseigel (Reviewer). PR: https://git.openjdk.java.net/jdk16/pull/14
Re: RFR: 8253797: [cgroups v2] Account for the fact that swap accounting is disabled on some systems
On Mon, 7 Dec 2020 17:48:01 GMT, Severin Gehwolf wrote: > This has been implemented for cgroups v1 with > [JDK-8250984](https://bugs.openjdk.java.net/browse/JDK-8250984) but was > lacking some tooling support for cgroups v2. With podman 2.2.0 release this > could now be implemented (and tested). The idea is the same as for the > cgroups v1 fix. If we've got no swap limit capabilities, return the memory > limit only. > > Note that for cgroups v2 doesn't implement CgroupV1Metrics (obviously) and, > thus, doesn't have `getMemoryAndSwapFailCount()` and > `getMemoryAndSwapMaxUsage()`. > > Testing: > - [x] submit testing > - [x] container tests on cgroups v2 with swapaccount=0. > - [x] Manual container tests involving `-XshowSettings:system` on cgroups v2. > > Thoughts? The changes look good. Thanks for doing this. Harold - Marked as reviewed by hseigel (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1672
Re: RFR: 8257596: Clarify trusted final fields for record classes
On Tue, 8 Dec 2020 22:52:37 GMT, Mandy Chung wrote: > This is a follow-up on JDK-8255342 that removes non-specified JVM checks on > classes with `RecordComponents` attributes. That introduces a regression in > `InstanceKlass::is_record` that returns true on a non-record class which has > `RecordComponents` attribute present. This causes unexpected semantics in > `JVM_IsRecord` and `JVM_GetRecordComponents` and also a regression to trust > final fields for non-record classes. > > I propose to change `InstanceKlass::is_record` to match the JLS semantic of a > record class, i.e. final direct subclass of `java.lang.Record` with the > presence of `RecordComponents` attribute. There is no change to JVM class > file validation. Also I propose clearly define: > - `JVM_IsRecord` returns true if the given class is a record i.e. final > and direct subclass of `java.lang.Record` with `RecordComponents` attribute > - `JVM_GetRecordComponents` returns an `RecordComponents` array if > `RecordComponents` attribute is present; otherwise, returns NULL. This does > not check if it's a record class or not. So it may return non-null on a > non-record class if it has `RecordComponents` attribute. So > `JVM_GetRecordComponents` returns the content of the classfile. Hi Mandy, Could you replace the comment starting at line 1854 in jvm.cpp with: // A class is a record if and only if it is final and a direct subclass // of java.lang.Record and the presence of `Record` attribute; // otherwise, it is not a record. Also, replace the comment starting at line 1872 in jvm.cpp with: // Returns an array containing the components of the Record attribute, // or NULL if the attribute is not present. // // Note that this function returns the components of the Record // attribute even if the class is not a Record. Also, please change the name of the attribute in the comments added to Class.java from RecordComponent to Record. Thanks, Harold - PR: https://git.openjdk.java.net/jdk/pull/1706
Re: RFR: 8257596: Clarify trusted final fields for record classes
On Tue, 8 Dec 2020 22:52:37 GMT, Mandy Chung wrote: > This is a follow-up on JDK-8255342 that removes non-specified JVM checks on > classes with `RecordComponents` attributes. That introduces a regression in > `InstanceKlass::is_record` that returns true on a non-record class which has > `RecordComponents` attribute present. This causes unexpected semantics in > `JVM_IsRecord` and `JVM_GetRecordComponents` and also a regression to trust > final fields for non-record classes. > > I propose to change `InstanceKlass::is_record` to match the JLS semantic of a > record class, i.e. final direct subclass of `java.lang.Record` with the > presence of `RecordComponents` attribute. There is no change to JVM class > file validation. Also I propose clearly define: > - `JVM_IsRecord` returns true if the given class is a record i.e. final > and direct subclass of `java.lang.Record` with `RecordComponents` attribute > - `JVM_GetRecordComponents` returns an `RecordComponents` array if > `RecordComponents` attribute is present; otherwise, returns NULL. This does > not check if it's a record class or not. So it may return non-null on a > non-record class if it has `RecordComponents` attribute. So > `JVM_GetRecordComponents` returns the content of the classfile. Hi Mandy, Could you regression test this change against the JCK lang and vm test suites and Mach5 tiers 1 and 2? Thanks, Harold - PR: https://git.openjdk.java.net/jdk/pull/1706
Integrated: 8256867: Classes with empty PermittedSubclasses attribute cannot be extended
On Mon, 7 Dec 2020 19:51:38 GMT, Harold Seigel wrote: > Please review this fix for JDK-8256867. This change no longer throws a > ClassFormatError exception when loading a class whose PermittedSubclasses > attribute is empty (contains no classes). Instead, the class is treated as a > sealed class which cannot be extended nor implemented. This new behavior > conforms to the JVM Spec. > > This change required changing Class.permittedSubclasses() to return an empty > array for classes with empty PermittedSubclasses attributes, and to return > null for non-sealed classes. > > This fix was tested with Mach5 tiers 1-2 on Linux, MacOS, and Windows, and > tiers 3-5 on Linux x64. > > Thanks, Harold This pull request has now been integrated. Changeset: d33a689b Author:Harold Seigel URL: https://git.openjdk.java.net/jdk/commit/d33a689b Stats: 167 lines in 8 files changed: 106 ins; 13 del; 48 mod 8256867: Classes with empty PermittedSubclasses attribute cannot be extended Reviewed-by: lfoltan, mchung, jlahoda, chegar - PR: https://git.openjdk.java.net/jdk/pull/1675
Re: RFR: 8256867: Classes with empty PermittedSubclasses attribute cannot be extended [v3]
On Tue, 8 Dec 2020 18:24:21 GMT, Mandy Chung wrote: >> Harold Seigel has updated the pull request incrementally with one additional >> commit since the last revision: >> >> 8256867: Classes with empty PermittedSubclasses attribute cannot be >> extended > > Marked as reviewed by mchung (Reviewer). Thanks Mandy, Chris, Lois, and Jan for reviewing this change. Harold - PR: https://git.openjdk.java.net/jdk/pull/1675
Re: RFR: 8256867: Classes with empty PermittedSubclasses attribute cannot be extended [v3]
> Please review this fix for JDK-8256867. This change no longer throws a > ClassFormatError exception when loading a class whose PermittedSubclasses > attribute is empty (contains no classes). Instead, the class is treated as a > sealed class which cannot be extended nor implemented. This new behavior > conforms to the JVM Spec. > > This change required changing Class.permittedSubclasses() to return an empty > array for classes with empty PermittedSubclasses attributes, and to return > null for non-sealed classes. > > This fix was tested with Mach5 tiers 1-2 on Linux, MacOS, and Windows, and > tiers 3-5 on Linux x64. > > Thanks, Harold Harold Seigel has updated the pull request incrementally with one additional commit since the last revision: 8256867: Classes with empty PermittedSubclasses attribute cannot be extended - Changes: - all: https://git.openjdk.java.net/jdk/pull/1675/files - new: https://git.openjdk.java.net/jdk/pull/1675/files/de461457..2ce2a993 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1675&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1675&range=01-02 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/1675.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1675/head:pull/1675 PR: https://git.openjdk.java.net/jdk/pull/1675
Re: RFR: 8256867: Classes with empty PermittedSubclasses attribute cannot be extended [v2]
On Tue, 8 Dec 2020 14:37:52 GMT, Chris Hegarty wrote:
>> Harold Seigel has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> 8256867: Classes with empty PermittedSubclasses attribute cannot be
>> extended
>
> src/java.base/share/classes/java/lang/Class.java line 4399:
>
>> 4397: * that is {@link #isSealed()} returns {@code false}, then this
>> method returns {@code null}.
>> 4398: * Conversely, if {@link #isSealed()} returns {@code true}, then
>> this method
>> 4399: * returns a non-null value."
>
> Trivially, the trailing quote can be removed.
Thanks Chris! I'll remove the trailing quote.
-
PR: https://git.openjdk.java.net/jdk/pull/1675
Re: RFR: 8256867: Classes with empty PermittedSubclasses attribute cannot be extended [v2]
On Tue, 8 Dec 2020 09:30:59 GMT, Chris Hegarty wrote:
>> src/java.base/share/classes/java/lang/Class.java line 4396:
>>
>>> 4394: * is unspecified. If this {@code Class} object represents a
>>> primitive type,
>>> 4395: * {@code void}, an array type, or a class or interface that is
>>> not sealed,
>>> 4396: * then null is returned.
>>
>> nit: s/null/`{@code null}`
>>
>> I'd suggest to clarify if this sealed class or interface has no permitted
>> subclass, something like this:
>> Returns an array containing {@code Class} objects representing the
>> direct subinterfaces or subclasses permitted to extend or
>> implement this class or interface if it is sealed. The order of such
>> elements
>> is unspecified. The array is empty if this sealed class or interface has no
>> permitted subclass.
>>
>> `@return` needs to be revised as well:
>> @return an array of {@code Class} objects of the permitted subclasses of
>> this sealed class or interface,
>> or {@null} if this class or interface is not sealed
>
> Mandy's suggested wording is good.
>
> I would like to add one more additional point of clarification. It would
> be good to strongly connect `isSealed` and `getPermittedClasses` in a
> first-class way in normative spec ( similar to isRecord and
> getRecordComponents ).
>
> For example,
>
> to `isSealed` add: "getPermittedSubclasses returns a non-null but possibly
>empty value for a sealed class."
>
> to `getPermittedSubclasses`: "If this class is not a sealed class, that is
> {@link
> * #isSealed()} returns {@code false}, then this method returns {@code
> null}.
> * Conversely, if {@link #isSealed()} returns {@code true}, then this
> method
> * returns a non-null value."
Please review the updated commit. It incorporates the changes to the comments
in Class.java suggested by Mandy and Chris.
Thanks, Harold
-
PR: https://git.openjdk.java.net/jdk/pull/1675
Re: RFR: 8256867: Classes with empty PermittedSubclasses attribute cannot be extended [v2]
On Tue, 8 Dec 2020 00:09:34 GMT, Mandy Chung wrote: >> src/hotspot/share/prims/jvm.cpp line 2130: >> >>> 2128: JvmtiVMObjectAllocEventCollector oam; >>> 2129: Array* subclasses = ik->permitted_subclasses(); >>> 2130: int length = subclasses == NULL ? 0 : subclasses->length(); >> >> Minor comment - you don't really need the check of subclasses == NULL here >> since subclasses will never be NULL. You could just assign length to >> subclasses->length(); > > +1. is_sealed returns true iff `_permitted_subclasses != NULL` Thanks for the reviews. I removed the check of subclasses == NULL in the updated commit. - PR: https://git.openjdk.java.net/jdk/pull/1675
Re: RFR: 8256867: Classes with empty PermittedSubclasses attribute cannot be extended [v2]
> Please review this fix for JDK-8256867. This change no longer throws a > ClassFormatError exception when loading a class whose PermittedSubclasses > attribute is empty (contains no classes). Instead, the class is treated as a > sealed class which cannot be extended nor implemented. This new behavior > conforms to the JVM Spec. > > This change required changing Class.permittedSubclasses() to return an empty > array for classes with empty PermittedSubclasses attributes, and to return > null for non-sealed classes. > > This fix was tested with Mach5 tiers 1-2 on Linux, MacOS, and Windows, and > tiers 3-5 on Linux x64. > > Thanks, Harold Harold Seigel has updated the pull request incrementally with one additional commit since the last revision: 8256867: Classes with empty PermittedSubclasses attribute cannot be extended - Changes: - all: https://git.openjdk.java.net/jdk/pull/1675/files - new: https://git.openjdk.java.net/jdk/pull/1675/files/89c61b95..de461457 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1675&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1675&range=00-01 Stats: 12 lines in 2 files changed: 6 ins; 0 del; 6 mod Patch: https://git.openjdk.java.net/jdk/pull/1675.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1675/head:pull/1675 PR: https://git.openjdk.java.net/jdk/pull/1675
Re: RFR: 8256867: Classes with empty PermittedSubclasses attribute cannot be extended
On Mon, 7 Dec 2020 21:18:45 GMT, Joe Darcy wrote: >> Please review this fix for JDK-8256867. This change no longer throws a >> ClassFormatError exception when loading a class whose PermittedSubclasses >> attribute is empty (contains no classes). Instead, the class is treated as >> a sealed class which cannot be extended nor implemented. This new behavior >> conforms to the JVM Spec. >> >> This change required changing Class.permittedSubclasses() to return an empty >> array for classes with empty PermittedSubclasses attributes, and to return >> null for non-sealed classes. >> >> This fix was tested with Mach5 tiers 1-2 on Linux, MacOS, and Windows, and >> tiers 3-5 on Linux x64. >> >> Thanks, Harold > > Is there already CSR coverage of this change? There is no CSR because I viewed this as a bug fix / spec conformance issue. But, I can write a CSR if you think one is needed. - PR: https://git.openjdk.java.net/jdk/pull/1675
RFR: 8256867: Classes with empty PermittedSubclasses attribute cannot be extended
Please review this fix for JDK-8256867. This change no longer throws a ClassFormatError exception when loading a class whose PermittedSubclasses attribute is empty (contains no classes). Instead, the class is treated as a sealed class which cannot be extended nor implemented. This new behavior conforms to the JVM Spec. This change required changing Class.permittedSubclasses() to return an empty array for classes with empty PermittedSubclasses attributes, and to return null for non-sealed classes. This fix was tested with Mach5 tiers 1-2 on Linux, MacOS, and Windows, and tiers 3-5 on Linux x64. Thanks, Harold - Commit messages: - 8256867: Classes with empty PermittedSubclasses attribute cannot be extended Changes: https://git.openjdk.java.net/jdk/pull/1675/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=1675&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8256867 Stats: 156 lines in 8 files changed: 100 ins; 13 del; 43 mod Patch: https://git.openjdk.java.net/jdk/pull/1675.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1675/head:pull/1675 PR: https://git.openjdk.java.net/jdk/pull/1675
Re: RFR: 8252180: [JEP 390] Deprecate wrapper class constructors for removal
On Sat, 5 Dec 2020 01:46:31 GMT, Dan Smith wrote: > Integration of [JEP 390](https://bugs.openjdk.java.net/browse/JDK-8249100). > > Development has been broken into 5 tasks, each with its own JBS issue: > - Deprecate wrapper class constructors for removal (rriggs) > - Revise "value-based class" & apply to wrappers (dlsmith) > - Define & apply annotation jdk.internal.ValueBased (rriggs) > - Add 'lint' warning for @ValueBased classes (sadayapalam) > - Diagnose synchronization on @ValueBased classes (lfoltan) > > All changes have been previously reviewed and integrated into the [`jep390` > branch](https://github.com/openjdk/valhalla/tree/jep390) of the `valhalla` > repository. See the subtasks of the 5 JBS issues for these changes, including > discussion and links to reviews. (Reviewers: mchung, dlsmith, jlaskey, > rriggs, lfoltan, fparain, hseigel.) > > CSRs have also been completed or are nearly complete: > > - [JDK-8254324](https://bugs.openjdk.java.net/browse/JDK-8254324) for wrapper > class constructor deprecation > - [JDK-8254944](https://bugs.openjdk.java.net/browse/JDK-8254944) for > revisions to "value-based class" > - [JDK-8256023](https://bugs.openjdk.java.net/browse/JDK-8256023) for new > `javac` lint warnings > > Here's an overview of the files changed: > > - `src/hotspot`: implementing diagnostics when `monitorenter` is applied to > an instance of a class tagged with `jdk.internal.ValueBased`. This enhances > previous work that produced such diagnostics for the primitive wrapper > classes. > > - `src/java.base/share/classes/java/lang`: deprecating for removal the > wrapper class constructors; revising the definition of "value-based class" in > `ValueBased.html` and the description used by linking classes; applying > "value-based class" to the primitive wrapper classes; marking value-based > classes with `@jdk.internal.ValueBased`. > > - `src/java.base/share/classes/java/lang/constant`: no longer designating > these classes as "value-based", since they rely heavily on field inheritance. > > - `src/java.base/share/classes/java/time`: revising the description used to > link to `ValueBased.html`; marking value-based classes with > `@jdk.internal.ValueBased`. > > - `src/java.base/share/classes/java/util`: revising the description used to > link to `ValueBased.html`; marking value-based classes with > `@jdk.internal.ValueBased`. > > - `src/java.base/share/classes/jdk/internal`: define the > `@jdk.internal.ValueBased` annotation. > > - `src/java.management.rmi`: removing uses of wrapper class constructors. > > - `src/java.xml`: removing uses of wrapper class constructors. > > - `src/jdk.compiler`: implementing the `synchronization` lint category, which > reports attempts to synchronize on classes and interfaces annotated with > `@jdk.internal.ValueBased`. > > - `src/jdk.incubator.foreign`: revising the description used to link to > `ValueBased.html`. (Applying `@jdk.internal.ValueBased` would require a > special module export, which was not deemed necessary for an incubating API.) > > - `src/jdk.internal.vm.compiler`: suppressing `javac` deprecation and > synchronization warnings in tests > > - `src/jdk.jfr`: supplementary changes for HotSpot diagnostics > > - `test`: testing new `javac` and HotSpot behavior; removing usages of > deprecated wrapper class constructors from tests, or suppressing deprecation > warnings; revising the description used to link to `ValueBased.html`. The hotspot changes look good. In a future change, could you add additional classes, such as ProcessHandle to test TestSyncOnValueBasedClassEvent. Currently, it only tests primitive classes. - Marked as reviewed by hseigel (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1636
Re: RFR: 8246778: Compiler implementation for Sealed Classes (Second Preview)
On Tue, 1 Dec 2020 23:16:45 GMT, Mandy Chung wrote: >> This pull request replaces https://github.com/openjdk/jdk/pull/1227. >> >> From the original PR: >> >>> Please review the code for the second iteration of sealed classes. In this >>> iteration we are: >>> >>> * Enhancing narrowing reference conversion to allow for stricter >>> checking of cast conversions with respect to sealed type hierarchies >>> >>> * Also local classes are not considered when determining implicitly >>> declared permitted direct subclasses of a sealed class or sealed interface >>> >>> * renaming Class::permittedSubclasses to Class::getPermittedSubclasses, >>> still in the same method, the return type has been changed to Class[] >>> instead of the previous ClassDesc[] >>> >>> * adding code to make sure that annotations can't be sealed >>> >>> * improving some tests >>> >>> >>> TIA >>> >>> Related specs: >>> [Sealed Classes >>> JSL](http://cr.openjdk.java.net/~gbierman/jep397/jep397-20201104/specs/sealed-classes-jls.html) >>> [Sealed Classes >>> JVMS](http://cr.openjdk.java.net/~gbierman/jep397/jep397-20201104/specs/sealed-classes-jvms.html) >>> [Additional: Contextual >>> Keywords](http://cr.openjdk.java.net/~gbierman/jep397/jep397-20201104/specs/contextual-keywords-jls.html) >> >> This PR strives to reflect the review comments from 1227: >> * adjustments to javadoc of j.l.Class methods >> * package access checks in Class.getPermittedSubclasses() >> * fixed to the narrowing conversion/castability as pointed out by Maurizio > > From David's comment: > >> These subtleties are what I was also getting at in the CSR discussion. If >> Foo lists Bar as a permitted subtype, but Bar does not extend Foo, is that >> an error? Should Bar not be returned by getPermittedSubclasses? The answer >> depends on exactly how you specify getPermittedSubclasses() and whether that >> aligns with reading the classfile attribute, or actually checking the >> runtime relationships. > > As I read the current spec of `Class::getPermittedSubclasses`, it intends to > return the direct subclasses or subinterfaces permitted to extend or > implement this class or interface if it's sealed. If not, it should be > clarified that it is the class file view. > > `NestMembers` and `PermittedSubclasses` attributes are critical to correct > interpretation of the class file by JVM. Prior to these two attributes, the > attributes inspected by core reflection APIs are all non-critical. API like > `Class::getDeclaringClass` reads `InnerClasses` attribute if present in order > to determine its declaring class but the current spec does not specify the > behavior on error cases (which I consider a spec bug - see JDK-8250226.) > > IMO it is reasonable for `getPermittedSubclasses` (and `getNestMembers` and > `getNestHost`) to return the runtime view as it returns `Class` objects since > these attributes are critical to correct interpretation of the class file. > It would be confusing if it returns `Foo` that is not a permitted subtype at > runtime. Additional changes may be needed to Class.permittedSubclasses() and/or Class.isSealed() as part of fixing bug JDK-8256867. The JVM is being changed to treat classes with empty PermittedSubclasses attributes as sealed classes that cannot be extended (or implemented). Current thinking is that Class.permittedSubclasses() will return an empty array for both non-sealed classes and for sealed classes with empty PermittedSubclasses attributes. And, Class.isSealed() will return False in the former case and True in the latter. This will require changing the implementation of Class.isSealed() to call the JVM directly instead of calling Class.permittedSubclasses(). Does this seem like a reasonable way to handle this corner case? - PR: https://git.openjdk.java.net/jdk/pull/1483
Re: RFR: 8246778: Compiler implementation for Sealed Classes (Second Preview) [v2]
On Mon, 30 Nov 2020 19:44:52 GMT, Mandy Chung wrote: >> Jan Lahoda has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains 12 commits: >> >> - Improving getPermittedSubclasses javadoc. >> - Merge branch 'master' into JDK-8246778 >> - Moving checkPackageAccess from getPermittedSubclasses to a separate >> method. >> - Improving getPermittedSubclasses() javadoc. >> - Enhancing the Class.getPermittedSubclasses() test to verify behavior both >> for sealed classes in named and unnamed modules. >> - Removing unnecessary file. >> - Tweaking javadoc. >> - Reflecting review comments w.r.t. narrowing conversion. >> - Improving checks in getPermittedSubclasses() >> - Merging master into JDK-8246778 >> - ... and 2 more: >> https://git.openjdk.java.net/jdk/compare/6e006223...4d484179 > > src/java.base/share/classes/java/lang/Class.java line 4480: > >> 4478: } >> 4479: >> 4480: private native Class[] getPermittedSubclasses0(); > > Does this JVM method return the permitted subclasses or subinterfaces with > the following conditions enforced by JLS: > >- If a sealed class C belongs to a named module, then every class named in > the permits clause of the declaration of C must belong to the same module as C >- If a sealed class C belongs to an unnamed module, then every class named > in the permits clause of the declaration of C must belong to the same package > as C > > I didn't check the VM implementation. > > If the return array contains only classes as specified above, > `checkPackageAccessForClasses` can be simplified. The JVM method that returns the permitted subclasses (and interfaces) does not weed out permitted subclasses based on the above module requirements. It returns all the classes listed in the PermittedSubclasses attribute that it is able to load. - PR: https://git.openjdk.java.net/jdk/pull/1483
Re: RFR: 8246778: Compiler implementation for Sealed Classes (Second Preview)
Hi David, Thanks for looking at this. The intent was for method Class.permittedSubclasses() to be implemented similarly to Class.getNestMembers(). Are you suggesting that a security manager check be added to permittedSubclasses() similar to the security manager check in getNestMembers()? Thanks, Harold On 11/18/2020 12:31 AM, David Holmes wrote: Hi Vincente, On 16/11/2020 11:36 pm, Vicente Romero wrote: Please review the code for the second iteration of sealed classes. In this iteration we are: - Enhancing narrowing reference conversion to allow for stricter checking of cast conversions with respect to sealed type hierarchies. - Also local classes are not considered when determining implicitly declared permitted direct subclasses of a sealed class or sealed interface The major change here seems to be that getPermittedSubclasses() now returns actual Class objects instead of ClassDesc. My recollection from earlier discussions here was that the use of ClassDesc was very deliberate as the permitted subclasses may not actually exist and there may be security concerns with returning them! Cheers, David - - Commit messages: - 8246778: Compiler implementation for Sealed Classes (Second Preview) Changes: https://git.openjdk.java.net/jdk/pull/1227/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=1227&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8246778 Stats: 589 lines in 12 files changed: 508 ins; 18 del; 63 mod Patch: https://git.openjdk.java.net/jdk/pull/1227.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1227/head:pull/1227 PR: https://git.openjdk.java.net/jdk/pull/1227
Re: RFR: 8255787: Tag container tests that use cGroups with cgroups keyword
On Wed, 11 Nov 2020 00:27:59 GMT, Serguei Spitsyn wrote: >> Please review this small change to add a cgroups keyword to tests that use >> cgroups. The fix was tested by running Mach5 container tests. > > Hi Harold, > > The fix looks good. > > Thanks, > Serguei Thanks Serguei! Harold - PR: https://git.openjdk.java.net/jdk/pull/1148
Integrated: 8255787: Tag container tests that use cGroups with cgroups keyword
On Tue, 10 Nov 2020 21:24:25 GMT, Harold Seigel wrote: > Please review this small change to add a cgroups keyword to tests that use > cgroups. The fix was tested by running Mach5 container tests. This pull request has now been integrated. Changeset: 4df8abc2 Author:Harold Seigel URL: https://git.openjdk.java.net/jdk/commit/4df8abc2 Stats: 20 lines in 15 files changed: 15 ins; 0 del; 5 mod 8255787: Tag container tests that use cGroups with cgroups keyword Reviewed-by: sspitsyn - PR: https://git.openjdk.java.net/jdk/pull/1148
RFR: 8255787: Tag container tests that use cGroups with cgroups keyword
Please review this small change to add a cgroups keyword to tests that use cgroups. The fix was tested by running Mach5 container tests. - Commit messages: - 8255787: Tag container tests that use cGroups with cgroups keyword Changes: https://git.openjdk.java.net/jdk/pull/1148/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=1148&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8255787 Stats: 20 lines in 15 files changed: 15 ins; 0 del; 5 mod Patch: https://git.openjdk.java.net/jdk/pull/1148.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1148/head:pull/1148 PR: https://git.openjdk.java.net/jdk/pull/1148
Re: RFR: 8238263: Create at-requires mechanism for containers [v2]
On Mon, 26 Oct 2020 20:24:35 GMT, Igor Ignatyev wrote: >> Harold Seigel has updated the pull request incrementally with one additional >> commit since the last revision: >> >> 8238263: Create at-requires mechanism for containers > > LGTM, modulo my earlier comment/doubt about the name, but I don’t think it’s > important enough Thanks Bob and Igor! - PR: https://git.openjdk.java.net/jdk/pull/844
Integrated: 8238263: Create at-requires mechanism for containers
On Fri, 23 Oct 2020 18:44:54 GMT, Harold Seigel wrote: > Please review this change to add an @requires mechanism called > "jdk.containerized" to help mark tests that are incompatible with containers. > Users would add "@requires jdk.containerized != true" to the incompatible > tests and then use "make test ... OPTIONS=-Djdk.containerized=true" or "bash > jib.sh mach5 -- remote-build-and-test ... --test-make-args > JTREG=OPTIONS=-Djdk.containerized=true" to exclude those tests when testing > with containers. This pull request has now been integrated. Changeset: ca8bba64 Author:Harold Seigel URL: https://git.openjdk.java.net/jdk/commit/ca8bba64 Stats: 10 lines in 3 files changed: 8 ins; 0 del; 2 mod 8238263: Create at-requires mechanism for containers Reviewed-by: bobv, iignatyev - PR: https://git.openjdk.java.net/jdk/pull/844
Re: RFR: 8238263: Create at-requires mechanism for containers
On Mon, 26 Oct 2020 14:25:14 GMT, Igor Ignatyev wrote: >> Defining an environment variable works when running JTReg from the command >> line. But, mach5 does not pass environment variable settings to its JTReg >> test runs. Some mach5 special command args would still be needed. > >> Defining an environment variable works when running JTReg from the command >> line. But, mach5 does not pass environment variable settings to its JTReg >> test runs. Some mach5 special command args would still be needed. > > right, yet given you also need to explicitly say mach5 that you want to run > testing within docker, that's not a huge problem. this is assuming we default > env. variable to `false`, `make` propagates this env. variable to `jtreg` and > `jtreg` propagates it to the JVM which runs `VMProps` class. Please review this updated webrev that adds environment variable TEST_JDK_CONTAINERIZED for setting @requires jdk.containerized. When TEST_JDK_CONTAINERIZED is unset, jdk.containerized is false. - PR: https://git.openjdk.java.net/jdk/pull/844
Re: RFR: 8238263: Create at-requires mechanism for containers [v2]
> Please review this change to add an @requires mechanism called > "jdk.containerized" to help mark tests that are incompatible with containers. > Users would add "@requires jdk.containerized != true" to the incompatible > tests and then use "make test ... OPTIONS=-Djdk.containerized=true" or "bash > jib.sh mach5 -- remote-build-and-test ... --test-make-args > JTREG=OPTIONS=-Djdk.containerized=true" to exclude those tests when testing > with containers. Harold Seigel has updated the pull request incrementally with one additional commit since the last revision: 8238263: Create at-requires mechanism for containers - Changes: - all: https://git.openjdk.java.net/jdk/pull/844/files - new: https://git.openjdk.java.net/jdk/pull/844/files/9ffe9ad7..7a5ae052 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=844&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=844&range=00-01 Stats: 6 lines in 1 file changed: 6 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/844.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/844/head:pull/844 PR: https://git.openjdk.java.net/jdk/pull/844
Re: RFR: 8238263: Create at-requires mechanism for containers
On Fri, 23 Oct 2020 20:08:01 GMT, Igor Ignatyev wrote: >>> I think it depends on whether the tests will be permanently or temporarily >>> excluded from running with containers. I thought this mechanism would be to >>> permanently exclude the tests. That's why I used `@requires`. >> >> I see, if this is for permanent exclusion then yes I agree that `@requires` >> is a better choice. >> enable this option based on an environment variable so we don?t have to remember the >> cryptic command line sequence. >>> I'll look into basing the option on an environment variable. >> >> one will still need to pass an environment variable to jtreg, and hence will >> need to remember some sort of "cryptic command line sequence". a solution >> for that might be to default `jdk.containerized` to `false` in >> `VMProps.java` and when only _containerized_ runs will have to set it up. >> >> >> btw, I'm not sure that `jdk.containerized` is the best name for this >> property as _containerization_ is more of an environmental characteristic >> than that of jdk. how about smth like `env.containerized` or >> `testenv.containerized`? > >> > one will still need to pass an environment variable to jtreg, and hence >> > will need to remember some sort of "cryptic command line sequence". a >> > solution for that might be to default `jdk.containerized` to `false` in >> > `VMProps.java` and when only _containerized_ runs will have to set it up. >> >> Why? Environment variables are inherited. For developers running jtreg, all >> they need to do is export the variable. >> >> % export JAVA_CONTAINERIZED=1 >> % bash >> % echo $JAVA_CONTAINERIZED >> % 1 > > b/c jtreg strips most of the environment variables, they might still be > defined in the process which runs `VMProps` though (I have never checked that) Defining an environment variable works when running JTReg from the command line. But, mach5 does not pass environment variable settings to its JTReg test runs. Some mach5 special command args would still be needed. - PR: https://git.openjdk.java.net/jdk/pull/844
Re: RFR: 8238263: Create at-requires mechanism for containers
On Fri, 23 Oct 2020 19:17:40 GMT, Igor Ignatyev wrote: >> Please review this change to add an @requires mechanism called >> "jdk.containerized" to help mark tests that are incompatible with >> containers. Users would add "@requires jdk.containerized != true" to the >> incompatible tests and then use "make test ... >> OPTIONS=-Djdk.containerized=true" or "bash jib.sh mach5 -- >> remote-build-and-test ... --test-make-args >> JTREG=OPTIONS=-Djdk.containerized=true" to exclude those tests when testing >> with containers. > > Hi Harold, > > I actually still think that having a separate ProblemList (as we do for > graal, zgc, Xcomp, aot) is a better solution. why did you choose `@requires` > over it? > > -- Igor Hi Igor, I think it depends on whether the tests will be permanently or temporarily excluded from running with containers. I thought this mechanism would be to permanently exclude the tests. That's why I used @requires. Thanks, Harold - PR: https://git.openjdk.java.net/jdk/pull/844
Re: RFR: 8238263: Create at-requires mechanism for containers
Hi Bob, Thanks for looking at this. I'll look into basing the option on an environment variable. Thanks, Harold On 10/23/2020 3:12 PM, Bob Vandette wrote: I wonder if it makes sense to add this option to open/test/jtreg-ext/requires/VMProps.java and have it automatically enable this option based on an environment variable so we don’t have to remember the cryptic command line sequence. It’s too bad we can’t automatically detect that we are running in a container. Bob. On Oct 23, 2020, at 2:50 PM, Harold Seigel wrote: Please review this change to add an @requires mechanism called "jdk.containerized" to help mark tests that are incompatible with containers. Users would add "@requires jdk.containerized != true" to the incompatible tests and then use "make test ... OPTIONS=-Djdk.containerized=true" or "bash jib.sh mach5 -- remote-build-and-test ... --test-make-args JTREG=OPTIONS=-Djdk.containerized=true" to exclude those tests when testing with containers. - Commit messages: - 8238263: Create at-requires mechanism for containers Changes: https://git.openjdk.java.net/jdk/pull/844/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=844&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8238263 Stats: 4 lines in 2 files changed: 2 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/844.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/844/head:pull/844 PR: https://git.openjdk.java.net/jdk/pull/844
RFR: 8238263: Create at-requires mechanism for containers
Please review this change to add an @requires mechanism called "jdk.containerized" to help mark tests that are incompatible with containers. Users would add "@requires jdk.containerized != true" to the incompatible tests and then use "make test ... OPTIONS=-Djdk.containerized=true" or "bash jib.sh mach5 -- remote-build-and-test ... --test-make-args JTREG=OPTIONS=-Djdk.containerized=true" to exclude those tests when testing with containers. - Commit messages: - 8238263: Create at-requires mechanism for containers Changes: https://git.openjdk.java.net/jdk/pull/844/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=844&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8238263 Stats: 4 lines in 2 files changed: 2 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/844.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/844/head:pull/844 PR: https://git.openjdk.java.net/jdk/pull/844
RFR: 8253476: TestUseContainerSupport.java fails on some Linux kernels w/o…
Please review this small change to remove "--memory 200m" option from TestUseContainerSupport.java. This can cause test failures on systems where swap accounting is disabled. - Commit messages: - 8253476: TestUseContainerSupport.java fails on some Linux kernels w/o swap limit capabilities Changes: https://git.openjdk.java.net/jdk/pull/303/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=303&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8253476 Stats: 2 lines in 1 file changed: 0 ins; 1 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/303.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/303/head:pull/303 PR: https://git.openjdk.java.net/jdk/pull/303
Re: RFR: JDK-8225056 VM support for sealed classes
Hi David,
Thanks for reviewing the latest changes.
I'll create the follow on RFE's once the sealed classes code is in mainline.
Harold
On 5/31/2020 9:34 PM, David Holmes wrote:
Hi Harold,
On 1/06/2020 8:57 am, Harold Seigel wrote:
Thanks for the comments.
Here's version 3 of the JDK and VM changes for sealed classes.
full webrev:
http://cr.openjdk.java.net/~hseigel/sealedClasses.8225056.3/webrev/
The new webrev contains just the following three changes:
1. The sealed classes API's in Class.java (permittedSubclasses() and
isSealed()) were revised and, in particular, API
permittedSubclasses() no longer uses reflection.
For those following along we have presently abandoned the attempt to
cache the array in ReflectionData.
Current changes look okay. But I note from the CSR there appears to be
a further minor update to the javadoc coming.
2. An unneeded 'if' statement was removed from
JVM_GetPermittedSubclasses() (pointed out by David.)
Looks good.
3. VM runtime test files SealedUnnamedModuleIntfTest.java and
Permitted.java were changed to add a test case for a non-public
permitted subclass and its sealed superclass being in the same
module and package.
Looks good.
Additionally, two follow on RFE's will be filed. One to add
additional VM sealed classes tests
Thanks. I think there is a more mechanical approach to testing here
that will allow the complete matrix to be easily covered with minimal
duplication between testing for named and unnamed modules.
and one to improve the implementations of the sealed classes API's in
Class.java.
Thanks.
David
-
Thanks, Harold
On 5/28/2020 8:30 PM, David Holmes wrote:
Hi Harold,
Sorry Mandy's comment raised a couple of issues ...
On 29/05/2020 7:12 am, Mandy Chung wrote:
Hi Harold,
On 5/27/20 1:35 PM, Harold Seigel wrote:
Incremental webrev:
http://cr.openjdk.java.net/~hseigel/sealedClasses.8225056.incr.2/
full webrev:
http://cr.openjdk.java.net/~hseigel/sealedClasses.8225056.2/webrev/
Class.java
4406 ReflectionData rd = reflectionData();
4407 ClassDesc[] tmp = rd.permittedSubclasses;
4408 if (tmp != null) {
4409 return tmp;
4410 }
4411
4412 if (isArray() || isPrimitive()) {
4413 rd.permittedSubclasses = new ClassDesc[0];
4414 return rd.permittedSubclasses;
4415 }
This causes an array class or primitive type to create a
ReflectionData. It should first check if this is non-sealed class
and returns a constant empty array.
It can't check if this is a non-sealed class as the isSealed() check
calls the above code! But for arrays and primitives which can't be
sealed we should just do:
4412 if (isArray() || isPrimitive()) {
4413 return new ClassDesc[0];
4414 }
But this then made me realize that we need to be creating defensive
copies of the returned arrays, as happens with other APIs that use
ReflectionData.
Backing up a bit I complained that:
public boolean isSealed() {
return permittedSubclasses().length != 0;
}
is a very inefficient way to answer the question as to whether a
class is sealed, so I suggested that the result of
permittedSubclasses() be cached. Caching is not without its own
issues as we are discovering, and when you add in defensive copies
this seems to be trading one inefficiency for another. For nestmates
we don't cache getNestMembers() because we don;t think it will be
called often - it is there to complete the introspection API of
Class rather than being anticipated as used in a regular
programmatic sense. I expect the same is true for
permittedSubclasses(). Do we expect isSealed() to be used often or
is it too just there for completeness? If just for completeness then
perhaps a VM query would be a better compromise on the efficiency
front? Otherwise I can accept the current implementation of
isSealed(), and a non-caching permittedClasses() for this initial
implementation of sealed classes. If efficiency turns out to be a
problem for isSealed() then we can revisit it then.
Thanks,
David
In fact, ReflectionData caches the derived names and reflected
members for performance and also they may be invalidated when the
class is redefined. It might be okay to add
ReflectionData::permittedSubclasses while `PermittedSubclasses`
attribute can't be redefined and getting this attribute is not
performance sensitive. For example, the result of
`getNestMembers` is not cached in ReflectionData. It may be better
not to add it in ReflectionData for modifiable and
performance-sensitive data.
4421 tmp = new ClassDesc[subclassNames.length];
4422 int i = 0;
4423 for (String subclassName : subclassNames) {
4424 try {
4425 tmp[i++] = ClassDesc.of(subclassName.replace('/', '.'));
4426 } catch (IllegalArgumentException iae) {
4427 throw new InternalError("Invalid type in permitted subclasses
information: " + subclassName, iae);
4428 }
4429 }
Nit:
Re: RFR: JDK-8225056 VM support for sealed classes
Thanks for the comments.
Here's version 3 of the JDK and VM changes for sealed classes.
full webrev:
http://cr.openjdk.java.net/~hseigel/sealedClasses.8225056.3/webrev/
The new webrev contains just the following three changes:
1. The sealed classes API's in Class.java (permittedSubclasses() and
isSealed()) were revised and, in particular, API
permittedSubclasses() no longer uses reflection.
2. An unneeded 'if' statement was removed from
JVM_GetPermittedSubclasses() (pointed out by David.)
3. VM runtime test files SealedUnnamedModuleIntfTest.java and
Permitted.java were changed to add a test case for a non-public
permitted subclass and its sealed superclass being in the same
module and package.
Additionally, two follow on RFE's will be filed. One to add additional
VM sealed classes tests and one to improve the implementations of the
sealed classes API's in Class.java.
Thanks, Harold
On 5/28/2020 8:30 PM, David Holmes wrote:
Hi Harold,
Sorry Mandy's comment raised a couple of issues ...
On 29/05/2020 7:12 am, Mandy Chung wrote:
Hi Harold,
On 5/27/20 1:35 PM, Harold Seigel wrote:
Incremental webrev:
http://cr.openjdk.java.net/~hseigel/sealedClasses.8225056.incr.2/
full webrev:
http://cr.openjdk.java.net/~hseigel/sealedClasses.8225056.2/webrev/
Class.java
4406 ReflectionData rd = reflectionData();
4407 ClassDesc[] tmp = rd.permittedSubclasses;
4408 if (tmp != null) {
4409 return tmp;
4410 }
4411
4412 if (isArray() || isPrimitive()) {
4413 rd.permittedSubclasses = new ClassDesc[0];
4414 return rd.permittedSubclasses;
4415 }
This causes an array class or primitive type to create a
ReflectionData. It should first check if this is non-sealed class
and returns a constant empty array.
It can't check if this is a non-sealed class as the isSealed() check
calls the above code! But for arrays and primitives which can't be
sealed we should just do:
4412 if (isArray() || isPrimitive()) {
4413 return new ClassDesc[0];
4414 }
But this then made me realize that we need to be creating defensive
copies of the returned arrays, as happens with other APIs that use
ReflectionData.
Backing up a bit I complained that:
public boolean isSealed() {
return permittedSubclasses().length != 0;
}
is a very inefficient way to answer the question as to whether a class
is sealed, so I suggested that the result of permittedSubclasses() be
cached. Caching is not without its own issues as we are discovering,
and when you add in defensive copies this seems to be trading one
inefficiency for another. For nestmates we don't cache
getNestMembers() because we don;t think it will be called often - it
is there to complete the introspection API of Class rather than being
anticipated as used in a regular programmatic sense. I expect the same
is true for permittedSubclasses(). Do we expect isSealed() to be used
often or is it too just there for completeness? If just for
completeness then perhaps a VM query would be a better compromise on
the efficiency front? Otherwise I can accept the current
implementation of isSealed(), and a non-caching permittedClasses() for
this initial implementation of sealed classes. If efficiency turns out
to be a problem for isSealed() then we can revisit it then.
Thanks,
David
In fact, ReflectionData caches the derived names and reflected
members for performance and also they may be invalidated when the
class is redefined. It might be okay to add
ReflectionData::permittedSubclasses while `PermittedSubclasses`
attribute can't be redefined and getting this attribute is not
performance sensitive. For example, the result of `getNestMembers`
is not cached in ReflectionData. It may be better not to add it in
ReflectionData for modifiable and performance-sensitive data.
4421 tmp = new ClassDesc[subclassNames.length];
4422 int i = 0;
4423 for (String subclassName : subclassNames) {
4424 try {
4425 tmp[i++] = ClassDesc.of(subclassName.replace('/', '.'));
4426 } catch (IllegalArgumentException iae) {
4427 throw new InternalError("Invalid type in permitted subclasses
information: " + subclassName, iae);
4428 }
4429 }
Nit: rename tmp to some other name e.g. descs
I read the JVMS but it isn't clear to me that the VM will validate
the names in `PermittedSubclasses`attribute are valid class
descriptors. I see ConstantPool::is_klass_or_reference check but
can't find where it validates the name is a valid class descriptor -
can you point me there? (otherwise, maybe define it to be unspecified?)
W.r.t. the APIs. I don't want to delay this review. I see that you
renamed the method to new API style as I brought up. OTOH, I expect
more discussion is needed. Below is a recent comment from John on
this topic [1]
One comment, really for the future, regarding the shape of the Java
API here: It uses Optional and omits the "
Re: RFR: JDK-8225056 VM support for sealed classes
Hi Mandy,
The entries in the PermittedSubclasses attribute are constant pool
ConstantClass_info entries. These names get validated by the VM in this
code in ClassFileParser::parse_constant_pool():
for (index = 1; index < length; index++) {
const jbyte tag = cp->tag_at(index).value();
switch (tag) {
case JVM_CONSTANT_UnresolvedClass: {
const Symbol* const class_name = cp->klass_name_at(index);
// check the name, even if _cp_patches will overwrite it
*verify_legal_class_name(class_name, CHECK);*
break;
}
Thanks, Harold
On 5/28/2020 5:12 PM, Mandy Chung wrote:
I read the JVMS but it isn't clear to me that the VM will validate the
names in `PermittedSubclasses`attribute are valid class descriptors.
I see ConstantPool::is_klass_or_reference check but can't find where
it validates the name is a valid class descriptor - can you point me
there? (otherwise, maybe define it to be unspecified?)
Re: RFR: JDK-8225056 VM support for sealed classes
Hi David,
Please review this updated webrev:
Incremental webrev:
http://cr.openjdk.java.net/~hseigel/sealedClasses.8225056.incr.2/
full webrev:
http://cr.openjdk.java.net/~hseigel/sealedClasses.8225056.2/webrev/
It includes the following changes:
* Indentation and simplification changes as suggested below
* If a class is not a valid permitted subclass of its sealed super
then an IncompatibleClassChangeError exception is thrown (as
specified in the JVM Spec) instead of VerifyError.
* Added a check that a non-public subclass must be in the same package
as its sealed super. And added appropriate testing.
* Method Class.permittedSubtypes() was changed.
See also inline comments.
On 5/24/2020 10:28 PM, David Holmes wrote:
Hi Harold,
On 22/05/2020 4:33 am, Harold Seigel wrote:
Hi David,
Thanks for looking at this! Please review this new webrev:
http://cr.openjdk.java.net/~hseigel/webrev.01/webrev/
I'll list all relevant commens here rather than interspersing below so
that it is easier to track. Mostly nits below, other than the
is_permitted_subclass check in the VM, and the use of ReflectionData
in java.lang.Class.
--
src/hotspot/share/classfile/classFileParser.cpp
+ bool ClassFileParser::supports_sealed_types() {
+ return _major_version == JVM_CLASSFILE_MAJOR_VERSION &&
+ _minor_version == JAVA_PREVIEW_MINOR_VERSION &&
+ Arguments::enable_preview();
+ }
Nowe there is too little indentation - the subclauses of the
conjunction expression should align[1]
+ bool ClassFileParser::supports_sealed_types() {
+ return _major_version == JVM_CLASSFILE_MAJOR_VERSION &&
+ _minor_version == JAVA_PREVIEW_MINOR_VERSION &&
+ Arguments::enable_preview();
+ }
Fixed. Along with indentation of supports_records().
3791 if (parsed_permitted_subclasses_attribute) {
3792 classfile_parse_error("Multiple
PermittedSubclasses attributes in class file %s", CHECK);
3793 // Classes marked ACC_FINAL cannot have a
PermittedSubclasses attribute.
3794 } else if (_access_flags.is_final()) {
3795 classfile_parse_error("PermittedSubclasses
attribute in final class file %s", CHECK);
3796 } else {
3797 parsed_permitted_subclasses_attribute = true;
3798 }
The indent of the comment at L3793 is wrong, and its placement is
awkward because it relates to the next condition. But we don't have to
use if-else here as any parse error results in immediate return due to
the CHECK macro. So the above can be reformatted as:
3791 if (parsed_permitted_subclasses_attribute) {
3792 classfile_parse_error("Multiple
PermittedSubclasses attributes in class file %s", CHECK);
3793 }
3794 // Classes marked ACC_FINAL cannot have a
PermittedSubclasses attribute.
3795 if (_access_flags.is_final()) {
3796 classfile_parse_error("PermittedSubclasses
attribute in final class file %s", CHECK);
3797 }
3798 parsed_permitted_subclasses_attribute = true;
Done.
---
src/hotspot/share/oops/instanceKlass.cpp
The logic in InstanceKlass::has_as_permitted_subclass still does not
implement the rules specified in the JVMS. It only implements a "same
module" check, whereas the JVMS specifies an accessibility requirement
as well.
Done.
730 bool InstanceKlass::is_sealed() const {
731 return _permitted_subclasses != NULL &&
732 _permitted_subclasses !=
Universe::the_empty_short_array() &&
733 _permitted_subclasses->length() > 0;
734 }
Please align subclauses.
Done.
---
src/hotspot/share/prims/jvm.cpp
2159 objArrayHandle result (THREAD, r);
Please remove space after "result".
Done.
As we will always create and return an arry, if you reverse these two
statements:
2156 if (length != 0) {
2157 objArrayOop r =
oopFactory::new_objArray(SystemDictionary::String_klass(),
2158 length, CHECK_NULL);
and these two:
2169 return (jobjectArray)JNIHandles::make_local(THREAD, result());
2170 }
then you can delete
2172 // if it gets to here return an empty array, cases will be: the
class is primitive, or an array, or just not sealed
2173 objArrayOop result =
oopFactory::new_objArray(SystemDictionary::String_klass(), 0,
CHECK_NULL);
2174 return (jobjectArray)JNIHandles::make_local(env, result);
The comment there is no longer accurate anyway.
Done.
---
src/hotspot/share/prims/jvmtiRedefineClasses.cpp
857 static jvmtiError
check_permitted_subclasses_attribute(InstanceKlass* the_class,
858 InstanceKlass* scratch_class) {
Please align.
Done.
---
src/hotspot/share/prims/jvmtiRedefineClasses.c
Re: RFR: JDK-8225056 VM support for sealed classes
Thanks Lois!
I'll add the two ResourceMarks before the changes get pushed.
Harold
On 5/22/2020 11:07 AM, Lois Foltan wrote:
On 5/21/2020 2:33 PM, Harold Seigel wrote:
Hi David,
Thanks for looking at this! Please review this new webrev:
http://cr.openjdk.java.net/~hseigel/webrev.01/webrev/
Hi Harold,
I think this webrev looks good! A couple of minor comments:
- oops/instanceKlass.cpp:
line #236, do you need a ResourceMark here? I noticed there is one
above at line #229 for the log_trace call that uses external_name().
- prims/jvmtiRedefineClasses.cpp:
line #878, I think you need a ResourceMark for this method as well
if you invoke the external_name for the log_trace calls and for
NEW_RESOURCE_ARRAY_RETURN_NULL()?
Tests look good.
Thanks,
Lois
This webrev contains the following significant changes:
1. The format/indentation issues in classFileParser.cpp were fixed.
2. Unneeded checks in InstanceKlass::has_as_permitted_subclass() were
removed and the TRAPS parameter was removed.
3. The changes to klassVtable.* and method.* were reverted. Those
changes were from when sealed classes were marked as final, and are
no longer valid.
4. Method getPermittedSubclasses() in Class.java was renamed to
permittedSubclasses() and its implementation was changed.
5. Variables and methods for 'asm' were renamed from
'permittedSubtypes' to 'permittedSubclasses'.
6. Classes for sealed classes tests were changed to start with capital
letters.
7. Changes to langtools tests were removed from this webrev. They are
covered by the javac webrev (JDK-8244556).
8. The CSR's for JVMTI, JDWP, and JDI are in progress.
Please also see comments inline.
On 5/19/2020 1:26 AM, David Holmes wrote:
Hi Harold,
Adding serviceability-dev for the serviceability related changes.
Nit: "VM support for sealed classes"
This RFR covers the VM, core-libs, serviceability and even some
langtools tests. AFAICS only the javac changes are not included here
so when and where will they be reviewed and under what bug id?
Ideally there will be a single JBS issue for "Implementation of JEP
360: Sealed types". It's okay to break up the RFRs across different
areas, but it should be done under one bug id.
The javac changes are being reviewed as bug JDK-8227406. We
understand the need to do the reviews under one bug.
Overall this looks good. I've looked at all files and mainly have
some style nits in various places. But there are some more
significant items below.
On 14/05/2020 7:09 am, Harold Seigel wrote:
Hi,
Please review this patch for JVM and Core-libs support for the JEP
360 Sealed Classes preview feature. Code changes include the
following:
* Processing of the new PermittedSubclasses attribute to enforce
that
a class or interface, whose super is a sealed class or interface,
must be listed in the super's PermittedSubclasses attribute.
* Disallow redefinition of a sealed class or interface if its
redefinition would change its PermittedSubclasses attribute.
* Support API's to determine if a class or interface is sealed
and, if
it's sealed, return a list of its permitted subclasses.
* asm support for the PermittedSubclasses attribute
I assume Remi is providing the upstream support in ASM? :) But also
see below ...
Open Webrev:
http://cr.openjdk.java.net/~vromero/8225056/webrev.00/index.html
make/data/jdwp/jdwp.spec
There needs to be a sub-task and associated CSR request for this
JDWP spec update. I couldn't see this covered anywhere.
---
src/hotspot/share/classfile/classFileParser.cpp
3215 u2
ClassFileParser::parse_classfile_permitted_subclasses_attribute(const
ClassFileStream* const cfs,
3216 const u1* const permitted_subclasses_attribute_start,
3217 TRAPS) {
Indention on L3216/17 needs fixing after the copy'n'edit.
3515 return _major_version == JVM_CLASSFILE_MAJOR_VERSION &&
3516 _minor_version == JAVA_PREVIEW_MINOR_VERSION &&
3517 Arguments::enable_preview();
Too much indentation on L3516/17
3790 // Check for PermittedSubclasses tag
That comment (copied from my nestmates code :) is in the wrong
place. It needs to be before
3788 if (tag == vmSymbols::tag_permitted_subclasses()) {
Minor nit: I would suggest checking
parsed_permitted_subclasses_attribute before checking ACC_FINAL.
3876 if (parsed_permitted_subclasses_attribute) {
3877 const u2 num_of_subclasses =
parse_classfile_permitted_subclasses_attribute(
3878 cfs,
3879 permitted_subclasses_attribute_start,
3880 CHECK);
Although it looks odd the preceding, similarly shaped, sections all
indent to the same absolute position. Can you make L3878/78/80 match
please.
3882 guarantee_property(
3883 permi
Re: RFR: JDK-8225056 VM support for sealed classes
Hi Mandy,
Thanks for the suggestions. They have been incorporated in the revised
webrev.
http://cr.openjdk.java.net/~hseigel/webrev.01/webrev/
Harold
On 5/20/2020 1:05 PM, Mandy Chung wrote:
Hi Vicente,
On 5/20/20 8:40 AM, Vicente Romero wrote:
Hi David,
src/java.base/share/classes/java/lang/Class.java
There needs to be a CSR request for these changes.
yes there is one already:
https://bugs.openjdk.java.net/browse/JDK-8244556
Adding to David's comment w.r.t. @throws IAE:
The Class::getXXX APIs returns `Class` or `Class[]` if the result is
about type(s). This new `getPermittedSubclasses` returns
`ClassDesc[]`. I wonder if this should be renamed to
`permittedSubclasses` to follow the convention of the new APIs added
to support descriptors e.g. `describeConstable`
Nit: {@linkplain Class} should be {@code Class}
Mandy
Re: RFR: JDK-8225056 VM support for sealed classes
Hi David,
Thanks for looking at this! Please review this new webrev:
http://cr.openjdk.java.net/~hseigel/webrev.01/webrev/
This webrev contains the following significant changes:
1. The format/indentation issues in classFileParser.cpp were fixed.
2. Unneeded checks in InstanceKlass::has_as_permitted_subclass() were
removed and the TRAPS parameter was removed.
3. The changes to klassVtable.* and method.* were reverted. Those
changes were from when sealed classes were marked as final, and are
no longer valid.
4. Method getPermittedSubclasses() in Class.java was renamed to
permittedSubclasses() and its implementation was changed.
5. Variables and methods for 'asm' were renamed from
'permittedSubtypes' to 'permittedSubclasses'.
6. Classes for sealed classes tests were changed to start with capital
letters.
7. Changes to langtools tests were removed from this webrev. They are
covered by the javac webrev (JDK-8244556).
8. The CSR's for JVMTI, JDWP, and JDI are in progress.
Please also see comments inline.
On 5/19/2020 1:26 AM, David Holmes wrote:
Hi Harold,
Adding serviceability-dev for the serviceability related changes.
Nit: "VM support for sealed classes"
This RFR covers the VM, core-libs, serviceability and even some
langtools tests. AFAICS only the javac changes are not included here
so when and where will they be reviewed and under what bug id? Ideally
there will be a single JBS issue for "Implementation of JEP 360:
Sealed types". It's okay to break up the RFRs across different areas,
but it should be done under one bug id.
The javac changes are being reviewed as bug JDK-8227406. We understand
the need to do the reviews under one bug.
Overall this looks good. I've looked at all files and mainly have some
style nits in various places. But there are some more significant
items below.
On 14/05/2020 7:09 am, Harold Seigel wrote:
Hi,
Please review this patch for JVM and Core-libs support for the JEP
360 Sealed Classes preview feature. Code changes include the following:
* Processing of the new PermittedSubclasses attribute to enforce that
a class or interface, whose super is a sealed class or interface,
must be listed in the super's PermittedSubclasses attribute.
* Disallow redefinition of a sealed class or interface if its
redefinition would change its PermittedSubclasses attribute.
* Support API's to determine if a class or interface is sealed and, if
it's sealed, return a list of its permitted subclasses.
* asm support for the PermittedSubclasses attribute
I assume Remi is providing the upstream support in ASM? :) But also
see below ...
Open Webrev:
http://cr.openjdk.java.net/~vromero/8225056/webrev.00/index.html
make/data/jdwp/jdwp.spec
There needs to be a sub-task and associated CSR request for this JDWP
spec update. I couldn't see this covered anywhere.
---
src/hotspot/share/classfile/classFileParser.cpp
3215 u2
ClassFileParser::parse_classfile_permitted_subclasses_attribute(const
ClassFileStream* const cfs,
3216 const u1* const permitted_subclasses_attribute_start,
3217 TRAPS) {
Indention on L3216/17 needs fixing after the copy'n'edit.
3515 return _major_version == JVM_CLASSFILE_MAJOR_VERSION &&
3516 _minor_version == JAVA_PREVIEW_MINOR_VERSION &&
3517 Arguments::enable_preview();
Too much indentation on L3516/17
3790 // Check for PermittedSubclasses tag
That comment (copied from my nestmates code :) is in the wrong place.
It needs to be before
3788 if (tag == vmSymbols::tag_permitted_subclasses()) {
Minor nit: I would suggest checking
parsed_permitted_subclasses_attribute before checking ACC_FINAL.
3876 if (parsed_permitted_subclasses_attribute) {
3877 const u2 num_of_subclasses =
parse_classfile_permitted_subclasses_attribute(
3878 cfs,
3879 permitted_subclasses_attribute_start,
3880 CHECK);
Although it looks odd the preceding, similarly shaped, sections all
indent to the same absolute position. Can you make L3878/78/80 match
please.
3882 guarantee_property(
3883 permitted_subclasses_attribute_length ==
3884 sizeof(num_of_subclasses) + sizeof(u2) *
num_of_subclasses,
3885 "Wrong PermittedSubclasses attribute length in class file
%s", CHECK);
Nits: please reformat as:
3882 guarantee_property(
3883 permitted_subclasses_attribute_length ==
sizeof(num_of_subclasses) + sizeof(u2) * num_of_subclasses,
3885 "Wrong PermittedSubclasses attribute length in class file
%s", CHECK);
It would also look slightly better if you shortened the name of the
num_of_subclasses variable.
All of the above classFileParser.cpp changes were done.
---
src/h
Re: RFR: JDK-8225056 VM support for sealed classes
Hi Remi, Thank you for reviewing the ASM changes. Harold On 5/19/2020 4:41 AM, Remi Forax wrote: - Mail original - De: "David Holmes" À: "Harold David Seigel" , "hotspot-runtime-dev" , "amber-dev" , "core-libs-dev" , "serviceability-dev" Envoyé: Mardi 19 Mai 2020 07:26:38 Objet: Re: RFR: JDK-8225056 VM support for sealed classes Hi Harold, Adding serviceability-dev for the serviceability related changes. Nit: "VM support for sealed classes" This RFR covers the VM, core-libs, serviceability and even some langtools tests. AFAICS only the javac changes are not included here so when and where will they be reviewed and under what bug id? Ideally there will be a single JBS issue for "Implementation of JEP 360: Sealed types". It's okay to break up the RFRs across different areas, but it should be done under one bug id. Overall this looks good. I've looked at all files and mainly have some style nits in various places. But there are some more significant items below. On 14/05/2020 7:09 am, Harold Seigel wrote: Hi, Please review this patch for JVM and Core-libs support for the JEP 360 Sealed Classes preview feature. Code changes include the following: * Processing of the new PermittedSubclasses attribute to enforce that a class or interface, whose super is a sealed class or interface, must be listed in the super's PermittedSubclasses attribute. * Disallow redefinition of a sealed class or interface if its redefinition would change its PermittedSubclasses attribute. * Support API's to determine if a class or interface is sealed and, if it's sealed, return a list of its permitted subclasses. * asm support for the PermittedSubclasses attribute I assume Remi is providing the upstream support in ASM? :) But also see below ... Currently the version of ASM used JDK already supports sealed classes but with the attribute named PermittedSubtypes instead of PermittedSubclasses. This patch renames the attribute which is the right thing to do. Then when JDK 15 will be released, we will release ASM 9 with full support of PermittedSubclasses, with the right method names, docs, etc, that will be then integrated in JDK 16. So with my ASM hat, the changes to the 3 ASM classes are good. Rémi
Re: RFR: JDK-8225056 VM support for sealed classes
That sounds good!
Thanks, Harold
On 5/19/2020 11:53 AM, serguei.spit...@oracle.com wrote:
Hi Harold,
The Serviceability part including the tests looks good to me.
I can file a JVMTI enhancement on the jvmtiRedefineClasses.cpp
refactoring if you are okay with it.
Thanks,
Serguei
On 5/18/20 22:26, David Holmes wrote:
Hi Harold,
Adding serviceability-dev for the serviceability related changes.
Nit: "VM support for sealed classes"
This RFR covers the VM, core-libs, serviceability and even some
langtools tests. AFAICS only the javac changes are not included here
so when and where will they be reviewed and under what bug id?
Ideally there will be a single JBS issue for "Implementation of JEP
360: Sealed types". It's okay to break up the RFRs across different
areas, but it should be done under one bug id.
Overall this looks good. I've looked at all files and mainly have
some style nits in various places. But there are some more
significant items below.
On 14/05/2020 7:09 am, Harold Seigel wrote:
Hi,
Please review this patch for JVM and Core-libs support for the JEP
360 Sealed Classes preview feature. Code changes include the
following:
* Processing of the new PermittedSubclasses attribute to enforce that
a class or interface, whose super is a sealed class or interface,
must be listed in the super's PermittedSubclasses attribute.
* Disallow redefinition of a sealed class or interface if its
redefinition would change its PermittedSubclasses attribute.
* Support API's to determine if a class or interface is sealed
and, if
it's sealed, return a list of its permitted subclasses.
* asm support for the PermittedSubclasses attribute
I assume Remi is providing the upstream support in ASM? :) But also
see below ...
Open Webrev:
http://cr.openjdk.java.net/~vromero/8225056/webrev.00/index.html
make/data/jdwp/jdwp.spec
There needs to be a sub-task and associated CSR request for this JDWP
spec update. I couldn't see this covered anywhere.
---
src/hotspot/share/classfile/classFileParser.cpp
3215 u2
ClassFileParser::parse_classfile_permitted_subclasses_attribute(const
ClassFileStream* const cfs,
3216 const u1* const permitted_subclasses_attribute_start,
3217 TRAPS) {
Indention on L3216/17 needs fixing after the copy'n'edit.
3515 return _major_version == JVM_CLASSFILE_MAJOR_VERSION &&
3516 _minor_version == JAVA_PREVIEW_MINOR_VERSION &&
3517 Arguments::enable_preview();
Too much indentation on L3516/17
3790 // Check for PermittedSubclasses tag
That comment (copied from my nestmates code :) is in the wrong place.
It needs to be before
3788 if (tag == vmSymbols::tag_permitted_subclasses()) {
Minor nit: I would suggest checking
parsed_permitted_subclasses_attribute before checking ACC_FINAL.
3876 if (parsed_permitted_subclasses_attribute) {
3877 const u2 num_of_subclasses =
parse_classfile_permitted_subclasses_attribute(
3878 cfs,
3879 permitted_subclasses_attribute_start,
3880 CHECK);
Although it looks odd the preceding, similarly shaped, sections all
indent to the same absolute position. Can you make L3878/78/80 match
please.
3882 guarantee_property(
3883 permitted_subclasses_attribute_length ==
3884 sizeof(num_of_subclasses) + sizeof(u2) *
num_of_subclasses,
3885 "Wrong PermittedSubclasses attribute length in class
file %s", CHECK);
Nits: please reformat as:
3882 guarantee_property(
3883 permitted_subclasses_attribute_length ==
sizeof(num_of_subclasses) + sizeof(u2) * num_of_subclasses,
3885 "Wrong PermittedSubclasses attribute length in class
file %s", CHECK);
It would also look slightly better if you shortened the name of the
num_of_subclasses variable.
---
src/hotspot/share/classfile/classFileParser.hpp
+ u2 parse_classfile_permitted_subclasses_attribute(const
ClassFileStream* const cfs,
+ const u1* const
permitted_subclasses_attribute_start,
+ TRAPS);
Please fix indentation after copy'n'edit.
---
src/hotspot/share/oops/instanceKlass.cpp
247 if (classloader1 != classloader2) {
I'm not clear what rule this is verifying. The same module check
follows this one. The rule is that the subclass must be accessible to
the superclass implying:
1. same named module (regardless of class access modifiers); or
2. (implicitly in un-named module) same package if subclass not
public; or
3. public subclass
Having the same classloader implies same package, but that alone
doesn't address 2 or 3. So this doesn't conform to proposed JVMS rules.
264 if (_constants->tag_at(cp_index).is_klass()) {
265 Klass* k2 = _c
Re: RFR: JDK-8225056 VM support for sealed classes
I think that's a good idea.
Thanks, Harold
On 5/19/2020 11:49 AM, serguei.spit...@oracle.com wrote:
Hi Harold and David,
Just one comment.
We could save on the CSR's for:
make/data/jdwp/jdwp.spec
|| src/jdk.jdi/share/classes/com/sun/jdi/VirtualMachine.java
||
src/java.instrument/share/classes/java/lang/instrument/Instrumentation.java
As we discussed with David, it could make sense to remove duplication
in the Serviceability class redefinition and retransformation specs.
I'd suggest something like this webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/redef-spec-dedup.1/
If the direction looks okay then I could file RFE+CSR (and post an RFR).
Thanks,
Serguei
On 5/18/20 22:26, David Holmes wrote:
Hi Harold,
Adding serviceability-dev for the serviceability related changes.
Nit: "VM support for sealed classes"
This RFR covers the VM, core-libs, serviceability and even some
langtools tests. AFAICS only the javac changes are not included here
so when and where will they be reviewed and under what bug id?
Ideally there will be a single JBS issue for "Implementation of JEP
360: Sealed types". It's okay to break up the RFRs across different
areas, but it should be done under one bug id.
Overall this looks good. I've looked at all files and mainly have
some style nits in various places. But there are some more
significant items below.
On 14/05/2020 7:09 am, Harold Seigel wrote:
Hi,
Please review this patch for JVM and Core-libs support for the JEP
360 Sealed Classes preview feature. Code changes include the
following:
* Processing of the new PermittedSubclasses attribute to enforce that
a class or interface, whose super is a sealed class or interface,
must be listed in the super's PermittedSubclasses attribute.
* Disallow redefinition of a sealed class or interface if its
redefinition would change its PermittedSubclasses attribute.
* Support API's to determine if a class or interface is sealed
and, if
it's sealed, return a list of its permitted subclasses.
* asm support for the PermittedSubclasses attribute
I assume Remi is providing the upstream support in ASM? :) But also
see below ...
Open Webrev:
http://cr.openjdk.java.net/~vromero/8225056/webrev.00/index.html
make/data/jdwp/jdwp.spec
There needs to be a sub-task and associated CSR request for this JDWP
spec update. I couldn't see this covered anywhere.
---
src/hotspot/share/classfile/classFileParser.cpp
3215 u2
ClassFileParser::parse_classfile_permitted_subclasses_attribute(const
ClassFileStream* const cfs,
3216 const u1* const permitted_subclasses_attribute_start,
3217 TRAPS) {
Indention on L3216/17 needs fixing after the copy'n'edit.
3515 return _major_version == JVM_CLASSFILE_MAJOR_VERSION &&
3516 _minor_version == JAVA_PREVIEW_MINOR_VERSION &&
3517 Arguments::enable_preview();
Too much indentation on L3516/17
3790 // Check for PermittedSubclasses tag
That comment (copied from my nestmates code :) is in the wrong place.
It needs to be before
3788 if (tag == vmSymbols::tag_permitted_subclasses()) {
Minor nit: I would suggest checking
parsed_permitted_subclasses_attribute before checking ACC_FINAL.
3876 if (parsed_permitted_subclasses_attribute) {
3877 const u2 num_of_subclasses =
parse_classfile_permitted_subclasses_attribute(
3878 cfs,
3879 permitted_subclasses_attribute_start,
3880 CHECK);
Although it looks odd the preceding, similarly shaped, sections all
indent to the same absolute position. Can you make L3878/78/80 match
please.
3882 guarantee_property(
3883 permitted_subclasses_attribute_length ==
3884 sizeof(num_of_subclasses) + sizeof(u2) *
num_of_subclasses,
3885 "Wrong PermittedSubclasses attribute length in class
file %s", CHECK);
Nits: please reformat as:
3882 guarantee_property(
3883 permitted_subclasses_attribute_length ==
sizeof(num_of_subclasses) + sizeof(u2) * num_of_subclasses,
3885 "Wrong PermittedSubclasses attribute length in class
file %s", CHECK);
It would also look slightly better if you shortened the name of the
num_of_subclasses variable.
---
src/hotspot/share/classfile/classFileParser.hpp
+ u2 parse_classfile_permitted_subclasses_attribute(const
ClassFileStream* const cfs,
+ const u1* const
permitted_subclasses_attribute_start,
+ TRAPS);
Please fix indentation after copy'n'edit.
---
src/hotspot/share/oops/instanceKlass.cpp
247 if (classloader1 != classloader2) {
I'm not clear what rule this is verifying. The same module check
follows this one. The rule is that the subclass must be accessible to
the
RFR: JDK-8225056 VM support for sealed classes
Hi, Please review this patch for JVM and Core-libs support for the JEP 360 Sealed Classes preview feature. Code changes include the following: * Processing of the new PermittedSubclasses attribute to enforce that a class or interface, whose super is a sealed class or interface, must be listed in the super's PermittedSubclasses attribute. * Disallow redefinition of a sealed class or interface if its redefinition would change its PermittedSubclasses attribute. * Support API's to determine if a class or interface is sealed and, if it's sealed, return a list of its permitted subclasses. * asm support for the PermittedSubclasses attribute Open Webrev: http://cr.openjdk.java.net/~vromero/8225056/webrev.00/index.html JBS bug: https://bugs.openjdk.java.net/browse/JDK-8225056 Java Language Spec changes: http://cr.openjdk.java.net/~gbierman/jep360/jep360-20200513/specs/sealed-classes-jls.html JVM Spec changes: http://cr.openjdk.java.net/~gbierman/jep360/jep360-20200513/specs/sealed-classes-jvms.html JEP 360: https://bugs.openjdk.java.net/browse/JDK-8227043 JVM CSR: https://bugs.openjdk.java.net/browse/JDK-8242578 Changes to javac and other language tools will be reviewed separately. Thanks, Harold
Re: RFR 8235359: Simplify method Class.getRecordComponents()
Thanks Mandy! Harold On 12/5/2019 1:59 PM, Mandy Chung wrote: Looks good. Mandy On 12/5/19 6:36 AM, Harold Seigel wrote: Hi, Please review this small change to simplify Class.getRecordComponents() by changing the return type of getRecordComponents0() to RecordComponent[]. Open Webrev: http://cr.openjdk.java.net/~hseigel/bug_8235359/webrev/index.html JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8235359 The fix was regression tested by running Mach5 tiers 1 and 2 tests and builds on Linux-x64, Solaris, Windows, and Mac OS X, by running Mach5 tiers 3-5 tests on Linux-x64, and JCK lang and VM tests on Linux-x64. Thanks, Harold
Re: RFR 8235359: Simplify method Class.getRecordComponents()
Thanks Vicente! Harold On 12/5/2019 11:44 AM, Vicente Romero wrote: looks good, Vicente On 12/5/19 9:36 AM, Harold Seigel wrote: Hi, Please review this small change to simplify Class.getRecordComponents() by changing the return type of getRecordComponents0() to RecordComponent[]. Open Webrev: http://cr.openjdk.java.net/~hseigel/bug_8235359/webrev/index.html JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8235359 The fix was regression tested by running Mach5 tiers 1 and 2 tests and builds on Linux-x64, Solaris, Windows, and Mac OS X, by running Mach5 tiers 3-5 tests on Linux-x64, and JCK lang and VM tests on Linux-x64. Thanks, Harold
Re: RFR 8235359: Simplify method Class.getRecordComponents()
Thanks Lois and Chris! Harold On 12/5/2019 9:56 AM, Chris Hegarty wrote: On 5 Dec 2019, at 14:36, Harold Seigel <mailto:harold.sei...@oracle.com>> wrote: Hi, Please review this small change to simplify Class.getRecordComponents() by changing the return type of getRecordComponents0() to RecordComponent[]. Open Webrev: http://cr.openjdk.java.net/~hseigel/bug_8235359/webrev/index.html LGTM. -Chris. P.S. Further changes in this area are being discussed on amber-spec-experts - https://mail.openjdk.java.net/pipermail/amber-spec-experts/2019-December/001840.html
Re: RFR 8235359: Simplify method Class.getRecordComponents()
Thanks Fred! Harold On 12/5/2019 9:50 AM, Frederic Parain wrote: Looks good to me. Fred On Dec 5, 2019, at 09:36, Harold Seigel wrote: Hi, Please review this small change to simplify Class.getRecordComponents() by changing the return type of getRecordComponents0() to RecordComponent[]. Open Webrev: http://cr.openjdk.java.net/~hseigel/bug_8235359/webrev/index.html JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8235359 The fix was regression tested by running Mach5 tiers 1 and 2 tests and builds on Linux-x64, Solaris, Windows, and Mac OS X, by running Mach5 tiers 3-5 tests on Linux-x64, and JCK lang and VM tests on Linux-x64. Thanks, Harold
RFR 8235359: Simplify method Class.getRecordComponents()
Hi, Please review this small change to simplify Class.getRecordComponents() by changing the return type of getRecordComponents0() to RecordComponent[]. Open Webrev: http://cr.openjdk.java.net/~hseigel/bug_8235359/webrev/index.html JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8235359 The fix was regression tested by running Mach5 tiers 1 and 2 tests and builds on Linux-x64, Solaris, Windows, and Mac OS X, by running Mach5 tiers 3-5 tests on Linux-x64, and JCK lang and VM tests on Linux-x64. Thanks, Harold
Re: RFR 8184289: Obsolete -XX:+UnsyncloadClass and -XX:+MustCallLoadClassInternal options
Hi Karen,
Thanks for looking at this!
I re-ran the ParallelClassLoading tests with no options, with
-XX:+AllowParallelDefineClass, and with -XX:+AlwaysLockClassLoader, and
all the tests passed. I also determined that the few Mach5 regression
test failures that I encountered were unrelated to my change.
Please see this latest webrev that adds 12 as an expiration date,
removes the 'guarantee' section, and fixes the comment at line 786 of
systemDictionary.cpp.
http://cr.openjdk.java.net/~hseigel/bug_8184289.3/webrev/index.html
Thanks, Harold
On 2/12/2018 2:39 PM, Karen Kinnear wrote:
Harold,
Thanks for doing this.
I think you told me that
1) the version change has made it in
2) you also put 12 as an expiration date
3) you are running the ParallelClassLoading tests with the remaining
two flags (you’ve already
run them without any flags):
AllowParallelDefineClass = true and AlwaysLockClassLoader=true
In terms of the guarantee in question
// For UnsyncloadClass only
848 // If they got a linkageError, check if a parallel class load
succeeded.
849 // If it did, then for bytecode resolution the specification
requires
850 // that we return the same result we did for the other thread, i.e.
the
851 // successfully loaded InstanceKlass
852 // Should not get here for classloaders that support parallelism
853 // with the new cleaner mechanism, even with
AllowParallelDefineClass
854 // Bootstrap goes through here to allow for an extra guarantee check
855 if (UnsyncloadClass || (class_loader.is_null())) {
856 if (k == NULL && HAS_PENDING_EXCEPTION
857 &&
PENDING_EXCEPTION->is_a(SystemDictionary::LinkageError_klass())) {
858 MutexLocker mu(SystemDictionary_lock, THREAD);
859 InstanceKlass* check = find_class(d_hash, name, dictionary);
860 if (check != NULL) {
861 // Klass is already loaded, so just use it
862 k = check;
863 CLEAR_PENDING_EXCEPTION;
864 guarantee((!class_loader.is_null()), "dup definition for bootstrap
loader?");
865 }
866 }
867 }
1) I agree you can remove the entire section
- the guarantee was there for future proofing in case we ever
allowed parallel class loading of the
same class for the null loader and to make sure I didn’t have
any logic holes.
- I would not put an assertion for the first half of the
condition - I would remove completely
- the code currently prevents parallel class loading of the same
class for the null loader at:
resolve_instance_class_or_null see line 785 …
while (!class_has_been_loaded && old probe && old
probe->instance_load_in_progress()) {
// case x: bootstrap class loader: prevent futile class loading,
// wait on first requestor
if (class_loader.is_null()) {
SystemDictionary_lock->wait();
This logic means that there is a registered INSTANCE_LOAD on this
placeholder entry.
Other minor comments (sorry if you already got these and I missed them
in earlier emails)
- all in SystemDictionary.cpp
1. line 72 comment “Five cases:” -> “Four cases:”
So you removed case 3 and renumbered, so old references to case 4 ->
case 3 ,and old references
to case 5 become case 4:
So - line 786, “Case 4” -> “case 3”
thanks,
Karen
On Feb 12, 2018, at 11:13 AM, harold seigel <mailto:harold.sei...@oracle.com>> wrote:
Hi Alan,
Thanks for looking at this.
Harold
On 2/12/2018 2:52 AM, Alan Bateman wrote:
On 12/02/2018 06:54, David Holmes wrote:
Hi Harold,
Adding core-libs-dev so they are aware of the ClassLoader change.
Thanks, that part is okay and good to see this going away.
-Alan
Re: RFR 8184289: Obsolete -XX:+UnsyncloadClass and -XX:+MustCallLoadClassInternal options
Hi Alan, Thanks for looking at this. Harold On 2/12/2018 2:52 AM, Alan Bateman wrote: On 12/02/2018 06:54, David Holmes wrote: Hi Harold, Adding core-libs-dev so they are aware of the ClassLoader change. Thanks, that part is okay and good to see this going away. -Alan
Re: RFR 8165634: Support multiple --add-module options on the command line
Thanks Lois, for the review. Harold On 9/8/2016 1:12 PM, Lois Foltan wrote: On 9/8/2016 9:23 AM, harold seigel wrote: Hi, Please review this fix for JDK-8165634. The fix changes the --add-modules option from being a 'last one wins' option to a cumulative one. With this change, if multiple --add-modules options are specified, the VM accumulates all the options' values, instead of ignoring all but the last option's value. The --add-modules values are reported back to the JDK as properties using the Arguments::create_numbered_property() function. JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8165634 Open webrevs: http://cr.openjdk.java.net/~hseigel/bug_8165634.hs/ http://cr.openjdk.java.net/~hseigel/bug_8165634.jdk/ The fix was tested with the JCK Lang and VM tests, the hotpot, and java/lang, java/util and other JTreg tests, the RBT tier2 tests, and the NSK non-colocated quick tests. The JDK changes were done by Mandy Chung (mchung). Hi Harold, Looks good, thanks for removing Arguments::append_to_addmods_property! One minor comment, shouldn't --patch-modules also be in the unsupported_options list for Arguments::check_unsupported_dumping_properties? Thanks, Lois Thanks, Harold
Re: RFR 8165634: Support multiple --add-module options on the command line
Hi Gerard, Thanks for the review! Please see comments inline. Harold On 9/8/2016 12:24 PM, Gerard Ziemski wrote: hi Harold, The changes look fine. I have a couple of questions though: #1 Would the "test/runtime/modules/ModuleOptionsTest.java” be more robust if we included a case with a non-error return value that takes more than one module? Ex: “java --add-modules=java.base --add-modules=jdk.internal.reflect=ALL-UNNAMED -version” The VM just converts each --add-modules option into a jdk.module.add.mods. property that it passes back to the JDK. The fact that the JDK throws an exception for the "--add-modules=i_dont_exist --add-modules=java.base" case means that the VM is successfully handling both --add-modules and returning both of their values to the JDK. So I don't think an additional test case is needed. Also, the JDK AddModsTest.java test has a multiple --add-modules test case that returns successfully. #2 Looking at the changes for jdk it appears we now also allow duplicate "--add-exports” and "--add-reads"? Does it also mean we allow duplicate "--add-modules”, Ex: “java --add-modules=java.base --add-modules=java.base -version”? Thanks for also reviewing the JDK changes. The core-libs reviewers asked that the support for duplicate --add-exports and --add-reads be removed from this webrev. (See next revision.) Duplicate --add-modules are allowed. cheers On Sep 8, 2016, at 8:23 AM, harold seigel wrote: Hi, Please review this fix for JDK-8165634. The fix changes the --add-modules option from being a 'last one wins' option to a cumulative one. With this change, if multiple --add-modules options are specified, the VM accumulates all the options' values, instead of ignoring all but the last option's value. The --add-modules values are reported back to the JDK as properties using the Arguments::create_numbered_property() function. JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8165634 Open webrevs: http://cr.openjdk.java.net/~hseigel/bug_8165634.hs/ http://cr.openjdk.java.net/~hseigel/bug_8165634.jdk/ The fix was tested with the JCK Lang and VM tests, the hotpot, and java/lang, java/util and other JTreg tests, the RBT tier2 tests, and the NSK non-colocated quick tests. The JDK changes were done by Mandy Chung (mchung). Thanks, Harold
RFR 8165634: Support multiple --add-module options on the command line
Hi, Please review this fix for JDK-8165634. The fix changes the --add-modules option from being a 'last one wins' option to a cumulative one. With this change, if multiple --add-modules options are specified, the VM accumulates all the options' values, instead of ignoring all but the last option's value. The --add-modules values are reported back to the JDK as properties using the Arguments::create_numbered_property() function. JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8165634 Open webrevs: http://cr.openjdk.java.net/~hseigel/bug_8165634.hs/ http://cr.openjdk.java.net/~hseigel/bug_8165634.jdk/ The fix was tested with the JCK Lang and VM tests, the hotpot, and java/lang, java/util and other JTreg tests, the RBT tier2 tests, and the NSK non-colocated quick tests. The JDK changes were done by Mandy Chung (mchung). Thanks, Harold
Re: RFR 8058575: IllegalAccessError trying to access package-private class from VM anonymous class
On 9/2/2016 4:17 PM, fo...@univ-mlv.fr wrote: - Mail original - De: "harold seigel" À: "Remi Forax" Cc: "Alan Bateman" , "Hotspot dev runtime" , core-libs-dev@openjdk.java.net Envoyé: Vendredi 2 Septembre 2016 20:32:55 Objet: Re: RFR 8058575: IllegalAccessError trying to access package-private class from VM anonymous class Hi Rémi, Thank you for looking at this change. Not allowing host classes to be array classes is not completely unrelated to this bug because it affects the implementation of the code that prepends the host class's package to the anonymous class. yes, right. but i've always believed that the name was more for debugging purpose, i.e. because a VM anonymous class name is not registered in a Classloader, so the VM will never find an anonymous class by it's name. Having a package name that matches its host class's ensures that the anonymous class is in the same module as its host class in cases where the VM determines a class's module from its Klass's package entry structure. Harold We decided to not allow array host classes in JDK-9 because it makes no sense. A user who does this is likely doing so in error, and should be flagged for it. yes, true. We recognize that this, and many other things, will have to change once array classes have their own methods. Thanks, Harold Thanks for the explanation, Rémi On 9/2/2016 11:25 AM, Remi Forax wrote: Harold, disallowing array classes as host classes seems unrelated and knowing that jdk 10 or 11 will certainly add default methods to arrays, we will want to have anonymous classes with arrays as host class in order to acts as bridges/mixins. regards, Rémi - Mail original - De: "harold seigel" À: "Alan Bateman" , "Hotspot dev runtime" , core-libs-dev@openjdk.java.net Envoyé: Vendredi 2 Septembre 2016 17:03:34 Objet: Re: RFR 8058575: IllegalAccessError trying to access package-private class from VM anonymous class Thanks Alan. I'll go ahead and make that change. Harold On 9/2/2016 10:43 AM, Alan Bateman wrote: On 02/09/2016 14:02, harold seigel wrote: Hi, Please review this new fix for JDK-8058575. This fix requires that a VM anonymous class be in either the same package as its host class or be in the unnamed package. If the anonymous class is in the unnamed package then this fix puts it into its host class's package, ensuring that the anonymous class and its host class are in the same module. This fix also throws an IllegalArgumentException if the host class is an array class. Additionally, the type of field ClassFileParser::_host_klass was changed to InstanceKlass* and some comments were cleaned up. JBS bug: https://bugs.openjdk.java.net/browse/JDK-8058575 Open webrevs: http://cr.openjdk.java.net/~hseigel/bug_8058575.jdk.3/ In GetModuleTest then one clean-up is to change it to use hostClass.getPackageName() and remove packageName(String). -Alan
Re: RFR 8058575: IllegalAccessError trying to access package-private class from VM anonymous class
Hi Rémi, Thank you for looking at this change. Not allowing host classes to be array classes is not completely unrelated to this bug because it affects the implementation of the code that prepends the host class's package to the anonymous class. We decided to not allow array host classes in JDK-9 because it makes no sense. A user who does this is likely doing so in error, and should be flagged for it. We recognize that this, and many other things, will have to change once array classes have their own methods. Thanks, Harold On 9/2/2016 11:25 AM, Remi Forax wrote: Harold, disallowing array classes as host classes seems unrelated and knowing that jdk 10 or 11 will certainly add default methods to arrays, we will want to have anonymous classes with arrays as host class in order to acts as bridges/mixins. regards, Rémi - Mail original - De: "harold seigel" À: "Alan Bateman" , "Hotspot dev runtime" , core-libs-dev@openjdk.java.net Envoyé: Vendredi 2 Septembre 2016 17:03:34 Objet: Re: RFR 8058575: IllegalAccessError trying to access package-private class from VM anonymous class Thanks Alan. I'll go ahead and make that change. Harold On 9/2/2016 10:43 AM, Alan Bateman wrote: On 02/09/2016 14:02, harold seigel wrote: Hi, Please review this new fix for JDK-8058575. This fix requires that a VM anonymous class be in either the same package as its host class or be in the unnamed package. If the anonymous class is in the unnamed package then this fix puts it into its host class's package, ensuring that the anonymous class and its host class are in the same module. This fix also throws an IllegalArgumentException if the host class is an array class. Additionally, the type of field ClassFileParser::_host_klass was changed to InstanceKlass* and some comments were cleaned up. JBS bug: https://bugs.openjdk.java.net/browse/JDK-8058575 Open webrevs: http://cr.openjdk.java.net/~hseigel/bug_8058575.jdk.3/ In GetModuleTest then one clean-up is to change it to use hostClass.getPackageName() and remove packageName(String). -Alan
Re: RFR 8058575: IllegalAccessError trying to access package-private class from VM anonymous class
Thanks Alan. I'll go ahead and make that change. Harold On 9/2/2016 10:43 AM, Alan Bateman wrote: On 02/09/2016 14:02, harold seigel wrote: Hi, Please review this new fix for JDK-8058575. This fix requires that a VM anonymous class be in either the same package as its host class or be in the unnamed package. If the anonymous class is in the unnamed package then this fix puts it into its host class's package, ensuring that the anonymous class and its host class are in the same module. This fix also throws an IllegalArgumentException if the host class is an array class. Additionally, the type of field ClassFileParser::_host_klass was changed to InstanceKlass* and some comments were cleaned up. JBS bug: https://bugs.openjdk.java.net/browse/JDK-8058575 Open webrevs: http://cr.openjdk.java.net/~hseigel/bug_8058575.jdk.3/ In GetModuleTest then one clean-up is to change it to use hostClass.getPackageName() and remove packageName(String). -Alan
RFR 8058575: IllegalAccessError trying to access package-private class from VM anonymous class
Hi, Please review this new fix for JDK-8058575. This fix requires that a VM anonymous class be in either the same package as its host class or be in the unnamed package. If the anonymous class is in the unnamed package then this fix puts it into its host class's package, ensuring that the anonymous class and its host class are in the same module. This fix also throws an IllegalArgumentException if the host class is an array class. Additionally, the type of field ClassFileParser::_host_klass was changed to InstanceKlass* and some comments were cleaned up. JBS bug: https://bugs.openjdk.java.net/browse/JDK-8058575 Open webrevs: http://cr.openjdk.java.net/~hseigel/bug_8058575.jdk.3/ http://cr.openjdk.java.net/~hseigel/bug_8058575.hs.3/ The fix was tested with the JCK API, Lang and VM tests, the hotpot, and java/lang, java/util and other JTreg tests, the RBT tier2 tests, and the NSK colocated tests and non-colocated quick tests. Thanks, Harold
Re: RFR(XS) 8148785: Update class file version to 53 for JDK-9
Hi Alan, Thanks for looking at the change. I'll drop the comment before checking it in. Harold On 2/3/2016 9:32 AM, Alan Bateman wrote: On 03/02/2016 14:12, harold seigel wrote: Hi Aleksey, Thanks for the review. The plan is to first change the runtime to accept version 53 files and then change the tools to generate them. Hopefully, this will reduce incompatibility problems. See JDK-8148651 <https://bugs.openjdk.java.net/browse/JDK-8148651> for details. Right, there are several things that need to happen and I think we need hotspot accepting 53.0 before javac starts to generate them. The webrevs look okay to me, except it might be better to drop "New Module attribute" from the comment in classfileParser.cpp until JSR 376 is further along and we line up the trucks to bring the module system into JDK 9. -Alan
