RFR: 8342704: GHA: Report truncation is broken after JDK-8341424
When GH output is too large, we do not actually truncate it after [JDK-8341424](https://bugs.openjdk.org/browse/JDK-8341424). The error log would be: Run bash ./.github/scripts/gen-test-results.sh "$GITHUB_STEP_SUMMARY" ./.github/scripts/gen-test-results.sh: line 28: report-utils.sh: No such file or directory ./.github/scripts/gen-test-results.sh: line 97: truncate_summary: command not found Error: Process completed with exit code 127. Error: $GITHUB_STEP_SUMMARY upload aborted, supports content up to a size of 1024k, got 1579k. For more information see: https://docs.github.com/actions/using-workflows/workflow-commands-for-github-actions#adding-a-markdown-summary This is because we import report-utils.sh incorrectly: scripts try to find it in current directory, which is not the same directory where the importing scripts are located. And we never see it in normal cases, because scripts are just eating the errors without any other observable effects. Additional testing: - [x] GHA checks work with deliberately failing tests - Commit messages: - Revert test hook - Test hook - Fix Changes: https://git.openjdk.org/jdk/pull/21617/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=21617&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8342704 Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/21617.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/21617/head:pull/21617 PR: https://git.openjdk.org/jdk/pull/21617
Re: RFR: 8338411: Implement JEP 486: Permanently Disable the Security Manager [v2]
On Fri, 18 Oct 2024 19:03:30 GMT, Sean Mullan wrote: >> This is the implementation of JEP 486: Permanently Disable the Security >> Manager. See [JEP 486](https://openjdk.org/jeps/486) for more details. The >> [CSR](https://bugs.openjdk.org/browse/JDK-8338412) describes in detail the >> main changes in the JEP and also includes an apidiff of the specification >> changes. >> >> NOTE: the majority (~95%) of the changes in this PR are test updates >> (removal/modifications) and API specification changes, the latter mostly to >> remove `@throws SecurityException`. The remaining changes are primarily the >> removal of the `SecurityManager`, `Policy`, `AccessController` and other >> Security Manager API implementations. There is very little new code. >> >> The code changes can be broken down into roughly the following categories: >> >> 1. Degrading the behavior of Security Manager APIs to either throw >> Exceptions by default or provide an execution environment that disallows >> access to all resources by default. >> 2. Changing hundreds of methods and constructors to no longer throw a >> `SecurityException` if a Security Manager was enabled. They will operate as >> they did in JDK 23 with no Security Manager enabled. >> 3. Changing the `java` command to exit with a fatal error if a Security >> Manager is enabled. >> 4. Removing the hotspot native code for the privileged stack walk and the >> inherited access control context. The remaining hotspot code and tests >> related to the Security Manager will be removed immediately after >> integration - see [JDK-8341916](https://bugs.openjdk.org/browse/JDK-8341916). >> 5. Removing or modifying hundreds of tests. Many tests that tested Security >> Manager behavior are no longer relevant and thus have been removed or >> modified. >> >> There are a handful of Security Manager related tests that are failing and >> are at the end of the `test/jdk/ProblemList.txt`, >> `test/langtools/ProblemList.txt` and `test/hotspot/jtreg/ProblemList.txt` >> files - these will be removed or separate bugs will be filed before >> integrating this PR. >> >> Inside the JDK, we have retained calls to >> `SecurityManager::getSecurityManager` and `AccessController::doPrivileged` >> for now, as these methods have been degraded to behave the same as they did >> in JDK 23 with no Security Manager enabled. After we integrate this JEP, >> those calls will be removed in each area (client-libs, core-libs, security, >> etc). >> >> I don't expect each reviewer to review all the code changes in this JEP. >> Rather, I advise that you only focus on the changes for the area >> (client-libs, core-libs, net, ... > > Sean Mullan has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 97 commits: > > - Merge remote-tracking branch 'jdk-sandbox/jep486' into JDK-8338411 > - Change apiNote to deprecated annotation on checkAccess methods. Change > method dedescription to "Does nothing". > - Sanitize the class descriptions of DelegationPermission and > ServicePermission >by removing text that refers to granting permissions, but avoid changes > that >affect the API specification, such as the description and format of input >parameters. > - Restored methods in RMIConnection to throw SecurityExceptions again but >with adjusted text that avoids the word "permission". > - Add text to class description of MBeanServer stating that implementations >may throw SecurityException if authorization doesn't allow access to > resource. > - Restore text about needing permissions from the desktop environment in the >getPixelColor and createScreenCapture methods. > - Add api note to getClassContext to use StackWalker instead and >add DROP_METHOD_INFO option to StackWalker. > - Change checkAccess() methods to be no-ops, rather than throwing >SecurityException. > - Merge > - Merge > - ... and 87 more: https://git.openjdk.org/jdk/compare/f50bd0d9...f89d9d09 The security-related test updates seem good to me. I noticed a few minor issues and pushed three commits (https://github.com/openjdk/jdk-sandbox/commit/807eb6e363cd78f7051ab2512fbb9fe7f72a036c, https://github.com/openjdk/jdk-sandbox/commit/b4f68e36260cba4cb9e3f72e86674666ee04f15b, and https://github.com/openjdk/jdk-sandbox/commit/02b4bf177555eaf2fa732e918448af9ff1efa8bf). - PR Comment: https://git.openjdk.org/jdk/pull/21498#issuecomment-2426923546
Re: RFR: 8338411: Implement JEP 486: Permanently Disable the Security Manager [v2]
On Fri, 18 Oct 2024 19:03:30 GMT, Sean Mullan wrote: >> This is the implementation of JEP 486: Permanently Disable the Security >> Manager. See [JEP 486](https://openjdk.org/jeps/486) for more details. The >> [CSR](https://bugs.openjdk.org/browse/JDK-8338412) describes in detail the >> main changes in the JEP and also includes an apidiff of the specification >> changes. >> >> NOTE: the majority (~95%) of the changes in this PR are test updates >> (removal/modifications) and API specification changes, the latter mostly to >> remove `@throws SecurityException`. The remaining changes are primarily the >> removal of the `SecurityManager`, `Policy`, `AccessController` and other >> Security Manager API implementations. There is very little new code. >> >> The code changes can be broken down into roughly the following categories: >> >> 1. Degrading the behavior of Security Manager APIs to either throw >> Exceptions by default or provide an execution environment that disallows >> access to all resources by default. >> 2. Changing hundreds of methods and constructors to no longer throw a >> `SecurityException` if a Security Manager was enabled. They will operate as >> they did in JDK 23 with no Security Manager enabled. >> 3. Changing the `java` command to exit with a fatal error if a Security >> Manager is enabled. >> 4. Removing the hotspot native code for the privileged stack walk and the >> inherited access control context. The remaining hotspot code and tests >> related to the Security Manager will be removed immediately after >> integration - see [JDK-8341916](https://bugs.openjdk.org/browse/JDK-8341916). >> 5. Removing or modifying hundreds of tests. Many tests that tested Security >> Manager behavior are no longer relevant and thus have been removed or >> modified. >> >> There are a handful of Security Manager related tests that are failing and >> are at the end of the `test/jdk/ProblemList.txt`, >> `test/langtools/ProblemList.txt` and `test/hotspot/jtreg/ProblemList.txt` >> files - these will be removed or separate bugs will be filed before >> integrating this PR. >> >> Inside the JDK, we have retained calls to >> `SecurityManager::getSecurityManager` and `AccessController::doPrivileged` >> for now, as these methods have been degraded to behave the same as they did >> in JDK 23 with no Security Manager enabled. After we integrate this JEP, >> those calls will be removed in each area (client-libs, core-libs, security, >> etc). >> >> I don't expect each reviewer to review all the code changes in this JEP. >> Rather, I advise that you only focus on the changes for the area >> (client-libs, core-libs, net, ... > > Sean Mullan has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 97 commits: > > - Merge remote-tracking branch 'jdk-sandbox/jep486' into JDK-8338411 > - Change apiNote to deprecated annotation on checkAccess methods. Change > method dedescription to "Does nothing". > - Sanitize the class descriptions of DelegationPermission and > ServicePermission >by removing text that refers to granting permissions, but avoid changes > that >affect the API specification, such as the description and format of input >parameters. > - Restored methods in RMIConnection to throw SecurityExceptions again but >with adjusted text that avoids the word "permission". > - Add text to class description of MBeanServer stating that implementations >may throw SecurityException if authorization doesn't allow access to > resource. > - Restore text about needing permissions from the desktop environment in the >getPixelColor and createScreenCapture methods. > - Add api note to getClassContext to use StackWalker instead and >add DROP_METHOD_INFO option to StackWalker. > - Change checkAccess() methods to be no-ops, rather than throwing >SecurityException. > - Merge > - Merge > - ... and 87 more: https://git.openjdk.org/jdk/compare/f50bd0d9...f89d9d09 Reviewing for management, src and test changes look good. - Marked as reviewed by kevinw (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/21498#pullrequestreview-2382585178
Re: RFR: 8338411: Implement JEP 486: Permanently Disable the Security Manager [v2]
On Sat, 19 Oct 2024 07:54:07 GMT, Alan Bateman wrote:
> There are a couple of micro benchmarks in test/micro that fork with
> `jvmArgsPrepend={"-Djava.security.manager=allow"})`, they will need to be
> examined.
Fixed, will be in next drop. There are a couple of other micro tests that test
the performance of `AccessController.doPrivileged` and `getContext` which
probably don't make sense anymore, but I've left them for now to be looked at
later.
-
PR Comment: https://git.openjdk.org/jdk/pull/21498#issuecomment-2427301539
Re: RFR: 8305895: Implement JEP 450: Compact Object Headers (Experimental) [v46]
On Mon, 21 Oct 2024 18:00:14 GMT, Roman Kennke wrote: >>> I've managed to reproduce the ECoreIndexOf crash locally by running with >>> -XX:+UseSerialGC -XX:+UnlockExperimentalVMOptions >>> -XX:+UseCompactObjectHeaders. The crash happens on line 773 when reading >>> past the needle. >>> >>> ``` >>> │ 762 __ movq(index, needle_len); >>> │ >>> │ 763 __ andq(index, 0xf); // nLen % 16 >>> │ 764 __ movq(offset, 0x10); >>> │ 765 __ subq(offset, index); // 16 - (nLen % 16) >>> │ 766 __ movq(index, offset); >>> │ 767 __ shlq(offset, 1); // * 2 >>> │ 768 __ negq(index); // -(16 - (nLen % 16)) >>> │ >>> │ 769 __ xorq(wr_index, wr_index); >>> │ 770 >>> │ 771 __ bind(L_top); >>> │ 772 // load needle and expand >>> │ 773 __ vpmovzxbw(xmm0, Address(needle, index, Address::times_1), >>> Assembler::AVX_256bit); >>> ``` >>> >>> We're reading this address: >>> >>> ``` >>> (SEGV_MAPERR), si_addr: 0x0007cffe >>> ``` >>> >>> which is just before the start of the heap: >>> >>> ``` >>> Heap address: 0x0007d000, size: 768 MB, Compressed Oops mode: Zero >>> based, Oop shift amount: 3 >>> ``` >>> >>> When this crashed I had: >>> >>> ``` >>> needle: 0x0007d00c >>> needle_len = 0x12 >>> index = 0xfffe >>> ``` >>> >>> There has been previous fix to not read past the haystack: Fix header < 16 >>> bytes in indexOf intrinsic, by @sviswa7 >>> [f65ef5d](https://github.com/openjdk/jdk/commit/f65ef5dc325212155a50a2fc3a7f4aad18b8d9d0) >>> >>> maybe we need something similar for the needle. >> >> @sviswa7 @vpaprotsk could you have a look? If we can have a reasonable fix >> for this soon, we could ship it in this PR, otherwise I'd defer it to a >> follow-up issue and disable indexOf intrinsic when running with >> +UseCompactObjectHeaders. > >> @rkennke Could you post the full command you used please? And perhaps also >> the seed that gets printed.. having trouble getting it to fail.. >> >> So far I added a few options and perrmitations of: >> `./build/linux-x86_64-server-fastdebug/images/jdk/bin/java -Xcomp >> -XX:-TieredCompilation -XX:+UnlockDiagnosticVMOptions >> -XX:+EnableX86ECoreOpts -XX:+UseSerialGC -XX:+UnlockExperimentalVMOptions >> -XX:+UseCompactObjectHeaders >> test/jdk/java/lang/StringBuffer/ECoreIndexOf.java` and lo luck.. >> IndexOf.java test checks "all interesting" lengths of haystack and needle >> and can't get it to fail either. > > I could reproduce on 3rd try with a fastdebug build with: > > make test TEST=java/lang/StringBuffer/ECoreIndexOf.java > TEST_VM_OPTS="-XX:+UnlockExperimentalVMOptions -XX:+UseCompactObjectHeaders > -XX:+UseSerialGC" > > > It prints: > > Seed set to 636754923980405411 > > > It probably depends on GC operation: It would only fail when the array > happens to be the very first object in the heap. The relevant GC/heap configs > would be: > > InitialHeapSize = 805306368 > MaxHeapSize = 805306368 > MaxNewSize = 268435456 > > > So you should probably also add `-Xmx805306368 -Xms805306368 -Xmn268435456` Thanks @rkennke able to reproduce now.. Sandhya will have a patch soon and I will re-verify - PR Comment: https://git.openjdk.org/jdk/pull/20677#issuecomment-2427477113
Re: RFR: 8338411: Implement JEP 486: Permanently Disable the Security Manager [v2]
On Fri, 18 Oct 2024 19:03:30 GMT, Sean Mullan wrote:
>> This is the implementation of JEP 486: Permanently Disable the Security
>> Manager. See [JEP 486](https://openjdk.org/jeps/486) for more details. The
>> [CSR](https://bugs.openjdk.org/browse/JDK-8338412) describes in detail the
>> main changes in the JEP and also includes an apidiff of the specification
>> changes.
>>
>> NOTE: the majority (~95%) of the changes in this PR are test updates
>> (removal/modifications) and API specification changes, the latter mostly to
>> remove `@throws SecurityException`. The remaining changes are primarily the
>> removal of the `SecurityManager`, `Policy`, `AccessController` and other
>> Security Manager API implementations. There is very little new code.
>>
>> The code changes can be broken down into roughly the following categories:
>>
>> 1. Degrading the behavior of Security Manager APIs to either throw
>> Exceptions by default or provide an execution environment that disallows
>> access to all resources by default.
>> 2. Changing hundreds of methods and constructors to no longer throw a
>> `SecurityException` if a Security Manager was enabled. They will operate as
>> they did in JDK 23 with no Security Manager enabled.
>> 3. Changing the `java` command to exit with a fatal error if a Security
>> Manager is enabled.
>> 4. Removing the hotspot native code for the privileged stack walk and the
>> inherited access control context. The remaining hotspot code and tests
>> related to the Security Manager will be removed immediately after
>> integration - see [JDK-8341916](https://bugs.openjdk.org/browse/JDK-8341916).
>> 5. Removing or modifying hundreds of tests. Many tests that tested Security
>> Manager behavior are no longer relevant and thus have been removed or
>> modified.
>>
>> There are a handful of Security Manager related tests that are failing and
>> are at the end of the `test/jdk/ProblemList.txt`,
>> `test/langtools/ProblemList.txt` and `test/hotspot/jtreg/ProblemList.txt`
>> files - these will be removed or separate bugs will be filed before
>> integrating this PR.
>>
>> Inside the JDK, we have retained calls to
>> `SecurityManager::getSecurityManager` and `AccessController::doPrivileged`
>> for now, as these methods have been degraded to behave the same as they did
>> in JDK 23 with no Security Manager enabled. After we integrate this JEP,
>> those calls will be removed in each area (client-libs, core-libs, security,
>> etc).
>>
>> I don't expect each reviewer to review all the code changes in this JEP.
>> Rather, I advise that you only focus on the changes for the area
>> (client-libs, core-libs, net, ...
>
> Sean Mullan has updated the pull request with a new target base due to a
> merge or a rebase. The pull request now contains 97 commits:
>
> - Merge remote-tracking branch 'jdk-sandbox/jep486' into JDK-8338411
> - Change apiNote to deprecated annotation on checkAccess methods. Change
> method dedescription to "Does nothing".
> - Sanitize the class descriptions of DelegationPermission and
> ServicePermission
>by removing text that refers to granting permissions, but avoid changes
> that
>affect the API specification, such as the description and format of input
>parameters.
> - Restored methods in RMIConnection to throw SecurityExceptions again but
>with adjusted text that avoids the word "permission".
> - Add text to class description of MBeanServer stating that implementations
>may throw SecurityException if authorization doesn't allow access to
> resource.
> - Restore text about needing permissions from the desktop environment in the
>getPixelColor and createScreenCapture methods.
> - Add api note to getClassContext to use StackWalker instead and
>add DROP_METHOD_INFO option to StackWalker.
> - Change checkAccess() methods to be no-ops, rather than throwing
>SecurityException.
> - Merge
> - Merge
> - ... and 87 more: https://git.openjdk.org/jdk/compare/f50bd0d9...f89d9d09
test/jdk/javax/imageio/CachePremissionsTest/CachePermissionsTest.java line 55:
> 53: * @run main/othervm -Djava.security.manager=allow
> CachePermissionsTest false w.policy
> 54: * @run main/othervm -Djava.security.manager=allow
> CachePermissionsTest false rw.policy
> 55: * @run main/othervm -Djava.security.manager=allow
> CachePermissionsTest true rwd.policy
Looks to me like we should delete these 3 policy files.
Also this test isn't about Permissions anymore. So should be renamed to
CacheUsedTest
Then we can get rid of the misspelt directory. in a follow up.Probably do this
test/jdk/javax/imageio/CachePremissionsTest/CachePermissionsTest.java line 76:
> 74: System.out.println("java.io.tmpdir is " +
> System.getProperty("java.io.tmpdir"));
> 75:
> 76: if (args.length > 1) {
The isFileCacheExpected logic does not make sense. The test sets set to use the
cache but then reads whether to expect it based o
Re: RFR: 8305895: Implement JEP 450: Compact Object Headers (Experimental) [v46]
On Thu, 17 Oct 2024 10:57:24 GMT, Roman Kennke wrote: >> This is the main body of the JEP 450: Compact Object Headers (Experimental). >> >> It is also a follow-up to #20640, which now also includes (and supersedes) >> #20603 and #20605, plus the Tiny Class-Pointers parts that have been >> previously missing. >> >> Main changes: >> - Introduction of the (experimental) flag UseCompactObjectHeaders. All >> changes in this PR are protected by this flag. The purpose of the flag is to >> provide a fallback, in case that users unexpectedly observe problems with >> the new implementation. The intention is that this flag will remain >> experimental and opt-in for at least one release, then make it on-by-default >> and diagnostic (?), and eventually deprecate and obsolete it. However, there >> are a few unknowns in that plan, specifically, we may want to further >> improve compact headers to 4 bytes, we are planning to enhance the Klass* >> encoding to support virtually unlimited number of Klasses, at which point we >> could also obsolete UseCompressedClassPointers. >> - The compressed Klass* can now be stored in the mark-word of objects. In >> order to be able to do this, we are add some changes to GC forwarding (see >> below) to protect the relevant (upper 22) bits of the mark-word. Significant >> parts of this PR deal with loading the compressed Klass* from the mark-word. >> This PR also changes some code paths (mostly in GCs) to be more careful when >> accessing Klass* (or mark-word or size) to be able to fetch it from the >> forwardee in case the object is forwarded. >> - Self-forwarding in GCs (which is used to deal with promotion failure) now >> uses a bit to indicate 'self-forwarding'. This is needed to preserve the >> crucial Klass* bits in the header. This also allows to get rid of >> preserved-header machinery in SerialGC and G1 (Parallel GC abuses >> preserved-marks to also find all other relevant oops). >> - Full GC forwarding now uses an encoding similar to compressed-oops. We >> have 40 bits for that, and can encode up to 8TB of heap. When exceeding 8TB, >> we turn off UseCompressedClassPointers (except in ZGC, which doesn't use the >> GC forwarding at all). >> - Instances can now have their base-offset (the offset where the field >> layouter starts to place fields) at offset 8 (instead of 12 or 16). >> - Arrays will now store their length at offset 8. >> - CDS can now write and read archives with the compressed header. However, >> it is not possible to read an archive that has been written with an opposite >> setting of UseCompactObjectHeaders. Some build machinery is added so that >> _co... > > Roman Kennke has updated the pull request incrementally with one additional > commit since the last revision: > > Compact header riscv (#3) > > Implement compact headers on RISCV > - > > Co-authored-by: hamlin The following test crashes `java/lang/StringBuffer/ECoreIndexOf.java#id1` when running with -XX:+UseCompactObjectHeaders. - PR Comment: https://git.openjdk.org/jdk/pull/20677#issuecomment-2425878646
Re: RFR: 8342578: GHA: RISC-V: Bootstrap using Debian snapshot is still failing [v2]
On Sat, 19 Oct 2024 11:57:17 GMT, Fei Yang wrote: >> In JDK-8339548, we switched to use Debian snapshot >> (https://snapshot.debian.org/archive/debian/20240228T034848Z/) for >> bootstrap. The reason is that we don't have a stable Debian release for >> RISC-V yet and Debian "sid" (https://httpredir.debian.org/debian) that we >> use for debootstrapping RISC-V breaks at that time. This works as expected >> for about one month. But bad news is that GHA linux-cross-build job for >> RISC-V starts to fail again this week. Sigh! I guess there might be some >> change on the distro running on GHA test machines as same debootstrap >> command still works on my x64 machine running Ubuntu 22.04.5. >> >> Good news is that that Debian "sid" can now bootstrap for RISC-V. So this >> simply switches back to Debian "sid". As the version >> of dpkg command on GHA machines is old (1.21.1), we need to add one extra >> option `--no-merged-usr` for bootstrap command to work (More discussion >> here: https://bugs.launchpad.net/ubuntu/+source/base-files/+bug/2054925). >> This option seems not necessary for newer dpkg versions like 1.22.6. >> >> Testing: >> - [x] GHA test > > Fei Yang has updated the pull request incrementally with one additional > commit since the last revision: > > Comment Marked as reviewed by shade (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/21575#pullrequestreview-2381384363
Integrated: 8342578: GHA: RISC-V: Bootstrap using Debian snapshot is still failing
On Fri, 18 Oct 2024 01:33:18 GMT, Fei Yang wrote: > In JDK-8339548, we switched to use Debian snapshot > (https://snapshot.debian.org/archive/debian/20240228T034848Z/) for bootstrap. > The reason is that we don't have a stable Debian release for RISC-V yet and > Debian "sid" (https://httpredir.debian.org/debian) that we use for > debootstrapping RISC-V breaks at that time. This works as expected for about > one month. But bad news is that GHA linux-cross-build job for RISC-V starts > to fail again this week. Sigh! I guess there might be some change on the > distro running on GHA test machines as same debootstrap command still works > on my x64 machine running Ubuntu 22.04.5. > > Good news is that that Debian "sid" can now bootstrap for RISC-V. So this > simply switches back to Debian "sid". As the version > of dpkg command on GHA machines is old (1.21.1), we need to add one extra > option `--no-merged-usr` for bootstrap command to work (More discussion here: > https://bugs.launchpad.net/ubuntu/+source/base-files/+bug/2054925). This > option seems not necessary for newer dpkg versions like 1.22.6. > > Testing: > - [x] GHA test This pull request has now been integrated. Changeset: 239d84a8 Author:Fei Yang URL: https://git.openjdk.org/jdk/commit/239d84a82a1e6f4ebbd5c5abb320e39cfd5bc330 Stats: 5 lines in 1 file changed: 4 ins; 0 del; 1 mod 8342578: GHA: RISC-V: Bootstrap using Debian snapshot is still failing Reviewed-by: shade, erikj - PR: https://git.openjdk.org/jdk/pull/21575
Re: RFR: 8342578: GHA: RISC-V: Bootstrap using Debian snapshot is still failing [v2]
On Sat, 19 Oct 2024 11:57:17 GMT, Fei Yang wrote: >> In JDK-8339548, we switched to use Debian snapshot >> (https://snapshot.debian.org/archive/debian/20240228T034848Z/) for >> bootstrap. The reason is that we don't have a stable Debian release for >> RISC-V yet and Debian "sid" (https://httpredir.debian.org/debian) that we >> use for debootstrapping RISC-V breaks at that time. This works as expected >> for about one month. But bad news is that GHA linux-cross-build job for >> RISC-V starts to fail again this week. Sigh! I guess there might be some >> change on the distro running on GHA test machines as same debootstrap >> command still works on my x64 machine running Ubuntu 22.04.5. >> >> Good news is that that Debian "sid" can now bootstrap for RISC-V. So this >> simply switches back to Debian "sid". As the version >> of dpkg command on GHA machines is old (1.21.1), we need to add one extra >> option `--no-merged-usr` for bootstrap command to work (More discussion >> here: https://bugs.launchpad.net/ubuntu/+source/base-files/+bug/2054925). >> This option seems not necessary for newer dpkg versions like 1.22.6. >> >> Testing: >> - [x] GHA test > > Fei Yang has updated the pull request incrementally with one additional > commit since the last revision: > > Comment Thanks all for the review. Let's get this integrated so that GHA linux-cross-compile job works for riscv. - PR Comment: https://git.openjdk.org/jdk/pull/21575#issuecomment-2426121843
Re: RFR: 8338411: Implement JEP 486: Permanently Disable the Security Manager [v2]
On Fri, 18 Oct 2024 19:03:30 GMT, Sean Mullan wrote: >> This is the implementation of JEP 486: Permanently Disable the Security >> Manager. See [JEP 486](https://openjdk.org/jeps/486) for more details. The >> [CSR](https://bugs.openjdk.org/browse/JDK-8338412) describes in detail the >> main changes in the JEP and also includes an apidiff of the specification >> changes. >> >> NOTE: the majority (~95%) of the changes in this PR are test updates >> (removal/modifications) and API specification changes, the latter mostly to >> remove `@throws SecurityException`. The remaining changes are primarily the >> removal of the `SecurityManager`, `Policy`, `AccessController` and other >> Security Manager API implementations. There is very little new code. >> >> The code changes can be broken down into roughly the following categories: >> >> 1. Degrading the behavior of Security Manager APIs to either throw >> Exceptions by default or provide an execution environment that disallows >> access to all resources by default. >> 2. Changing hundreds of methods and constructors to no longer throw a >> `SecurityException` if a Security Manager was enabled. They will operate as >> they did in JDK 23 with no Security Manager enabled. >> 3. Changing the `java` command to exit with a fatal error if a Security >> Manager is enabled. >> 4. Removing the hotspot native code for the privileged stack walk and the >> inherited access control context. The remaining hotspot code and tests >> related to the Security Manager will be removed immediately after >> integration - see [JDK-8341916](https://bugs.openjdk.org/browse/JDK-8341916). >> 5. Removing or modifying hundreds of tests. Many tests that tested Security >> Manager behavior are no longer relevant and thus have been removed or >> modified. >> >> There are a handful of Security Manager related tests that are failing and >> are at the end of the `test/jdk/ProblemList.txt`, >> `test/langtools/ProblemList.txt` and `test/hotspot/jtreg/ProblemList.txt` >> files - these will be removed or separate bugs will be filed before >> integrating this PR. >> >> Inside the JDK, we have retained calls to >> `SecurityManager::getSecurityManager` and `AccessController::doPrivileged` >> for now, as these methods have been degraded to behave the same as they did >> in JDK 23 with no Security Manager enabled. After we integrate this JEP, >> those calls will be removed in each area (client-libs, core-libs, security, >> etc). >> >> I don't expect each reviewer to review all the code changes in this JEP. >> Rather, I advise that you only focus on the changes for the area >> (client-libs, core-libs, net, ... > > Sean Mullan has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 97 commits: > > - Merge remote-tracking branch 'jdk-sandbox/jep486' into JDK-8338411 > - Change apiNote to deprecated annotation on checkAccess methods. Change > method dedescription to "Does nothing". > - Sanitize the class descriptions of DelegationPermission and > ServicePermission >by removing text that refers to granting permissions, but avoid changes > that >affect the API specification, such as the description and format of input >parameters. > - Restored methods in RMIConnection to throw SecurityExceptions again but >with adjusted text that avoids the word "permission". > - Add text to class description of MBeanServer stating that implementations >may throw SecurityException if authorization doesn't allow access to > resource. > - Restore text about needing permissions from the desktop environment in the >getPixelColor and createScreenCapture methods. > - Add api note to getClassContext to use StackWalker instead and >add DROP_METHOD_INFO option to StackWalker. > - Change checkAccess() methods to be no-ops, rather than throwing >SecurityException. > - Merge > - Merge > - ... and 87 more: https://git.openjdk.org/jdk/compare/f50bd0d9...f89d9d09 Thanks for the changes in JMX and java.logging. Looks good to me. - Marked as reviewed by dfuchs (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/21498#pullrequestreview-2382119997
Re: RFR: 8338411: Implement JEP 486: Permanently Disable the Security Manager [v2]
On Fri, 18 Oct 2024 19:52:35 GMT, Sean Mullan wrote: >> I assume for the second one above you mean >> `javax.security.auth.kerberos.ServicePermission`. These classes still have a >> lot of words like "grant" and "trust". I will make some changes to the >> class descriptions of those classes, please review them in the next update. > > See the changes I made in > https://github.com/openjdk/jdk/pull/21498/commits/9dd59a12e984c347a34a25e6fd820340b1e12505. > Sometimes it is difficult to remove all text about granting the permission, > which is why we added the API note in all Permission subclasses stating that > the permission can no longer be used to protect resources. I see. I was wondering why some files are modified and some are not. The new text looks fine to me. Thanks. - PR Review Comment: https://git.openjdk.org/jdk/pull/21498#discussion_r1808841888
Integrated: 8340818: Add a new jtreg test root to test the generated documentation
On Mon, 30 Sep 2024 18:53:57 GMT, Nizar Benalla wrote: > Please review this change that adds a new test root `docs` dedicated to > testing the documentation, which has been a work in progress for a while. > Tests for links, encoding, HTML, accessibility will be later added in > following PRs. > > We also define a new make target `test-docs` meant for local use and depends > on the docs. > This also adds the necessary configurations needed at Oracle. > > This patch includes a test `TestDocs` which serves to show developers how > they are meant to resolve the docs to test them, I want to include it > temporarily until better tests are added later. > > TIA This pull request has now been integrated. Changeset: 07f550b8 Author:Nizar Benalla URL: https://git.openjdk.org/jdk/commit/07f550b85a3910edd28d8761e2adfb8d6a1352f6 Stats: 254 lines in 11 files changed: 234 ins; 3 del; 17 mod 8340818: Add a new jtreg test root to test the generated documentation Reviewed-by: erikj - PR: https://git.openjdk.org/jdk/pull/21272
Re: RFR: 8340818: Add a new jtreg test root to test the generated documentation [v12]
On Fri, 18 Oct 2024 12:51:46 GMT, Nizar Benalla wrote: >> Please review this change that adds a new test root `docs` dedicated to >> testing the documentation, which has been a work in progress for a while. >> Tests for links, encoding, HTML, accessibility will be later added in >> following PRs. >> >> We also define a new make target `test-docs` meant for local use and depends >> on the docs. >> This also adds the necessary configurations needed at Oracle. >> >> This patch includes a test `TestDocs` which serves to show developers how >> they are meant to resolve the docs to test them, I want to include it >> temporarily until better tests are added later. >> >> TIA > > Nizar Benalla has updated the pull request incrementally with one additional > commit since the last revision: > > respect 80 line char limit I will need to file followup RFEs for things that were discussed during code review - PR Comment: https://git.openjdk.org/jdk/pull/21272#issuecomment-2426262837
Re: RFR: 8305895: Implement JEP 450: Compact Object Headers (Experimental) [v44]
On Wed, 16 Oct 2024 15:37:59 GMT, Coleen Phillimore wrote: >> Roman Kennke has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Problem-list SharedBaseAddress tests on aarch64 > > src/hotspot/share/oops/compressedKlass.cpp line 185: > >> 183: #endif >> 184: >> 185: DEBUG_ONLY(sanity_check_after_initialization();) > > This is here twice. sanity_check_after_initialization is called from both initialization routines, but we only call either one or the other. - PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1808735324
Re: RFR: 8342662: C2: Add new phase for backend-specific lowering
On Mon, 21 Oct 2024 04:11:03 GMT, Jasmine Karthikeyan
wrote:
> Hi all,
> This patch adds a new pass to consolidate lowering of complex
> backend-specific code patterns, such as `MacroLogicV` and the optimization
> proposed by #21244. Moving these optimizations to backend code can simplify
> shared code, while also making it easier to develop more in-depth
> optimizations. The linked bug has an example of a new optimization this could
> enable. The new phase does GVN to de-duplicate nodes and calls nodes'
> `Value()` method, but it does not call `Identity()` or `Ideal()` to avoid
> undoing any changes done during lowering. It also reuses the IGVN worklist to
> avoid needing to re-create the notification mechanism.
>
> In this PR only the skeleton code for the pass is added, moving `MacroLogicV`
> to this system will be done separately in a future patch. Tier 1 tests pass
> on my linux x64 machine. Feedback on this patch would be greatly appreciated!
Build changes look good (but would be slightly better without the extra blank
line). I have not reviewed the actual hotspot changes.
make/hotspot/gensrc/GensrcAdlc.gmk line 60:
> 58:
> 59: ADLC_CFLAGS += -D$(HOTSPOT_TARGET_CPU_DEFINE)
> 60:
Maybe skip this blank line?
src/hotspot/cpu/aarch64/c2_lowering_aarch64.cpp line 31:
> 29: Node* PhaseLowering::lower_node(Node* in) {
> 30: return nullptr;
> 31: }
Note that this, and several other of the new files, are missing a trailing
newline on the last line (marked by the red circle/dash icon). I thought this
was checked by jcheck, but apparently not. It is still not recommended, though.
-
Marked as reviewed by ihse (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/21599#pullrequestreview-2381995734
PR Review Comment: https://git.openjdk.org/jdk/pull/21599#discussion_r1808752950
PR Review Comment: https://git.openjdk.org/jdk/pull/21599#discussion_r1808763526
Re: RFR: 8342646: JTREG_TEST_THREAD_FACTORY in testing.md should be TEST_THREAD_FACTORY
On Sun, 20 Oct 2024 02:11:11 GMT, SendaoYan wrote: > Hi all, > In > [make/RunTests.gmk](https://github.com/openjdk/jdk/blob/master/make/RunTests.gmk#L208), > the keyword is 'TEST_THREAD_FACTORY'. > > So the below test command will print error: > > make test TEST=test/jdk/java/math/BigInteger/TestValueExact.java > CONF=linux-x86_64-server-release JTREG="JTREG_TEST_THREAD_FACTORY=Virtual" > > > Building target 'test' in configuration 'linux-x86_64-server-release' > JTREG_TEST_THREAD_FACTORY=Virtual is not a valid keyword for JTREG. > Valid keywords: JOBS TIMEOUT_FACTOR FAILURE_HANDLER_TIMEOUT TEST_MODE ASSERT > VERBOSE RETAIN TEST_THREAD_FACTORY MAX_MEM RUN_PROBLEM_LISTS RETRY_COUNT > REPEAT_COUNT MAX_OUTPUT REPORT OPTIONS JAVA_OPTIONS VM_OPTIONS KEYWORDS > EXTRA_PROBLEM_LISTS LAUNCHER_OPTIONS. > RunTests.gmk:206: *** Cannot continue. Stop. > > > So I think we should fix the document bug in `doc/testing.md` to avoid take > the same mistake. Trivial fix, no risk. test/hotspot/jtreg/vmTestbase/nsk/jvmti/GetObjectMonitorUsage/objmonusage001.java line 66: > 64: // Virtual threads are not supported by GetObjectMonitorUsage. > 65: // Correct the expected values if the test is executed with > 66: // TEST_THREAD_FACTORY=Virtual. Perhaps these references should use the full `JTREG=TEST_THREAD_FACTORY=Virtual` to make it clearer. - PR Review Comment: https://git.openjdk.org/jdk/pull/21594#discussion_r1808734633
Re: RFR: 8305895: Implement JEP 450: Compact Object Headers (Experimental) [v46]
On Thu, 17 Oct 2024 10:57:24 GMT, Roman Kennke wrote: >> This is the main body of the JEP 450: Compact Object Headers (Experimental). >> >> It is also a follow-up to #20640, which now also includes (and supersedes) >> #20603 and #20605, plus the Tiny Class-Pointers parts that have been >> previously missing. >> >> Main changes: >> - Introduction of the (experimental) flag UseCompactObjectHeaders. All >> changes in this PR are protected by this flag. The purpose of the flag is to >> provide a fallback, in case that users unexpectedly observe problems with >> the new implementation. The intention is that this flag will remain >> experimental and opt-in for at least one release, then make it on-by-default >> and diagnostic (?), and eventually deprecate and obsolete it. However, there >> are a few unknowns in that plan, specifically, we may want to further >> improve compact headers to 4 bytes, we are planning to enhance the Klass* >> encoding to support virtually unlimited number of Klasses, at which point we >> could also obsolete UseCompressedClassPointers. >> - The compressed Klass* can now be stored in the mark-word of objects. In >> order to be able to do this, we are add some changes to GC forwarding (see >> below) to protect the relevant (upper 22) bits of the mark-word. Significant >> parts of this PR deal with loading the compressed Klass* from the mark-word. >> This PR also changes some code paths (mostly in GCs) to be more careful when >> accessing Klass* (or mark-word or size) to be able to fetch it from the >> forwardee in case the object is forwarded. >> - Self-forwarding in GCs (which is used to deal with promotion failure) now >> uses a bit to indicate 'self-forwarding'. This is needed to preserve the >> crucial Klass* bits in the header. This also allows to get rid of >> preserved-header machinery in SerialGC and G1 (Parallel GC abuses >> preserved-marks to also find all other relevant oops). >> - Full GC forwarding now uses an encoding similar to compressed-oops. We >> have 40 bits for that, and can encode up to 8TB of heap. When exceeding 8TB, >> we turn off UseCompressedClassPointers (except in ZGC, which doesn't use the >> GC forwarding at all). >> - Instances can now have their base-offset (the offset where the field >> layouter starts to place fields) at offset 8 (instead of 12 or 16). >> - Arrays will now store their length at offset 8. >> - CDS can now write and read archives with the compressed header. However, >> it is not possible to read an archive that has been written with an opposite >> setting of UseCompactObjectHeaders. Some build machinery is added so that >> _co... > > Roman Kennke has updated the pull request incrementally with one additional > commit since the last revision: > > Compact header riscv (#3) > > Implement compact headers on RISCV > - > > Co-authored-by: hamlin Marked as reviewed by ihse (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/20677#pullrequestreview-2382030332
Re: RFR: 8305895: Implement JEP 450: Compact Object Headers (Experimental) [v46]
On Thu, 17 Oct 2024 10:57:24 GMT, Roman Kennke wrote: >> This is the main body of the JEP 450: Compact Object Headers (Experimental). >> >> It is also a follow-up to #20640, which now also includes (and supersedes) >> #20603 and #20605, plus the Tiny Class-Pointers parts that have been >> previously missing. >> >> Main changes: >> - Introduction of the (experimental) flag UseCompactObjectHeaders. All >> changes in this PR are protected by this flag. The purpose of the flag is to >> provide a fallback, in case that users unexpectedly observe problems with >> the new implementation. The intention is that this flag will remain >> experimental and opt-in for at least one release, then make it on-by-default >> and diagnostic (?), and eventually deprecate and obsolete it. However, there >> are a few unknowns in that plan, specifically, we may want to further >> improve compact headers to 4 bytes, we are planning to enhance the Klass* >> encoding to support virtually unlimited number of Klasses, at which point we >> could also obsolete UseCompressedClassPointers. >> - The compressed Klass* can now be stored in the mark-word of objects. In >> order to be able to do this, we are add some changes to GC forwarding (see >> below) to protect the relevant (upper 22) bits of the mark-word. Significant >> parts of this PR deal with loading the compressed Klass* from the mark-word. >> This PR also changes some code paths (mostly in GCs) to be more careful when >> accessing Klass* (or mark-word or size) to be able to fetch it from the >> forwardee in case the object is forwarded. >> - Self-forwarding in GCs (which is used to deal with promotion failure) now >> uses a bit to indicate 'self-forwarding'. This is needed to preserve the >> crucial Klass* bits in the header. This also allows to get rid of >> preserved-header machinery in SerialGC and G1 (Parallel GC abuses >> preserved-marks to also find all other relevant oops). >> - Full GC forwarding now uses an encoding similar to compressed-oops. We >> have 40 bits for that, and can encode up to 8TB of heap. When exceeding 8TB, >> we turn off UseCompressedClassPointers (except in ZGC, which doesn't use the >> GC forwarding at all). >> - Instances can now have their base-offset (the offset where the field >> layouter starts to place fields) at offset 8 (instead of 12 or 16). >> - Arrays will now store their length at offset 8. >> - CDS can now write and read archives with the compressed header. However, >> it is not possible to read an archive that has been written with an opposite >> setting of UseCompactObjectHeaders. Some build machinery is added so that >> _co... > > Roman Kennke has updated the pull request incrementally with one additional > commit since the last revision: > > Compact header riscv (#3) > > Implement compact headers on RISCV > - > > Co-authored-by: hamlin I think a lot of copyright headers need to be updated. - PR Comment: https://git.openjdk.org/jdk/pull/20677#issuecomment-2426605597
Re: RFR: 8305895: Implement JEP 450: Compact Object Headers (Experimental) [v46]
On Thu, 17 Oct 2024 10:57:24 GMT, Roman Kennke wrote: >> This is the main body of the JEP 450: Compact Object Headers (Experimental). >> >> It is also a follow-up to #20640, which now also includes (and supersedes) >> #20603 and #20605, plus the Tiny Class-Pointers parts that have been >> previously missing. >> >> Main changes: >> - Introduction of the (experimental) flag UseCompactObjectHeaders. All >> changes in this PR are protected by this flag. The purpose of the flag is to >> provide a fallback, in case that users unexpectedly observe problems with >> the new implementation. The intention is that this flag will remain >> experimental and opt-in for at least one release, then make it on-by-default >> and diagnostic (?), and eventually deprecate and obsolete it. However, there >> are a few unknowns in that plan, specifically, we may want to further >> improve compact headers to 4 bytes, we are planning to enhance the Klass* >> encoding to support virtually unlimited number of Klasses, at which point we >> could also obsolete UseCompressedClassPointers. >> - The compressed Klass* can now be stored in the mark-word of objects. In >> order to be able to do this, we are add some changes to GC forwarding (see >> below) to protect the relevant (upper 22) bits of the mark-word. Significant >> parts of this PR deal with loading the compressed Klass* from the mark-word. >> This PR also changes some code paths (mostly in GCs) to be more careful when >> accessing Klass* (or mark-word or size) to be able to fetch it from the >> forwardee in case the object is forwarded. >> - Self-forwarding in GCs (which is used to deal with promotion failure) now >> uses a bit to indicate 'self-forwarding'. This is needed to preserve the >> crucial Klass* bits in the header. This also allows to get rid of >> preserved-header machinery in SerialGC and G1 (Parallel GC abuses >> preserved-marks to also find all other relevant oops). >> - Full GC forwarding now uses an encoding similar to compressed-oops. We >> have 40 bits for that, and can encode up to 8TB of heap. When exceeding 8TB, >> we turn off UseCompressedClassPointers (except in ZGC, which doesn't use the >> GC forwarding at all). >> - Instances can now have their base-offset (the offset where the field >> layouter starts to place fields) at offset 8 (instead of 12 or 16). >> - Arrays will now store their length at offset 8. >> - CDS can now write and read archives with the compressed header. However, >> it is not possible to read an archive that has been written with an opposite >> setting of UseCompactObjectHeaders. Some build machinery is added so that >> _co... > > Roman Kennke has updated the pull request incrementally with one additional > commit since the last revision: > > Compact header riscv (#3) > > Implement compact headers on RISCV > - > > Co-authored-by: hamlin I've managed to reproduce the ECoreIndexOf crash locally by running with -XX:+UseSerialGC -XX:+UnlockExperimentalVMOptions -XX:+UseCompactObjectHeaders. The crash happens on line 773 when reading past the needle. │ 762 __ movq(index, needle_len); │ │ 763 __ andq(index, 0xf); // nLen % 16 │ 764 __ movq(offset, 0x10); │ 765 __ subq(offset, index); // 16 - (nLen % 16) │ 766 __ movq(index, offset); │ 767 __ shlq(offset, 1); // * 2 │ 768 __ negq(index); // -(16 - (nLen % 16)) │ │ 769 __ xorq(wr_index, wr_index); │ 770 │ 771 __ bind(L_top); │ 772 // load needle and expand │ 773 __ vpmovzxbw(xmm0, Address(needle, index, Address::times_1), Assembler::AVX_256bit); We're reading this address: (SEGV_MAPERR), si_addr: 0x0007cffe which is just before the start of the heap: Heap address: 0x0007d000, size: 768 MB, Compressed Oops mode: Zero based, Oop shift amount: 3 When this crashed I had: needle: 0x0007d00c needle_len = 0x12 index = 0xfffe There has been previous fix to not read past the haystack: Fix header < 16 bytes in indexOf intrinsic, by @sviswa7 https://github.com/openjdk/jdk/pull/20677/commits/f65ef5dc325212155a50a2fc3a7f4aad18b8d9d0 maybe we need something similar for the needle. - PR Comment: https://git.openjdk.org/jdk/pull/20677#issuecomment-2426614072
Re: RFR: 8342646: JTREG_TEST_THREAD_FACTORY in testing.md should be TEST_THREAD_FACTORY
On Sun, 20 Oct 2024 02:11:11 GMT, SendaoYan wrote: > Hi all, > In > [make/RunTests.gmk](https://github.com/openjdk/jdk/blob/master/make/RunTests.gmk#L208), > the keyword is 'TEST_THREAD_FACTORY'. > > So the below test command will print error: > > make test TEST=test/jdk/java/math/BigInteger/TestValueExact.java > CONF=linux-x86_64-server-release JTREG="JTREG_TEST_THREAD_FACTORY=Virtual" > > > Building target 'test' in configuration 'linux-x86_64-server-release' > JTREG_TEST_THREAD_FACTORY=Virtual is not a valid keyword for JTREG. > Valid keywords: JOBS TIMEOUT_FACTOR FAILURE_HANDLER_TIMEOUT TEST_MODE ASSERT > VERBOSE RETAIN TEST_THREAD_FACTORY MAX_MEM RUN_PROBLEM_LISTS RETRY_COUNT > REPEAT_COUNT MAX_OUTPUT REPORT OPTIONS JAVA_OPTIONS VM_OPTIONS KEYWORDS > EXTRA_PROBLEM_LISTS LAUNCHER_OPTIONS. > RunTests.gmk:206: *** Cannot continue. Stop. > > > So I think we should fix the document bug in `doc/testing.md` to avoid take > the same mistake. Trivial fix, no risk. Marked as reviewed by ihse (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/21594#pullrequestreview-2382036363
Re: RFR: 8312425: [vectorapi] AArch64: Optimize vector math operations with SLEEF [v3]
On Wed, 16 Oct 2024 14:00:37 GMT, Hamlin Li wrote:
>> Hi,
>> Can you help to review the patch? Previously it's
>> https://github.com/openjdk/jdk/pull/18605.
>> This pr is based on https://github.com/openjdk/jdk/pull/20781.
>>
>> Thanks!
>>
>> ## Test
>> ### tests:
>> * test/jdk/jdk/incubator/vector/
>> * test/hotspot/jtreg/compiler/vectorapi/
>>
>> ### options:
>> * -XX:UseSVE=1 -XX:+EnableVectorSupport -XX:+UseVectorStubs
>> * -XX:UseSVE=0 -XX:+EnableVectorSupport -XX:+UseVectorStubs
>> * -XX:+EnableVectorSupport -XX:-UseVectorStubs
>>
>> ## Performance
>>
>> ### Tests
>> jmh tests are test/micro/org/openjdk/bench/jdk/incubator/vector/operation/
>> from vectorIntrinsics branch in panama-vector. It's good to have these tests
>> in jdk main stream, I will do it in a separate pr later. (These tests are
>> auto-generated tests from some script&template, it's good to also have those
>> scrip&template in jdk main stream, but those scrip&template generates more
>> other tests than what we need here, so better to add these tests and
>> script&template in another pr).
>>
>> ### Options
>> * +intrinsic:
>> 'FORK=1;ITER=10;WARMUP_ITER=10;JAVA_OPTIONS=-XX:+UnlockExperimentalVMOptions
>> -XX:+EnableVectorSupport -XX:+UseVectorStubs'
>> * -intrinsic:
>> 'FORK=1;ITER=10;WARMUP_ITER=10;JAVA_OPTIONS=-XX:+UnlockExperimentalVMOptions
>> -XX:+EnableVectorSupport -XX:-UseVectorStubs'
>>
>> ### Performance data
>> I have re-tested, there is no much difference from
>> https://github.com/openjdk/jdk/pull/18605, so please check performance data
>> in that pr.
>
> Hamlin Li has updated the pull request incrementally with one additional
> commit since the last revision:
>
> add missing files
src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp line 8207:
> 8205: for (int op = 0; op < VectorSupport::NUM_VECTOR_OP_MATH; op++) {
> 8206: int vop = VectorSupport::VECTOR_OP_MATH_START + op;
> 8207: if (vop == VectorSupport::VECTOR_OP_TANH) {
Could you please add a comment that mentions the reason, for example
`// Skip "tanh" because there is performance regression`
src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp line 8225:
> 8223: for (int op = 0; op < VectorSupport::NUM_VECTOR_OP_MATH; op++) {
> 8224: int vop = VectorSupport::VECTOR_OP_MATH_START + op;
> 8225: if (vop == VectorSupport::VECTOR_OP_TANH) {
Ditto
-
PR Review Comment: https://git.openjdk.org/jdk/pull/21502#discussion_r1808455401
PR Review Comment: https://git.openjdk.org/jdk/pull/21502#discussion_r1808470048
Re: RFR: 8339480: Build static-jdk image with a statically linked launcher [v7]
On Fri, 18 Oct 2024 18:23:13 GMT, Johan Vos wrote:
>> Magnus Ihse Bursie has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Don't hardcode server variant
>
> src/java.base/unix/native/libjli/java_md.c line 279:
>
>> 277:char jvmpath[], jint so_jvmpath,
>> 278:char jvmcfg[], jint so_jvmcfg) {
>> 279: /* Compute/set the name of the executable. This is needed for
>> macOS. */
>
> But this file is not used when on macOS, is it? (cfr java_md_macosx.m) )
Hm. I think both are used? I'll need to double-check that.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20837#discussion_r1808787882
Re: RFR: 8342646: JTREG_TEST_THREAD_FACTORY in testing.md should be TEST_THREAD_FACTORY [v2]
> Hi all, > In > [make/RunTests.gmk](https://github.com/openjdk/jdk/blob/master/make/RunTests.gmk#L208), > the keyword is 'TEST_THREAD_FACTORY'. > > So the below test command will print error: > > make test TEST=test/jdk/java/math/BigInteger/TestValueExact.java > CONF=linux-x86_64-server-release JTREG="JTREG_TEST_THREAD_FACTORY=Virtual" > > > Building target 'test' in configuration 'linux-x86_64-server-release' > JTREG_TEST_THREAD_FACTORY=Virtual is not a valid keyword for JTREG. > Valid keywords: JOBS TIMEOUT_FACTOR FAILURE_HANDLER_TIMEOUT TEST_MODE ASSERT > VERBOSE RETAIN TEST_THREAD_FACTORY MAX_MEM RUN_PROBLEM_LISTS RETRY_COUNT > REPEAT_COUNT MAX_OUTPUT REPORT OPTIONS JAVA_OPTIONS VM_OPTIONS KEYWORDS > EXTRA_PROBLEM_LISTS LAUNCHER_OPTIONS. > RunTests.gmk:206: *** Cannot continue. Stop. > > > So I think we should fix the document bug in `doc/testing.md` to avoid take > the same mistake. Trivial fix, no risk. SendaoYan has updated the pull request incrementally with two additional commits since the last revision: - JTREG="TEST_THREAD_FACTORY=Virtual" - Use JTREG="JTREG_TEST_THREAD_FACTORY=Virtual" to make comment more cleaner - Changes: - all: https://git.openjdk.org/jdk/pull/21594/files - new: https://git.openjdk.org/jdk/pull/21594/files/f1aab5dc..7c6a7bb4 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=21594&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=21594&range=00-01 Stats: 3 lines in 2 files changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/21594.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/21594/head:pull/21594 PR: https://git.openjdk.org/jdk/pull/21594
Re: RFR: 8342646: JTREG_TEST_THREAD_FACTORY in testing.md should be TEST_THREAD_FACTORY [v2]
On Mon, 21 Oct 2024 12:48:55 GMT, Erik Joelsson wrote: >> SendaoYan has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - JTREG="TEST_THREAD_FACTORY=Virtual" >> - Use JTREG="JTREG_TEST_THREAD_FACTORY=Virtual" to make comment more cleaner > > test/hotspot/jtreg/vmTestbase/nsk/jvmti/GetObjectMonitorUsage/objmonusage001.java > line 66: > >> 64: // Virtual threads are not supported by GetObjectMonitorUsage. >> 65: // Correct the expected values if the test is executed with >> 66: // TEST_THREAD_FACTORY=Virtual. > > Perhaps these references should use the full > `JTREG=TEST_THREAD_FACTORY=Virtual` to make it clearer. Thanks for the review, use `JTREG="TEST_THREAD_FACTORY=Virtual"` to make comment more clearer - PR Review Comment: https://git.openjdk.org/jdk/pull/21594#discussion_r1808787158
Re: RFR: 8339480: Build static-jdk image with a statically linked launcher [v7]
On Tue, 15 Oct 2024 20:22:52 GMT, Magnus Ihse Bursie wrote: >> As a prerequisite for Hermetic Java, we need a statically linked `java` >> launcher. It should behave like the normal, dynamically linked `java` >> launcher, except that all JDK native libraries should be statically, not >> dynamically, linked. >> >> This patch is the first step towards this goal. It will generate a >> `static-jdk` image with a statically linked launcher. This launcher is >> missing several native libs, however, and does therefore not behave like a >> proper dynamic java. One of the reasons for this is that local symbol hiding >> in static libraries are not implemented yet, which causes symbol clashes >> when linking all static libraries together. This will be addressed in an >> upcoming patch. >> >> All changes in the `src` directory are copied from, or inspired by, changes >> made in [the hermetic-java-runtime branch in Project >> Leyden](https://github.com/openjdk/leyden/tree/hermetic-java-runtime). > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Don't hardcode server variant When trying to sort out the LDFLAGS issues, it turned out that I could not run the linux launcher at all, not even when checking out older commits of this PR. I am almost at a loss here; I assume that this worked when I created this PR (even though I only did ad hoc testing at the time), and I'm not sure if my testing then was inadequate or if something else has changed during that time with my environment. I'm trying to retrace my steps in how I got to this branch, but I have unfortunately deleted much of the intermediate steps. @jianglizhou Can you check if you can build and run a simple HelloWorld.java with the static launcher in this PR on linux? `images/static-jdk/bin/java --version` works for me, but not `images/static-jdk/bin/java HelloWorld.java`, which fails with an error that indicates it cannot locate the jimage library. - PR Comment: https://git.openjdk.org/jdk/pull/20837#issuecomment-2426658524
Re: RFR: 8342646: JTREG_TEST_THREAD_FACTORY in testing.md should be TEST_THREAD_FACTORY [v2]
On Mon, 21 Oct 2024 13:12:55 GMT, SendaoYan wrote: >> Hi all, >> In >> [make/RunTests.gmk](https://github.com/openjdk/jdk/blob/master/make/RunTests.gmk#L208), >> the keyword is 'TEST_THREAD_FACTORY'. >> >> So the below test command will print error: >> >> make test TEST=test/jdk/java/math/BigInteger/TestValueExact.java >> CONF=linux-x86_64-server-release JTREG="JTREG_TEST_THREAD_FACTORY=Virtual" >> >> >> Building target 'test' in configuration 'linux-x86_64-server-release' >> JTREG_TEST_THREAD_FACTORY=Virtual is not a valid keyword for JTREG. >> Valid keywords: JOBS TIMEOUT_FACTOR FAILURE_HANDLER_TIMEOUT TEST_MODE ASSERT >> VERBOSE RETAIN TEST_THREAD_FACTORY MAX_MEM RUN_PROBLEM_LISTS RETRY_COUNT >> REPEAT_COUNT MAX_OUTPUT REPORT OPTIONS JAVA_OPTIONS VM_OPTIONS KEYWORDS >> EXTRA_PROBLEM_LISTS LAUNCHER_OPTIONS. >> RunTests.gmk:206: *** Cannot continue. Stop. >> >> >> So I think we should fix the document bug in `doc/testing.md` to avoid take >> the same mistake. Trivial fix, no risk. > > SendaoYan has updated the pull request incrementally with two additional > commits since the last revision: > > - JTREG="TEST_THREAD_FACTORY=Virtual" > - Use JTREG="JTREG_TEST_THREAD_FACTORY=Virtual" to make comment more cleaner Marked as reviewed by ihse (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/21594#pullrequestreview-2382087028
Re: RFR: 8305895: Implement JEP 450: Compact Object Headers (Experimental) [v23]
On Fri, 20 Sep 2024 17:46:21 GMT, Coleen Phillimore wrote: >> Roman Kennke has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - Merge remote-tracking branch 'lilliput/JEP-450-temporary-fix-branch-2' >> into JDK-8305895-v4 >> - review feedback > > src/hotspot/share/memory/metaspace.cpp line 799: > >> 797: >> 798: // Set up compressed class pointer encoding. >> 799: // In CDS=off mode, we give the JVM some leeway to choose a >> favorable base/shift combination. > > I don't know why this comment is here. Seems out of place. Its not, but maybe too vague. There are two ways to initialize CompressedKlassPointers : - `CompressedKlassPointers::initialize(address, size)` - called here - is used for no CDS case and allows the JVM to freely pick encoding base and shift. - `CompressedKlassPointers::initialize_for_given_encoding` is called when encoding base and shift are predetermined (when using CDS). Then, the JVM has no freedom at all, it just does sanity checks. The comment basically says "since here we are not using CDS, we are calling CompressedKlassPointers::initialize(address, size) to give the JVM some freedom when choosing encoding base and shift". Is this clearer? Should I just remove the code? - PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1808683312
Integrated: 8339570: Add Tidy build support for JDK tests
On Fri, 4 Oct 2024 00:17:14 GMT, Nizar Benalla wrote: > Can I get a review for this patch that adds the necessary changes for local > support of the `tidy` library. > > The dependency can be retrieved by running `make/devkit/createTidyBundle.sh` > on Linux and MacOs systems. > > This dependency is primarily going to be used to test the generated > documentation. > > This patch is meant to be integrated before #21272. > > Note: we need to be a very specific revision of `tidy` and cannot use any of > the available artifacts, as older versions do not recognize some HTML 5 > elements. > > TIA This pull request has now been integrated. Changeset: 5d5d88ab Author:Nizar Benalla URL: https://git.openjdk.org/jdk/commit/5d5d88ab9a862ab11bdd622aff07c688e6d96210 Stats: 132 lines in 6 files changed: 127 ins; 0 del; 5 mod 8339570: Add Tidy build support for JDK tests Co-authored-by: Magnus Ihse Bursie Reviewed-by: erikj, ihse - PR: https://git.openjdk.org/jdk/pull/21341
Re: RFR: 8339570: Add Tidy build support for JDK tests [v14]
On Fri, 18 Oct 2024 12:44:36 GMT, Nizar Benalla wrote: >> Can I get a review for this patch that adds the necessary changes for local >> support of the `tidy` library. >> >> The dependency can be retrieved by running `make/devkit/createTidyBundle.sh` >> on Linux and MacOs systems. >> >> This dependency is primarily going to be used to test the generated >> documentation. >> >> This patch is meant to be integrated before #21272. >> >> Note: we need to be a very specific revision of `tidy` and cannot use any of >> the available artifacts, as older versions do not recognize some HTML 5 >> elements. >> >> TIA > > Nizar Benalla has updated the pull request incrementally with one additional > commit since the last revision: > > lower case other usage of tidy_version Thank you all for the reviews and help getting this feature in. Here goes - PR Comment: https://git.openjdk.org/jdk/pull/21341#issuecomment-2426233921
Re: RFR: 8305895: Implement JEP 450: Compact Object Headers (Experimental) [v46]
On Thu, 17 Oct 2024 10:57:24 GMT, Roman Kennke wrote: >> This is the main body of the JEP 450: Compact Object Headers (Experimental). >> >> It is also a follow-up to #20640, which now also includes (and supersedes) >> #20603 and #20605, plus the Tiny Class-Pointers parts that have been >> previously missing. >> >> Main changes: >> - Introduction of the (experimental) flag UseCompactObjectHeaders. All >> changes in this PR are protected by this flag. The purpose of the flag is to >> provide a fallback, in case that users unexpectedly observe problems with >> the new implementation. The intention is that this flag will remain >> experimental and opt-in for at least one release, then make it on-by-default >> and diagnostic (?), and eventually deprecate and obsolete it. However, there >> are a few unknowns in that plan, specifically, we may want to further >> improve compact headers to 4 bytes, we are planning to enhance the Klass* >> encoding to support virtually unlimited number of Klasses, at which point we >> could also obsolete UseCompressedClassPointers. >> - The compressed Klass* can now be stored in the mark-word of objects. In >> order to be able to do this, we are add some changes to GC forwarding (see >> below) to protect the relevant (upper 22) bits of the mark-word. Significant >> parts of this PR deal with loading the compressed Klass* from the mark-word. >> This PR also changes some code paths (mostly in GCs) to be more careful when >> accessing Klass* (or mark-word or size) to be able to fetch it from the >> forwardee in case the object is forwarded. >> - Self-forwarding in GCs (which is used to deal with promotion failure) now >> uses a bit to indicate 'self-forwarding'. This is needed to preserve the >> crucial Klass* bits in the header. This also allows to get rid of >> preserved-header machinery in SerialGC and G1 (Parallel GC abuses >> preserved-marks to also find all other relevant oops). >> - Full GC forwarding now uses an encoding similar to compressed-oops. We >> have 40 bits for that, and can encode up to 8TB of heap. When exceeding 8TB, >> we turn off UseCompressedClassPointers (except in ZGC, which doesn't use the >> GC forwarding at all). >> - Instances can now have their base-offset (the offset where the field >> layouter starts to place fields) at offset 8 (instead of 12 or 16). >> - Arrays will now store their length at offset 8. >> - CDS can now write and read archives with the compressed header. However, >> it is not possible to read an archive that has been written with an opposite >> setting of UseCompactObjectHeaders. Some build machinery is added so that >> _co... > > Roman Kennke has updated the pull request incrementally with one additional > commit since the last revision: > > Compact header riscv (#3) > > Implement compact headers on RISCV > - > > Co-authored-by: hamlin > I've managed to reproduce the ECoreIndexOf crash locally by running with > -XX:+UseSerialGC -XX:+UnlockExperimentalVMOptions > -XX:+UseCompactObjectHeaders. The crash happens on line 773 when reading past > the needle. > > ``` > │ 762 __ movq(index, needle_len); > │ > │ 763 __ andq(index, 0xf); // nLen % 16 > │ 764 __ movq(offset, 0x10); > │ 765 __ subq(offset, index); // 16 - (nLen % 16) > │ 766 __ movq(index, offset); > │ 767 __ shlq(offset, 1); // * 2 > │ 768 __ negq(index); // -(16 - (nLen % 16)) > │ > │ 769 __ xorq(wr_index, wr_index); > │ 770 > │ 771 __ bind(L_top); > │ 772 // load needle and expand > │ 773 __ vpmovzxbw(xmm0, Address(needle, index, Address::times_1), > Assembler::AVX_256bit); > ``` > > We're reading this address: > > ``` > (SEGV_MAPERR), si_addr: 0x0007cffe > ``` > > which is just before the start of the heap: > > ``` > Heap address: 0x0007d000, size: 768 MB, Compressed Oops mode: Zero > based, Oop shift amount: 3 > ``` > > When this crashed I had: > > ``` > needle: 0x0007d00c > needle_len = 0x12 > index = 0xfffe > ``` > > There has been previous fix to not read past the haystack: Fix header < 16 > bytes in indexOf intrinsic, by @sviswa7 > [f65ef5d](https://github.com/openjdk/jdk/commit/f65ef5dc325212155a50a2fc3a7f4aad18b8d9d0) > > maybe we need something similar for the needle. @sviswa7 @vpaprotsk could you have a look? If we can have a reasonable fix for this soon, we could ship it in this PR, otherwise I'd defer it to a follow-up issue and disable indexOf intrinsic when running with +UseCompactObjectHeaders. - PR Comment: https://git.openjdk.org/jdk/pull/20677#issuecomment-2426754934
Re: RFR: 8305895: Implement JEP 450: Compact Object Headers (Experimental) [v46]
On Mon, 21 Oct 2024 13:53:58 GMT, Roman Kennke wrote: >> Roman Kennke has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Compact header riscv (#3) >> >> Implement compact headers on RISCV >> - >> >> Co-authored-by: hamlin > >> I've managed to reproduce the ECoreIndexOf crash locally by running with >> -XX:+UseSerialGC -XX:+UnlockExperimentalVMOptions >> -XX:+UseCompactObjectHeaders. The crash happens on line 773 when reading >> past the needle. >> >> ``` >> │ 762 __ movq(index, needle_len); >> │ >> │ 763 __ andq(index, 0xf); // nLen % 16 >> │ 764 __ movq(offset, 0x10); >> │ 765 __ subq(offset, index); // 16 - (nLen % 16) >> │ 766 __ movq(index, offset); >> │ 767 __ shlq(offset, 1); // * 2 >> │ 768 __ negq(index); // -(16 - (nLen % 16)) >> │ >> │ 769 __ xorq(wr_index, wr_index); >> │ 770 >> │ 771 __ bind(L_top); >> │ 772 // load needle and expand >> │ 773 __ vpmovzxbw(xmm0, Address(needle, index, Address::times_1), >> Assembler::AVX_256bit); >> ``` >> >> We're reading this address: >> >> ``` >> (SEGV_MAPERR), si_addr: 0x0007cffe >> ``` >> >> which is just before the start of the heap: >> >> ``` >> Heap address: 0x0007d000, size: 768 MB, Compressed Oops mode: Zero >> based, Oop shift amount: 3 >> ``` >> >> When this crashed I had: >> >> ``` >> needle: 0x0007d00c >> needle_len = 0x12 >> index = 0xfffe >> ``` >> >> There has been previous fix to not read past the haystack: Fix header < 16 >> bytes in indexOf intrinsic, by @sviswa7 >> [f65ef5d](https://github.com/openjdk/jdk/commit/f65ef5dc325212155a50a2fc3a7f4aad18b8d9d0) >> >> maybe we need something similar for the needle. > > @sviswa7 @vpaprotsk could you have a look? If we can have a reasonable fix > for this soon, we could ship it in this PR, otherwise I'd defer it to a > follow-up issue and disable indexOf intrinsic when running with > +UseCompactObjectHeaders. > @rkennke Could you post the full command you used please? And perhaps also > the seed that gets printed.. having trouble getting it to fail.. > > So far I added a few options and perrmitations of: > `./build/linux-x86_64-server-fastdebug/images/jdk/bin/java -Xcomp > -XX:-TieredCompilation -XX:+UnlockDiagnosticVMOptions -XX:+EnableX86ECoreOpts > -XX:+UseSerialGC -XX:+UnlockExperimentalVMOptions > -XX:+UseCompactObjectHeaders > test/jdk/java/lang/StringBuffer/ECoreIndexOf.java` and lo luck.. IndexOf.java > test checks "all interesting" lengths of haystack and needle and can't get it > to fail either. I could reproduce on 3rd try with a fastdebug build with: make test TEST=java/lang/StringBuffer/ECoreIndexOf.java TEST_VM_OPTS="-XX:+UnlockExperimentalVMOptions -XX:+UseCompactObjectHeaders -XX:+UseSerialGC" It prints: Seed set to 636754923980405411 It probably depends on GC operation: It would only fail when the array happens to be the very first object in the heap. The relevant GC/heap configs would be: InitialHeapSize = 805306368 MaxHeapSize = 805306368 MaxNewSize = 268435456 So you should probably also add `-Xmx805306368 -Xms805306368 -Xmn268435456` - PR Comment: https://git.openjdk.org/jdk/pull/20677#issuecomment-2427375886
Re: RFR: 8305895: Implement JEP 450: Compact Object Headers (Experimental) [v46]
On Mon, 21 Oct 2024 13:53:58 GMT, Roman Kennke wrote: >> Roman Kennke has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Compact header riscv (#3) >> >> Implement compact headers on RISCV >> - >> >> Co-authored-by: hamlin > >> I've managed to reproduce the ECoreIndexOf crash locally by running with >> -XX:+UseSerialGC -XX:+UnlockExperimentalVMOptions >> -XX:+UseCompactObjectHeaders. The crash happens on line 773 when reading >> past the needle. >> >> ``` >> │ 762 __ movq(index, needle_len); >> │ >> │ 763 __ andq(index, 0xf); // nLen % 16 >> │ 764 __ movq(offset, 0x10); >> │ 765 __ subq(offset, index); // 16 - (nLen % 16) >> │ 766 __ movq(index, offset); >> │ 767 __ shlq(offset, 1); // * 2 >> │ 768 __ negq(index); // -(16 - (nLen % 16)) >> │ >> │ 769 __ xorq(wr_index, wr_index); >> │ 770 >> │ 771 __ bind(L_top); >> │ 772 // load needle and expand >> │ 773 __ vpmovzxbw(xmm0, Address(needle, index, Address::times_1), >> Assembler::AVX_256bit); >> ``` >> >> We're reading this address: >> >> ``` >> (SEGV_MAPERR), si_addr: 0x0007cffe >> ``` >> >> which is just before the start of the heap: >> >> ``` >> Heap address: 0x0007d000, size: 768 MB, Compressed Oops mode: Zero >> based, Oop shift amount: 3 >> ``` >> >> When this crashed I had: >> >> ``` >> needle: 0x0007d00c >> needle_len = 0x12 >> index = 0xfffe >> ``` >> >> There has been previous fix to not read past the haystack: Fix header < 16 >> bytes in indexOf intrinsic, by @sviswa7 >> [f65ef5d](https://github.com/openjdk/jdk/commit/f65ef5dc325212155a50a2fc3a7f4aad18b8d9d0) >> >> maybe we need something similar for the needle. > > @sviswa7 @vpaprotsk could you have a look? If we can have a reasonable fix > for this soon, we could ship it in this PR, otherwise I'd defer it to a > follow-up issue and disable indexOf intrinsic when running with > +UseCompactObjectHeaders. @rkennke looking! - PR Comment: https://git.openjdk.org/jdk/pull/20677#issuecomment-2426828440
RFR: 8342682: Errors related to unused code on Windows after 8339120
After 8339120, gcc began catching many different instances of unused code in the Windows specific codebase. Some of these seem to be bugs. I've taken the effort to mark out all the relevant globals and locals that trigger the unused warnings and addressed all of them by commenting out the code as appropriate. I am confident that in many cases this simplistic approach of commenting out code does not fix the underlying issue, and the warning actually found a bug that should be fixed. In these instances, I will be aiming to fix these bugs with help from reviewers, so I recommend anyone reviewing who knows more about the code than I do to see whether there is indeed a bug that needs fixing in a different way than what I did. This will require reviews from core-libs, awt, security, accessibility, jpackage, and whatever team is responsible for HotSpot debugging. This fix requires C++17, so it will have to wait for the switch to C++17. I will be working towards that in the meantime and addressing the issues that come with changing the language standard elsewhere. This will only go in once the JDK actually starts using C++17 - Commit messages: - Compile as C++17 - 8342682 Changes: https://git.openjdk.org/jdk/pull/21616/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=21616&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8342682 Stats: 72 lines in 25 files changed: 25 ins; 1 del; 46 mod Patch: https://git.openjdk.org/jdk/pull/21616.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/21616/head:pull/21616 PR: https://git.openjdk.org/jdk/pull/21616
Re: RFR: 8342682: Errors related to unused code on Windows after 8339120
On Mon, 21 Oct 2024 14:34:30 GMT, Julian Waters wrote: > After 8339120, gcc began catching many different instances of unused code in > the Windows specific codebase. Some of these seem to be bugs. I've taken the > effort to mark out all the relevant globals and locals that trigger the > unused warnings and addressed all of them by commenting out the code as > appropriate. I am confident that in many cases this simplistic approach of > commenting out code does not fix the underlying issue, and the warning > actually found a bug that should be fixed. In these instances, I will be > aiming to fix these bugs with help from reviewers, so I recommend anyone > reviewing who knows more about the code than I do to see whether there is > indeed a bug that needs fixing in a different way than what I did. This will > require reviews from core-libs, awt, security, accessibility, jpackage, and > whatever team is responsible for HotSpot debugging. > > This fix requires C++17, so it will have to wait for the switch to C++17. I > will be working towards that in the meantime and addressing the issues that > come with changing the language standard elsewhere. This will only go in once > the JDK actually starts using C++17 src/java.desktop/share/native/common/awt/debug/debug_trace.h line 66: > 64: /* each file includes this flag indicating module trace status */ > 65: #ifdef __cplusplus > 66: [[maybe_unused]] I don't particularly like this one, but I haven't managed to find a cleaner solution. This is because a static global in a header is not great to begin with. It's practically begging for an unused-variable error to happen - PR Review Comment: https://git.openjdk.org/jdk/pull/21616#discussion_r1808952439
Re: RFR: 8338411: Implement JEP 486: Permanently Disable the Security Manager [v2]
On Fri, 18 Oct 2024 19:03:30 GMT, Sean Mullan wrote: >> This is the implementation of JEP 486: Permanently Disable the Security >> Manager. See [JEP 486](https://openjdk.org/jeps/486) for more details. The >> [CSR](https://bugs.openjdk.org/browse/JDK-8338412) describes in detail the >> main changes in the JEP and also includes an apidiff of the specification >> changes. >> >> NOTE: the majority (~95%) of the changes in this PR are test updates >> (removal/modifications) and API specification changes, the latter mostly to >> remove `@throws SecurityException`. The remaining changes are primarily the >> removal of the `SecurityManager`, `Policy`, `AccessController` and other >> Security Manager API implementations. There is very little new code. >> >> The code changes can be broken down into roughly the following categories: >> >> 1. Degrading the behavior of Security Manager APIs to either throw >> Exceptions by default or provide an execution environment that disallows >> access to all resources by default. >> 2. Changing hundreds of methods and constructors to no longer throw a >> `SecurityException` if a Security Manager was enabled. They will operate as >> they did in JDK 23 with no Security Manager enabled. >> 3. Changing the `java` command to exit with a fatal error if a Security >> Manager is enabled. >> 4. Removing the hotspot native code for the privileged stack walk and the >> inherited access control context. The remaining hotspot code and tests >> related to the Security Manager will be removed immediately after >> integration - see [JDK-8341916](https://bugs.openjdk.org/browse/JDK-8341916). >> 5. Removing or modifying hundreds of tests. Many tests that tested Security >> Manager behavior are no longer relevant and thus have been removed or >> modified. >> >> There are a handful of Security Manager related tests that are failing and >> are at the end of the `test/jdk/ProblemList.txt`, >> `test/langtools/ProblemList.txt` and `test/hotspot/jtreg/ProblemList.txt` >> files - these will be removed or separate bugs will be filed before >> integrating this PR. >> >> Inside the JDK, we have retained calls to >> `SecurityManager::getSecurityManager` and `AccessController::doPrivileged` >> for now, as these methods have been degraded to behave the same as they did >> in JDK 23 with no Security Manager enabled. After we integrate this JEP, >> those calls will be removed in each area (client-libs, core-libs, security, >> etc). >> >> I don't expect each reviewer to review all the code changes in this JEP. >> Rather, I advise that you only focus on the changes for the area >> (client-libs, core-libs, net, ... > > Sean Mullan has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 97 commits: > > - Merge remote-tracking branch 'jdk-sandbox/jep486' into JDK-8338411 > - Change apiNote to deprecated annotation on checkAccess methods. Change > method dedescription to "Does nothing". > - Sanitize the class descriptions of DelegationPermission and > ServicePermission >by removing text that refers to granting permissions, but avoid changes > that >affect the API specification, such as the description and format of input >parameters. > - Restored methods in RMIConnection to throw SecurityExceptions again but >with adjusted text that avoids the word "permission". > - Add text to class description of MBeanServer stating that implementations >may throw SecurityException if authorization doesn't allow access to > resource. > - Restore text about needing permissions from the desktop environment in the >getPixelColor and createScreenCapture methods. > - Add api note to getClassContext to use StackWalker instead and >add DROP_METHOD_INFO option to StackWalker. > - Change checkAccess() methods to be no-ops, rather than throwing >SecurityException. > - Merge > - Merge > - ... and 87 more: https://git.openjdk.org/jdk/compare/f50bd0d9...f89d9d09 For the record I have also reviewed the `java.naming` and `jdk.httpserver` source changes. They look good to me. - PR Comment: https://git.openjdk.org/jdk/pull/21498#issuecomment-2427040743
Re: RFR: 8305895: Implement JEP 450: Compact Object Headers (Experimental) [v46]
On Mon, 21 Oct 2024 13:53:58 GMT, Roman Kennke wrote: >> Roman Kennke has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Compact header riscv (#3) >> >> Implement compact headers on RISCV >> - >> >> Co-authored-by: hamlin > >> I've managed to reproduce the ECoreIndexOf crash locally by running with >> -XX:+UseSerialGC -XX:+UnlockExperimentalVMOptions >> -XX:+UseCompactObjectHeaders. The crash happens on line 773 when reading >> past the needle. >> >> ``` >> │ 762 __ movq(index, needle_len); >> │ >> │ 763 __ andq(index, 0xf); // nLen % 16 >> │ 764 __ movq(offset, 0x10); >> │ 765 __ subq(offset, index); // 16 - (nLen % 16) >> │ 766 __ movq(index, offset); >> │ 767 __ shlq(offset, 1); // * 2 >> │ 768 __ negq(index); // -(16 - (nLen % 16)) >> │ >> │ 769 __ xorq(wr_index, wr_index); >> │ 770 >> │ 771 __ bind(L_top); >> │ 772 // load needle and expand >> │ 773 __ vpmovzxbw(xmm0, Address(needle, index, Address::times_1), >> Assembler::AVX_256bit); >> ``` >> >> We're reading this address: >> >> ``` >> (SEGV_MAPERR), si_addr: 0x0007cffe >> ``` >> >> which is just before the start of the heap: >> >> ``` >> Heap address: 0x0007d000, size: 768 MB, Compressed Oops mode: Zero >> based, Oop shift amount: 3 >> ``` >> >> When this crashed I had: >> >> ``` >> needle: 0x0007d00c >> needle_len = 0x12 >> index = 0xfffe >> ``` >> >> There has been previous fix to not read past the haystack: Fix header < 16 >> bytes in indexOf intrinsic, by @sviswa7 >> [f65ef5d](https://github.com/openjdk/jdk/commit/f65ef5dc325212155a50a2fc3a7f4aad18b8d9d0) >> >> maybe we need something similar for the needle. > > @sviswa7 @vpaprotsk could you have a look? If we can have a reasonable fix > for this soon, we could ship it in this PR, otherwise I'd defer it to a > follow-up issue and disable indexOf intrinsic when running with > +UseCompactObjectHeaders. @rkennke Could you post the full command you used please? And perhaps also the seed that gets printed.. having trouble getting it to fail.. So far I added a few options and perrmitations of: `./build/linux-x86_64-server-fastdebug/images/jdk/bin/java -Xcomp -XX:-TieredCompilation -XX:+UnlockDiagnosticVMOptions -XX:+EnableX86ECoreOpts -XX:+UseSerialGC -XX:+UnlockExperimentalVMOptions -XX:+UseCompactObjectHeaders test/jdk/java/lang/StringBuffer/ECoreIndexOf.java` and lo luck.. IndexOf.java test checks "all interesting" lengths of haystack and needle and can't get it to fail either. - PR Comment: https://git.openjdk.org/jdk/pull/20677#issuecomment-2427232140
Re: RFR: 8342646: JTREG_TEST_THREAD_FACTORY in testing.md should be TEST_THREAD_FACTORY [v2]
On Mon, 21 Oct 2024 13:12:55 GMT, SendaoYan wrote: >> Hi all, >> In >> [make/RunTests.gmk](https://github.com/openjdk/jdk/blob/master/make/RunTests.gmk#L208), >> the keyword is 'TEST_THREAD_FACTORY'. >> >> So the below test command will print error: >> >> make test TEST=test/jdk/java/math/BigInteger/TestValueExact.java >> CONF=linux-x86_64-server-release JTREG="JTREG_TEST_THREAD_FACTORY=Virtual" >> >> >> Building target 'test' in configuration 'linux-x86_64-server-release' >> JTREG_TEST_THREAD_FACTORY=Virtual is not a valid keyword for JTREG. >> Valid keywords: JOBS TIMEOUT_FACTOR FAILURE_HANDLER_TIMEOUT TEST_MODE ASSERT >> VERBOSE RETAIN TEST_THREAD_FACTORY MAX_MEM RUN_PROBLEM_LISTS RETRY_COUNT >> REPEAT_COUNT MAX_OUTPUT REPORT OPTIONS JAVA_OPTIONS VM_OPTIONS KEYWORDS >> EXTRA_PROBLEM_LISTS LAUNCHER_OPTIONS. >> RunTests.gmk:206: *** Cannot continue. Stop. >> >> >> So I think we should fix the document bug in `doc/testing.md` to avoid take >> the same mistake. Trivial fix, no risk. > > SendaoYan has updated the pull request incrementally with two additional > commits since the last revision: > > - JTREG="TEST_THREAD_FACTORY=Virtual" > - Use JTREG="JTREG_TEST_THREAD_FACTORY=Virtual" to make comment more cleaner Marked as reviewed by erikj (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/21594#pullrequestreview-2382234395
Re: RFR: 8342704: GHA: Report truncation is broken after JDK-8341424
On Mon, 21 Oct 2024 14:41:48 GMT, Aleksey Shipilev wrote: > When GH output is too large, we do not actually truncate it after > [JDK-8341424](https://bugs.openjdk.org/browse/JDK-8341424). The error log > would be: > > > Run bash ./.github/scripts/gen-test-results.sh "$GITHUB_STEP_SUMMARY" > ./.github/scripts/gen-test-results.sh: line 28: report-utils.sh: No such file > or directory > ./.github/scripts/gen-test-results.sh: line 97: truncate_summary: command not > found > > Error: Process completed with exit code 127. > Error: $GITHUB_STEP_SUMMARY upload aborted, supports content up to a size of > 1024k, got 1579k. For more information see: > https://docs.github.com/actions/using-workflows/workflow-commands-for-github-actions#adding-a-markdown-summary > > > This is because we import report-utils.sh incorrectly: scripts try to find it > in current directory, which is not the same directory where the importing > scripts are located. And we never see it in normal cases, because scripts are > just eating the errors without any other observable effects. > > Additional testing: > - [x] GHA checks work with deliberately failing tests Marked as reviewed by erikj (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/21617#pullrequestreview-2382634121
Re: RFR: 8342704: GHA: Report truncation is broken after JDK-8341424
On Mon, 21 Oct 2024 14:41:48 GMT, Aleksey Shipilev wrote: > When GH output is too large, we do not actually truncate it after > [JDK-8341424](https://bugs.openjdk.org/browse/JDK-8341424). The error log > would be: > > > Run bash ./.github/scripts/gen-test-results.sh "$GITHUB_STEP_SUMMARY" > ./.github/scripts/gen-test-results.sh: line 28: report-utils.sh: No such file > or directory > ./.github/scripts/gen-test-results.sh: line 97: truncate_summary: command not > found > > Error: Process completed with exit code 127. > Error: $GITHUB_STEP_SUMMARY upload aborted, supports content up to a size of > 1024k, got 1579k. For more information see: > https://docs.github.com/actions/using-workflows/workflow-commands-for-github-actions#adding-a-markdown-summary > > > This is because we import report-utils.sh incorrectly: scripts try to find it > in current directory, which is not the same directory where the importing > scripts are located. And we never see it in normal cases, because scripts are > just eating the errors without any other observable effects. > > Additional testing: > - [x] GHA checks work with deliberately failing tests Looks good and trivial - Marked as reviewed by jwaters (Committer). PR Review: https://git.openjdk.org/jdk/pull/21617#pullrequestreview-2383568121
Re: RFR: 8342682: Errors related to unused code on Windows after 8339120
On Mon, 21 Oct 2024 14:34:30 GMT, Julian Waters wrote: > and whatever team is responsible for HotSpot debugging. I don't see anything hotspot related here. I think you would be better off splitting this up into distinct issues and PRs for different areas. I expect the client team in particular would prefer that. - PR Comment: https://git.openjdk.org/jdk/pull/21616#issuecomment-2428037844
Re: RFR: 8342682: Errors related to unused code on Windows after 8339120
On Tue, 22 Oct 2024 01:40:11 GMT, David Holmes wrote: > > and whatever team is responsible for HotSpot debugging. > > I don't see anything hotspot related here. > > I think you would be better off splitting this up into distinct issues and > PRs for different areas. I expect the client team in particular would prefer > that. Aren't the dt_shmem and jdwp changes related to HotSpot? But I guess I'll split it up in that case, I thought it might have been quicker to group it all together in one shot. This particular Pull Request I'll probably reserve for jdwp and the like, core-libs and client (And everything else) I'll move to their own PRs - PR Comment: https://git.openjdk.org/jdk/pull/21616#issuecomment-2428041172
Re: RFR: 8342662: C2: Add new phase for backend-specific lowering
On Mon, 21 Oct 2024 04:11:03 GMT, Jasmine Karthikeyan wrote: > Hi all, > This patch adds a new pass to consolidate lowering of complex > backend-specific code patterns, such as `MacroLogicV` and the optimization > proposed by #21244. Moving these optimizations to backend code can simplify > shared code, while also making it easier to develop more in-depth > optimizations. The linked bug has an example of a new optimization this could > enable. The new phase does GVN to de-duplicate nodes and calls nodes' > `Value()` method, but it does not call `Identity()` or `Ideal()` to avoid > undoing any changes done during lowering. It also reuses the IGVN worklist to > avoid needing to re-create the notification mechanism. > > In this PR only the skeleton code for the pass is added, moving `MacroLogicV` > to this system will be done separately in a future patch. Tier 1 tests pass > on my linux x64 machine. Feedback on this patch would be greatly appreciated! @jaskarth , Being MacroLogic optimization pass author, I volenteer to move it to lowering phase once this patch gets integrated. - PR Comment: https://git.openjdk.org/jdk/pull/21599#issuecomment-2428128710
Re: RFR: 8305895: Implement JEP 450: Compact Object Headers (Experimental) [v46]
On Mon, 21 Oct 2024 18:52:46 GMT, Volodymyr Paprotski wrote: > Thanks @rkennke able to reproduce now.. Sandhya will have a patch soon and I > will re-verify @rkennke @vpaprotsk Please find attached the patch which should fix the problem. [smallneedlefix.patch](https://github.com/user-attachments/files/17466073/smallneedlefix.patch) - PR Comment: https://git.openjdk.org/jdk/pull/20677#issuecomment-2427536012
Re: RFR: 8305895: Implement JEP 450: Compact Object Headers (Experimental) [v46]
On Mon, 21 Oct 2024 18:00:14 GMT, Roman Kennke wrote: >>> I've managed to reproduce the ECoreIndexOf crash locally by running with >>> -XX:+UseSerialGC -XX:+UnlockExperimentalVMOptions >>> -XX:+UseCompactObjectHeaders. The crash happens on line 773 when reading >>> past the needle. >>> >>> ``` >>> │ 762 __ movq(index, needle_len); >>> │ >>> │ 763 __ andq(index, 0xf); // nLen % 16 >>> │ 764 __ movq(offset, 0x10); >>> │ 765 __ subq(offset, index); // 16 - (nLen % 16) >>> │ 766 __ movq(index, offset); >>> │ 767 __ shlq(offset, 1); // * 2 >>> │ 768 __ negq(index); // -(16 - (nLen % 16)) >>> │ >>> │ 769 __ xorq(wr_index, wr_index); >>> │ 770 >>> │ 771 __ bind(L_top); >>> │ 772 // load needle and expand >>> │ 773 __ vpmovzxbw(xmm0, Address(needle, index, Address::times_1), >>> Assembler::AVX_256bit); >>> ``` >>> >>> We're reading this address: >>> >>> ``` >>> (SEGV_MAPERR), si_addr: 0x0007cffe >>> ``` >>> >>> which is just before the start of the heap: >>> >>> ``` >>> Heap address: 0x0007d000, size: 768 MB, Compressed Oops mode: Zero >>> based, Oop shift amount: 3 >>> ``` >>> >>> When this crashed I had: >>> >>> ``` >>> needle: 0x0007d00c >>> needle_len = 0x12 >>> index = 0xfffe >>> ``` >>> >>> There has been previous fix to not read past the haystack: Fix header < 16 >>> bytes in indexOf intrinsic, by @sviswa7 >>> [f65ef5d](https://github.com/openjdk/jdk/commit/f65ef5dc325212155a50a2fc3a7f4aad18b8d9d0) >>> >>> maybe we need something similar for the needle. >> >> @sviswa7 @vpaprotsk could you have a look? If we can have a reasonable fix >> for this soon, we could ship it in this PR, otherwise I'd defer it to a >> follow-up issue and disable indexOf intrinsic when running with >> +UseCompactObjectHeaders. > >> @rkennke Could you post the full command you used please? And perhaps also >> the seed that gets printed.. having trouble getting it to fail.. >> >> So far I added a few options and perrmitations of: >> `./build/linux-x86_64-server-fastdebug/images/jdk/bin/java -Xcomp >> -XX:-TieredCompilation -XX:+UnlockDiagnosticVMOptions >> -XX:+EnableX86ECoreOpts -XX:+UseSerialGC -XX:+UnlockExperimentalVMOptions >> -XX:+UseCompactObjectHeaders >> test/jdk/java/lang/StringBuffer/ECoreIndexOf.java` and lo luck.. >> IndexOf.java test checks "all interesting" lengths of haystack and needle >> and can't get it to fail either. > > I could reproduce on 3rd try with a fastdebug build with: > > make test TEST=java/lang/StringBuffer/ECoreIndexOf.java > TEST_VM_OPTS="-XX:+UnlockExperimentalVMOptions -XX:+UseCompactObjectHeaders > -XX:+UseSerialGC" > > > It prints: > > Seed set to 636754923980405411 > > > It probably depends on GC operation: It would only fail when the array > happens to be the very first object in the heap. The relevant GC/heap configs > would be: > > InitialHeapSize = 805306368 > MaxHeapSize = 805306368 > MaxNewSize = 268435456 > > > So you should probably also add `-Xmx805306368 -Xms805306368 -Xmn268435456` > > Thanks @rkennke able to reproduce now.. Sandhya will have a patch soon and > > I will re-verify > > @rkennke @vpaprotsk Please find attached the patch which should fix the > problem. > > [smallneedlefix.patch](https://github.com/user-attachments/files/17466073/smallneedlefix.patch) Testing now. Runs the reproducer in a loop since half an hour without crashing. I'll let it run overnight, and if @vpaprotsk approves the changes, then I'll intergrate them tomorrow morning. - PR Comment: https://git.openjdk.org/jdk/pull/20677#issuecomment-2427660618
Re: RFR: 8305895: Implement JEP 450: Compact Object Headers (Experimental) [v46]
On Mon, 21 Oct 2024 20:31:28 GMT, Roman Kennke wrote: >>> @rkennke Could you post the full command you used please? And perhaps also >>> the seed that gets printed.. having trouble getting it to fail.. >>> >>> So far I added a few options and perrmitations of: >>> `./build/linux-x86_64-server-fastdebug/images/jdk/bin/java -Xcomp >>> -XX:-TieredCompilation -XX:+UnlockDiagnosticVMOptions >>> -XX:+EnableX86ECoreOpts -XX:+UseSerialGC -XX:+UnlockExperimentalVMOptions >>> -XX:+UseCompactObjectHeaders >>> test/jdk/java/lang/StringBuffer/ECoreIndexOf.java` and lo luck.. >>> IndexOf.java test checks "all interesting" lengths of haystack and needle >>> and can't get it to fail either. >> >> I could reproduce on 3rd try with a fastdebug build with: >> >> make test TEST=java/lang/StringBuffer/ECoreIndexOf.java >> TEST_VM_OPTS="-XX:+UnlockExperimentalVMOptions -XX:+UseCompactObjectHeaders >> -XX:+UseSerialGC" >> >> >> It prints: >> >> Seed set to 636754923980405411 >> >> >> It probably depends on GC operation: It would only fail when the array >> happens to be the very first object in the heap. The relevant GC/heap >> configs would be: >> >> InitialHeapSize = 805306368 >> MaxHeapSize = 805306368 >> MaxNewSize = 268435456 >> >> >> So you should probably also add `-Xmx805306368 -Xms805306368 -Xmn268435456` > >> > Thanks @rkennke able to reproduce now.. Sandhya will have a patch soon and >> > I will re-verify >> >> @rkennke @vpaprotsk Please find attached the patch which should fix the >> problem. >> >> [smallneedlefix.patch](https://github.com/user-attachments/files/17466073/smallneedlefix.patch) > > Testing now. Runs the reproducer in a loop since half an hour without > crashing. I'll let it run overnight, and if @vpaprotsk approves the changes, > then I'll intergrate them tomorrow morning. @rkennke I've been running the patch too, for about 2.5 hours now, looks good to me. Also looked things over again, looks good. Just to explain/document what I reviewed.. - Looked at other uses of the needle (that code didn't change, so not exhaustive claim). Typically size of the needle being less then 16 'doesnt matter'.. i.e. broadcast first char, last char, if first/last character mask matches, switch-table for comparing middle - i.e. no reading headers needed - The case Sandhya fixes, handles UL special case (i.e. haystack unicode, needle regular). For cases of needle less then 32 bytes, copy the needle to the stack, and expand 8->16 so regular UU code can be used. Previous code looped 256bit loads at a time, now we loop 128 instead. - PR Comment: https://git.openjdk.org/jdk/pull/20677#issuecomment-2427718674
