Integrated: 8345286: Remove use of SecurityManager API from misc areas
On Mon, 2 Dec 2024 12:13:57 GMT, Jaikiran Pai wrote: > Can I please get a review of this change which removes usages of > SecurityManager related APIs and some leftover related to SecurityManager > changes? > > This addresses https://bugs.openjdk.org/browse/JDK-8345286. Most of these > changes are trivial. The > `src/java.base/linux/classes/jdk/internal/platform/CgroupUtil.java` used to > expose utility methods for dealing with SecurityManager permissions and it > was called from a few places. That class is no longer needed with the clean > up done in this PR. > > No new tests have been introduced and tier testing is currently in progress. This pull request has now been integrated. Changeset: 3d49665b Author:Jaikiran Pai URL: https://git.openjdk.org/jdk/commit/3d49665b85619038c082566b0bc38c0ebe5f752e Stats: 269 lines in 22 files changed: 8 ins; 226 del; 35 mod 8345286: Remove use of SecurityManager API from misc areas Reviewed-by: alanb, kevinw, sgehwolf - PR: https://git.openjdk.org/jdk/pull/22478
Re: RFR: 8345286: Remove use of SecurityManager API from misc areas [v9]
On Tue, 3 Dec 2024 06:12:13 GMT, Jaikiran Pai wrote: >> Can I please get a review of this change which removes usages of >> SecurityManager related APIs and some leftover related to SecurityManager >> changes? >> >> This addresses https://bugs.openjdk.org/browse/JDK-8345286. Most of these >> changes are trivial. The >> `src/java.base/linux/classes/jdk/internal/platform/CgroupUtil.java` used to >> expose utility methods for dealing with SecurityManager permissions and it >> was called from a few places. That class is no longer needed with the clean >> up done in this PR. >> >> No new tests have been introduced and tier testing is currently in progress. > > Jaikiran Pai has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 15 commits: > > - merge latest from master branch > - remove permission text from InnocuousThread > - merge latest from master branch > - remove changes to > src/java.base/unix/classes/sun/security/provider/NativePRNG.java > - remove unused import > - replace remaining Paths.get() with Path.of() in the updated files > - Update > src/java.base/linux/classes/jdk/internal/platform/CgroupSubsystemController.java > >Co-authored-by: Severin Gehwolf > - Update > src/java.base/linux/classes/jdk/internal/platform/cgroupv2/CgroupV2Subsystem.java > >Co-authored-by: Severin Gehwolf > - Path.of() instead of Paths.get() in CgroupV2Subsystem.java > - remove unnecessary space > - ... and 5 more: https://git.openjdk.org/jdk/compare/a3b58ee5...d53fe8ad Thank you everyone for the help in reviewing this. - PR Comment: https://git.openjdk.org/jdk/pull/22478#issuecomment-2516665394
Re: RFR: 8345286: Remove use of SecurityManager API from misc areas [v9]
On Tue, 3 Dec 2024 06:12:13 GMT, Jaikiran Pai wrote: >> Can I please get a review of this change which removes usages of >> SecurityManager related APIs and some leftover related to SecurityManager >> changes? >> >> This addresses https://bugs.openjdk.org/browse/JDK-8345286. Most of these >> changes are trivial. The >> `src/java.base/linux/classes/jdk/internal/platform/CgroupUtil.java` used to >> expose utility methods for dealing with SecurityManager permissions and it >> was called from a few places. That class is no longer needed with the clean >> up done in this PR. >> >> No new tests have been introduced and tier testing is currently in progress. > > Jaikiran Pai has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 15 commits: > > - merge latest from master branch > - remove permission text from InnocuousThread > - merge latest from master branch > - remove changes to > src/java.base/unix/classes/sun/security/provider/NativePRNG.java > - remove unused import > - replace remaining Paths.get() with Path.of() in the updated files > - Update > src/java.base/linux/classes/jdk/internal/platform/CgroupSubsystemController.java > >Co-authored-by: Severin Gehwolf > - Update > src/java.base/linux/classes/jdk/internal/platform/cgroupv2/CgroupV2Subsystem.java > >Co-authored-by: Severin Gehwolf > - Path.of() instead of Paths.get() in CgroupV2Subsystem.java > - remove unnecessary space > - ... and 5 more: https://git.openjdk.org/jdk/compare/a3b58ee5...d53fe8ad Although trivial, there are some changes to files from the serviceability area. So it would be good if someone from that area could review this too. - PR Comment: https://git.openjdk.org/jdk/pull/22478#issuecomment-2513925153
Re: RFR: 8345286: Remove use of SecurityManager API from misc areas [v8]
On Mon, 2 Dec 2024 18:54:33 GMT, Alan Bateman wrote: >> src/java.base/share/classes/jdk/internal/misc/InnocuousThread.java line 31: >> >>> 29: >>> 30: /** >>> 31: * A thread that has no permissions, is not a member of any user-defined >> >> I think you can also remove the words "has no permissions" as it no longer >> applies. @AlanBateman ? > > Yes, that can be removed. Done. - PR Review Comment: https://git.openjdk.org/jdk/pull/22478#discussion_r1867112404
Re: RFR: 8345286: Remove use of SecurityManager API from misc areas [v8]
On Mon, 2 Dec 2024 17:47:57 GMT, Daniel Fuchs wrote:
>> Jaikiran Pai has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 13 commits:
>>
>> - merge latest from master branch
>> - remove changes to
>> src/java.base/unix/classes/sun/security/provider/NativePRNG.java
>> - remove unused import
>> - replace remaining Paths.get() with Path.of() in the updated files
>> - Update
>> src/java.base/linux/classes/jdk/internal/platform/CgroupSubsystemController.java
>>
>>Co-authored-by: Severin Gehwolf
>> - Update
>> src/java.base/linux/classes/jdk/internal/platform/cgroupv2/CgroupV2Subsystem.java
>>
>>Co-authored-by: Severin Gehwolf
>> - Path.of() instead of Paths.get() in CgroupV2Subsystem.java
>> - remove unnecessary space
>> - Path.of() instead of Paths.get()
>> - fix formatting of try-with-resources in CgroupSubsystemController.java
>> - ... and 3 more: https://git.openjdk.org/jdk/compare/30b8bbe2...9baa279b
>
> src/java.base/linux/classes/jdk/internal/platform/CgroupSubsystemController.java
> line 68:
>
>> 66: try (Stream lines = Files.lines(filePath)) {
>> 67: Optional firstLine = lines.findFirst();
>> 68: return firstLine.orElse(null);
>
> you could probably just:
>
>
> return lines.findFirst().orElse(null);
>
>
> unless the local variable is needed or type inference?
Hello Daniel, no syntactical reason behind this. Severin proposed this style in
his review and I noticed that this area of the code has been using this style
of local variable assigment and immediate return in one or two other places, so
I decided to stick with it. I don't have a strong preference but am willing to
update it if you or others think we should.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/22478#discussion_r1867112305
Re: RFR: 8345286: Remove use of SecurityManager API from misc areas [v9]
> Can I please get a review of this change which removes usages of > SecurityManager related APIs and some leftover related to SecurityManager > changes? > > This addresses https://bugs.openjdk.org/browse/JDK-8345286. Most of these > changes are trivial. The > `src/java.base/linux/classes/jdk/internal/platform/CgroupUtil.java` used to > expose utility methods for dealing with SecurityManager permissions and it > was called from a few places. That class is no longer needed with the clean > up done in this PR. > > No new tests have been introduced and tier testing is currently in progress. Jaikiran Pai has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 15 commits: - merge latest from master branch - remove permission text from InnocuousThread - merge latest from master branch - remove changes to src/java.base/unix/classes/sun/security/provider/NativePRNG.java - remove unused import - replace remaining Paths.get() with Path.of() in the updated files - Update src/java.base/linux/classes/jdk/internal/platform/CgroupSubsystemController.java Co-authored-by: Severin Gehwolf - Update src/java.base/linux/classes/jdk/internal/platform/cgroupv2/CgroupV2Subsystem.java Co-authored-by: Severin Gehwolf - Path.of() instead of Paths.get() in CgroupV2Subsystem.java - remove unnecessary space - ... and 5 more: https://git.openjdk.org/jdk/compare/a3b58ee5...d53fe8ad - Changes: https://git.openjdk.org/jdk/pull/22478/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=22478&range=08 Stats: 269 lines in 22 files changed: 8 ins; 226 del; 35 mod Patch: https://git.openjdk.org/jdk/pull/22478.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/22478/head:pull/22478 PR: https://git.openjdk.org/jdk/pull/22478
Re: RFR: 8345286: Remove use of SecurityManager API from misc areas [v6]
> Can I please get a review of this change which removes usages of > SecurityManager related APIs and some leftover related to SecurityManager > changes? > > This addresses https://bugs.openjdk.org/browse/JDK-8345286. Most of these > changes are trivial. The > `src/java.base/linux/classes/jdk/internal/platform/CgroupUtil.java` used to > expose utility methods for dealing with SecurityManager permissions and it > was called from a few places. That class is no longer needed with the clean > up done in this PR. > > No new tests have been introduced and tier testing is currently in progress. Jaikiran Pai has updated the pull request incrementally with two additional commits since the last revision: - remove unused import - replace remaining Paths.get() with Path.of() in the updated files - Changes: - all: https://git.openjdk.org/jdk/pull/22478/files - new: https://git.openjdk.org/jdk/pull/22478/files/26df25b2..b018ef09 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=22478&range=05 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=22478&range=04-05 Stats: 7 lines in 2 files changed: 0 ins; 3 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/22478.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/22478/head:pull/22478 PR: https://git.openjdk.org/jdk/pull/22478
Re: RFR: 8345286: Remove use of SecurityManager API from misc areas [v4]
> Can I please get a review of this change which removes usages of > SecurityManager related APIs and some leftover related to SecurityManager > changes? > > This addresses https://bugs.openjdk.org/browse/JDK-8345286. Most of these > changes are trivial. The > `src/java.base/linux/classes/jdk/internal/platform/CgroupUtil.java` used to > expose utility methods for dealing with SecurityManager permissions and it > was called from a few places. That class is no longer needed with the clean > up done in this PR. > > No new tests have been introduced and tier testing is currently in progress. Jaikiran Pai has updated the pull request incrementally with one additional commit since the last revision: Update src/java.base/linux/classes/jdk/internal/platform/cgroupv2/CgroupV2Subsystem.java Co-authored-by: Severin Gehwolf - Changes: - all: https://git.openjdk.org/jdk/pull/22478/files - new: https://git.openjdk.org/jdk/pull/22478/files/ad62bd4c..52a47f72 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=22478&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=22478&range=02-03 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/22478.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/22478/head:pull/22478 PR: https://git.openjdk.org/jdk/pull/22478
Re: RFR: 8345286: Remove use of SecurityManager API from misc areas [v3]
> Can I please get a review of this change which removes usages of > SecurityManager related APIs and some leftover related to SecurityManager > changes? > > This addresses https://bugs.openjdk.org/browse/JDK-8345286. Most of these > changes are trivial. The > `src/java.base/linux/classes/jdk/internal/platform/CgroupUtil.java` used to > expose utility methods for dealing with SecurityManager permissions and it > was called from a few places. That class is no longer needed with the clean > up done in this PR. > > No new tests have been introduced and tier testing is currently in progress. Jaikiran Pai has updated the pull request incrementally with one additional commit since the last revision: Path.of() instead of Paths.get() in CgroupV2Subsystem.java - Changes: - all: https://git.openjdk.org/jdk/pull/22478/files - new: https://git.openjdk.org/jdk/pull/22478/files/75511e30..ad62bd4c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=22478&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=22478&range=01-02 Stats: 3 lines in 1 file changed: 1 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/22478.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/22478/head:pull/22478 PR: https://git.openjdk.org/jdk/pull/22478
Re: RFR: 8345286: Remove use of SecurityManager API from misc areas [v7]
On Mon, 2 Dec 2024 15:42:37 GMT, Alan Bateman wrote:
>> Jaikiran Pai has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> remove changes to
>> src/java.base/unix/classes/sun/security/provider/NativePRNG.java
>
> src/java.base/unix/classes/sun/security/provider/NativePRNG.java line 129:
>
>> 127: * Create a RandomIO object for all I/O of this Variant type.
>> 128: */
>> 129: private static RandomIO initIO(final Variant v) {
>
> Sean has included in this one in the changes for sun.security (pull/22418) so
> I think you can drop it from this PR.
Done. Removed it from this PR.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/22478#discussion_r1866107838
Re: RFR: 8345286: Remove use of SecurityManager API from misc areas [v7]
> Can I please get a review of this change which removes usages of > SecurityManager related APIs and some leftover related to SecurityManager > changes? > > This addresses https://bugs.openjdk.org/browse/JDK-8345286. Most of these > changes are trivial. The > `src/java.base/linux/classes/jdk/internal/platform/CgroupUtil.java` used to > expose utility methods for dealing with SecurityManager permissions and it > was called from a few places. That class is no longer needed with the clean > up done in this PR. > > No new tests have been introduced and tier testing is currently in progress. Jaikiran Pai has updated the pull request incrementally with one additional commit since the last revision: remove changes to src/java.base/unix/classes/sun/security/provider/NativePRNG.java - Changes: - all: https://git.openjdk.org/jdk/pull/22478/files - new: https://git.openjdk.org/jdk/pull/22478/files/b018ef09..5a96fa5f Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=22478&range=06 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=22478&range=05-06 Stats: 101 lines in 1 file changed: 41 ins; 26 del; 34 mod Patch: https://git.openjdk.org/jdk/pull/22478.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/22478/head:pull/22478 PR: https://git.openjdk.org/jdk/pull/22478
Re: RFR: 8345286: Remove use of SecurityManager API from misc areas [v5]
> Can I please get a review of this change which removes usages of > SecurityManager related APIs and some leftover related to SecurityManager > changes? > > This addresses https://bugs.openjdk.org/browse/JDK-8345286. Most of these > changes are trivial. The > `src/java.base/linux/classes/jdk/internal/platform/CgroupUtil.java` used to > expose utility methods for dealing with SecurityManager permissions and it > was called from a few places. That class is no longer needed with the clean > up done in this PR. > > No new tests have been introduced and tier testing is currently in progress. Jaikiran Pai has updated the pull request incrementally with one additional commit since the last revision: Update src/java.base/linux/classes/jdk/internal/platform/CgroupSubsystemController.java Co-authored-by: Severin Gehwolf - Changes: - all: https://git.openjdk.org/jdk/pull/22478/files - new: https://git.openjdk.org/jdk/pull/22478/files/52a47f72..26df25b2 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=22478&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=22478&range=03-04 Stats: 4 lines in 1 file changed: 0 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/22478.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/22478/head:pull/22478 PR: https://git.openjdk.org/jdk/pull/22478
Re: RFR: 8345286: Remove use of SecurityManager API from misc areas [v3]
On Mon, 2 Dec 2024 13:06:05 GMT, Severin Gehwolf wrote:
>> Jaikiran Pai has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Path.of() instead of Paths.get() in CgroupV2Subsystem.java
>
> src/java.base/linux/classes/jdk/internal/platform/cgroupv2/CgroupV2Subsystem.java
> line 332:
>
>> 330:
>> 331: private long sumTokensIOStat(Function mapFunc) {
>> 332: try (Stream lines =
>> Files.lines(Paths.get(unified.path(), "io.stat"))) {
>
> Suggestion:
>
> try (Stream lines = Files.lines(Paths.of(unified.path(),
> "io.stat"))) {
Done.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/22478#discussion_r1865903682
Re: RFR: 8345286: Remove use of SecurityManager API from misc areas [v8]
> Can I please get a review of this change which removes usages of > SecurityManager related APIs and some leftover related to SecurityManager > changes? > > This addresses https://bugs.openjdk.org/browse/JDK-8345286. Most of these > changes are trivial. The > `src/java.base/linux/classes/jdk/internal/platform/CgroupUtil.java` used to > expose utility methods for dealing with SecurityManager permissions and it > was called from a few places. That class is no longer needed with the clean > up done in this PR. > > No new tests have been introduced and tier testing is currently in progress. Jaikiran Pai has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 13 commits: - merge latest from master branch - remove changes to src/java.base/unix/classes/sun/security/provider/NativePRNG.java - remove unused import - replace remaining Paths.get() with Path.of() in the updated files - Update src/java.base/linux/classes/jdk/internal/platform/CgroupSubsystemController.java Co-authored-by: Severin Gehwolf - Update src/java.base/linux/classes/jdk/internal/platform/cgroupv2/CgroupV2Subsystem.java Co-authored-by: Severin Gehwolf - Path.of() instead of Paths.get() in CgroupV2Subsystem.java - remove unnecessary space - Path.of() instead of Paths.get() - fix formatting of try-with-resources in CgroupSubsystemController.java - ... and 3 more: https://git.openjdk.org/jdk/compare/30b8bbe2...9baa279b - Changes: https://git.openjdk.org/jdk/pull/22478/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=22478&range=07 Stats: 268 lines in 22 files changed: 8 ins; 226 del; 34 mod Patch: https://git.openjdk.org/jdk/pull/22478.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/22478/head:pull/22478 PR: https://git.openjdk.org/jdk/pull/22478
Re: RFR: 8345286: Remove use of SecurityManager API from misc areas [v5]
On Mon, 2 Dec 2024 14:17:10 GMT, Severin Gehwolf wrote: > cgroups changes look good. Haven't looked at the other bits. Thank you Severin. > src/java.base/linux/classes/jdk/internal/platform/CgroupSubsystemController.java > line 29: > >> 27: package jdk.internal.platform; >> 28: >> 29: import java.io.BufferedReader; > > Unused now? Yes, it's now unused. I've removed it in the latest update to the PR and also replaced a few more Paths.get() with Path.of() in the current set of files that have been updated in this PR. - PR Comment: https://git.openjdk.org/jdk/pull/22478#issuecomment-2511675143 PR Review Comment: https://git.openjdk.org/jdk/pull/22478#discussion_r1865937624
Re: RFR: 8345286: Remove use of SecurityManager API from misc areas [v2]
On Mon, 2 Dec 2024 14:07:45 GMT, Severin Gehwolf wrote: >> Is the catch for `UncheckedIOException` due to some previously known failure >> resulting in the use of these APIs? > > Without using `Files.lines()` no `UncheckedIOException` would be thrown. Just > `IOException` and `null` returned. This extra catch is there to avoid new > `UncheckedIOException` being thrown on `findFirst()`. I.e. to keep semantics > the same as before. Thank you that's a good point. I hadn't taken into account exceptions propagated from `java.util.stream.Stream` operations. - PR Review Comment: https://git.openjdk.org/jdk/pull/22478#discussion_r1865922009
Re: RFR: 8345286: Remove use of SecurityManager API from misc areas [v6]
On Mon, 2 Dec 2024 14:25:12 GMT, Alan Bateman wrote: >> Jaikiran Pai has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - remove unused import >> - replace remaining Paths.get() with Path.of() in the updated files > > src/java.base/linux/classes/jdk/internal/platform/CgroupSubsystemController.java > line 3: > >> 1: /* >> 2: * Copyright (c) 2020, 2022, Red Hat Inc. >> 3: * Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved. > > Not sure about this as it's not a significant change to the file. Do you mean for just this file or the rest of the files in this PR as well? I am open to removing the copyright additions. - PR Review Comment: https://git.openjdk.org/jdk/pull/22478#discussion_r1865951827
Re: RFR: 8345286: Remove use of SecurityManager API from misc areas [v2]
On Mon, 2 Dec 2024 13:43:41 GMT, Severin Gehwolf wrote:
>> Jaikiran Pai has updated the pull request incrementally with four additional
>> commits since the last revision:
>>
>> - remove unnecessary space
>> - Path.of() instead of Paths.get()
>> - fix formatting of try-with-resources in CgroupSubsystemController.java
>> - leave out MethodUtil from the clean up
>
> src/java.base/linux/classes/jdk/internal/platform/CgroupSubsystemController.java
> line 70:
>
>> 68: String line = bufferedReader.readLine();
>> 69: return line;
>> 70: } catch (IOException e) {
>
> We can use `Files.lines` here right away:
>
> Suggestion:
>
> try (Stream lines = Files.lines(filePath)) {
> Optional firstLine = lines.findFirst();
> return firstLine.orElse(null);
> } catch (UncheckedIOException | IOException e) {
Is the catch for `UncheckedIOException` due to some previously known failure
resulting in the use of these APIs?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/22478#discussion_r1865907116
Re: RFR: 8345286: Remove use of SecurityManager API from misc areas [v2]
> Can I please get a review of this change which removes usages of > SecurityManager related APIs and some leftover related to SecurityManager > changes? > > This addresses https://bugs.openjdk.org/browse/JDK-8345286. Most of these > changes are trivial. The > `src/java.base/linux/classes/jdk/internal/platform/CgroupUtil.java` used to > expose utility methods for dealing with SecurityManager permissions and it > was called from a few places. That class is no longer needed with the clean > up done in this PR. > > No new tests have been introduced and tier testing is currently in progress. Jaikiran Pai has updated the pull request incrementally with four additional commits since the last revision: - remove unnecessary space - Path.of() instead of Paths.get() - fix formatting of try-with-resources in CgroupSubsystemController.java - leave out MethodUtil from the clean up - Changes: - all: https://git.openjdk.org/jdk/pull/22478/files - new: https://git.openjdk.org/jdk/pull/22478/files/e36c5207..75511e30 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=22478&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=22478&range=00-01 Stats: 21 lines in 3 files changed: 7 ins; 1 del; 13 mod Patch: https://git.openjdk.org/jdk/pull/22478.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/22478/head:pull/22478 PR: https://git.openjdk.org/jdk/pull/22478
Re: RFR: 8345286: Remove use of SecurityManager API from misc areas [v2]
On Mon, 2 Dec 2024 12:42:47 GMT, Alan Bateman wrote:
>> Jaikiran Pai has updated the pull request incrementally with four additional
>> commits since the last revision:
>>
>> - remove unnecessary space
>> - Path.of() instead of Paths.get()
>> - fix formatting of try-with-resources in CgroupSubsystemController.java
>> - leave out MethodUtil from the clean up
>
> src/java.base/linux/classes/jdk/internal/platform/CgroupSubsystemController.java
> line 68:
>
>> 66:
>> 67: try (BufferedReader bufferedReader =
>> 68:
>> Files.newBufferedReader(Paths.get(controller.path(), param))) {
>
> The formatting has got messed up here. If you create `Path path =
> Path.of(controller.path(), param)` then the try line would fit on one line
> and would fix the formatting issue. Maybe some future cleanup will replace
> this with `Files.lines` as this just needs to return the first line.
Fixed. I moved the `Path.of()` outside of the try-with-resources and the line
length is now more reasonable.
> src/java.base/linux/classes/jdk/internal/platform/CgroupSubsystemController.java
> line 167:
>
>> 165: if (controller == null) return defaultRetval;
>> 166:
>> 167: try (Stream lines =
>> Files.lines(Paths.get(controller.path(), param))) {
>
> Using Path.of might be clearer here.
Done, here and one other place in this file.
> src/java.base/share/classes/sun/reflect/misc/MethodUtil.java line 36:
>
>> 34: import java.security.CodeSource;
>> 35: import java.security.PermissionCollection;
>> 36: import java.security.SecureClassLoader;
>
> I'm half tempted to suggest leaving MethodUtil out of this change. There is
> further work required her and leaving the SM usage is a reminder of that.
Understood. I've now updated this PR to revert back the changes in this file.
> src/java.management/share/classes/sun/management/VMManagementImpl.java line
> 249:
>
>> 247:
>> 248: // construct PerfInstrumentation object
>> 249: Perf perf = Perf.getPerf();
>
> An extra space crept in that at some point
Thanks for spotting that, I hadn't noticed it.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/22478#discussion_r1865864725
PR Review Comment: https://git.openjdk.org/jdk/pull/22478#discussion_r1865865592
PR Review Comment: https://git.openjdk.org/jdk/pull/22478#discussion_r1865863888
PR Review Comment: https://git.openjdk.org/jdk/pull/22478#discussion_r1865865984
Re: RFR: 8345286: Remove use of SecurityManager API from misc areas
On Mon, 2 Dec 2024 12:13:57 GMT, Jaikiran Pai wrote: > Can I please get a review of this change which removes usages of > SecurityManager related APIs and some leftover related to SecurityManager > changes? > > This addresses https://bugs.openjdk.org/browse/JDK-8345286. Most of these > changes are trivial. The > `src/java.base/linux/classes/jdk/internal/platform/CgroupUtil.java` used to > expose utility methods for dealing with SecurityManager permissions and it > was called from a few places. That class is no longer needed with the clean > up done in this PR. > > No new tests have been introduced and tier testing is currently in progress. Hello Severin @jerboaa, given your work in the cgroups area of the JDK, could you help review the updates in this PR for those classes? - PR Comment: https://git.openjdk.org/jdk/pull/22478#issuecomment-2511382273
RFR: 8345286: Remove use of SecurityManager API from misc areas
Can I please get a review of this change which removes usages of SecurityManager related APIs and some leftover related to SecurityManager changes? This addresses https://bugs.openjdk.org/browse/JDK-8345286. Most of these changes are trivial. The `src/java.base/linux/classes/jdk/internal/platform/CgroupUtil.java` used to expose utility methods for dealing with SecurityManager permissions and it was called from a few places. That class is no longer needed with the clean up done in this PR. No new tests have been introduced and tier testing is currently in progress. - Commit messages: - update VectorIntrinsics too - 8345286: Remove use of SecurityManager API from misc areas Changes: https://git.openjdk.org/jdk/pull/22478/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=22478&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8345286 Stats: 432 lines in 26 files changed: 53 ins; 320 del; 59 mod Patch: https://git.openjdk.org/jdk/pull/22478.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/22478/head:pull/22478 PR: https://git.openjdk.org/jdk/pull/22478
Re: RFR: 8343894: ProblemList javax/management/remote/mandatory/notif/EmptyDomainNotificationTest.java
On Mon, 11 Nov 2024 01:24:38 GMT, David Holmes wrote: > This is causing hundreds of failures in our CI and > [JDK-8343838](https://bugs.openjdk.org/browse/JDK-8343838) may not be fixed > for a number of hours yet. > > Thanks Looks reasonable and trivial to me. - Marked as reviewed by jpai (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/22001#pullrequestreview-2425957977
Re: RFR: 8342827: Fix order of @param tags in other modules [v2]
On Thu, 24 Oct 2024 12:17:41 GMT, Hannes Wallnöfer wrote: >> Please review a doc-only change to fix the order of javadoc @param tags in >> in various OpenJDK modules. This is the third and last PR to fix the order >> of @param tags in OpenJDK libraries. >> >> We are working on a javadoc feature to add an opt-in doclint warning for >> @param tags that don't match the order of parameters in the documented >> element. The warning will be enabled in OpenJDK builds and covers all uses >> of the @param tag, i.e. parameters of executable elements, type parameters, >> and record components. >> >> I compared the generated API docs built with this branch with API docs built >> from master branch to make sure they are identical. > > Hannes Wallnöfer has updated the pull request incrementally with one > additional commit since the last revision: > > Fix copyright header This doc-only change which merely reorders the `@param` tags looks OK to me. - Marked as reviewed by jpai (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/21637#pullrequestreview-2392367946
Re: RFR: 8342827: Fix order of @param tags in other modules
On Tue, 22 Oct 2024 13:36:43 GMT, Hannes Wallnöfer wrote: > Please review a doc-only change to fix the order of javadoc @param tags in in > various OpenJDK modules. This is the third and last PR to fix the order of > @param tags in OpenJDK libraries. > > We are working on a javadoc feature to add an opt-in doclint warning for > @param tags that don't match the order of parameters in the documented > element. The warning will be enabled in OpenJDK builds and covers all uses of > the @param tag, i.e. parameters of executable elements, type parameters, and > record components. > > I compared the generated API docs built with this branch with API docs built > from master branch to make sure they are identical. src/java.xml/share/classes/javax/xml/validation/SchemaFactoryConfigurationError.java line 2: > 1: /* > 2: * Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved. Hello Hans, this looks like an oversight - the copyright year update should have retained the 2013 and additionally introduced 2024. - PR Review Comment: https://git.openjdk.org/jdk/pull/21637#discussion_r1814810579
Re: RFR: 8342827: Fix order of @param tags in other modules
On Tue, 22 Oct 2024 13:36:43 GMT, Hannes Wallnöfer wrote: > Please review a doc-only change to fix the order of javadoc @param tags in in > various OpenJDK modules. This is the third and last PR to fix the order of > @param tags in OpenJDK libraries. > > We are working on a javadoc feature to add an opt-in doclint warning for > @param tags that don't match the order of parameters in the documented > element. The warning will be enabled in OpenJDK builds and covers all uses of > the @param tag, i.e. parameters of executable elements, type parameters, and > record components. > > I compared the generated API docs built with this branch with API docs built > from master branch to make sure they are identical. Hello Hannes, since `@param` requires a parameter name and since javadoc already warns about incorrect `@param` names (that don't match the name of the method parameter), does the order in which the `@param` is listed in the javadoc text matter? I looked at https://bugs.openjdk.org/browse/JDK-8279931 but that doesn't tell why the order would be important. - PR Comment: https://git.openjdk.org/jdk/pull/21637#issuecomment-2429541439
Re: RFR: 8319873: Add windows implementation for jcmd System.map and System.dump_map [v13]
On Wed, 18 Sep 2024 11:42:47 GMT, David Holmes wrote: > I can't file a bug at present but please attend to this ASAP. I have filed https://bugs.openjdk.org/browse/JDK-8340368 to track this. - PR Comment: https://git.openjdk.org/jdk/pull/20597#issuecomment-2358266461
Integrated: 8340176: Replace usage of -noclassgc with -Xnoclassgc in test/jdk/java/lang/management/MemoryMXBean/LowMemoryTest2.java
On Mon, 16 Sep 2024 09:21:22 GMT, Jaikiran Pai wrote: > Can I please get a review of this test-only change which replaces the usage > of `-noclassgc` with `-Xnoclassgc` option when launching the test? > > As noted in https://bugs.openjdk.org/browse/JDK-8340176, the `-noclassgc` is > an undocumented option of the `java` launcher, which it converts to > `-Xnoclassgc` before passing it to the JVM. Instead of that option, the > documented `-Xnoclassgc` should be used. > > No new test has been introduced and the modified test continues to pass after > this change. This pull request has now been integrated. Changeset: 3e03e667 Author:Jaikiran Pai URL: https://git.openjdk.org/jdk/commit/3e03e6673acfea543d0dbbc64b7a4f52e3292c2b Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod 8340176: Replace usage of -noclassgc with -Xnoclassgc in test/jdk/java/lang/management/MemoryMXBean/LowMemoryTest2.java Reviewed-by: kevinw, lmesnik - PR: https://git.openjdk.org/jdk/pull/21012
Re: RFR: 8340176: Replace usage of -noclassgc with -Xnoclassgc in test/jdk/java/lang/management/MemoryMXBean/LowMemoryTest2.java
On Mon, 16 Sep 2024 09:21:22 GMT, Jaikiran Pai wrote: > Can I please get a review of this test-only change which replaces the usage > of `-noclassgc` with `-Xnoclassgc` option when launching the test? > > As noted in https://bugs.openjdk.org/browse/JDK-8340176, the `-noclassgc` is > an undocumented option of the `java` launcher, which it converts to > `-Xnoclassgc` before passing it to the JVM. Instead of that option, the > documented `-Xnoclassgc` should be used. > > No new test has been introduced and the modified test continues to pass after > this change. Thank you Kevin and Leonid for the reviews. - PR Comment: https://git.openjdk.org/jdk/pull/21012#issuecomment-2354283759
RFR: 8340176: Replace usage of -noclassgc with -Xnoclassgc in test/jdk/java/lang/management/MemoryMXBean/LowMemoryTest2.java
Can I please get a review of this test-only change which replaces the usage of `-noclassgc` with `-Xnoclassgc` option when launching the test? As noted in https://bugs.openjdk.org/browse/JDK-8340176, the `-noclassgc` is an undocumented option of the `java` launcher, which it converts to `-Xnoclassgc` before passing it to the JVM. Instead of that option, the documented `-Xnoclassgc` should be used. No new test has been introduced and the modified test continues to pass after this change. - Commit messages: - 8340176: Replace usage of -noclassgc with -Xnoclassgc in test/jdk/java/lang/management/MemoryMXBean/LowMemoryTest2.java Changes: https://git.openjdk.org/jdk/pull/21012/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=21012&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8340176 Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/21012.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/21012/head:pull/21012 PR: https://git.openjdk.org/jdk/pull/21012
Re: RFR: 8336289: Obliterate most references to _snprintf in the Windows JDK [v5]
On Mon, 26 Aug 2024 12:24:30 GMT, Julian Waters wrote: > If no objections are raised by tomorrow morning I'll proceed with integration I've run this PR against latest mainline in our CI for tier1, tier2 and tier3 and there were no failures. So yes, I think you can go ahead with the integration. - PR Comment: https://git.openjdk.org/jdk/pull/20153#issuecomment-2310538487
Re: RFR: 8336289: Obliterate most references to _snprintf in the Windows JDK [v5]
On Sat, 24 Aug 2024 05:12:42 GMT, Julian Waters wrote: >> snprintf has been available for all officially and unofficially supported >> compilers for Windows, Visual Studio since version 2015 and gcc since, well, >> forever. snprintf is conforming to C99 since the start when compiling using >> gcc, and 2015 when using Visual Studio. Since it conforms to C99 and >> provides better semantics than _snprintf, and is not compiler specific, we >> should use it in most places where we currently use _snprintf. This makes >> JDK code where this is used more robust due to the null terminating >> guarantee of snprintf. Since most of the changes are extremely simple, I've >> included all of them hoping to get this all done in one shot. The only >> places _snprintf is not replaced is places where I'm not sure whether the >> code is ours or not (As in, imported source code for external libraries). >> Note that the existing checks in place for the size of the characters >> written still work, as the return value of snprintf works mostly the same as >> _snprintf. The only difference is the nu ll terminating character at the end and the returning of the number of written characters if the buffer was terminated early, as seen here: https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/snprintf-snprintf-snprintf-l-snwprintf-snwprintf-l?view=msvc-170 >> >> Reviews Required: 2 for HotSpot, jdk.hotspot.agent, and jdk.jdwp.agent, 1 >> for awt and jdk.accessibility, 1 for java.base, 1 for security, 1 for >> jdk.management(?) > > Julian Waters has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains five additional > commits since the last revision: > > - Merge branch 'master' into snprintf > - Change copyright years and formatting > - Revert Standard Integer type rewrite > - USe os::snprintf in HotSpot > - Obliterate most references to _snprintf in the Windows JDK Marked as reviewed by jpai (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/20153#pullrequestreview-2258410435
Re: RFR: 8337331: crash: pinned virtual thread will lead to jvm crash when running with the javaagent option [v11]
On Thu, 8 Aug 2024 08:54:54 GMT, Jiawei Tang wrote:
>> `@clean jdk.test.lib.util.JavaAgentBuilder` cannot solve the problem.
>> I find that the reason why `AgentWithVThreadTest.java` can pass is that when
>> compiling `AgentWithVThread.java`, it uses jdk.test.lib.Utils through
>> `import` (`import jdk.test.lib.process.ProcessTools;`, `import
>> jdk.test.lib.Utils;` is in the ProcessTools.java), so the test dir contains
>> jdk.test.lib.Utils. However, my new testcase doesn't use it.
>> > src=https://github.com/user-attachments/assets/cd80f76e-24b4-463b-bcfd-4b4ba32374cd
>> width=300 />
>>
>> I tryed add `import jdk.test.lib.Utils;` in my testcase, it can pass.
>> > src=https://github.com/user-attachments/assets/cfb10e17-23ae-47f4-b0da-e107fe06c711
>> width=400 />
>>
>>
>> If I only run the new testcase, it can pass and the work dir look like this:
>> > src=https://github.com/user-attachments/assets/ade018b0-c0b0-4675-b3a0-784b4ef82b78
>> width=300 />
>>
>> If I run `AgentWithVThread.java` and then `TestPinCaseWithCFLH.java`, the
>> Utils.class is missed in test/lib/jdk/test/lib:
>> > src=https://github.com/user-attachments/assets/83dd910c-ff61-4c06-9ccc-256b2d0ccf1f
>> width=300 />
>
> A stable way to reproduce the problem: run AgentWithVThread.java and then
> TestPinCaseWithCFLH.java.
>
>
> jtreg -v:error,fail -jdk:{JDKPATH}
> ./test/hotspot/jtreg/serviceability/jvmti/vthread/premain/AgentWithVThreadTest.java
> jtreg -v:error,fail -jdk:{JDKPATH}
> ./test/hotspot/jtreg/serviceability/jvmti/vthread/TestPinCaseWithCFLH/TestPinCaseWithCFLH.java
When I proposed the `@clean` option, I had tried it locally and that had worked
for me and I wasn't able to reproduce that issue anymore locally. However,
looking at the failed GitHub actions job with the `@clean` option, I see this:
#section:clean
--messages:(5/232)--
command: clean jdk.test.lib.util.JavaAgentBuilder
reason: User specified action: run clean jdk.test.lib.util.JavaAgentBuilder
started: Thu Aug 08 07:40:24 UTC 2024
finished: Thu Aug 08 07:40:24 UTC 2024
elapsed time (seconds): 0.0
--rerun:(2/367)*--
cd
/Users/runner/work/jdk/jdk/build/run-test-prebuilt/test-support/jtreg_test_hotspot_jtreg_tier1_serviceability/scratch/0
&& \\
rm -f
/Users/runner/work/jdk/jdk/build/run-test-prebuilt/test-support/jtreg_test_hotspot_jtreg_tier1_serviceability/classes/0/serviceability/jvmti/vthread/TestPinCaseWithCFLH/TestPinCaseWithCFLH.d/jdk/test/lib/util/JavaAgentBuilder.class
result: Passed. Clean successful
#section:build
--messages:(5/194)--
command: build jdk.test.lib.util.JavaAgentBuilder
reason: Named class compiled on demand
started: Thu Aug 08 07:40:24 UTC 2024
finished: Thu Aug 08 07:40:24 UTC 2024
elapsed time (seconds): 0.0
result: Passed. All files up to date
So jtreg in its clean action appears to have only deleted the test specific
work directory:
rm -f
/Users/runner/work/jdk/jdk/build/run-test-prebuilt/test-support/jtreg_test_hotspot_jtreg_tier1_serviceability/classes/0/serviceability/jvmti/vthread/TestPinCaseWithCFLH/TestPinCaseWithCFLH.d/jdk/test/lib/util/JavaAgentBuilder.class
and of course that `JavaAgentBuilder.class` won't be there and is instead
present in the shared directory at
`/Users/runner/work/jdk/jdk/build/run-test-prebuilt/test-support/jtreg_test_hotspot_jtreg_tier1_serviceability/classes/0/test/lib`.
So jtreg didn't clean up the shared directory location and let that class stay
around which effectively meant the `@clean` ended up being a no-op.
I have run out of ideas to introduce a proper workaround here. The right fix of
course needs to happen in jtreg, which I will see if there are ways to
implement it there.
For now, it looks like the `@build jdk.test.lib.Utils` approach you used and
made the test pass is the only way to make this consistently pass.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20373#discussion_r1709023985
Re: RFR: 8337331: crash: pinned virtual thread will lead to jvm crash when running with the javaagent option [v11]
On Thu, 8 Aug 2024 02:21:39 GMT, Jiawei Tang wrote: >> I was inspired by >> [CODETOOLS-7901986](https://bugs.openjdk.org/browse/CODETOOLS-7901986). By >> adding `@build jdk.test.lib.Utils` all tests are passed. The GitHub Actions >> jobs are finished successfully, too. I hope I could get a final review now. > > Thank you for your help. I already made it by adding `@build > jdk.test.lib.Utils`. Do I need to try to add `@clean > jdk.test.lib.util.JavaAgentBuilder` and test again? I am not too sure adding the `@build jdk.test.lib.Utils` is a good thing. This test definition nor the test code uses/references that class. So it's odd to be adding a build tag for an indirect dependent class (and only that specific class). I felt the `@clean jdk.test.lib.util.JavaAgentBuilder` would be a better option since that `jdk.test.lib.util.JavaAgentBuilder` class is being used by the test definition. Having said that, it's just a personal opinion and I would let hotspot and serviceability area members to decide what approach to use here. - PR Review Comment: https://git.openjdk.org/jdk/pull/20373#discussion_r1708446392
Re: RFR: 8337331: crash: pinned virtual thread will lead to jvm crash when running with the javaagent option [v11]
On Wed, 7 Aug 2024 08:31:35 GMT, Jaikiran Pai wrote: >> I see that the `NoClassDefFoundError` failure that you are mentioning is >> actually being reported even in the GitHub Actions job failures. It does >> look odd (although might be a pre-known jtreg issue). It will need a bit of >> investigation to see what's going on. > > I was able to reproduce this `NoClassDefFoundError` in this new test locally. > I will take a look to see if I can figure out what's going on. I looked into this locally and this is a (known) bug in jtreg https://bugs.openjdk.org/browse/CODETOOLS-7902847. What's happening here is that the tests are launched using make test TEST=test/hotspot/jtreg/:tier1_serviceability. One of those tests is the (pre-existing unrelated to this PR) AgentWithVThreadTest. That test has a `@compile AgentWithVThread.java AgentWithVThreadTest.java`. This triggers compilation of those classes and also any referenced classes in those 2 classes. One such class happens to be the test library's jdk.test.lib.Utils. This is a test library class (used/referenced indirectly in that test). jtreg ends up issuing a javac command with destination directory as the AgentWithVThreadTest's test specific work directory: -d build/macosx-aarch64/test-support/jtreg_test_hotspot_jtreg_tier1_serviceability/classes/0/serviceability/jvmti/vthread/premain/AgentWithVThreadTest.d So the `jdk.test.lib.Utils.class` (along with other classes) file ends up being compiled to `build/macosx-aarch64/test-support/jtreg_test_hotspot_jtreg_tier1_serviceability/classes/0/serviceability/jvmti/vthread/premain/AgentWithVThreadTest.d/jdk/test/lib/Utils.class`. During this compilation, the `jdk.test.lib.util.JavaAgentBuilder` doesn't get compiled because that class isn't referenced by AgentWithVThreadTest (neither directly or indirectly). Then during the same test execution, jtreg notices a `@run` statement: @run driver jdk.test.lib.util.JavaAgentBuilder And to launch that run action, it first builds and compiles the jdk.test.lib.util.JavaAgentBuilder and since this is a test library class, jtreg ends up launching `javac` with a destination directory which is common/shared by multiple tests: -d build/macosx-aarch64/test-support/jtreg_test_hotspot_jtreg_tier1_serviceability/classes/0/test/lib Additionally, since this is being compiled in context of the AgentWithVThreadTest, jtreg also passes the test specific work directory (`build/macosx-aarch64/test-support/jtreg_test_hotspot_jtreg_tier1_serviceability/classes/0/serviceability/jvmti/vthread/premain/AgentWithVThreadTest.d`) as the classpath to the javac command. So effectively, this compilation ends up finding the `jdk.test.lib.Utils.class` in the test specific directory and doesn't recompile to the shared location. Since the `jdk.test.lib.util.JavaAgentBuilder` hasn't yet been compiled nor is located in the test specific work directory, javac ends up compiling it and placing it in the destination directory which was passed to the javac invocation and happens to be a shared directory for tests `build/macosx-aarch64/test-support/jtreg_test_hotspot_jtreg_tier1_serviceability/classes/0/test/lib`. Effectively we now have a test library class (the `JavaAgentBuilder`) in the common shared directory and some of its referenced classes (`Utils.class`) in a test specific directory. At a later point in time, this new test `TestPinCaseWithCFLH` being proposed in this PR, gets launched and jtreg notices the `@run` statement: @run driver jdk.test.lib.util.JavaAgentBuilder ... Just like previously, since this is a test library class, jtreg tries to locate this class in the shared directory and it finds it in the shared directory `build/macosx-aarch64/test-support/jtreg_test_hotspot_jtreg_tier1_serviceability/classes/0/test/lib` (since it was compiled to that location by an unrelated test). Since it finds the class, jtreg then skips compilation of that test library class and thus the `Utils.class` (or any of the classes referenced by `JavaAgentBuilder`) isn't recompiled again. jtreg then launches the test with this shared directory and this test's specific work directory (which is different from the AgentWithVThreadTest's work directory) in the classpath. Since neither of these directories contain the `Utils.class`, we end up with this missing class error. Like I noted, this is a known problem in jtreg. I'll see if we can do something in that area. For now though, I think one workaround is to add a: @clean jdk.test.lib.util.JavaAgentBuilder before the `@run` tag to allow for the jdk.test.lib.util.JavaAgentBuilder and its referenced classes to be compiled afresh (into the shared test library directory). This is what the change would look like in your PR: diff --git a/test/hotspot/jtreg/serviceability/jvmti/vthread/TestPinCaseWithCFL
Re: RFR: 8337331: crash: pinned virtual thread will lead to jvm crash when running with the javaagent option [v11]
On Tue, 6 Aug 2024 07:59:12 GMT, Jaikiran Pai wrote: >> It cannot solve the NoClassDefFoundError. Besides, this new testcase is not >> included in linux-x86 / test (hs/tier1 serviceability). > > I see that the `NoClassDefFoundError` failure that you are mentioning is > actually being reported even in the GitHub Actions job failures. It does look > odd (although might be a pre-known jtreg issue). It will need a bit of > investigation to see what's going on. I was able to reproduce this `NoClassDefFoundError` in this new test locally. I will take a look to see if I can figure out what's going on. - PR Review Comment: https://git.openjdk.org/jdk/pull/20373#discussion_r1706606412
Re: RFR: 8337331: crash: pinned virtual thread will lead to jvm crash when running with the javaagent option [v11]
On Tue, 6 Aug 2024 07:53:12 GMT, Jiawei Tang wrote: >>> Should I try to add @build jdk.test.lib.thread.VThreadPinner? >> >> Yes, please give that a try. > > It cannot solve the NoClassDefFoundError. Besides, this new testcase is not > included in linux-x86 / test (hs/tier1 serviceability). I see that the `NoClassDefFoundError` failure that you are mentioning is actually being reported even in the GitHub Actions job failures. It does look odd (although might be a pre-known jtreg issue). It will need a bit of investigation to see what's going on. - PR Review Comment: https://git.openjdk.org/jdk/pull/20373#discussion_r1705086644
Re: RFR: 8337331: crash: pinned virtual thread will lead to jvm crash when running with the javaagent option [v11]
On Tue, 6 Aug 2024 06:10:38 GMT, Jiawei Tang wrote: > Should I try to add @build jdk.test.lib.thread.VThreadPinner? Yes, please give that a try. - PR Review Comment: https://git.openjdk.org/jdk/pull/20373#discussion_r1704964895
Re: RFR: 8337331: crash: pinned virtual thread will lead to jvm crash when running with the javaagent option [v11]
On Mon, 5 Aug 2024 10:34:56 GMT, Jaikiran Pai wrote: >> Jiawei Tang has updated the pull request incrementally with one additional >> commit since the last revision: >> >> change the format of the testcase file > > test/hotspot/jtreg/serviceability/jvmti/vthread/TestPinCaseWithCFLH/TestPinCaseWithCFLH.java > line 37: > >> 35: * @modules java.base/java.lang:+open >> 36: * @compile TestPinCaseWithCFLH.java >> 37: * @run driver jdk.test.lib.util.JavaAgentBuilder > > Hello @jia-wei-tang, does adding an additional `@build > jdk.test.lib.thread.VThreadPinner` line before the `@run` line help fix the > `NoClassDefFoundError` you note in your comment? Hello @jia-wei-tang, I see that a `@build jdk.test.lib.util.JavaAgentBuilder` was added in the latest update to the PR. Did that help solve the NoClassDefFoundError you were running into when running those tests? I find it surprising that this specific class is required in the `@build` declaration, since that class itself is part of the `@run` directive. - PR Review Comment: https://git.openjdk.org/jdk/pull/20373#discussion_r1704897105
Re: RFR: 8337331: crash: pinned virtual thread will lead to jvm crash when running with the javaagent option [v11]
On Mon, 5 Aug 2024 07:37:11 GMT, Jiawei Tang wrote: >> I add the testcase which can reproduce the crash. I hope that I could get >> some advise if the codes need changing. > > Jiawei Tang has updated the pull request incrementally with one additional > commit since the last revision: > > change the format of the testcase file test/hotspot/jtreg/serviceability/jvmti/vthread/TestPinCaseWithCFLH/TestPinCaseWithCFLH.java line 37: > 35: * @modules java.base/java.lang:+open > 36: * @compile TestPinCaseWithCFLH.java > 37: * @run driver jdk.test.lib.util.JavaAgentBuilder Hello @jia-wei-tang, does adding an additional `@build jdk.test.lib.thread.VThreadPinner` line before the `@run` line help fix the `NoClassDefFoundError` you note in your comment? - PR Review Comment: https://git.openjdk.org/jdk/pull/20373#discussion_r1703911631
Re: RFR: 8336289: Obliterate most references to _snprintf in the Windows JDK [v3]
On Tue, 16 Jul 2024 08:59:20 GMT, Julian Waters wrote: >> snprintf has been available for all officially and unofficially supported >> compilers for Windows, Visual Studio since version 2015 and gcc since, well, >> forever. snprintf is conforming to C99 since the start when compiling using >> gcc, and 2015 when using Visual Studio. Since it conforms to C99 and >> provides better semantics than _snprintf, and is not compiler specific, we >> should use it in most places where we currently use _snprintf. This makes >> JDK code where this is used more robust due to the null terminating >> guarantee of snprintf. Since most of the changes are extremely simple, I've >> included all of them hoping to get this all done in one shot. The only >> places _snprintf is not replaced is places where I'm not sure whether the >> code is ours or not (As in, imported source code for external libraries). >> Note that the existing checks in place for the size of the characters >> written still work, as the return value of snprintf works mostly the same as >> _snprintf. The only difference is the nu ll terminating character at the end and the returning of the number of written characters if the buffer was terminated early, as seen here: https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/snprintf-snprintf-snprintf-l-snwprintf-snwprintf-l?view=msvc-170 >> >> Reviews Required: 2 for HotSpot, jdk.hotspot.agent, and jdk.jdwp.agent, 1 >> for awt and jdk.accessibility, 1 for java.base, 1 for security, 1 for >> jdk.management(?) > > Julian Waters has updated the pull request incrementally with one additional > commit since the last revision: > > Revert Standard Integer type rewrite Hello Julian, > Just need approval for java.base and security Daniel has approved this PR and I think that should take care of java.base review. In any case, I had a look at the java.base change and it looks OK to me. A several of these files will need a copyright year change. Could you update those files to use 2024? - Marked as reviewed by jpai (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/20153#pullrequestreview-2218070566
[jdk23] Integrated: 8334167: Test java/lang/instrument/NativeMethodPrefixApp.java timed out
On Thu, 25 Jul 2024 15:13:29 GMT, Jaikiran Pai wrote: > Can I please get a review of this test-only backport of the fix that we did > in master branch a few weeks back? > > This isn't a clean backport, there were trivial merge issues in the > `NativeMethodPrefixApp` test class, which I have resolved manually. The > merged content matches with what we have in master branch. The original fix > was done in https://github.com/openjdk/jdk/pull/20154 and we have seen this > test fail since it was integrated. > > Backporting this to jdk23 branch will provide test stability in that branch > where currently this test occasionally times out. This pull request has now been integrated. Changeset: 946c6cc6 Author:Jaikiran Pai URL: https://git.openjdk.org/jdk/commit/946c6cc6c77a16919e13a619a4372ac989c35e26 Stats: 144 lines in 3 files changed: 82 ins; 40 del; 22 mod 8334167: Test java/lang/instrument/NativeMethodPrefixApp.java timed out Reviewed-by: dholmes, sspitsyn Backport-of: 3babffd4002be62f9f75a1a773c9561804612fad - PR: https://git.openjdk.org/jdk/pull/20333
Re: [jdk23] RFR: 8334167: Test java/lang/instrument/NativeMethodPrefixApp.java timed out
On Thu, 25 Jul 2024 15:13:29 GMT, Jaikiran Pai wrote: > Can I please get a review of this test-only backport of the fix that we did > in master branch a few weeks back? > > This isn't a clean backport, there were trivial merge issues in the > `NativeMethodPrefixApp` test class, which I have resolved manually. The > merged content matches with what we have in master branch. The original fix > was done in https://github.com/openjdk/jdk/pull/20154 and we have seen this > test fail since it was integrated. > > Backporting this to jdk23 branch will provide test stability in that branch > where currently this test occasionally times out. Thank you Serguei. - PR Comment: https://git.openjdk.org/jdk/pull/20333#issuecomment-2260215530
Re: [jdk23] RFR: 8334167: Test java/lang/instrument/NativeMethodPrefixApp.java timed out
On Thu, 25 Jul 2024 15:13:29 GMT, Jaikiran Pai wrote: > Can I please get a review of this test-only backport of the fix that we did > in master branch a few weeks back? > > This isn't a clean backport, there were trivial merge issues in the > `NativeMethodPrefixApp` test class, which I have resolved manually. The > merged content matches with what we have in master branch. The original fix > was done in https://github.com/openjdk/jdk/pull/20154 and we have seen this > test fail since it was integrated. > > Backporting this to jdk23 branch will provide test stability in that branch > where currently this test occasionally times out. Thank you David for the review. Given that this is the serviceability area, can I get a second review please? - PR Comment: https://git.openjdk.org/jdk/pull/20333#issuecomment-2259632800
Re: [jdk23] RFR: 8334167: Test java/lang/instrument/NativeMethodPrefixApp.java timed out
On Thu, 25 Jul 2024 15:13:29 GMT, Jaikiran Pai wrote: > Can I please get a review of this test-only backport of the fix that we did > in master branch a few weeks back? > > This isn't a clean backport, there were trivial merge issues in the > `NativeMethodPrefixApp` test class, which I have resolved manually. The > merged content matches with what we have in master branch. The original fix > was done in https://github.com/openjdk/jdk/pull/20154 and we have seen this > test fail since it was integrated. > > Backporting this to jdk23 branch will provide test stability in that branch > where currently this test occasionally times out. Looking for reviews to backport this one, please. - PR Comment: https://git.openjdk.org/jdk/pull/20333#issuecomment-2257470071
[jdk23] RFR: 8334167: Test java/lang/instrument/NativeMethodPrefixApp.java timed out
Can I please get a review of this test-only backport of the fix that we did in master branch a few weeks back? This isn't a clean backport, there were trivial merge issues in the `NativeMethodPrefixApp` test class, which I have resolved manually. The merged content matches with what we have in master branch. The original fix was done in https://github.com/openjdk/jdk/pull/20154 and we have seen this test fail since it was integrated. Backporting this to jdk23 branch will provide test stability in that branch where currently this test occasionally times out. - Commit messages: - 8334167: Test java/lang/instrument/NativeMethodPrefixApp.java timed out Changes: https://git.openjdk.org/jdk/pull/20333/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=20333&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8334167 Stats: 144 lines in 3 files changed: 82 ins; 40 del; 22 mod Patch: https://git.openjdk.org/jdk/pull/20333.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20333/head:pull/20333 PR: https://git.openjdk.org/jdk/pull/20333
Re: RFR: 8334771: [TESTBUG] Run TestDockerMemoryMetrics.java with -Xcomp fails exitValue = 137
On Thu, 18 Jul 2024 11:42:17 GMT, SendaoYan wrote: >> Hi all, >> After [JDK-8294960](https://bugs.openjdk.org/browse/JDK-8294960), the >> footprint memory usage increased significantly when run the testcase with >> -Xcomp jvm options, then cause the testcase was killed by docker by OOM. >> Maybe the footprint memory usage increased was inevitable, so I think we >> should increase the smallest memory limite for this testcase. >> Only change the testcase, the change has been verified, no risk. > >> Unfortunately I'm not familiar with these tests. > >> After [JDK-8294960](https://bugs.openjdk.org/browse/JDK-8294960), the >> codecache usage increased significantly, non-profiled 3068Kb->3583Kb, >> profiled 6408Kb->7846Kb. > > Can you confirm that the codecache usage increased is expected or not after > [JDK-8294960](https://bugs.openjdk.org/browse/JDK-8294960) with -Xcomp jvm > option. @sendaoYan, Given Adam's inputs and the reviews you have had for this change, I think you should be able to go ahead and integrate this. - PR Comment: https://git.openjdk.org/jdk/pull/19864#issuecomment-2238072647
Re: RFR: 8336691: Test LongArgTest.java intermittent fails java.lang.NoClassDefFoundError: jdk/test/lib/Utils [v2]
On Thu, 18 Jul 2024 07:30:47 GMT, SendaoYan wrote: >> Hi all, >> The testcase `serviceability/attach/LongArgTest.java` intermittent fails >> `java.lang.NoClassDefFoundError: jdk/test/lib/Utils`. Jtreg doesn't >> automatically compile `jdk/test/lib/Utils.class` and >> `jdk/test/lib/apps/LingeredApp.class` etc.. Maybe it's a jtreg framework >> bug. >> I think it's necessory to compile `jdk.test.lib.Utils` and >> `jdk.test.lib.apps.LingeredApp` explicitly. >> Only change the testcase, the change has been verified, no risk. > > SendaoYan has updated the pull request incrementally with one additional > commit since the last revision: > > delete @build LongArgTest Hello Chen, > Does this mean that even if libraries are imported with `@library /test/lib` > they still need explicit `@build` commands? If that's the case, many core > library tests need review as well. Yes, jtreg does expect explicit `@build` for such library usages. It has this to say https://openjdk.org/jtreg/tag-spec.html: > In general, classes in library directories are not automatically compiled as > part of a compilation command explicitly naming the source files containing > those classes. A test that relies upon library classes should contain > appropriate @build directives to ensure that the classes will be compiled. It > is strongly recommended that tests do not rely on the use of implicit > compilation by the Java compiler. Such an approach is generally fragile, and > may lead to incomplete recompilation when a test or library code has been > modified. - PR Comment: https://git.openjdk.org/jdk/pull/20228#issuecomment-2236372312
Re: RFR: 8334771: [TESTBUG] Run TestDockerMemoryMetrics.java with -Xcomp fails exitValue = 137
On Mon, 24 Jun 2024 16:16:29 GMT, SendaoYan wrote: > After [JDK-8294960](https://bugs.openjdk.org/browse/JDK-8294960), the > footprint memory usage increased significantly when run the testcase with > -Xcomp jvm options, then cause the testcase was killed by docker by OOM. Maybe the footprint memory usage increased was inevitable, so I think we should increase the smallest memory limite for this testcase. Hello @sendaoYan, after changes in JDK-8294960, there were a couple of issues reported. From what I see in the linked issues, Adam reviewed those and integrated relevant fixes. In JDK-8334771 you note: > After [JDK-8294960](https://bugs.openjdk.org/browse/JDK-8294960), the > codecache usage increased significantly, non-profiled 3068Kb->3583Kb, > profiled 6408Kb->7846Kb. So I think we should have this increase in memory reviewed by @asotona or someone familiar in that area, before deciding whether these tests should be changed. - PR Comment: https://git.openjdk.org/jdk/pull/19864#issuecomment-2235607869
Re: RFR: 8336691: Test LongArgTest.java intermittent fails java.lang.NoClassDefFoundError: jdk/test/lib/Utils
On Thu, 18 Jul 2024 01:49:54 GMT, SendaoYan wrote: > Hi all, > The testcase `serviceability/attach/LongArgTest.java` intermittent fails > `java.lang.NoClassDefFoundError: jdk/test/lib/Utils`. Jtreg doesn't > automatically compile `jdk/test/lib/Utils.class` and > `jdk/test/lib/apps/LingeredApp.class` etc.. Maybe it's a jtreg framework bug. > I think it's necessory to compile `jdk.test.lib.Utils` and > `jdk.test.lib.apps.LingeredApp` explicitly. > Only change the testcase, the change has been verified, no risk. test/hotspot/jtreg/serviceability/attach/LongArgTest.java line 30: > 28: * @library /test/lib > 29: * @modules jdk.attach/sun.tools.attach > 30: * @build jdk.test.lib.Utils jdk.test.lib.apps.LingeredApp LongArgTest Hello @sendaoYan, the test class `LongArgTest` isn't needed in the `@build` set. - PR Review Comment: https://git.openjdk.org/jdk/pull/20228#discussion_r1682116429
Integrated: 8334167: Test java/lang/instrument/NativeMethodPrefixApp.java timed out
On Fri, 12 Jul 2024 09:22:54 GMT, Jaikiran Pai wrote: > Can I please get a review of this test-only change which proposes to fix the > test timeout reported in https://bugs.openjdk.org/browse/JDK-8334167? > > The JBS issue has comments which explains what causes the timeout. The commit > in this PR addresses those issues by updating the test specific > `ClassFileTransformer` to only instrument application specific class instead > of all (core) classes. The test was introduced several years back to verify > the feature introduced in https://bugs.openjdk.org/browse/JDK-6263319. As > such, the proposed changes in this PR continue to test that feature - we now > merely use application specific class' native method to verify the semantics > of that feature. > > Additional cleanups have been done in the test to make sure that if any > exception does occur in the ClassFileTransformer then it does get recorded > and that then causes the test to fail. > > With this change, I have run tier1 through tier6 and the test passes. > Additionally, without this change I've run the test with a test repeat of 100 > with virtual threads enabled and the test hangs occasionally (as expected). > With this proposed fix, I have then run the test with virtual threads, around > 300 times and it hasn't failed or hung in any of those instances. This pull request has now been integrated. Changeset: 3babffd4 Author:Jaikiran Pai URL: https://git.openjdk.org/jdk/commit/3babffd4002be62f9f75a1a773c9561804612fad Stats: 151 lines in 3 files changed: 80 ins; 47 del; 24 mod 8334167: Test java/lang/instrument/NativeMethodPrefixApp.java timed out Reviewed-by: dholmes, sspitsyn, alanb - PR: https://git.openjdk.org/jdk/pull/20154
Re: RFR: 8334167: Test java/lang/instrument/NativeMethodPrefixApp.java timed out [v4]
On Mon, 15 Jul 2024 10:21:23 GMT, Jaikiran Pai wrote: >> Can I please get a review of this test-only change which proposes to fix the >> test timeout reported in https://bugs.openjdk.org/browse/JDK-8334167? >> >> The JBS issue has comments which explains what causes the timeout. The >> commit in this PR addresses those issues by updating the test specific >> `ClassFileTransformer` to only instrument application specific class instead >> of all (core) classes. The test was introduced several years back to verify >> the feature introduced in https://bugs.openjdk.org/browse/JDK-6263319. As >> such, the proposed changes in this PR continue to test that feature - we now >> merely use application specific class' native method to verify the semantics >> of that feature. >> >> Additional cleanups have been done in the test to make sure that if any >> exception does occur in the ClassFileTransformer then it does get recorded >> and that then causes the test to fail. >> >> With this change, I have run tier1 through tier6 and the test passes. >> Additionally, without this change I've run the test with a test repeat of >> 100 with virtual threads enabled and the test hangs occasionally (as >> expected). With this proposed fix, I have then run the test with virtual >> threads, around 300 times and it hasn't failed or hung in any of those >> instances. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > update code comment in test Thank you all for the reviews and inputs. I'll go ahead with the integration in a few hours from now, once the CI test jobs I trigger for this, complete. - PR Comment: https://git.openjdk.org/jdk/pull/20154#issuecomment-2229945315
Re: RFR: 8334167: Test java/lang/instrument/NativeMethodPrefixApp.java timed out [v3]
On Mon, 15 Jul 2024 10:05:24 GMT, David Holmes wrote: >> Jaikiran Pai has updated the pull request incrementally with one additional >> commit since the last revision: >> >> better formatting for transform() method of the test's agent > > test/jdk/java/lang/instrument/NativeMethodPrefixApp.java line 50: > >> 48: // It assumes that a specific non-native library method will call a >> specific >> 49: // native method. The below may need to be updated based on library >> changes. >> 50: static String goldenNativeMethodName = "fooBarNativeMethod"; > > The comment block above seems no longer applicable now this is not a library > class method. I've now updated the code comment to note what that constant is for. - PR Review Comment: https://git.openjdk.org/jdk/pull/20154#discussion_r1677605240
Re: RFR: 8334167: Test java/lang/instrument/NativeMethodPrefixApp.java timed out [v4]
> Can I please get a review of this test-only change which proposes to fix the > test timeout reported in https://bugs.openjdk.org/browse/JDK-8334167? > > The JBS issue has comments which explains what causes the timeout. The commit > in this PR addresses those issues by updating the test specific > `ClassFileTransformer` to only instrument application specific class instead > of all (core) classes. The test was introduced several years back to verify > the feature introduced in https://bugs.openjdk.org/browse/JDK-6263319. As > such, the proposed changes in this PR continue to test that feature - we now > merely use application specific class' native method to verify the semantics > of that feature. > > Additional cleanups have been done in the test to make sure that if any > exception does occur in the ClassFileTransformer then it does get recorded > and that then causes the test to fail. > > With this change, I have run tier1 through tier6 and the test passes. > Additionally, without this change I've run the test with a test repeat of 100 > with virtual threads enabled and the test hangs occasionally (as expected). > With this proposed fix, I have then run the test with virtual threads, around > 300 times and it hasn't failed or hung in any of those instances. Jaikiran Pai has updated the pull request incrementally with one additional commit since the last revision: update code comment in test - Changes: - all: https://git.openjdk.org/jdk/pull/20154/files - new: https://git.openjdk.org/jdk/pull/20154/files/1d303c90..7105565d Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=20154&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20154&range=02-03 Stats: 3 lines in 1 file changed: 0 ins; 1 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/20154.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20154/head:pull/20154 PR: https://git.openjdk.org/jdk/pull/20154
Re: RFR: 8334167: Test java/lang/instrument/NativeMethodPrefixApp.java timed out
On Mon, 15 Jul 2024 08:36:01 GMT, Alan Bateman wrote: > Would it be possible to at least re-format the declaration of the "transform" > method as it's very messy and hard to see what is declaration vs. body. Done. - PR Comment: https://git.openjdk.org/jdk/pull/20154#issuecomment-2228024140
Re: RFR: 8334167: Test java/lang/instrument/NativeMethodPrefixApp.java timed out [v3]
> Can I please get a review of this test-only change which proposes to fix the > test timeout reported in https://bugs.openjdk.org/browse/JDK-8334167? > > The JBS issue has comments which explains what causes the timeout. The commit > in this PR addresses those issues by updating the test specific > `ClassFileTransformer` to only instrument application specific class instead > of all (core) classes. The test was introduced several years back to verify > the feature introduced in https://bugs.openjdk.org/browse/JDK-6263319. As > such, the proposed changes in this PR continue to test that feature - we now > merely use application specific class' native method to verify the semantics > of that feature. > > Additional cleanups have been done in the test to make sure that if any > exception does occur in the ClassFileTransformer then it does get recorded > and that then causes the test to fail. > > With this change, I have run tier1 through tier6 and the test passes. > Additionally, without this change I've run the test with a test repeat of 100 > with virtual threads enabled and the test hangs occasionally (as expected). > With this proposed fix, I have then run the test with virtual threads, around > 300 times and it hasn't failed or hung in any of those instances. Jaikiran Pai has updated the pull request incrementally with one additional commit since the last revision: better formatting for transform() method of the test's agent - Changes: - all: https://git.openjdk.org/jdk/pull/20154/files - new: https://git.openjdk.org/jdk/pull/20154/files/fa36314b..1d303c90 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=20154&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20154&range=01-02 Stats: 7 lines in 1 file changed: 0 ins; 4 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/20154.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20154/head:pull/20154 PR: https://git.openjdk.org/jdk/pull/20154
Re: RFR: 8334167: Test java/lang/instrument/NativeMethodPrefixApp.java timed out [v2]
On Mon, 15 Jul 2024 07:00:12 GMT, Alan Bateman wrote: >> Jaikiran Pai has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains five additional >> commits since the last revision: >> >> - David's suggestion - remove cosmetic/style changes >> - no need to timeout=240 >> - include stdio.h in test native library >> - merge latest from master branch >> - 8334167: Test java/lang/instrument/NativeMethodPrefixApp.java timed out > > test/jdk/java/lang/instrument/NativeMethodPrefixApp.java line 44: > >> 42: * @comment The test uses asmlib/Instrumentor.java which relies on >> ClassFile API PreviewFeature. >> 43: * @run main/native/timeout=240 NativeMethodPrefixApp roleDriver >> 44: * @comment The test uses a higher timeout to prevent test timeouts >> noted in JDK-6528548 > > Is /timeout=240 (and the comment) needed now. If I read the old issue > correctly it was due to use a JDK mounted on a network file system. In https://github.com/openjdk/jdk/pull/19495#discussion_r1625470816 we had considered removing the higher timeout from this test. But then decided to let it stay at that time. I think we don't need it anymore, not even when the test runs with `-Xcomp`. The recent runs in our CI for this test haven't shown the necessity of using this higher timeout. I have updated the PR to remove this. I plan to run additional tests in our CI to be sure that the removal of the timeout doesn't cause unexpected failures. > test/jdk/java/lang/instrument/libNativeMethodPrefix.c line 24: > >> 22: */ >> 23: >> 24: #include "jni.h" > > I assume this needs `#include `. Updated the PR to include this. - PR Review Comment: https://git.openjdk.org/jdk/pull/20154#discussion_r1677457539 PR Review Comment: https://git.openjdk.org/jdk/pull/20154#discussion_r1677454337
Re: RFR: 8334167: Test java/lang/instrument/NativeMethodPrefixApp.java timed out
On Fri, 12 Jul 2024 09:22:54 GMT, Jaikiran Pai wrote: > Can I please get a review of this test-only change which proposes to fix the > test timeout reported in https://bugs.openjdk.org/browse/JDK-8334167? > > The JBS issue has comments which explains what causes the timeout. The commit > in this PR addresses those issues by updating the test specific > `ClassFileTransformer` to only instrument application specific class instead > of all (core) classes. The test was introduced several years back to verify > the feature introduced in https://bugs.openjdk.org/browse/JDK-6263319. As > such, the proposed changes in this PR continue to test that feature - we now > merely use application specific class' native method to verify the semantics > of that feature. > > Additional cleanups have been done in the test to make sure that if any > exception does occur in the ClassFileTransformer then it does get recorded > and that then causes the test to fail. > > With this change, I have run tier1 through tier6 and the test passes. > Additionally, without this change I've run the test with a test repeat of 100 > with virtual threads enabled and the test hangs occasionally (as expected). > With this proposed fix, I have then run the test with virtual threads, around > 300 times and it hasn't failed or hung in any of those instances. Hello David, > @jaikiran there is a lot of unrelated refactoring and style changes here that > obscures what the actual fix is. I've updated the PR to undo some of the cosmetic changes that were introduced to cleanup the code a bit. The updated state of the PR now just includes functional updates to the test to address the issue noted in the linked JBS. - PR Comment: https://git.openjdk.org/jdk/pull/20154#issuecomment-2227935588
Re: RFR: 8334167: Test java/lang/instrument/NativeMethodPrefixApp.java timed out [v2]
> Can I please get a review of this test-only change which proposes to fix the > test timeout reported in https://bugs.openjdk.org/browse/JDK-8334167? > > The JBS issue has comments which explains what causes the timeout. The commit > in this PR addresses those issues by updating the test specific > `ClassFileTransformer` to only instrument application specific class instead > of all (core) classes. The test was introduced several years back to verify > the feature introduced in https://bugs.openjdk.org/browse/JDK-6263319. As > such, the proposed changes in this PR continue to test that feature - we now > merely use application specific class' native method to verify the semantics > of that feature. > > Additional cleanups have been done in the test to make sure that if any > exception does occur in the ClassFileTransformer then it does get recorded > and that then causes the test to fail. > > With this change, I have run tier1 through tier6 and the test passes. > Additionally, without this change I've run the test with a test repeat of 100 > with virtual threads enabled and the test hangs occasionally (as expected). > With this proposed fix, I have then run the test with virtual threads, around > 300 times and it hasn't failed or hung in any of those instances. Jaikiran Pai has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains five additional commits since the last revision: - David's suggestion - remove cosmetic/style changes - no need to timeout=240 - include stdio.h in test native library - merge latest from master branch - 8334167: Test java/lang/instrument/NativeMethodPrefixApp.java timed out - Changes: - all: https://git.openjdk.org/jdk/pull/20154/files - new: https://git.openjdk.org/jdk/pull/20154/files/a16f5cdf..fa36314b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=20154&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20154&range=00-01 Stats: 1599 lines in 104 files changed: 688 ins; 537 del; 374 mod Patch: https://git.openjdk.org/jdk/pull/20154.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20154/head:pull/20154 PR: https://git.openjdk.org/jdk/pull/20154
RFR: 8334167: Test java/lang/instrument/NativeMethodPrefixApp.java timed out
Can I please get a review of this test-only change which proposes to fix the test timeout reported in https://bugs.openjdk.org/browse/JDK-8334167? The JBS issue has comments which explains what causes the timeout. The commit in this PR addresses those issues by updating the test specific `ClassFileTransformer` to only instrument application specific class instead of all (core) classes. The test was introduced several years back to verify the feature introduced in https://bugs.openjdk.org/browse/JDK-6263319. As such, the proposed changes in this PR continue to test that feature - we now merely use application specific class' native method to verify the semantics of that feature. Additional cleanups have been done in the test to make sure that if any exception does occur in the ClassFileTransformer then it does get recorded and that then causes the test to fail. With this change, I have run tier1 through tier6 and the test passes. Additionally, without this change I've run the test with a test repeat of 100 with virtual threads enabled and the test hangs occasionally (as expected). With this proposed fix, I have then run the test with virtual threads, around 300 times and it hasn't failed or hung in any of those instances. - Commit messages: - 8334167: Test java/lang/instrument/NativeMethodPrefixApp.java timed out Changes: https://git.openjdk.org/jdk/pull/20154/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=20154&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8334167 Stats: 152 lines in 3 files changed: 74 ins; 37 del; 41 mod Patch: https://git.openjdk.org/jdk/pull/20154.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20154/head:pull/20154 PR: https://git.openjdk.org/jdk/pull/20154
[jdk23] Integrated: 8335966: Remove incorrect problem listing of java/lang/instrument/NativeMethodPrefixAgent.java in ProblemList-Virtual.txt
On Thu, 11 Jul 2024 03:45:38 GMT, Jaikiran Pai wrote: > I would like to backport this test-only change into `jdk23`. This cleans up > the incorrect problem listing that was caused by a change that we introduced > in JDK 23 as part of https://bugs.openjdk.org/browse/JDK-8333130. > > This is a clean backport of the commit that was integrated into master branch > through https://github.com/openjdk/jdk/pull/20094. This pull request has now been integrated. Changeset: 6720685a Author:Jaikiran Pai URL: https://git.openjdk.org/jdk/commit/6720685abdc1c91f5083255782810cdb2d7d72f4 Stats: 2 lines in 1 file changed: 0 ins; 2 del; 0 mod 8335966: Remove incorrect problem listing of java/lang/instrument/NativeMethodPrefixAgent.java in ProblemList-Virtual.txt Reviewed-by: kevinw Backport-of: dcf4e0d51f392afe2711223484e932e3826e8864 - PR: https://git.openjdk.org/jdk/pull/20132
Re: [jdk23] RFR: 8335966: Remove incorrect problem listing of java/lang/instrument/NativeMethodPrefixAgent.java in ProblemList-Virtual.txt
On Thu, 11 Jul 2024 03:45:38 GMT, Jaikiran Pai wrote: > I would like to backport this test-only change into `jdk23`. This cleans up > the incorrect problem listing that was caused by a change that we introduced > in JDK 23 as part of https://bugs.openjdk.org/browse/JDK-8333130. > > This is a clean backport of the commit that was integrated into master branch > through https://github.com/openjdk/jdk/pull/20094. Thank you Kevin for the review. - PR Comment: https://git.openjdk.org/jdk/pull/20132#issuecomment-320049
[jdk23] RFR: 8335966: Remove incorrect problem listing of java/lang/instrument/NativeMethodPrefixAgent.java in ProblemList-Virtual.txt
I would like to backport this test-only change into `jdk23`. This cleans up the incorrect problem listing that was caused by a change that we introduced in JDK 23 as part of https://bugs.openjdk.org/browse/JDK-8333130. This is a clean backport of the commit that was integrated into master branch through https://github.com/openjdk/jdk/pull/20094. - Commit messages: - Backport dcf4e0d51f392afe2711223484e932e3826e8864 Changes: https://git.openjdk.org/jdk/pull/20132/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=20132&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8335966 Stats: 2 lines in 1 file changed: 0 ins; 2 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/20132.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20132/head:pull/20132 PR: https://git.openjdk.org/jdk/pull/20132
Integrated: 8335966: Remove incorrect problem listing of java/lang/instrument/NativeMethodPrefixAgent.java in ProblemList-Virtual.txt
On Tue, 9 Jul 2024 08:39:39 GMT, Jaikiran Pai wrote: > Can I please get a review of this change which removes an incorrect entry > from `ProblemList-Virtual.txt`? > > As noted in https://bugs.openjdk.org/browse/JDK-8335966 this entry is a left > over which went unnoticed when we did the changes in > https://bugs.openjdk.org/browse/JDK-8333130. > > The refactored test `NativeMethodPrefixApp` has been functioning without the > errors for which `NativeMethodPrefixAgent` was previously problem listed. So > I haven't replaced that problem list entry and instead have just removed it. > > `NativeMethodPrefixApp` is known to run into an intermittent timeout with > virtual threads and that's tracked in > https://bugs.openjdk.org/browse/JDK-8334167 and I think it's not problem > listing worthy given the relatively low frequency of such timeouts. > Furthermore, in the coming days, I plan to propose a change to this > `NativeMethodPrefixApp` test which should fix the timeout failures. > > To verify that this removal doesn't cause any unexpected issues, I've > triggered a CI run of the relevant tier. This pull request has now been integrated. Changeset: dcf4e0d5 Author:Jaikiran Pai URL: https://git.openjdk.org/jdk/commit/dcf4e0d51f392afe2711223484e932e3826e8864 Stats: 2 lines in 1 file changed: 0 ins; 2 del; 0 mod 8335966: Remove incorrect problem listing of java/lang/instrument/NativeMethodPrefixAgent.java in ProblemList-Virtual.txt Reviewed-by: kevinw, amenkov - PR: https://git.openjdk.org/jdk/pull/20094
Re: RFR: 8335966: Remove incorrect problem listing of java/lang/instrument/NativeMethodPrefixAgent.java in ProblemList-Virtual.txt
On Tue, 9 Jul 2024 08:39:39 GMT, Jaikiran Pai wrote: > Can I please get a review of this change which removes an incorrect entry > from `ProblemList-Virtual.txt`? > > As noted in https://bugs.openjdk.org/browse/JDK-8335966 this entry is a left > over which went unnoticed when we did the changes in > https://bugs.openjdk.org/browse/JDK-8333130. > > The refactored test `NativeMethodPrefixApp` has been functioning without the > errors for which `NativeMethodPrefixAgent` was previously problem listed. So > I haven't replaced that problem list entry and instead have just removed it. > > `NativeMethodPrefixApp` is known to run into an intermittent timeout with > virtual threads and that's tracked in > https://bugs.openjdk.org/browse/JDK-8334167 and I think it's not problem > listing worthy given the relatively low frequency of such timeouts. > Furthermore, in the coming days, I plan to propose a change to this > `NativeMethodPrefixApp` test which should fix the timeout failures. > > To verify that this removal doesn't cause any unexpected issues, I've > triggered a CI run of the relevant tier. Thank you Kevin and Alex for the reviews. tier5, where this problem listing file is in use, completed without issues. I will go ahead and integrate this in the next few hours. - PR Comment: https://git.openjdk.org/jdk/pull/20094#issuecomment-2219305134
RFR: 8335966: Remove incorrect problem listing of java/lang/instrument/NativeMethodPrefixAgent.java in ProblemList-Virtual.txt
Can I please get a review of this change which removes an incorrect entry from `ProblemList-Virtual.txt`? As noted in https://bugs.openjdk.org/browse/JDK-8335966 this entry is a left over which went unnoticed when we did the changes in https://bugs.openjdk.org/browse/JDK-8333130. The refactored test `NativeMethodPrefixApp` has been functioning without the errors for which `NativeMethodPrefixAgent` was previously problem listed. So I haven't replaced that problem list entry and instead have just removed it. `NativeMethodPrefixApp` is known to run into an intermittent timeout with virtual threads and that's tracked in https://bugs.openjdk.org/browse/JDK-8334167 and I think it's not problem listing worthy given the relatively low frequency of such timeouts. Furthermore, in the coming days, I plan to propose a change to this `NativeMethodPrefixApp` test which should fix the timeout failures. To verify that this removal doesn't cause any unexpected issues, I've triggered a CI run of the relevant tier. - Commit messages: - 8335966: Remove incorrect problem listing of java/lang/instrument/NativeMethodPrefixAgent.java in ProblemList-Virtual.txt Changes: https://git.openjdk.org/jdk/pull/20094/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=20094&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8335966 Stats: 2 lines in 1 file changed: 0 ins; 2 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/20094.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20094/head:pull/20094 PR: https://git.openjdk.org/jdk/pull/20094
Integrated: 8333756: java/lang/instrument/NativeMethodPrefixApp.java failed due to missing intrinsic
On Fri, 7 Jun 2024 10:31:22 GMT, Jaikiran Pai wrote:
> Can I please get a review of this test-only change which fixes an issue that
> was introduced due to the refactoring that we did in
> https://bugs.openjdk.org/browse/JDK-8333130? This PR addresses the failure
> reported in https://bugs.openjdk.org/browse/JDK-8333756.
>
> The `NativeMethodPrefixApp` test uses a `javaagent` `NativeMethodPrefixAgent`
> which modifies the name of the native methods using the
> `java.lang.instrument.Instrumentation` instance:
>
> public static void premain (String agentArgs, Instrumentation instArg) {
> inst = instArg;
> System.out.println("Premain");
>
> ...
> instArg.setNativeMethodPrefix(t0, "wrapped_tr0_");
> instArg.setNativeMethodPrefix(t1, "wrapped_tr1_");
> instArg.setNativeMethodPrefix(t2, "wrapped_tr2_");
>
>
> The Hotspot VM allows for methods on a class to be annotated with an (VM
> internal) `jdk.internal.vm.annotation.IntrinsicCandidate` annotation. When a
> class that contains any methods that are annotated with `@IntrinsicCandidate`
> is loaded, the VM checks that the corresponding method(s) have an intrinsic
> available. It uses the method name to check for the presence of the
> intrinsic. In the absence of an intrinsic for a `@IntrinsicCandidate` method,
> the VM throws an error and exits. This behaviour is controlled by the
> `-XX:+/-CheckIntrinsics` option. By default that option is enabled, implying
> that an error will be thrown if the intrinsic isn't found.
>
> In the case where/when this test fails, it so happens that the JVM loads a
> class which has a `@IntrinsicCandidate` on a `native` Java method. For
> example, on the failing host, I could see this class loading sequence:
>
>
> tr2: Loading java/util/Date
> tr1: Loading java/util/Date
> tr0: Loading java/util/Date
> tr2: Loading sun/util/calendar/CalendarSystem
> tr1: Loading sun/util/calendar/CalendarSystem
> tr0: Loading sun/util/calendar/CalendarSystem
> tr2: Loading sun/util/calendar/CalendarSystem$GregorianHolder
> tr1: Loading sun/util/calendar/CalendarSystem$GregorianHolder
> tr0: Loading sun/util/calendar/CalendarSystem$GregorianHolder
> ...
> tr2: Loading sun/util/calendar/ZoneInfoFile$Checksum
> tr1: Loading sun/util/calendar/ZoneInfoFile$Checksum
> tr0: Loading sun/util/calendar/ZoneInfoFile$Checksum
> tr2: Loading java/util/zip/Checksum
> tr1: Loading java/util/zip/Checksum
> tr0: Loading java/util/zip/Checksum
> tr2: Loading java/util/zip/CRC32
> tr1: Loading java/util/zip/CRC32
> tr0: Loading java/util/zip/CRC32
> Method [java.util.zip.CRC32.wrapped_tr2_update(II)I] is annotated with
> @IntrinsicCandidate, but no...
This pull request has now been integrated.
Changeset: 41c88bc3
Author:Jaikiran Pai
URL:
https://git.openjdk.org/jdk/commit/41c88bc395ab8c927bcafca9dc6c8a77de72dfc7
Stats: 9 lines in 1 file changed: 9 ins; 0 del; 0 mod
8333756: java/lang/instrument/NativeMethodPrefixApp.java failed due to missing
intrinsic
Reviewed-by: amenkov, cjplummer
-
PR: https://git.openjdk.org/jdk/pull/19595
Re: RFR: 8333756: java/lang/instrument/NativeMethodPrefixApp.java failed due to missing intrinsic [v2]
On Tue, 11 Jun 2024 02:10:46 GMT, Jaikiran Pai wrote:
>> Can I please get a review of this test-only change which fixes an issue that
>> was introduced due to the refactoring that we did in
>> https://bugs.openjdk.org/browse/JDK-8333130? This PR addresses the failure
>> reported in https://bugs.openjdk.org/browse/JDK-8333756.
>>
>> The `NativeMethodPrefixApp` test uses a `javaagent`
>> `NativeMethodPrefixAgent` which modifies the name of the native methods
>> using the `java.lang.instrument.Instrumentation` instance:
>>
>> public static void premain (String agentArgs, Instrumentation instArg) {
>> inst = instArg;
>> System.out.println("Premain");
>>
>> ...
>> instArg.setNativeMethodPrefix(t0, "wrapped_tr0_");
>> instArg.setNativeMethodPrefix(t1, "wrapped_tr1_");
>> instArg.setNativeMethodPrefix(t2, "wrapped_tr2_");
>>
>>
>> The Hotspot VM allows for methods on a class to be annotated with an (VM
>> internal) `jdk.internal.vm.annotation.IntrinsicCandidate` annotation. When a
>> class that contains any methods that are annotated with
>> `@IntrinsicCandidate` is loaded, the VM checks that the corresponding
>> method(s) have an intrinsic available. It uses the method name to check for
>> the presence of the intrinsic. In the absence of an intrinsic for a
>> `@IntrinsicCandidate` method, the VM throws an error and exits. This
>> behaviour is controlled by the `-XX:+/-CheckIntrinsics` option. By default
>> that option is enabled, implying that an error will be thrown if the
>> intrinsic isn't found.
>>
>> In the case where/when this test fails, it so happens that the JVM loads a
>> class which has a `@IntrinsicCandidate` on a `native` Java method. For
>> example, on the failing host, I could see this class loading sequence:
>>
>>
>> tr2: Loading java/util/Date
>> tr1: Loading java/util/Date
>> tr0: Loading java/util/Date
>> tr2: Loading sun/util/calendar/CalendarSystem
>> tr1: Loading sun/util/calendar/CalendarSystem
>> tr0: Loading sun/util/calendar/CalendarSystem
>> tr2: Loading sun/util/calendar/CalendarSystem$GregorianHolder
>> tr1: Loading sun/util/calendar/CalendarSystem$GregorianHolder
>> tr0: Loading sun/util/calendar/CalendarSystem$GregorianHolder
>> ...
>> tr2: Loading sun/util/calendar/ZoneInfoFile$Checksum
>> tr1: Loading sun/util/calendar/ZoneInfoFile$Checksum
>> tr0: Loading sun/util/calendar/ZoneInfoFile$Checksum
>> tr2: Loading java/util/zip/Checksum
>> tr1: Loading java/util/zip/Checksum
>> tr0: Loading java/util/zip/Checksum
>> tr2: Loading java/util/zip/CRC32
>> tr1: Loading java/util/zip/CRC32
>> tr0: Loading java/util/zip/CRC32
>> Method [java.util.zi...
>
> Jaikiran Pai has updated the pull request incrementally with one additional
> commit since the last revision:
>
> update code comment
Thank you for the reviews, Alex and Chris.
-
PR Comment: https://git.openjdk.org/jdk/pull/19595#issuecomment-2159725610
Re: RFR: 8333756: java/lang/instrument/NativeMethodPrefixApp.java failed due to missing intrinsic [v2]
On Mon, 10 Jun 2024 19:05:36 GMT, Chris Plummer wrote: >> Jaikiran Pai has updated the pull request incrementally with one additional >> commit since the last revision: >> >> update code comment > > test/jdk/java/lang/instrument/NativeMethodPrefixApp.java line 103: > >> 101: // the native method names, which then causes a failure >> in the VM check >> 102: // for the presence of an intrinsic on a >> @IntrinsicCandidate native method >> 103: "-XX:+UnlockDiagnosticVMOptions", >> "-XX:-CheckIntrinsics", > > Can you update both comments to begin with a capital letter and end with a > period. Thanks. Done, I've updated the PR with this change. - PR Review Comment: https://git.openjdk.org/jdk/pull/19595#discussion_r1634061698
Re: RFR: 8333756: java/lang/instrument/NativeMethodPrefixApp.java failed due to missing intrinsic [v2]
> Can I please get a review of this test-only change which fixes an issue that
> was introduced due to the refactoring that we did in
> https://bugs.openjdk.org/browse/JDK-8333130? This PR addresses the failure
> reported in https://bugs.openjdk.org/browse/JDK-8333756.
>
> The `NativeMethodPrefixApp` test uses a `javaagent` `NativeMethodPrefixAgent`
> which modifies the name of the native methods using the
> `java.lang.instrument.Instrumentation` instance:
>
> public static void premain (String agentArgs, Instrumentation instArg) {
> inst = instArg;
> System.out.println("Premain");
>
> ...
> instArg.setNativeMethodPrefix(t0, "wrapped_tr0_");
> instArg.setNativeMethodPrefix(t1, "wrapped_tr1_");
> instArg.setNativeMethodPrefix(t2, "wrapped_tr2_");
>
>
> The Hotspot VM allows for methods on a class to be annotated with an (VM
> internal) `jdk.internal.vm.annotation.IntrinsicCandidate` annotation. When a
> class that contains any methods that are annotated with `@IntrinsicCandidate`
> is loaded, the VM checks that the corresponding method(s) have an intrinsic
> available. It uses the method name to check for the presence of the
> intrinsic. In the absence of an intrinsic for a `@IntrinsicCandidate` method,
> the VM throws an error and exits. This behaviour is controlled by the
> `-XX:+/-CheckIntrinsics` option. By default that option is enabled, implying
> that an error will be thrown if the intrinsic isn't found.
>
> In the case where/when this test fails, it so happens that the JVM loads a
> class which has a `@IntrinsicCandidate` on a `native` Java method. For
> example, on the failing host, I could see this class loading sequence:
>
>
> tr2: Loading java/util/Date
> tr1: Loading java/util/Date
> tr0: Loading java/util/Date
> tr2: Loading sun/util/calendar/CalendarSystem
> tr1: Loading sun/util/calendar/CalendarSystem
> tr0: Loading sun/util/calendar/CalendarSystem
> tr2: Loading sun/util/calendar/CalendarSystem$GregorianHolder
> tr1: Loading sun/util/calendar/CalendarSystem$GregorianHolder
> tr0: Loading sun/util/calendar/CalendarSystem$GregorianHolder
> ...
> tr2: Loading sun/util/calendar/ZoneInfoFile$Checksum
> tr1: Loading sun/util/calendar/ZoneInfoFile$Checksum
> tr0: Loading sun/util/calendar/ZoneInfoFile$Checksum
> tr2: Loading java/util/zip/Checksum
> tr1: Loading java/util/zip/Checksum
> tr0: Loading java/util/zip/Checksum
> tr2: Loading java/util/zip/CRC32
> tr1: Loading java/util/zip/CRC32
> tr0: Loading java/util/zip/CRC32
> Method [java.util.zip.CRC32.wrapped_tr2_update(II)I] is annotated with
> @IntrinsicCandidate, but no...
Jaikiran Pai has updated the pull request incrementally with one additional
commit since the last revision:
update code comment
-
Changes:
- all: https://git.openjdk.org/jdk/pull/19595/files
- new: https://git.openjdk.org/jdk/pull/19595/files/95a939a1..17eb4421
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=19595&range=01
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=19595&range=00-01
Stats: 4 lines in 1 file changed: 0 ins; 0 del; 4 mod
Patch: https://git.openjdk.org/jdk/pull/19595.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/19595/head:pull/19595
PR: https://git.openjdk.org/jdk/pull/19595
Re: RFR: 8333756: java/lang/instrument/NativeMethodPrefixApp.java failed due to missing intrinsic
On Fri, 7 Jun 2024 10:31:22 GMT, Jaikiran Pai wrote:
> Can I please get a review of this test-only change which fixes an issue that
> was introduced due to the refactoring that we did in
> https://bugs.openjdk.org/browse/JDK-8333130? This PR addresses the failure
> reported in https://bugs.openjdk.org/browse/JDK-8333756.
>
> The `NativeMethodPrefixApp` test uses a `javaagent` `NativeMethodPrefixAgent`
> which modifies the name of the native methods using the
> `java.lang.instrument.Instrumentation` instance:
>
> public static void premain (String agentArgs, Instrumentation instArg) {
> inst = instArg;
> System.out.println("Premain");
>
> ...
> instArg.setNativeMethodPrefix(t0, "wrapped_tr0_");
> instArg.setNativeMethodPrefix(t1, "wrapped_tr1_");
> instArg.setNativeMethodPrefix(t2, "wrapped_tr2_");
>
>
> The Hotspot VM allows for methods on a class to be annotated with an (VM
> internal) `jdk.internal.vm.annotation.IntrinsicCandidate` annotation. When a
> class that contains any methods that are annotated with `@IntrinsicCandidate`
> is loaded, the VM checks that the corresponding method(s) have an intrinsic
> available. It uses the method name to check for the presence of the
> intrinsic. In the absence of an intrinsic for a `@IntrinsicCandidate` method,
> the VM throws an error and exits. This behaviour is controlled by the
> `-XX:+/-CheckIntrinsics` option. By default that option is enabled, implying
> that an error will be thrown if the intrinsic isn't found.
>
> In the case where/when this test fails, it so happens that the JVM loads a
> class which has a `@IntrinsicCandidate` on a `native` Java method. For
> example, on the failing host, I could see this class loading sequence:
>
>
> tr2: Loading java/util/Date
> tr1: Loading java/util/Date
> tr0: Loading java/util/Date
> tr2: Loading sun/util/calendar/CalendarSystem
> tr1: Loading sun/util/calendar/CalendarSystem
> tr0: Loading sun/util/calendar/CalendarSystem
> tr2: Loading sun/util/calendar/CalendarSystem$GregorianHolder
> tr1: Loading sun/util/calendar/CalendarSystem$GregorianHolder
> tr0: Loading sun/util/calendar/CalendarSystem$GregorianHolder
> ...
> tr2: Loading sun/util/calendar/ZoneInfoFile$Checksum
> tr1: Loading sun/util/calendar/ZoneInfoFile$Checksum
> tr0: Loading sun/util/calendar/ZoneInfoFile$Checksum
> tr2: Loading java/util/zip/Checksum
> tr1: Loading java/util/zip/Checksum
> tr0: Loading java/util/zip/Checksum
> tr2: Loading java/util/zip/CRC32
> tr1: Loading java/util/zip/CRC32
> tr0: Loading java/util/zip/CRC32
> Method [java.util.zip.CRC32.wrapped_tr2_update(II)I] is annotated with
> @IntrinsicCandidate, but no...
Looking for a second review of this PR, please.
-
PR Comment: https://git.openjdk.org/jdk/pull/19595#issuecomment-2157319670
Re: RFR: 8333813: Seviceability tests fail due to stderr containing Temporarily processing option UseNotificationThread
On Sat, 8 Jun 2024 07:17:55 GMT, Chris Plummer wrote:
>> Hello Chris, given these similary named methods in this class, perhaps we
>> should instead just have one single `stderrShouldBeEmptyIgnoring(...)`
>> method which takes the messages that should be ignored. Something like:
>>
>>
>> public static final String VM_WARNING_MSG = ".* VM warning:.*";
>>
>> public static final String VM_WARNING_DEPRECATED_MSG = ".* VM warning:.*
>> deprecated.*";
>> ...
>>
>> public OutputAnalyzer stderrShouldBeEmptyIgnoring(String ignoreMsg,
>> String... additionalIgnoreMsgs) {
>> String stdErrContent = getStderr().replaceAll(ignoreMsg + "\\R", "");
>> if (additionalIgnoreMsgs != null) {
>> for (String additionalIgnore : additionalIgnoreMsgs) {
>> stdErrContent = stdErrContent.replaceAll(additionalIgnore +
>> "\\R", "");
>> }
>> }
>> if (!stdErrContent.isEmpty()) {
>> reportDiagnosticSummary();
>> throw new RuntimeException("stderr was not empty");
>> }
>> return this;
>> }
>>
>>
>> We make those private fields in OutputAnalyzer public and have the caller
>> pass them:
>>
>>
>> oa.stderrShouldBeEmptyIgnoring(OutputAnalyzer.VM_WARNING_DEPRECATED_MSG)
>
> That seems like a lot of busy work and over abstraction for trying to solve a
> rather trivial issue that already has a very simple solution, but maybe could
> use a better API name.
In that case, would `stderrShouldBeEmptyIgnoreVMOptionDeprecations` be OK?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19606#discussion_r1631938967
Re: RFR: 8333813: Seviceability tests fail due to stderr containing Temporarily processing option UseNotificationThread
On Sat, 8 Jun 2024 06:53:21 GMT, Chris Plummer wrote:
>> test/lib/jdk/test/lib/process/OutputAnalyzer.java line 187:
>>
>>> 185: * If stderr was not empty
>>> 186: */
>>> 187: public OutputAnalyzer
>>> stderrShouldBeEmptyIgnoreDeprecatedWarnings() {
>>
>> Maybe this should be renamed to reflect that it is about VM option warnings.
>
> Possibly. It means updating 15 tests. Also need to come up with a new name.
> Any suggestions?
Hello Chris, given these similary named methods in this class, perhaps we
should instead just have one single `stderrShouldBeEmptyIgnoring(...)` method
which takes the messages that should be ignored. Something like:
public static final String VM_WARNING_MSG = ".* VM warning:.*";
public static final String VM_WARNING_DEPRECATED_MSG = ".* VM warning:.*
deprecated.*";
...
public OutputAnalyzer stderrShouldBeEmptyIgnoring(String ignoreMsg, String...
additionalIgnoreMsgs) {
String stdErrContent = getStderr().replaceAll(ignoreMsg + "\\R", "");
if (additionalIgnoreMsgs != null) {
for (String additionalIgnore : additionalIgnoreMsgs) {
stdErrContent = getStderr().replaceAll(additionalIgnore +
"\\R", "");
}
}
if (!stdErrContent.isEmpty()) {
reportDiagnosticSummary();
throw new RuntimeException("stderr was not empty");
}
return this;
}
We make those private fields in OutputAnalyzer public and have the caller pass
them:
oa.stderrShouldBeEmptyIgnoring(OutputAnalyzer.VM_WARNING_DEPRECATED_MSG)
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19606#discussion_r1631923540
Re: RFR: 8333756: java/lang/instrument/NativeMethodPrefixApp.java failed due to missing intrinsic
On Fri, 7 Jun 2024 10:31:22 GMT, Jaikiran Pai wrote:
> Can I please get a review of this test-only change which fixes an issue that
> was introduced due to the refactoring that we did in
> https://bugs.openjdk.org/browse/JDK-8333130? This PR addresses the failure
> reported in https://bugs.openjdk.org/browse/JDK-8333756.
>
> The `NativeMethodPrefixApp` test uses a `javaagent` `NativeMethodPrefixAgent`
> which modifies the name of the native methods using the
> `java.lang.instrument.Instrumentation` instance:
>
> public static void premain (String agentArgs, Instrumentation instArg) {
> inst = instArg;
> System.out.println("Premain");
>
> ...
> instArg.setNativeMethodPrefix(t0, "wrapped_tr0_");
> instArg.setNativeMethodPrefix(t1, "wrapped_tr1_");
> instArg.setNativeMethodPrefix(t2, "wrapped_tr2_");
>
>
> The Hotspot VM allows for methods on a class to be annotated with an (VM
> internal) `jdk.internal.vm.annotation.IntrinsicCandidate` annotation. When a
> class that contains any methods that are annotated with `@IntrinsicCandidate`
> is loaded, the VM checks that the corresponding method(s) have an intrinsic
> available. It uses the method name to check for the presence of the
> intrinsic. In the absence of an intrinsic for a `@IntrinsicCandidate` method,
> the VM throws an error and exits. This behaviour is controlled by the
> `-XX:+/-CheckIntrinsics` option. By default that option is enabled, implying
> that an error will be thrown if the intrinsic isn't found.
>
> In the case where/when this test fails, it so happens that the JVM loads a
> class which has a `@IntrinsicCandidate` on a `native` Java method. For
> example, on the failing host, I could see this class loading sequence:
>
>
> tr2: Loading java/util/Date
> tr1: Loading java/util/Date
> tr0: Loading java/util/Date
> tr2: Loading sun/util/calendar/CalendarSystem
> tr1: Loading sun/util/calendar/CalendarSystem
> tr0: Loading sun/util/calendar/CalendarSystem
> tr2: Loading sun/util/calendar/CalendarSystem$GregorianHolder
> tr1: Loading sun/util/calendar/CalendarSystem$GregorianHolder
> tr0: Loading sun/util/calendar/CalendarSystem$GregorianHolder
> ...
> tr2: Loading sun/util/calendar/ZoneInfoFile$Checksum
> tr1: Loading sun/util/calendar/ZoneInfoFile$Checksum
> tr0: Loading sun/util/calendar/ZoneInfoFile$Checksum
> tr2: Loading java/util/zip/Checksum
> tr1: Loading java/util/zip/Checksum
> tr0: Loading java/util/zip/Checksum
> tr2: Loading java/util/zip/CRC32
> tr1: Loading java/util/zip/CRC32
> tr0: Loading java/util/zip/CRC32
> Method [java.util.zip.CRC32.wrapped_tr2_update(II)I] is annotated with
> @IntrinsicCandidate, but no...
tier1 through tier6 testing completed without related issues with this change.
-
PR Comment: https://git.openjdk.org/jdk/pull/19595#issuecomment-2155833037
RFR: 8333756: java/lang/instrument/NativeMethodPrefixApp.java failed due to missing intrinsic
Can I please get a review of this test-only change which fixes an issue that
was introduced due to the refactoring that we did in
https://bugs.openjdk.org/browse/JDK-8333130? This PR addresses the failure
reported in https://bugs.openjdk.org/browse/JDK-8333756.
The `NativeMethodPrefixApp` test uses a `javaagent` `NativeMethodPrefixAgent`
which modifies the name of the native methods using the
`java.lang.instrument.Instrumentation` instance:
public static void premain (String agentArgs, Instrumentation instArg) {
inst = instArg;
System.out.println("Premain");
...
instArg.setNativeMethodPrefix(t0, "wrapped_tr0_");
instArg.setNativeMethodPrefix(t1, "wrapped_tr1_");
instArg.setNativeMethodPrefix(t2, "wrapped_tr2_");
The Hotspot VM allows for methods on a class to be annotated with an (VM
internal) `jdk.internal.vm.annotation.IntrinsicCandidate` annotation. When a
class that contains any methods that are annotated with `@IntrinsicCandidate`
is loaded, the VM checks that the corresponding method(s) have an intrinsic
available. It uses the method name to check for the presence of the intrinsic.
In the absence of an intrinsic for a `@IntrinsicCandidate` method, the VM
throws an error and exits. This behaviour is controlled by the
`-XX:+/-CheckIntrinsics` option. By default that option is enabled, implying
that an error will be thrown if the intrinsic isn't found.
In the case where/when this test fails, it so happens that the JVM loads a
class which has a `@IntrinsicCandidate` on a `native` Java method. For example,
on the failing host, I could see this class loading sequence:
tr2: Loading java/util/Date
tr1: Loading java/util/Date
tr0: Loading java/util/Date
tr2: Loading sun/util/calendar/CalendarSystem
tr1: Loading sun/util/calendar/CalendarSystem
tr0: Loading sun/util/calendar/CalendarSystem
tr2: Loading sun/util/calendar/CalendarSystem$GregorianHolder
tr1: Loading sun/util/calendar/CalendarSystem$GregorianHolder
tr0: Loading sun/util/calendar/CalendarSystem$GregorianHolder
...
tr2: Loading sun/util/calendar/ZoneInfoFile$Checksum
tr1: Loading sun/util/calendar/ZoneInfoFile$Checksum
tr0: Loading sun/util/calendar/ZoneInfoFile$Checksum
tr2: Loading java/util/zip/Checksum
tr1: Loading java/util/zip/Checksum
tr0: Loading java/util/zip/Checksum
tr2: Loading java/util/zip/CRC32
tr1: Loading java/util/zip/CRC32
tr0: Loading java/util/zip/CRC32
Method [java.util.zip.CRC32.wrapped_tr2_update(II)I] is annotated with
@IntrinsicCandidate, but no compiler intrinsic is defined for the method.
Exiting.
So a class loading sequence lead to loading of the `java.util.zip.CRC32` class
which has a `native` method annotated with `@IntrinsicCandidate`:
@IntrinsicCandidate
private static native int update(int crc, int b);
Since the `NativeMethodPrefixAgent` modifies the native method names, the VM
thus cannot find a matching intrinsic and thus fails with that error.
The reason why this wasn't uncovered during the testing of changes for
https://bugs.openjdk.org/browse/JDK-8333130 is purely incidental. Even now, the
test fails only intermittently. That's because it's only triggered if some
class with a `native` method with `@IntrinsicCandidate` gets loadded. As an
additional reference, I also found https://bugs.openjdk.org/browse/JDK-8151100
which is when the `-XX:-CheckIntrinsics` was introduced in this test and that
too explains this same issue.
The commit in this PR fixes the issue by reintroducing the
`"-XX:+UnlockDiagnosticVMOptions", "-XX:-CheckIntrinsics"` when launching the
test program. Additionally, the test program now intentionally loads the
`CRC32` class to make the test more deterministic for cases like these and to
prevent any accidental removal of the `-XX:-CheckIntrinsics` in future.
tier testing is currently in progress with this change in our CI.
-
Commit messages:
- 8333756: java/lang/instrument/NativeMethodPrefixApp.java failed due to
missing intrinsic
Changes: https://git.openjdk.org/jdk/pull/19595/files
Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=19595&range=00
Issue: https://bugs.openjdk.org/browse/JDK-8333756
Stats: 9 lines in 1 file changed: 9 ins; 0 del; 0 mod
Patch: https://git.openjdk.org/jdk/pull/19595.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/19595/head:pull/19595
PR: https://git.openjdk.org/jdk/pull/19595
Re: RFR: 8333130: MakeJAR2.sh uses hard-coded JDK version [v3]
On Tue, 4 Jun 2024 01:30:55 GMT, Jaikiran Pai wrote: >> test/jdk/java/lang/instrument/RetransformApp.java line 81: >> >>> 79: final OutputAnalyzer oa = ProcessTools.executeTestJava( >>> 80: "--enable-preview", // due to usage of ClassFile API >>> PreviewFeature in the agent >>> 81: "-XX:+UnlockDiagnosticVMOptions", >>> "-XX:-CheckIntrinsics", >> >> This `-XX` flags were not present for this test (and I don't think they are >> needed for NativeMethodPrefix test too) > > You are right - it was a copy/paste error on my part to have include these > flags for the `RetransformApp`. Based on your suggestion, I have removed > them from the other test too. The tests continue to pass. It turns out we do need the `-XX:-CheckIntrinsics` for the `NativeMethodPrefix` test. Without that we have a test failure in a higher tier https://bugs.openjdk.org/browse/JDK-8333756. - PR Review Comment: https://git.openjdk.org/jdk/pull/19495#discussion_r1630482492
Integrated: 8333130: MakeJAR2.sh uses hard-coded JDK version
On Fri, 31 May 2024 09:01:27 GMT, Jaikiran Pai wrote: > Can I please get a review of this test-only change which addresses > https://bugs.openjdk.org/browse/JDK-8333130? > > There are a couple of tests `NativeMethodPrefixApp` and `RetransformApp` > under `test/jdk/java/lang/instrument/` which launch the app/test with a > `-javaagent:` pointing to a test specific jar. The test, launched with > that `javaagent`, then verifies the test specific details. > > In their current form, in order to construct the agent `.jar` and make it > available to the jtreg test that's launched, they `@run` a jtreg `shell` > test. This `shell` test uses the `MakeJAR2.sh` script to generate the jar > file. The contents of the script is relatively straightforward - it compiles > (using `javac`) some boot classpath classes, some agent specific classes and > then generates a jar file with the agent classes and a `MANIFEST.MF` which > points to the boot classpath classes along with test specific manifest > attributes. In a recent PR the `MakeJAR2.sh` script was updated to pass > `--enable-preview --release 23` since one of the existing agent class was > updated to use the ClassFile API PreviewFeature > (https://github.com/openjdk/jdk/pull/13009). > > This generated agent jar then is passed as `-javaagent:` to the subsequent > `@run main` test which does the actual testing. > > So essentially the `shell` test which uses the `MakeJAR2.sh` is merely there > to create a jar file that's needed by the actual test. Within the JDK we have > several tests which compile other classes and generate jar files using > in-process test libraries and the `java.util.spi.ToolProvider` > implementations. These 2 tests too can be updated to use a similar technique, > to completely get rid of the `MakeJAR2.sh`. Removing the `MakeJAR2.sh` will > simplify the ability to pass around value for `--release` when using > `--enable-preview`. > > The commit in this PR updates these 2 tests to use `@run driver` which then > compiles relevant classes (maintaining the semantics of `MakeJAR2.sh`) and > generates the agent jar file and then finally launching the test process. As > part of the update, I did not retain the `@author` tag from the 2 test > definitions, since it's my understanding that we no longer use those. I will > add them back if we should retain them. > > The tests continue to pass locally with this change. I've also triggered tier > testing in our CI for this change. This pull request has now been integrated. Changeset: 4369856c Author:Jaikiran Pai URL: https://git.openjdk.org/jdk/commit/4369856c6dbef15b5d73aa1da07216f372a01294 Stats: 263 lines in 5 files changed: 118 ins; 131 del; 14 mod 8333130: MakeJAR2.sh uses hard-coded JDK version Reviewed-by: lancea, darcy, sspitsyn, amenkov - PR: https://git.openjdk.org/jdk/pull/19495
Re: RFR: 8333130: MakeJAR2.sh uses hard-coded JDK version [v6]
On Tue, 4 Jun 2024 18:49:03 GMT, Alex Menkov wrote: >> I have now updated the PR to use the same timeout that was used for the >> `shell` test before the changes in this PR. > > I think `timeout`s are not needed for the refactored tests. > Per JDK-6528548 the shell action has timed out using a network mounted JDK > (MakeJAR2.sh run `javac` 3 times and `jar` 1 time). > But I don't see big problem here - up to the author to keep or remove them. Hello Alex, I agree that the timeout may not be needed. I haven't run the higher tiers in our CI where `-Xcomp` and other options might slow tests down in general. So, given Serguei's input, I'll let the timeout stay for now and once we see how this behaves in higher tiers, then we can remove the timeout in a subsequent PR if necessary. - PR Review Comment: https://git.openjdk.org/jdk/pull/19495#discussion_r1626802777
Re: RFR: 8333130: MakeJAR2.sh uses hard-coded JDK version [v6]
On Tue, 4 Jun 2024 12:35:33 GMT, Jaikiran Pai wrote: >> Can I please get a review of this test-only change which addresses >> https://bugs.openjdk.org/browse/JDK-8333130? >> >> There are a couple of tests `NativeMethodPrefixApp` and `RetransformApp` >> under `test/jdk/java/lang/instrument/` which launch the app/test with a >> `-javaagent:` pointing to a test specific jar. The test, launched with >> that `javaagent`, then verifies the test specific details. >> >> In their current form, in order to construct the agent `.jar` and make it >> available to the jtreg test that's launched, they `@run` a jtreg `shell` >> test. This `shell` test uses the `MakeJAR2.sh` script to generate the jar >> file. The contents of the script is relatively straightforward - it compiles >> (using `javac`) some boot classpath classes, some agent specific classes and >> then generates a jar file with the agent classes and a `MANIFEST.MF` which >> points to the boot classpath classes along with test specific manifest >> attributes. In a recent PR the `MakeJAR2.sh` script was updated to pass >> `--enable-preview --release 23` since one of the existing agent class was >> updated to use the ClassFile API PreviewFeature >> (https://github.com/openjdk/jdk/pull/13009). >> >> This generated agent jar then is passed as `-javaagent:` to the subsequent >> `@run main` test which does the actual testing. >> >> So essentially the `shell` test which uses the `MakeJAR2.sh` is merely there >> to create a jar file that's needed by the actual test. Within the JDK we >> have several tests which compile other classes and generate jar files using >> in-process test libraries and the `java.util.spi.ToolProvider` >> implementations. These 2 tests too can be updated to use a similar >> technique, to completely get rid of the `MakeJAR2.sh`. Removing the >> `MakeJAR2.sh` will simplify the ability to pass around value for `--release` >> when using `--enable-preview`. >> >> The commit in this PR updates these 2 tests to use `@run driver` which then >> compiles relevant classes (maintaining the semantics of `MakeJAR2.sh`) and >> generates the agent jar file and then finally launching the test process. As >> part of the update, I did not retain the `@author` tag from the 2 test >> definitions, since it's my understanding that we no longer use those. I will >> add them back if we should retain them. >> >> The tests continue to pass locally with this change. I've also triggered >> tier testing in our CI for this change. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > Serguei's suggestion - use a higher timeout for the test to match what was > being used for the shell test Thank you everyone for the help in refactoring this test and the reviews. I will go ahead and integrate this today in the next few hours. - PR Comment: https://git.openjdk.org/jdk/pull/19495#issuecomment-2148684740
Re: RFR: 8333130: MakeJAR2.sh uses hard-coded JDK version [v6]
On Tue, 4 Jun 2024 11:47:25 GMT, Serguei Spitsyn wrote: >> Hello Serguei, that `timeout=240` appears to have been added as part of >> https://bugs.openjdk.org/browse/JDK-6528548 to several of these tests back >> in Java 7 days because the `shell` test which was creating the jar file was >> timing out. The actual test itself, the one which uses the generated jar to >> run the agent, was completing within a second or two. >> >> With the current state of the PR I have run tier3 in our CI and there too >> both these tests complete within a second on all platforms. So I decided not >> to copy over this timeout as part of this change. > > My guess is these tests were timed out when executed with some specific > options like -Xint, -Xcomp or any other that can slow down the execution > (e.g. DeoptimizeALot). I have now updated the PR to use the same timeout that was used for the `shell` test before the changes in this PR. - PR Review Comment: https://git.openjdk.org/jdk/pull/19495#discussion_r1625919837
Re: RFR: 8333130: MakeJAR2.sh uses hard-coded JDK version [v6]
> Can I please get a review of this test-only change which addresses > https://bugs.openjdk.org/browse/JDK-8333130? > > There are a couple of tests `NativeMethodPrefixApp` and `RetransformApp` > under `test/jdk/java/lang/instrument/` which launch the app/test with a > `-javaagent:` pointing to a test specific jar. The test, launched with > that `javaagent`, then verifies the test specific details. > > In their current form, in order to construct the agent `.jar` and make it > available to the jtreg test that's launched, they `@run` a jtreg `shell` > test. This `shell` test uses the `MakeJAR2.sh` script to generate the jar > file. The contents of the script is relatively straightforward - it compiles > (using `javac`) some boot classpath classes, some agent specific classes and > then generates a jar file with the agent classes and a `MANIFEST.MF` which > points to the boot classpath classes along with test specific manifest > attributes. In a recent PR the `MakeJAR2.sh` script was updated to pass > `--enable-preview --release 23` since one of the existing agent class was > updated to use the ClassFile API PreviewFeature > (https://github.com/openjdk/jdk/pull/13009). > > This generated agent jar then is passed as `-javaagent:` to the subsequent > `@run main` test which does the actual testing. > > So essentially the `shell` test which uses the `MakeJAR2.sh` is merely there > to create a jar file that's needed by the actual test. Within the JDK we have > several tests which compile other classes and generate jar files using > in-process test libraries and the `java.util.spi.ToolProvider` > implementations. These 2 tests too can be updated to use a similar technique, > to completely get rid of the `MakeJAR2.sh`. Removing the `MakeJAR2.sh` will > simplify the ability to pass around value for `--release` when using > `--enable-preview`. > > The commit in this PR updates these 2 tests to use `@run driver` which then > compiles relevant classes (maintaining the semantics of `MakeJAR2.sh`) and > generates the agent jar file and then finally launching the test process. As > part of the update, I did not retain the `@author` tag from the 2 test > definitions, since it's my understanding that we no longer use those. I will > add them back if we should retain them. > > The tests continue to pass locally with this change. I've also triggered tier > testing in our CI for this change. Jaikiran Pai has updated the pull request incrementally with one additional commit since the last revision: Serguei's suggestion - use a higher timeout for the test to match what was being used for the shell test - Changes: - all: https://git.openjdk.org/jdk/pull/19495/files - new: https://git.openjdk.org/jdk/pull/19495/files/51c20928..aae8b0ce Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19495&range=05 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19495&range=04-05 Stats: 4 lines in 2 files changed: 2 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/19495.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19495/head:pull/19495 PR: https://git.openjdk.org/jdk/pull/19495
Re: RFR: 8333130: MakeJAR2.sh uses hard-coded JDK version [v5]
On Tue, 4 Jun 2024 07:12:27 GMT, Serguei Spitsyn wrote: >> Jaikiran Pai has updated the pull request incrementally with one additional >> commit since the last revision: >> >> unused import > > test/jdk/java/lang/instrument/NativeMethodPrefixAgent.java line 34: > >> 32: * @modules java.management >> 33: * java.instrument >> 34: * @run shell/timeout=240 MakeJAR2.sh NativeMethodPrefixAgent >> NativeMethodPrefixApp 'Can-Retransform-Classes: true' >> 'Can-Set-Native-Method-Prefix: true' > > Just a question. The original test had a timeout:`timeout=240`. My guess is > that the default timeout was not enough. Do you think, it is not needed > anymore or you just missed to add it? > The same question applies to the other test. Hello Serguei, that `timeout=240` appears to have been added as part of https://bugs.openjdk.org/browse/JDK-6528548 to several of these tests back in Java 7 days because the `shell` test which was creating the jar file was timing out. The actual test itself, the one which uses the generated jar to run the agent, was completing within a second or two. With the current state of the PR I have run tier3 in our CI and there too both these tests complete within a second on all platforms. So I decided not to copy over this timeout as part of this change. - PR Review Comment: https://git.openjdk.org/jdk/pull/19495#discussion_r1625628639
Re: RFR: 8333130: MakeJAR2.sh uses hard-coded JDK version [v4]
On Tue, 4 Jun 2024 07:13:39 GMT, Serguei Spitsyn wrote: >> Jaikiran Pai has updated the pull request incrementally with three >> additional commits since the last revision: >> >> - Alex suggestion - remove -XX:-CheckIntrinsics from NativeMethodPrefixApp >> test too >> - Alex's suggestion - no need to include only the bootreporter classes in >> boot classpath >> - RetransformApp test did not previously use -XX:-CheckIntrinsics > > test/jdk/java/lang/instrument/NativeMethodPrefixApp.java line 26: > >> 24: >> 25: import java.io.File; >> 26: import java.nio.file.Files; > > Nit: It seems the import at line 26 is not needed. Fixed. - PR Review Comment: https://git.openjdk.org/jdk/pull/19495#discussion_r1625617051
Re: RFR: 8333130: MakeJAR2.sh uses hard-coded JDK version [v5]
> Can I please get a review of this test-only change which addresses > https://bugs.openjdk.org/browse/JDK-8333130? > > There are a couple of tests `NativeMethodPrefixApp` and `RetransformApp` > under `test/jdk/java/lang/instrument/` which launch the app/test with a > `-javaagent:` pointing to a test specific jar. The test, launched with > that `javaagent`, then verifies the test specific details. > > In their current form, in order to construct the agent `.jar` and make it > available to the jtreg test that's launched, they `@run` a jtreg `shell` > test. This `shell` test uses the `MakeJAR2.sh` script to generate the jar > file. The contents of the script is relatively straightforward - it compiles > (using `javac`) some boot classpath classes, some agent specific classes and > then generates a jar file with the agent classes and a `MANIFEST.MF` which > points to the boot classpath classes along with test specific manifest > attributes. In a recent PR the `MakeJAR2.sh` script was updated to pass > `--enable-preview --release 23` since one of the existing agent class was > updated to use the ClassFile API PreviewFeature > (https://github.com/openjdk/jdk/pull/13009). > > This generated agent jar then is passed as `-javaagent:` to the subsequent > `@run main` test which does the actual testing. > > So essentially the `shell` test which uses the `MakeJAR2.sh` is merely there > to create a jar file that's needed by the actual test. Within the JDK we have > several tests which compile other classes and generate jar files using > in-process test libraries and the `java.util.spi.ToolProvider` > implementations. These 2 tests too can be updated to use a similar technique, > to completely get rid of the `MakeJAR2.sh`. Removing the `MakeJAR2.sh` will > simplify the ability to pass around value for `--release` when using > `--enable-preview`. > > The commit in this PR updates these 2 tests to use `@run driver` which then > compiles relevant classes (maintaining the semantics of `MakeJAR2.sh`) and > generates the agent jar file and then finally launching the test process. As > part of the update, I did not retain the `@author` tag from the 2 test > definitions, since it's my understanding that we no longer use those. I will > add them back if we should retain them. > > The tests continue to pass locally with this change. I've also triggered tier > testing in our CI for this change. Jaikiran Pai has updated the pull request incrementally with one additional commit since the last revision: unused import - Changes: - all: https://git.openjdk.org/jdk/pull/19495/files - new: https://git.openjdk.org/jdk/pull/19495/files/3342dda7..51c20928 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19495&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19495&range=03-04 Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/19495.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19495/head:pull/19495 PR: https://git.openjdk.org/jdk/pull/19495
Re: RFR: 8333130: MakeJAR2.sh uses hard-coded JDK version [v4]
> Can I please get a review of this test-only change which addresses > https://bugs.openjdk.org/browse/JDK-8333130? > > There are a couple of tests `NativeMethodPrefixApp` and `RetransformApp` > under `test/jdk/java/lang/instrument/` which launch the app/test with a > `-javaagent:` pointing to a test specific jar. The test, launched with > that `javaagent`, then verifies the test specific details. > > In their current form, in order to construct the agent `.jar` and make it > available to the jtreg test that's launched, they `@run` a jtreg `shell` > test. This `shell` test uses the `MakeJAR2.sh` script to generate the jar > file. The contents of the script is relatively straightforward - it compiles > (using `javac`) some boot classpath classes, some agent specific classes and > then generates a jar file with the agent classes and a `MANIFEST.MF` which > points to the boot classpath classes along with test specific manifest > attributes. In a recent PR the `MakeJAR2.sh` script was updated to pass > `--enable-preview --release 23` since one of the existing agent class was > updated to use the ClassFile API PreviewFeature > (https://github.com/openjdk/jdk/pull/13009). > > This generated agent jar then is passed as `-javaagent:` to the subsequent > `@run main` test which does the actual testing. > > So essentially the `shell` test which uses the `MakeJAR2.sh` is merely there > to create a jar file that's needed by the actual test. Within the JDK we have > several tests which compile other classes and generate jar files using > in-process test libraries and the `java.util.spi.ToolProvider` > implementations. These 2 tests too can be updated to use a similar technique, > to completely get rid of the `MakeJAR2.sh`. Removing the `MakeJAR2.sh` will > simplify the ability to pass around value for `--release` when using > `--enable-preview`. > > The commit in this PR updates these 2 tests to use `@run driver` which then > compiles relevant classes (maintaining the semantics of `MakeJAR2.sh`) and > generates the agent jar file and then finally launching the test process. As > part of the update, I did not retain the `@author` tag from the 2 test > definitions, since it's my understanding that we no longer use those. I will > add them back if we should retain them. > > The tests continue to pass locally with this change. I've also triggered tier > testing in our CI for this change. Jaikiran Pai has updated the pull request incrementally with three additional commits since the last revision: - Alex suggestion - remove -XX:-CheckIntrinsics from NativeMethodPrefixApp test too - Alex's suggestion - no need to include only the bootreporter classes in boot classpath - RetransformApp test did not previously use -XX:-CheckIntrinsics - Changes: - all: https://git.openjdk.org/jdk/pull/19495/files - new: https://git.openjdk.org/jdk/pull/19495/files/3c7512b2..3342dda7 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19495&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19495&range=02-03 Stats: 23 lines in 2 files changed: 0 ins; 21 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/19495.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19495/head:pull/19495 PR: https://git.openjdk.org/jdk/pull/19495
Re: RFR: 8333130: MakeJAR2.sh uses hard-coded JDK version [v3]
On Mon, 3 Jun 2024 20:24:49 GMT, Alex Menkov wrote:
>> Jaikiran Pai has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Alex's input - simplify the test by using ClassFileInstaller
>
> test/jdk/java/lang/instrument/NativeMethodPrefixApp.java line 86:
>
>> 84: Can-Set-Native-Method-Prefix: true
>> 85: """
>> 86: + "Boot-Class-Path: " +
>> bootClassesDir.toString().replace(File.separatorChar, '/') + "/"
>
> AFAIU Boot-Class-Path is needed only because the agent needs
> bootreporter.StringIdCallbackReporter
> I don't see much point in separate bootreporter dir.
> So we can set Boot-Class-Path to System.getProperty("test.classes") and drop
> createBootClassesDir() logic
Hello Alex, I have updated the PR to add "test.classes" to Boot-Class-Path
instead of just those 2 classes. Tests in the updated PR continue to pass.
> test/jdk/java/lang/instrument/RetransformApp.java line 81:
>
>> 79: final OutputAnalyzer oa = ProcessTools.executeTestJava(
>> 80: "--enable-preview", // due to usage of ClassFile API
>> PreviewFeature in the agent
>> 81: "-XX:+UnlockDiagnosticVMOptions", "-XX:-CheckIntrinsics",
>
> This `-XX` flags were not present for this test (and I don't think they are
> needed for NativeMethodPrefix test too)
You are right - it was a copy/paste error on my part to have include these
flags for the `RetransformApp`. Based on your suggestion, I have removed them
from the other test too. The tests continue to pass.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19495#discussion_r1625219101
PR Review Comment: https://git.openjdk.org/jdk/pull/19495#discussion_r1625219542
Re: RFR: 8333130: MakeJAR2.sh uses hard-coded JDK version [v2]
On Fri, 31 May 2024 17:33:47 GMT, Leonid Mesnik wrote:
>> Jaikiran Pai has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> fix Boot-Class-Path value for Windows
>
> test/jdk/java/lang/instrument/NativeMethodPrefixApp.java line 168:
>
>> 166:
>> 167: private static void launchApp(final Path agentJar) throws Exception
>> {
>> 168: final OutputAnalyzer oa = ProcessTools.executeLimitedTestJava(
>
> Is there any reason the test uses 'executeLimitedTestJava' and not
> 'executeTestJava'?
> It might make sense to run it with different VM flags.
Hello Leonid, I had intentionally used `executeLimitedTestJava()` to avoid
passing along any additional options when launching that test application jar
because it wasn't clear to me if passing those along will be of any use.
Especially, options like `-Xcomp` which might be used when launching jtreg.
That was the sole reason.
Having said that, I have now updated the tests to use `executeTestJava()` to
allow them to pass along these options to the jar being launched, since prior
to this change, the test would have anyway ended up using those additional
jtreg launch specific options.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19495#discussion_r1623170660
Re: RFR: 8333130: MakeJAR2.sh uses hard-coded JDK version [v2]
On Fri, 31 May 2024 12:01:14 GMT, Jaikiran Pai wrote: >> Can I please get a review of this test-only change which addresses >> https://bugs.openjdk.org/browse/JDK-8333130? >> >> There are a couple of tests `NativeMethodPrefixApp` and `RetransformApp` >> under `test/jdk/java/lang/instrument/` which launch the app/test with a >> `-javaagent:` pointing to a test specific jar. The test, launched with >> that `javaagent`, then verifies the test specific details. >> >> In their current form, in order to construct the agent `.jar` and make it >> available to the jtreg test that's launched, they `@run` a jtreg `shell` >> test. This `shell` test uses the `MakeJAR2.sh` script to generate the jar >> file. The contents of the script is relatively straightforward - it compiles >> (using `javac`) some boot classpath classes, some agent specific classes and >> then generates a jar file with the agent classes and a `MANIFEST.MF` which >> points to the boot classpath classes along with test specific manifest >> attributes. In a recent PR the `MakeJAR2.sh` script was updated to pass >> `--enable-preview --release 23` since one of the existing agent class was >> updated to use the ClassFile API PreviewFeature >> (https://github.com/openjdk/jdk/pull/13009). >> >> This generated agent jar then is passed as `-javaagent:` to the subsequent >> `@run main` test which does the actual testing. >> >> So essentially the `shell` test which uses the `MakeJAR2.sh` is merely there >> to create a jar file that's needed by the actual test. Within the JDK we >> have several tests which compile other classes and generate jar files using >> in-process test libraries and the `java.util.spi.ToolProvider` >> implementations. These 2 tests too can be updated to use a similar >> technique, to completely get rid of the `MakeJAR2.sh`. Removing the >> `MakeJAR2.sh` will simplify the ability to pass around value for `--release` >> when using `--enable-preview`. >> >> The commit in this PR updates these 2 tests to use `@run driver` which then >> compiles relevant classes (maintaining the semantics of `MakeJAR2.sh`) and >> generates the agent jar file and then finally launching the test process. As >> part of the update, I did not retain the `@author` tag from the 2 test >> definitions, since it's my understanding that we no longer use those. I will >> add them back if we should retain them. >> >> The tests continue to pass locally with this change. I've also triggered >> tier testing in our CI for this change. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > fix Boot-Class-Path value for Windows Alex's input to use `ClassFileInstaller` test utility helped. I've now updated the PR to simplify the jar creation. The updated tests continue to pass with these changes. - PR Comment: https://git.openjdk.org/jdk/pull/19495#issuecomment-2143343918
Re: RFR: 8333130: MakeJAR2.sh uses hard-coded JDK version [v3]
> Can I please get a review of this test-only change which addresses > https://bugs.openjdk.org/browse/JDK-8333130? > > There are a couple of tests `NativeMethodPrefixApp` and `RetransformApp` > under `test/jdk/java/lang/instrument/` which launch the app/test with a > `-javaagent:` pointing to a test specific jar. The test, launched with > that `javaagent`, then verifies the test specific details. > > In their current form, in order to construct the agent `.jar` and make it > available to the jtreg test that's launched, they `@run` a jtreg `shell` > test. This `shell` test uses the `MakeJAR2.sh` script to generate the jar > file. The contents of the script is relatively straightforward - it compiles > (using `javac`) some boot classpath classes, some agent specific classes and > then generates a jar file with the agent classes and a `MANIFEST.MF` which > points to the boot classpath classes along with test specific manifest > attributes. In a recent PR the `MakeJAR2.sh` script was updated to pass > `--enable-preview --release 23` since one of the existing agent class was > updated to use the ClassFile API PreviewFeature > (https://github.com/openjdk/jdk/pull/13009). > > This generated agent jar then is passed as `-javaagent:` to the subsequent > `@run main` test which does the actual testing. > > So essentially the `shell` test which uses the `MakeJAR2.sh` is merely there > to create a jar file that's needed by the actual test. Within the JDK we have > several tests which compile other classes and generate jar files using > in-process test libraries and the `java.util.spi.ToolProvider` > implementations. These 2 tests too can be updated to use a similar technique, > to completely get rid of the `MakeJAR2.sh`. Removing the `MakeJAR2.sh` will > simplify the ability to pass around value for `--release` when using > `--enable-preview`. > > The commit in this PR updates these 2 tests to use `@run driver` which then > compiles relevant classes (maintaining the semantics of `MakeJAR2.sh`) and > generates the agent jar file and then finally launching the test process. As > part of the update, I did not retain the `@author` tag from the 2 test > definitions, since it's my understanding that we no longer use those. I will > add them back if we should retain them. > > The tests continue to pass locally with this change. I've also triggered tier > testing in our CI for this change. Jaikiran Pai has updated the pull request incrementally with one additional commit since the last revision: Alex's input - simplify the test by using ClassFileInstaller - Changes: - all: https://git.openjdk.org/jdk/pull/19495/files - new: https://git.openjdk.org/jdk/pull/19495/files/88997f4f..3c7512b2 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19495&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19495&range=01-02 Stats: 187 lines in 2 files changed: 13 ins; 144 del; 30 mod Patch: https://git.openjdk.org/jdk/pull/19495.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19495/head:pull/19495 PR: https://git.openjdk.org/jdk/pull/19495
Re: RFR: 8333130: MakeJAR2.sh uses hard-coded JDK version [v2]
On Fri, 31 May 2024 12:01:14 GMT, Jaikiran Pai wrote: >> Can I please get a review of this test-only change which addresses >> https://bugs.openjdk.org/browse/JDK-8333130? >> >> There are a couple of tests `NativeMethodPrefixApp` and `RetransformApp` >> under `test/jdk/java/lang/instrument/` which launch the app/test with a >> `-javaagent:` pointing to a test specific jar. The test, launched with >> that `javaagent`, then verifies the test specific details. >> >> In their current form, in order to construct the agent `.jar` and make it >> available to the jtreg test that's launched, they `@run` a jtreg `shell` >> test. This `shell` test uses the `MakeJAR2.sh` script to generate the jar >> file. The contents of the script is relatively straightforward - it compiles >> (using `javac`) some boot classpath classes, some agent specific classes and >> then generates a jar file with the agent classes and a `MANIFEST.MF` which >> points to the boot classpath classes along with test specific manifest >> attributes. In a recent PR the `MakeJAR2.sh` script was updated to pass >> `--enable-preview --release 23` since one of the existing agent class was >> updated to use the ClassFile API PreviewFeature >> (https://github.com/openjdk/jdk/pull/13009). >> >> This generated agent jar then is passed as `-javaagent:` to the subsequent >> `@run main` test which does the actual testing. >> >> So essentially the `shell` test which uses the `MakeJAR2.sh` is merely there >> to create a jar file that's needed by the actual test. Within the JDK we >> have several tests which compile other classes and generate jar files using >> in-process test libraries and the `java.util.spi.ToolProvider` >> implementations. These 2 tests too can be updated to use a similar >> technique, to completely get rid of the `MakeJAR2.sh`. Removing the >> `MakeJAR2.sh` will simplify the ability to pass around value for `--release` >> when using `--enable-preview`. >> >> The commit in this PR updates these 2 tests to use `@run driver` which then >> compiles relevant classes (maintaining the semantics of `MakeJAR2.sh`) and >> generates the agent jar file and then finally launching the test process. As >> part of the update, I did not retain the `@author` tag from the 2 test >> definitions, since it's my understanding that we no longer use those. I will >> add them back if we should retain them. >> >> The tests continue to pass locally with this change. I've also triggered >> tier testing in our CI for this change. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > fix Boot-Class-Path value for Windows Hello Alex, what you suggest seems interesting and like you note much more simpler. I'll try it out and if that will work fine with the use of `--release` for the `asmlib/Instrumentor.java`. - PR Comment: https://git.openjdk.org/jdk/pull/19495#issuecomment-2143194801
Re: RFR: 8333130: MakeJAR2.sh uses hard-coded JDK version [v2]
> Can I please get a review of this test-only change which addresses > https://bugs.openjdk.org/browse/JDK-8333130? > > There are a couple of tests `NativeMethodPrefixApp` and `RetransformApp` > under `test/jdk/java/lang/instrument/` which launch the app/test with a > `-javaagent:` pointing to a test specific jar. The test, launched with > that `javaagent`, then verifies the test specific details. > > In their current form, in order to construct the agent `.jar` and make it > available to the jtreg test that's launched, they `@run` a jtreg `shell` > test. This `shell` test uses the `MakeJAR2.sh` script to generate the jar > file. The contents of the script is relatively straightforward - it compiles > (using `javac`) some boot classpath classes, some agent specific classes and > then generates a jar file with the agent classes and a `MANIFEST.MF` which > points to the boot classpath classes along with test specific manifest > attributes. In a recent PR the `MakeJAR2.sh` script was updated to pass > `--enable-preview --release 23` since one of the existing agent class was > updated to use the ClassFile API PreviewFeature > (https://github.com/openjdk/jdk/pull/13009). > > This generated agent jar then is passed as `-javaagent:` to the subsequent > `@run main` test which does the actual testing. > > So essentially the `shell` test which uses the `MakeJAR2.sh` is merely there > to create a jar file that's needed by the actual test. Within the JDK we have > several tests which compile other classes and generate jar files using > in-process test libraries and the `java.util.spi.ToolProvider` > implementations. These 2 tests too can be updated to use a similar technique, > to completely get rid of the `MakeJAR2.sh`. Removing the `MakeJAR2.sh` will > simplify the ability to pass around value for `--release` when using > `--enable-preview`. > > The commit in this PR updates these 2 tests to use `@run driver` which then > compiles relevant classes (maintaining the semantics of `MakeJAR2.sh`) and > generates the agent jar file and then finally launching the test process. As > part of the update, I did not retain the `@author` tag from the 2 test > definitions, since it's my understanding that we no longer use those. I will > add them back if we should retain them. > > The tests continue to pass locally with this change. I've also triggered tier > testing in our CI for this change. Jaikiran Pai has updated the pull request incrementally with one additional commit since the last revision: fix Boot-Class-Path value for Windows - Changes: - all: https://git.openjdk.org/jdk/pull/19495/files - new: https://git.openjdk.org/jdk/pull/19495/files/a792b964..88997f4f Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19495&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19495&range=00-01 Stats: 10 lines in 2 files changed: 4 ins; 0 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/19495.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19495/head:pull/19495 PR: https://git.openjdk.org/jdk/pull/19495
RFR: 8333130: MakeJAR2.sh uses hard-coded JDK version
Can I please get a review of this test-only change which addresses https://bugs.openjdk.org/browse/JDK-8333130? There are a couple of tests `NativeMethodPrefixApp` and `RetransformApp` under `test/jdk/java/lang/instrument/` which launch the app/test with a `-javaagent:` pointing to a test specific jar. The test, launched with that `javaagent`, then verifies the test specific details. In their current form, in order to construct the agent `.jar` and make it available to the jtreg test that's launched, they `@run` a jtreg `shell` test. This `shell` test uses the `MakeJAR2.sh` script to generate the jar file. The contents of the script is relatively straightforward - it compiles (using `javac`) some boot classpath classes, some agent specific classes and then generates a jar file with the agent classes and a `MANIFEST.MF` which points to the boot classpath classes along with test specific manifest attributes. In a recent PR the `MakeJAR2.sh` script was updated to pass `--enable-preview --release 23` since one of the existing agent class was updated to use the ClassFile API PreviewFeature (https://github.com/openjdk/jdk/pull/13009). This generated agent jar then is passed as `-javaagent:` to the subsequent `@run main` test which does the actual testing. So essentially the `shell` test which uses the `MakeJAR2.sh` is merely there to create a jar file that's needed by the actual test. Within the JDK we have several tests which compile other classes and generate jar files using in-process test libraries and the `java.util.spi.ToolProvider` implementations. These 2 tests too can be updated to use a similar technique, to completely get rid of the `MakeJAR2.sh`. Removing the `MakeJAR2.sh` will simplify the ability to pass around value for `--release` when using `--enable-preview`. The commit in this PR updates these 2 tests to use `@run driver` which then compiles relevant classes (maintaining the semantics of `MakeJAR2.sh`) and generates the agent jar file and then finally launching the test process. As part of the update, I did not retain the `@author` tag from the 2 test definitions, since it's my understanding that we no longer use those. I will add them back if we should retain them. The tests continue to pass locally with this change. I've also triggered tier testing in our CI for this change. - Commit messages: - 8333130: MakeJAR2.sh uses hard-coded JDK version Changes: https://git.openjdk.org/jdk/pull/19495/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=19495&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8333130 Stats: 409 lines in 5 files changed: 265 ins; 131 del; 13 mod Patch: https://git.openjdk.org/jdk/pull/19495.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19495/head:pull/19495 PR: https://git.openjdk.org/jdk/pull/19495
Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v8]
On Fri, 17 May 2024 13:38:25 GMT, Maurizio Cimadamore wrote: >> This PR implements [JEP 472](https://openjdk.org/jeps/472), by restricting >> the use of JNI in the following ways: >> >> * `System::load` and `System::loadLibrary` are now restricted methods >> * `Runtime::load` and `Runtime::loadLibrary` are now restricted methods >> * binding a JNI `native` method declaration to a native implementation is >> now considered a restricted operation >> >> This PR slightly changes the way in which the JDK deals with restricted >> methods, even for FFM API calls. In Java 22, the single >> `--enable-native-access` was used both to specify a set of modules for which >> native access should be allowed *and* to specify whether illegal native >> access (that is, native access occurring from a module not specified by >> `--enable-native-access`) should be treated as an error or a warning. More >> specifically, an error is only issued if the `--enable-native-access flag` >> is used at least once. >> >> Here, a new flag is introduced, namely >> `illegal-native-access=allow/warn/deny`, which is used to specify what >> should happen when access to a restricted method and/or functionality is >> found outside the set of modules specified with `--enable-native-access`. >> The default policy is `warn`, but users can select `allow` to suppress the >> warnings, or `deny` to cause `IllegalCallerException` to be thrown. This >> aligns the treatment of restricted methods with other mechanisms, such as >> `--illegal-access` and the more recent `--sun-misc-unsafe-memory-access`. >> >> Some changes were required in the package-info javadoc for >> `java.lang.foreign`, to reflect the changes in the command line flags >> described above. > > Maurizio Cimadamore has updated the pull request incrementally with one > additional commit since the last revision: > > Address review comments Marked as reviewed by jpai (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19213#pullrequestreview-2064736036
Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v6]
On Thu, 16 May 2024 11:47:13 GMT, Maurizio Cimadamore wrote: >> src/java.base/share/classes/sun/launcher/resources/launcher.properties line >> 72: >> >>> 70: \ by code in modules for which native access is not >>> explicitly enabled.\n\ >>> 71: \ is one of "deny", "warn" or "allow".\n\ >>> 72: \ This option will be removed in a future release.\n\ >> >> Should this specify the current default value for this option if it isn't >> set? > > We already do, see > https://github.com/openjdk/jdk/pull/19213/files/1c45e5d56c429205ab8185481bc1044a86ab3bc6#diff-d05029afe6aed86f860a10901114402f1f6af4fe1e4b46d883141ab1d2a527b8R582 This is slightly different from what we do in the other PR for unsafe memory access where we specify the default in the launcher's help text too https://github.com/openjdk/jdk/pull/19174/files#diff-799093930b698e97b23ead98c6496261af1e2e33ec7aa9261584870cbee8a5eaR219. I don't have a strong opinion on this, I think either one is fine. - PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1603205279
Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v6]
On Wed, 15 May 2024 16:08:17 GMT, Maurizio Cimadamore wrote: >> This PR implements [JEP 472](https://openjdk.org/jeps/472), by restricting >> the use of JNI in the following ways: >> >> * `System::load` and `System::loadLibrary` are now restricted methods >> * `Runtime::load` and `Runtime::loadLibrary` are now restricted methods >> * binding a JNI `native` method declaration to a native implementation is >> now considered a restricted operation >> >> This PR slightly changes the way in which the JDK deals with restricted >> methods, even for FFM API calls. In Java 22, the single >> `--enable-native-access` was used both to specify a set of modules for which >> native access should be allowed *and* to specify whether illegal native >> access (that is, native access occurring from a module not specified by >> `--enable-native-access`) should be treated as an error or a warning. More >> specifically, an error is only issued if the `--enable-native-access flag` >> is used at least once. >> >> Here, a new flag is introduced, namely >> `illegal-native-access=allow/warn/deny`, which is used to specify what >> should happen when access to a restricted method and/or functionality is >> found outside the set of modules specified with `--enable-native-access`. >> The default policy is `warn`, but users can select `allow` to suppress the >> warnings, or `deny` to cause `IllegalCallerException` to be thrown. This >> aligns the treatment of restricted methods with other mechanisms, such as >> `--illegal-access` and the more recent `--sun-misc-unsafe-memory-access`. >> >> Some changes were required in the package-info javadoc for >> `java.lang.foreign`, to reflect the changes in the command line flags >> described above. > > Maurizio Cimadamore has updated the pull request incrementally with one > additional commit since the last revision: > > Address review comment Hello Maurizio, in the current mainline, we have code in `LauncherHelper` https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/sun/launcher/LauncherHelper.java#L636 where we enable native access to all unnamed modules if an executable jar with `Enable-Native-Access: ALL-UNNAMED` manifest is being launched. For such executable jars, what is the expected semantics when the launch also explicitly has a `--enable-native-access=M1,M2` option. Something like: java --enable-native-access=M1,M2 -jar foo.jar where `foo.jar` has `Enable-Native-Access: ALL-UNNAMED` in its manifest. - PR Comment: https://git.openjdk.org/jdk/pull/19213#issuecomment-2115005638
Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v6]
On Wed, 15 May 2024 16:08:17 GMT, Maurizio Cimadamore wrote: >> This PR implements [JEP 472](https://openjdk.org/jeps/472), by restricting >> the use of JNI in the following ways: >> >> * `System::load` and `System::loadLibrary` are now restricted methods >> * `Runtime::load` and `Runtime::loadLibrary` are now restricted methods >> * binding a JNI `native` method declaration to a native implementation is >> now considered a restricted operation >> >> This PR slightly changes the way in which the JDK deals with restricted >> methods, even for FFM API calls. In Java 22, the single >> `--enable-native-access` was used both to specify a set of modules for which >> native access should be allowed *and* to specify whether illegal native >> access (that is, native access occurring from a module not specified by >> `--enable-native-access`) should be treated as an error or a warning. More >> specifically, an error is only issued if the `--enable-native-access flag` >> is used at least once. >> >> Here, a new flag is introduced, namely >> `illegal-native-access=allow/warn/deny`, which is used to specify what >> should happen when access to a restricted method and/or functionality is >> found outside the set of modules specified with `--enable-native-access`. >> The default policy is `warn`, but users can select `allow` to suppress the >> warnings, or `deny` to cause `IllegalCallerException` to be thrown. This >> aligns the treatment of restricted methods with other mechanisms, such as >> `--illegal-access` and the more recent `--sun-misc-unsafe-memory-access`. >> >> Some changes were required in the package-info javadoc for >> `java.lang.foreign`, to reflect the changes in the command line flags >> described above. > > Maurizio Cimadamore has updated the pull request incrementally with one > additional commit since the last revision: > > Address review comment src/java.base/share/classes/sun/launcher/resources/launcher.properties line 72: > 70: \ by code in modules for which native access is not > explicitly enabled.\n\ > 71: \ is one of "deny", "warn" or "allow".\n\ > 72: \ This option will be removed in a future release.\n\ Should this specify the current default value for this option if it isn't set? - PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1603157916
Re: RFR: 8332098: Add missing @ since tags to jdk.jdi [v2]
On Mon, 13 May 2024 19:30:31 GMT, Nizar Benalla wrote: >> Please review this simple change where I add "@ since" tags to the >> package-info file of the following packages >> >> com.sun.jdi >> com.sun.jdi.connect >> com.sun.jdi.connect.spi >> com.sun.jdi.event >> com.sun.jdi.request >> >> I used the unix grep command to find the oldest "@ since" in each package >> and used that value, as it's hard to get the source code of JDK 1-5 >> TIA > > Nizar Benalla has updated the pull request incrementally with one additional > commit since the last revision: > > empty commit No changes have been done to the PR since it was last approved by Chris few days back (that's a good thing). I'll go ahead and sponsor this now. - PR Comment: https://git.openjdk.org/jdk/pull/19200#issuecomment-2114924570
Integrated: 8307778: com/sun/jdi/cds tests fail with jtreg's Virtual test thread factory
On Wed, 15 May 2024 05:59:56 GMT, Jaikiran Pai wrote: > Can I please get a review of this change which removes the > `test/jdk/com/sun/jdi/cds/` tests from being problem listed when jtreg > launches these tests through a virtual thread? > > These tests aren't actually incompatible with virtual threads. The real issue > is that in https://bugs.openjdk.org/browse/JDK-8305608, a test infrastructure > class `test/jdk/com/sun/jdi/VMConnection.java` was changed to use the > `test.class.path` system property instead of the previously used > `test.classes`. > > That change missed out updating the `test/jdk/com/sun/jdi/cds/` tests to pass > along the classpath through the `test.class.path` system property. As a > result these tests still use the old `test.classes` system property to pass > around the test classpath and thus causes these tests to fail. The reason why > this only impacts virtual threads is noted by Chris in this JBS comment > https://bugs.openjdk.org/browse/JDK-8305608?focusedId=14572100&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14572100. > > The commit in this PR updates the `CDSJDITest` to pass along > `test.class.path` instead of `test.classes`. The `CDSJDITest` is only used in > the tests under `test/jdk/com/sun/jdi/cds/`, so nothing else should be > impacted by this change. > > I ran these changes against both launching with platform thread as well as > virtual thread and these tests now pass in both these cases. This pull request has now been integrated. Changeset: fe8a2aff Author:Jaikiran Pai URL: https://git.openjdk.org/jdk/commit/fe8a2aff3129b515c2a0f3ab96f5e3ad6cef7b70 Stats: 5 lines in 2 files changed: 0 ins; 3 del; 2 mod 8307778: com/sun/jdi/cds tests fail with jtreg's Virtual test thread factory Reviewed-by: dholmes, sspitsyn, cjplummer, lmesnik - PR: https://git.openjdk.org/jdk/pull/19244
Re: RFR: 8307778: com/sun/jdi/cds tests fail with jtreg's Virtual test thread factory
On Wed, 15 May 2024 05:59:56 GMT, Jaikiran Pai wrote: > Can I please get a review of this change which removes the > `test/jdk/com/sun/jdi/cds/` tests from being problem listed when jtreg > launches these tests through a virtual thread? > > These tests aren't actually incompatible with virtual threads. The real issue > is that in https://bugs.openjdk.org/browse/JDK-8305608, a test infrastructure > class `test/jdk/com/sun/jdi/VMConnection.java` was changed to use the > `test.class.path` system property instead of the previously used > `test.classes`. > > That change missed out updating the `test/jdk/com/sun/jdi/cds/` tests to pass > along the classpath through the `test.class.path` system property. As a > result these tests still use the old `test.classes` system property to pass > around the test classpath and thus causes these tests to fail. The reason why > this only impacts virtual threads is noted by Chris in this JBS comment > https://bugs.openjdk.org/browse/JDK-8305608?focusedId=14572100&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14572100. > > The commit in this PR updates the `CDSJDITest` to pass along > `test.class.path` instead of `test.classes`. The `CDSJDITest` is only used in > the tests under `test/jdk/com/sun/jdi/cds/`, so nothing else should be > impacted by this change. > > I ran these changes against both launching with platform thread as well as > virtual thread and these tests now pass in both these cases. Thank you all for the reviews. > I think the CR needs a better title. Something that mentions "Virtual". Also, > it's not the "test factory" we are talking about here. It is the "test thread > factory". I have now updated the title of the JBS issue (and this PR) to "com/sun/jdi/cds tests fail with jtreg's Virtual test thread factory". No other changes have been done. I'll integrate this in a few hours from now. - PR Comment: https://git.openjdk.org/jdk/pull/19244#issuecomment-2113729065
RFR: 8307778: com/sun/jdi/cds tests are not compatible with jtreg test factory
Can I please get a review of this change which removes the `test/jdk/com/sun/jdi/cds/` tests from being problem listed when jtreg launches these tests through a virtual thread? These tests aren't actually incompatible with virtual threads. The real issue is that in https://bugs.openjdk.org/browse/JDK-8305608, a test infrastructure class `test/jdk/com/sun/jdi/VMConnection.java` was changed to use the `test.class.path` system property instead of the previously used `test.classes`. That change missed out updating the `test/jdk/com/sun/jdi/cds/` tests to pass along the classpath through the `test.class.path` system property. As a result these tests still use the old `test.classes` system property to pass around the test classpath and thus causes these tests to fail. The reason why this only impacts virtual threads is noted by Chris in this JBS comment https://bugs.openjdk.org/browse/JDK-8305608?focusedId=14572100&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14572100. The commit in this PR updates the `CDSJDITest` to pass along `test.class.path` instead of `test.classes`. The `CDSJDITest` is only used in the tests under `test/jdk/com/sun/jdi/cds/`, so nothing else should be impacted by this change. I ran these changes against both launching with platform thread as well as virtual thread and these tests now pass in both these cases. - Commit messages: - 8307778: com/sun/jdi/cds tests are not compatible with jtreg test factory Changes: https://git.openjdk.org/jdk/pull/19244/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=19244&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8307778 Stats: 5 lines in 2 files changed: 0 ins; 3 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/19244.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19244/head:pull/19244 PR: https://git.openjdk.org/jdk/pull/19244
