[jdk21] Integrated: 8311186: ProblemList javax/management/remote/mandatory/subjectDelegation/SubjectDelegation1Test.java on linux-aarch64
On Fri, 30 Jun 2023 22:17:40 GMT, Daniel D. Daugherty wrote: > Backport 140b70fb29e2b83e5d33765906ee76a36442a687 > > Trivial ProblemListing/disable changes: > > [JDK-8311186](https://bugs.openjdk.org/browse/JDK-8311186) ProblemList > javax/management/remote/mandatory/subjectDelegation/SubjectDelegation1Test.java > on linux-aarch64 > [JDK-8311189](https://bugs.openjdk.org/browse/JDK-8311189) disable > gc/z/TestHighUsage.java > [JDK-8311190](https://bugs.openjdk.org/browse/JDK-8311190) ProblemList > javax/management/remote/mandatory/connection/DeadLockTest.java with virtual > threads on windows-x64 > [JDK-8311191](https://bugs.openjdk.org/browse/JDK-8311191) ProblemList > javax/management/remote/mandatory/connection/ConnectionTest.java with virtual > threads on windows-x64 > [JDK-8311193](https://bugs.openjdk.org/browse/JDK-8311193) ProblemList > vmTestbase/nsk/jdb/interrupt/interrupt001/interrupt001.java on linux-all > [JDK-8311195](https://bugs.openjdk.org/browse/JDK-8311195) ProblemList > vmTestbase/nsk/jvmti/scenarios/capability/CM03/cm03t001/TestDescription.java > with Xcomp on macosx-x64 This pull request has now been integrated. Changeset: 35d592e3 Author:Daniel D. Daugherty URL: https://git.openjdk.org/jdk21/commit/35d592e39d2bc99543d1b6bf671fc73fbef35bc8 Stats: 13 lines in 6 files changed: 11 ins; 2 del; 0 mod 8311186: ProblemList javax/management/remote/mandatory/subjectDelegation/SubjectDelegation1Test.java on linux-aarch64 8311189: disable gc/z/TestHighUsage.java 8311190: ProblemList javax/management/remote/mandatory/connection/DeadLockTest.java with virtual threads on windows-x64 8311191: ProblemList javax/management/remote/mandatory/connection/ConnectionTest.java with virtual threads on windows-x64 8311193: ProblemList vmTestbase/nsk/jdb/interrupt/interrupt001/interrupt001.java on linux-all 8311195: ProblemList vmTestbase/nsk/jvmti/scenarios/capability/CM03/cm03t001/TestDescription.java with Xcomp on macosx-x64 Reviewed-by: lmesnik Backport-of: 140b70fb29e2b83e5d33765906ee76a36442a687 - PR: https://git.openjdk.org/jdk21/pull/88
Re: [jdk21] RFR: 8311186: ProblemList javax/management/remote/mandatory/subjectDelegation/SubjectDelegation1Test.java on linux-aarch64
On Fri, 30 Jun 2023 22:28:34 GMT, Leonid Mesnik wrote: >> Backport 140b70fb29e2b83e5d33765906ee76a36442a687 >> >> Trivial ProblemListing/disable changes: >> >> [JDK-8311186](https://bugs.openjdk.org/browse/JDK-8311186) ProblemList >> javax/management/remote/mandatory/subjectDelegation/SubjectDelegation1Test.java >> on linux-aarch64 >> [JDK-8311189](https://bugs.openjdk.org/browse/JDK-8311189) disable >> gc/z/TestHighUsage.java >> [JDK-8311190](https://bugs.openjdk.org/browse/JDK-8311190) ProblemList >> javax/management/remote/mandatory/connection/DeadLockTest.java with virtual >> threads on windows-x64 >> [JDK-8311191](https://bugs.openjdk.org/browse/JDK-8311191) ProblemList >> javax/management/remote/mandatory/connection/ConnectionTest.java with >> virtual threads on windows-x64 >> [JDK-8311193](https://bugs.openjdk.org/browse/JDK-8311193) ProblemList >> vmTestbase/nsk/jdb/interrupt/interrupt001/interrupt001.java on linux-all >> [JDK-8311195](https://bugs.openjdk.org/browse/JDK-8311195) ProblemList >> vmTestbase/nsk/jvmti/scenarios/capability/CM03/cm03t001/TestDescription.java >> with Xcomp on macosx-x64 > > Marked as reviewed by lmesnik (Reviewer). @lmesnik - Thanks for the fast review! - PR Comment: https://git.openjdk.org/jdk21/pull/88#issuecomment-1615247775
Re: [jdk21] RFR: 8311186: ProblemList javax/management/remote/mandatory/subjectDelegation/SubjectDelegation1Test.java on linux-aarch64
On Fri, 30 Jun 2023 22:17:40 GMT, Daniel D. Daugherty wrote: > Backport 140b70fb29e2b83e5d33765906ee76a36442a687 > > Trivial ProblemListing/disable changes: > > [JDK-8311186](https://bugs.openjdk.org/browse/JDK-8311186) ProblemList > javax/management/remote/mandatory/subjectDelegation/SubjectDelegation1Test.java > on linux-aarch64 > [JDK-8311189](https://bugs.openjdk.org/browse/JDK-8311189) disable > gc/z/TestHighUsage.java > [JDK-8311190](https://bugs.openjdk.org/browse/JDK-8311190) ProblemList > javax/management/remote/mandatory/connection/DeadLockTest.java with virtual > threads on windows-x64 > [JDK-8311191](https://bugs.openjdk.org/browse/JDK-8311191) ProblemList > javax/management/remote/mandatory/connection/ConnectionTest.java with virtual > threads on windows-x64 > [JDK-8311193](https://bugs.openjdk.org/browse/JDK-8311193) ProblemList > vmTestbase/nsk/jdb/interrupt/interrupt001/interrupt001.java on linux-all > [JDK-8311195](https://bugs.openjdk.org/browse/JDK-8311195) ProblemList > vmTestbase/nsk/jvmti/scenarios/capability/CM03/cm03t001/TestDescription.java > with Xcomp on macosx-x64 Marked as reviewed by lmesnik (Reviewer). - PR Review: https://git.openjdk.org/jdk21/pull/88#pullrequestreview-1507948046
[jdk21] RFR: 8311186: ProblemList javax/management/remote/mandatory/subjectDelegation/SubjectDelegation1Test.java on linux-aarch64
Backport 140b70fb29e2b83e5d33765906ee76a36442a687 Trivial ProblemListing/disable changes: [JDK-8311186](https://bugs.openjdk.org/browse/JDK-8311186) ProblemList javax/management/remote/mandatory/subjectDelegation/SubjectDelegation1Test.java on linux-aarch64 [JDK-8311189](https://bugs.openjdk.org/browse/JDK-8311189) disable gc/z/TestHighUsage.java [JDK-8311190](https://bugs.openjdk.org/browse/JDK-8311190) ProblemList javax/management/remote/mandatory/connection/DeadLockTest.java with virtual threads on windows-x64 [JDK-8311191](https://bugs.openjdk.org/browse/JDK-8311191) ProblemList javax/management/remote/mandatory/connection/ConnectionTest.java with virtual threads on windows-x64 [JDK-8311193](https://bugs.openjdk.org/browse/JDK-8311193) ProblemList vmTestbase/nsk/jdb/interrupt/interrupt001/interrupt001.java on linux-all [JDK-8311195](https://bugs.openjdk.org/browse/JDK-8311195) ProblemList vmTestbase/nsk/jvmti/scenarios/capability/CM03/cm03t001/TestDescription.java with Xcomp on macosx-x64 - Commit messages: - Backport 140b70fb29e2b83e5d33765906ee76a36442a687 Changes: https://git.openjdk.org/jdk21/pull/88/files Webrev: https://webrevs.openjdk.org/?repo=jdk21&pr=88&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8311186 Stats: 13 lines in 6 files changed: 11 ins; 2 del; 0 mod Patch: https://git.openjdk.org/jdk21/pull/88.diff Fetch: git fetch https://git.openjdk.org/jdk21.git pull/88/head:pull/88 PR: https://git.openjdk.org/jdk21/pull/88
Re: RFR: JDK-8306441: Two-stage Segmented Heap Dump [v14]
On Wed, 28 Jun 2023 08:04:21 GMT, Yi Yang wrote: >> ### Motivation and proposal >> Hi, heap dump brings about pauses for application's execution(STW), this is >> a well-known pain. JDK-8252842 have added parallel support to heapdump in an >> attempt to alleviate this issue. However, all concurrent threads >> competitively write heap data to the same file, and more memory is required >> to maintain the concurrent buffer queue. In experiments, we did not feel a >> significant performance improvement from that. >> >> The minor-pause solution, which is presented in this PR, is a two-stage >> segmented heap dump: >> >> 1. Stage One(STW): Concurrent threads directly write data to multiple heap >> files. >> 2. Stage Two(Non-STW): Merge multiple heap files into one complete heap dump >> file. >> >> Now concurrent worker threads are not required to maintain a buffer queue, >> which would result in more memory overhead, nor do they need to compete for >> locks. The changes in the overall design are as follows: >> >>  >> Fig1. Before >> >>  >> Fig2. After this patch >> >> ### Performance evaluation >> | memory | numOfThread | STW | Total | Compression | >> | --- | - | -- | | | >> | 8g | 1 thread | 15.612 secs | 15.612 secs | N | >> | 8g | 32 thread | 2.5617250 secs | 14.498 secs | N | >> | 8g | 32 thread | 2.3084878 secs | 3.198 secs | Compress1 | >> | 8g | 32 thread | 10.9355128 secs | 21.882 secs | Compress2 | >> | 8g | 96 thread | 2.6790452 secs | 14.012 secs | N | >> | 8g | 96 thread | 2.3044796 secs | 3.589 secs | Compress1 | >> | 8g | 96 thread | 9.7585151 secs | 20.219 secs| Compress2 | >> | 16g | 1 thread | 26.278 secs | 26.278 secs | N | >> | 16g | 32 thread | 5.2313740 secs | 26.417 secs | N | >> | 16g | 32 thread | 5.6946983 secs | 6.538 secs | Compress1 | >> | 16g | 32 thread | 21.8211105 secs | 41.133 secs | Compress2 | >> | 16g | 96 thread | 6.2445556 secs | 27.141 secs | N | >> | 16g | 96 thread | 4.6007096 secs | 6.259 secs | Compress1 | >> | 16g | 96 thread | 19.2965783 secs | 39.007 secs | Compress2 | >> | 32g | 1 thread | 48.149 secs | 48.149 secs | N | >> | 32g | 32 thread | 10.7734677 secs | 61.643 secs | N | >> | 32g | 32 thread | 10.1642097 secs | 10.903 secs | Compress1 | >> | 32g | 32 thread | 43.8407607 secs | 88.152 secs | Compress2 | >> | 32g | 96 thread | 13.1522042 ... > > Yi Yang has updated the pull request incrementally with one additional commit > since the last revision: > > memory leak I think commit "use HandshakeClosure instead of VMOperation" is a wrong way to go. It restricts use of parallel dumping only to attach case. I have pending changes in heap dumper to support virtual threads and I'm going switch heap dumper to always use DumpMerger. - PR Comment: https://git.openjdk.org/jdk/pull/13667#issuecomment-1615243585
Re: RFR: 8311186: ProblemList javax/management/remote/mandatory/subjectDelegation/SubjectDelegation1Test.java on linux-aarch64
On Fri, 30 Jun 2023 19:26:22 GMT, Daniel D. Daugherty wrote: > Trivial ProblemListing/disable changes: > - [JDK-8311186](https://bugs.openjdk.org/browse/JDK-8311186) ProblemList > javax/management/remote/mandatory/subjectDelegation/SubjectDelegation1Test.java > on linux-aarch64 > - [JDK-8311189](https://bugs.openjdk.org/browse/JDK-8311189) disable > gc/z/TestHighUsage.java > - [JDK-8311190](https://bugs.openjdk.org/browse/JDK-8311190) ProblemList > javax/management/remote/mandatory/connection/DeadLockTest.java with virtual > threads on windows-x64 > - [JDK-8311191](https://bugs.openjdk.org/browse/JDK-8311191) ProblemList > javax/management/remote/mandatory/connection/ConnectionTest.java with virtual > threads on windows-x64 > - [JDK-8311193](https://bugs.openjdk.org/browse/JDK-8311193) ProblemList > vmTestbase/nsk/jdb/interrupt/interrupt001/interrupt001.java on linux-all > - [JDK-8311195](https://bugs.openjdk.org/browse/JDK-8311195) ProblemList > vmTestbase/nsk/jvmti/scenarios/capability/CM03/cm03t001/TestDescription.java > with Xcomp on macosx-x64 And my point is that the test executes only with Generational ZGC and cannot always be ProblemListed via the ProblemList-generational-zgc.txt file. The test internally specifies -XX:+UseZGC and -XX:+ZGenerational, but not in a way where the ProblemList-generational-zgc.txt file comes into play. - PR Comment: https://git.openjdk.org/jdk/pull/14741#issuecomment-1615229673
Integrated: 8311186: ProblemList javax/management/remote/mandatory/subjectDelegation/SubjectDelegation1Test.java on linux-aarch64
On Fri, 30 Jun 2023 19:26:22 GMT, Daniel D. Daugherty wrote: > Trivial ProblemListing/disable changes: > - [JDK-8311186](https://bugs.openjdk.org/browse/JDK-8311186) ProblemList > javax/management/remote/mandatory/subjectDelegation/SubjectDelegation1Test.java > on linux-aarch64 > - [JDK-8311189](https://bugs.openjdk.org/browse/JDK-8311189) disable > gc/z/TestHighUsage.java > - [JDK-8311190](https://bugs.openjdk.org/browse/JDK-8311190) ProblemList > javax/management/remote/mandatory/connection/DeadLockTest.java with virtual > threads on windows-x64 > - [JDK-8311191](https://bugs.openjdk.org/browse/JDK-8311191) ProblemList > javax/management/remote/mandatory/connection/ConnectionTest.java with virtual > threads on windows-x64 > - [JDK-8311193](https://bugs.openjdk.org/browse/JDK-8311193) ProblemList > vmTestbase/nsk/jdb/interrupt/interrupt001/interrupt001.java on linux-all > - [JDK-8311195](https://bugs.openjdk.org/browse/JDK-8311195) ProblemList > vmTestbase/nsk/jvmti/scenarios/capability/CM03/cm03t001/TestDescription.java > with Xcomp on macosx-x64 This pull request has now been integrated. Changeset: 140b70fb Author:Daniel D. Daugherty URL: https://git.openjdk.org/jdk/commit/140b70fb29e2b83e5d33765906ee76a36442a687 Stats: 13 lines in 6 files changed: 11 ins; 2 del; 0 mod 8311186: ProblemList javax/management/remote/mandatory/subjectDelegation/SubjectDelegation1Test.java on linux-aarch64 8311189: disable gc/z/TestHighUsage.java 8311190: ProblemList javax/management/remote/mandatory/connection/DeadLockTest.java with virtual threads on windows-x64 8311191: ProblemList javax/management/remote/mandatory/connection/ConnectionTest.java with virtual threads on windows-x64 8311193: ProblemList vmTestbase/nsk/jdb/interrupt/interrupt001/interrupt001.java on linux-all 8311195: ProblemList vmTestbase/nsk/jvmti/scenarios/capability/CM03/cm03t001/TestDescription.java with Xcomp on macosx-x64 Reviewed-by: lmesnik - PR: https://git.openjdk.org/jdk/pull/14741
Re: RFR: 8311186: ProblemList javax/management/remote/mandatory/subjectDelegation/SubjectDelegation1Test.java on linux-aarch64
On Fri, 30 Jun 2023 19:26:22 GMT, Daniel D. Daugherty wrote: > Trivial ProblemListing/disable changes: > - [JDK-8311186](https://bugs.openjdk.org/browse/JDK-8311186) ProblemList > javax/management/remote/mandatory/subjectDelegation/SubjectDelegation1Test.java > on linux-aarch64 > - [JDK-8311189](https://bugs.openjdk.org/browse/JDK-8311189) disable > gc/z/TestHighUsage.java > - [JDK-8311190](https://bugs.openjdk.org/browse/JDK-8311190) ProblemList > javax/management/remote/mandatory/connection/DeadLockTest.java with virtual > threads on windows-x64 > - [JDK-8311191](https://bugs.openjdk.org/browse/JDK-8311191) ProblemList > javax/management/remote/mandatory/connection/ConnectionTest.java with virtual > threads on windows-x64 > - [JDK-8311193](https://bugs.openjdk.org/browse/JDK-8311193) ProblemList > vmTestbase/nsk/jdb/interrupt/interrupt001/interrupt001.java on linux-all > - [JDK-8311195](https://bugs.openjdk.org/browse/JDK-8311195) ProblemList > vmTestbase/nsk/jvmti/scenarios/capability/CM03/cm03t001/TestDescription.java > with Xcomp on macosx-x64 I guess test/hotspot/jtreg/gc/z/TestHighUsage.java could be added to the ProblemList.txt file. However, that feels a bit misleading to me. It's a Generational ZGC only test that can't be ProblemListed in the usual way by putting it on the Generational ZGC ProblemList. In this case, I just wanted to be sure that it just doesn't run anywhere for any reason. Especially after the two previous (bad) ProblemListing attempts. - PR Comment: https://git.openjdk.org/jdk/pull/14741#issuecomment-1615220031
Re: RFR: 8311186: ProblemList javax/management/remote/mandatory/subjectDelegation/SubjectDelegation1Test.java on linux-aarch64
On Fri, 30 Jun 2023 19:26:22 GMT, Daniel D. Daugherty wrote: > Trivial ProblemListing/disable changes: > - [JDK-8311186](https://bugs.openjdk.org/browse/JDK-8311186) ProblemList > javax/management/remote/mandatory/subjectDelegation/SubjectDelegation1Test.java > on linux-aarch64 > - [JDK-8311189](https://bugs.openjdk.org/browse/JDK-8311189) disable > gc/z/TestHighUsage.java > - [JDK-8311190](https://bugs.openjdk.org/browse/JDK-8311190) ProblemList > javax/management/remote/mandatory/connection/DeadLockTest.java with virtual > threads on windows-x64 > - [JDK-8311191](https://bugs.openjdk.org/browse/JDK-8311191) ProblemList > javax/management/remote/mandatory/connection/ConnectionTest.java with virtual > threads on windows-x64 > - [JDK-8311193](https://bugs.openjdk.org/browse/JDK-8311193) ProblemList > vmTestbase/nsk/jdb/interrupt/interrupt001/interrupt001.java on linux-all > - [JDK-8311195](https://bugs.openjdk.org/browse/JDK-8311195) ProblemList > vmTestbase/nsk/jvmti/scenarios/capability/CM03/cm03t001/TestDescription.java > with Xcomp on macosx-x64 There is misunderstanding here. Generational ZGC ProblemList is used not to problemlist Gen ZGC tests, but probleml list any test with -XX:ZGC + Generational enabled execution. - PR Comment: https://git.openjdk.org/jdk/pull/14741#issuecomment-1615221446
Re: RFR: 8311186: ProblemList javax/management/remote/mandatory/subjectDelegation/SubjectDelegation1Test.java on linux-aarch64
On Fri, 30 Jun 2023 19:26:22 GMT, Daniel D. Daugherty wrote: > Trivial ProblemListing/disable changes: > - [JDK-8311186](https://bugs.openjdk.org/browse/JDK-8311186) ProblemList > javax/management/remote/mandatory/subjectDelegation/SubjectDelegation1Test.java > on linux-aarch64 > - [JDK-8311189](https://bugs.openjdk.org/browse/JDK-8311189) disable > gc/z/TestHighUsage.java > - [JDK-8311190](https://bugs.openjdk.org/browse/JDK-8311190) ProblemList > javax/management/remote/mandatory/connection/DeadLockTest.java with virtual > threads on windows-x64 > - [JDK-8311191](https://bugs.openjdk.org/browse/JDK-8311191) ProblemList > javax/management/remote/mandatory/connection/ConnectionTest.java with virtual > threads on windows-x64 > - [JDK-8311193](https://bugs.openjdk.org/browse/JDK-8311193) ProblemList > vmTestbase/nsk/jdb/interrupt/interrupt001/interrupt001.java on linux-all > - [JDK-8311195](https://bugs.openjdk.org/browse/JDK-8311195) ProblemList > vmTestbase/nsk/jvmti/scenarios/capability/CM03/cm03t001/TestDescription.java > with Xcomp on macosx-x64 I understand that test/hotspot/jtreg/gc/z/TestHighUsage.java can't be problemlisted using zgc test lists. But why not to put into plain ProblemList.txt? Also, might be it makes sense to remove it from *zgc problem list while adding ignore? But might be it could be done while fixing bug. - Marked as reviewed by lmesnik (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14741#pullrequestreview-1507915619
RFR: 8311186: ProblemList javax/management/remote/mandatory/subjectDelegation/SubjectDelegation1Test.java on linux-aarch64
Trivial ProblemListing/disable changes: - [JDK-8311186](https://bugs.openjdk.org/browse/JDK-8311186) ProblemList javax/management/remote/mandatory/subjectDelegation/SubjectDelegation1Test.java on linux-aarch64 - [JDK-8311189](https://bugs.openjdk.org/browse/JDK-8311189) disable gc/z/TestHighUsage.java - [JDK-8311190](https://bugs.openjdk.org/browse/JDK-8311190) ProblemList javax/management/remote/mandatory/connection/DeadLockTest.java with virtual threads on windows-x64 - [JDK-8311191](https://bugs.openjdk.org/browse/JDK-8311191) ProblemList javax/management/remote/mandatory/connection/ConnectionTest.java with virtual threads on windows-x64 - [JDK-8311193](https://bugs.openjdk.org/browse/JDK-8311193) ProblemList vmTestbase/nsk/jdb/interrupt/interrupt001/interrupt001.java on linux-all - [JDK-8311195](https://bugs.openjdk.org/browse/JDK-8311195) ProblemList vmTestbase/nsk/jvmti/scenarios/capability/CM03/cm03t001/TestDescription.java with Xcomp on macosx-x64 - Commit messages: - 8311195: ProblemList vmTestbase/nsk/jvmti/scenarios/capability/CM03/cm03t001/TestDescription.java with Xcomp on macosx-x64 - 8311193: ProblemList vmTestbase/nsk/jdb/interrupt/interrupt001/interrupt001.java on linux-all - 8311191: ProblemList javax/management/remote/mandatory/connection/ConnectionTest.java with virtual threads on windows-x64 - 8311190: ProblemList javax/management/remote/mandatory/connection/DeadLockTest.java with virtual threads on windows-x64 - 8311189: disable gc/z/TestHighUsage.java - 8311186: ProblemList javax/management/remote/mandatory/subjectDelegation/SubjectDelegation1Test.java on linux-aarch64 Changes: https://git.openjdk.org/jdk/pull/14741/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=14741&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8311186 Stats: 13 lines in 6 files changed: 11 ins; 2 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/14741.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14741/head:pull/14741 PR: https://git.openjdk.org/jdk/pull/14741
Integrated: 8303086: SIGSEGV in JavaThread::is_interp_only_mode()
On Fri, 30 Jun 2023 11:27:58 GMT, Serguei Spitsyn wrote:
> The JVMTI function `SetEventNotificationMode` can set notification mode
> globally (`event_thread == nullptr`) for all threads or for a specific thread
> (`event_thread != nullptr`). To get a stable mount/unmount vision of virtual
> threads a JvmtiVTMSTransitionDisabler helper object is created :
> `JvmtiVTMSTransitionDisabler disabler(event_thread);`
>
> In a case if `event_thread == nullptr` the VTMS transitions are disabled for
> all virtual thread,
> otherwise they are disabled for a specific thread if it is virtual.
> The call to `JvmtiEventController::set_user_enabled()` makes a call to
> `recompute_enabled()` at the end of its work to do a required bookkeeping. As
> part of this work, the `recompute_thread_enabled(state)` is called for each
> thread from the `ThreadsListHandle`, not only for the given `event_thread`:
>
> ThreadsListHandle tlh;
> for (; state != nullptr; state = state->next()) {
> any_env_thread_enabled |= recompute_thread_enabled(state);
> }
>
> This can cause crashes as VTMS transitions for other virtual threads are
> allowed.
> Crashes are observed in this small function:
>
> bool is_interp_only_mode() {
> return _thread == nullptr ? _saved_interp_only_mode != 0 :
> _thread->is_interp_only_mode();
> }
>
> In a case `_thread != nullptr` then the call needs to be executed:
> `_thread->is_interp_only_mode()`.
> But the filed `_thread` can be already changed to `nullptr` by a VTMS
> transition.
>
> The fix is to always disable all transitions.
> Thanks to Dan and Patricio for great analysis of this crash!
>
> Testing:
> - In progress: mach5 tiers 1-6
This pull request has now been integrated.
Changeset: 971c2efb
Author:Serguei Spitsyn
URL:
https://git.openjdk.org/jdk/commit/971c2efb698065c65dcf7373d8c3027f58d5f503
Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
8303086: SIGSEGV in JavaThread::is_interp_only_mode()
Reviewed-by: pchilanomate, cjplummer, lmesnik
-
PR: https://git.openjdk.org/jdk/pull/14728
Re: RFR: 8303086: SIGSEGV in JavaThread::is_interp_only_mode()
On Fri, 30 Jun 2023 11:27:58 GMT, Serguei Spitsyn wrote:
> The JVMTI function `SetEventNotificationMode` can set notification mode
> globally (`event_thread == nullptr`) for all threads or for a specific thread
> (`event_thread != nullptr`). To get a stable mount/unmount vision of virtual
> threads a JvmtiVTMSTransitionDisabler helper object is created :
> `JvmtiVTMSTransitionDisabler disabler(event_thread);`
>
> In a case if `event_thread == nullptr` the VTMS transitions are disabled for
> all virtual thread,
> otherwise they are disabled for a specific thread if it is virtual.
> The call to `JvmtiEventController::set_user_enabled()` makes a call to
> `recompute_enabled()` at the end of its work to do a required bookkeeping. As
> part of this work, the `recompute_thread_enabled(state)` is called for each
> thread from the `ThreadsListHandle`, not only for the given `event_thread`:
>
> ThreadsListHandle tlh;
> for (; state != nullptr; state = state->next()) {
> any_env_thread_enabled |= recompute_thread_enabled(state);
> }
>
> This can cause crashes as VTMS transitions for other virtual threads are
> allowed.
> Crashes are observed in this small function:
>
> bool is_interp_only_mode() {
> return _thread == nullptr ? _saved_interp_only_mode != 0 :
> _thread->is_interp_only_mode();
> }
>
> In a case `_thread != nullptr` then the call needs to be executed:
> `_thread->is_interp_only_mode()`.
> But the filed `_thread` can be already changed to `nullptr` by a VTMS
> transition.
>
> The fix is to always disable all transitions.
> Thanks to Dan and Patricio for great analysis of this crash!
>
> Testing:
> - In progress: mach5 tiers 1-6
Patricio, Chris and Leonid, thank you for review.
-
PR Comment: https://git.openjdk.org/jdk/pull/14728#issuecomment-1615188480
Re: RFR: JDK-8310066: Improve test coverage for JVMTI GetThreadState on carrier and mounted vthread [v4]
On Fri, 30 Jun 2023 05:54:12 GMT, Serguei Spitsyn wrote:
>> Alex Menkov has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> updated comment
>
> test/hotspot/jtreg/serviceability/jvmti/vthread/GetThreadStateMountedTest/libGetThreadStateMountedTest.cpp
> line 112:
>
>> 110: LOG(" ERROR: all expected 'weak' bits are set\n");
>> 111: }
>> 112: }
>
> Nit: I guess, error message at line 110 has to be:
> `ERROR: not all expected 'weak' bits are set`
> Also, it looks like the check 3a is not really needed as the 3b should cover
> it.
> But I leave it up to you, if you think check 3a gives some convenience.
'weak' value is for 'sleeping' testcase. In the case we have 2 valid states:
JVMTI_THREAD_STATE_SLEEPING and JVMTI_THREAD_STATE_PARKED, but only one of them
should be reported.
Test passes expected_weak == JVMTI_THREAD_STATE_SLEEPING |
JVMTI_THREAD_STATE_PARKED.
check 3a verifies that at least 1 bit is set (JVMTI_THREAD_STATE_SLEEPING,
JVMTI_THREAD_STATE_PARKED or both).
check 3b verifies that only one of the bits is set (i.e. we don't have both
JVMTI_THREAD_STATE_SLEEPING and JVMTI_THREAD_STATE_PARKED)
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14622#discussion_r1248265672
Integrated: 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit
On Sun, 25 Jun 2023 06:58:14 GMT, Doug Simon wrote: > The VMSupport class is required for translating an exception between the > HotSpot and libgraal heaps. > Loading it lazily can result in a loading exception, obscuring the exception > being translated. > To avoid this, VMSupport is loaded eagerly along with the other vmClasses. This pull request has now been integrated. Changeset: f6bdccb4 Author:Doug Simon URL: https://git.openjdk.org/jdk/commit/f6bdccb45caca0f69918a773a9ad9b2ad91b702f Stats: 176 lines in 6 files changed: 123 ins; 21 del; 32 mod 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit Reviewed-by: never, kvn - PR: https://git.openjdk.org/jdk/pull/14641
Re: RFR: 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit [v8]
On Fri, 30 Jun 2023 15:15:16 GMT, Doug Simon wrote: >> The VMSupport class is required for translating an exception between the >> HotSpot and libgraal heaps. >> Loading it lazily can result in a loading exception, obscuring the exception >> being translated. >> To avoid this, VMSupport is loaded eagerly along with the other vmClasses. > > Doug Simon has updated the pull request incrementally with one additional > commit since the last revision: > > fix warning about conversion from size_t to int - take 2 Thanks for the reviews. - PR Comment: https://git.openjdk.org/jdk/pull/14641#issuecomment-1615145850
Re: RFR: 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit [v6]
On Fri, 30 Jun 2023 17:30:33 GMT, Vladimir Kozlov wrote: > > > But, please, activate GHA testing for this branch. > > > > > > Isn't GHA a strict subset of or equal to tier1 mach5 testing? If so, what's > > the point of doing redundant testing? > > It builds and tests configurations (32-bit) we don't have in our testing. Good to know - thanks! - PR Comment: https://git.openjdk.org/jdk/pull/14641#issuecomment-1615144598
Re: RFR: 8303086: SIGSEGV in JavaThread::is_interp_only_mode()
On Fri, 30 Jun 2023 11:27:58 GMT, Serguei Spitsyn wrote:
> The JVMTI function `SetEventNotificationMode` can set notification mode
> globally (`event_thread == nullptr`) for all threads or for a specific thread
> (`event_thread != nullptr`). To get a stable mount/unmount vision of virtual
> threads a JvmtiVTMSTransitionDisabler helper object is created :
> `JvmtiVTMSTransitionDisabler disabler(event_thread);`
>
> In a case if `event_thread == nullptr` the VTMS transitions are disabled for
> all virtual thread,
> otherwise they are disabled for a specific thread if it is virtual.
> The call to `JvmtiEventController::set_user_enabled()` makes a call to
> `recompute_enabled()` at the end of its work to do a required bookkeeping. As
> part of this work, the `recompute_thread_enabled(state)` is called for each
> thread from the `ThreadsListHandle`, not only for the given `event_thread`:
>
> ThreadsListHandle tlh;
> for (; state != nullptr; state = state->next()) {
> any_env_thread_enabled |= recompute_thread_enabled(state);
> }
>
> This can cause crashes as VTMS transitions for other virtual threads are
> allowed.
> Crashes are observed in this small function:
>
> bool is_interp_only_mode() {
> return _thread == nullptr ? _saved_interp_only_mode != 0 :
> _thread->is_interp_only_mode();
> }
>
> In a case `_thread != nullptr` then the call needs to be executed:
> `_thread->is_interp_only_mode()`.
> But the filed `_thread` can be already changed to `nullptr` by a VTMS
> transition.
>
> The fix is to always disable all transitions.
> Thanks to Dan and Patricio for great analysis of this crash!
>
> Testing:
> - In progress: mach5 tiers 1-6
Marked as reviewed by lmesnik (Reviewer).
-
PR Review: https://git.openjdk.org/jdk/pull/14728#pullrequestreview-1507763983
Re: RFR: 8303086: SIGSEGV in JavaThread::is_interp_only_mode()
On Fri, 30 Jun 2023 11:27:58 GMT, Serguei Spitsyn wrote:
> The JVMTI function `SetEventNotificationMode` can set notification mode
> globally (`event_thread == nullptr`) for all threads or for a specific thread
> (`event_thread != nullptr`). To get a stable mount/unmount vision of virtual
> threads a JvmtiVTMSTransitionDisabler helper object is created :
> `JvmtiVTMSTransitionDisabler disabler(event_thread);`
>
> In a case if `event_thread == nullptr` the VTMS transitions are disabled for
> all virtual thread,
> otherwise they are disabled for a specific thread if it is virtual.
> The call to `JvmtiEventController::set_user_enabled()` makes a call to
> `recompute_enabled()` at the end of its work to do a required bookkeeping. As
> part of this work, the `recompute_thread_enabled(state)` is called for each
> thread from the `ThreadsListHandle`, not only for the given `event_thread`:
>
> ThreadsListHandle tlh;
> for (; state != nullptr; state = state->next()) {
> any_env_thread_enabled |= recompute_thread_enabled(state);
> }
>
> This can cause crashes as VTMS transitions for other virtual threads are
> allowed.
> Crashes are observed in this small function:
>
> bool is_interp_only_mode() {
> return _thread == nullptr ? _saved_interp_only_mode != 0 :
> _thread->is_interp_only_mode();
> }
>
> In a case `_thread != nullptr` then the call needs to be executed:
> `_thread->is_interp_only_mode()`.
> But the filed `_thread` can be already changed to `nullptr` by a VTMS
> transition.
>
> The fix is to always disable all transitions.
> Thanks to Dan and Patricio for great analysis of this crash!
>
> Testing:
> - In progress: mach5 tiers 1-6
Marked as reviewed by cjplummer (Reviewer).
-
PR Review: https://git.openjdk.org/jdk/pull/14728#pullrequestreview-1507636034
Integrated: 8310993: Missing @since tags in jdk.attach
On Fri, 30 Jun 2023 08:44:22 GMT, Kevin Walls wrote: > Simple since tag addition in selected files. This pull request has now been integrated. Changeset: 19601ebe Author:Kevin Walls URL: https://git.openjdk.org/jdk/commit/19601ebe9192773a1a47ec3e003b9f1a6808d3de Stats: 11 lines in 4 files changed: 7 ins; 0 del; 4 mod 8310993: Missing @since tags in jdk.attach Reviewed-by: sspitsyn, cjplummer - PR: https://git.openjdk.org/jdk/pull/14724
Re: RFR: 8310993: Missing @since tags in jdk.attach
On Fri, 30 Jun 2023 08:44:22 GMT, Kevin Walls wrote: > Simple since tag addition in selected files. Thanks for the reviews! - PR Comment: https://git.openjdk.org/jdk/pull/14724#issuecomment-1614982322
Re: [jdk21] RFR: 8310380: Handle problems in core-related tests on macOS when codesign tool does not work
On Fri, 30 Jun 2023 12:35:27 GMT, Matthias Baesken wrote: > 8310380: Handle problems in core-related tests on macOS when codesign tool > does not work Marked as reviewed by cjplummer (Reviewer). - PR Review: https://git.openjdk.org/jdk21/pull/87#pullrequestreview-1507621309
Re: RFR: 8310993: Missing @since tags in jdk.attach
On Fri, 30 Jun 2023 08:44:22 GMT, Kevin Walls wrote: > Simple since tag addition in selected files. Marked as reviewed by cjplummer (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/14724#pullrequestreview-1507616881
Re: RFR: 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit [v6]
On Fri, 30 Jun 2023 14:35:10 GMT, Doug Simon wrote: > > But, please, activate GHA testing for this branch. > > Isn't GHA a strict subset of or equal to tier1 mach5 testing? If so, what's > the point of doing redundant testing? It builds and tests configurations (32-bit) we don't have in our testing. - PR Comment: https://git.openjdk.org/jdk/pull/14641#issuecomment-1614962751
Re: RFR: 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit [v8]
On Fri, 30 Jun 2023 15:15:16 GMT, Doug Simon wrote: >> The VMSupport class is required for translating an exception between the >> HotSpot and libgraal heaps. >> Loading it lazily can result in a loading exception, obscuring the exception >> being translated. >> To avoid this, VMSupport is loaded eagerly along with the other vmClasses. > > Doug Simon has updated the pull request incrementally with one additional > commit since the last revision: > > fix warning about conversion from size_t to int - take 2 Good. - Marked as reviewed by kvn (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14641#pullrequestreview-1507604563
Re: RFR: 8303086: SIGSEGV in JavaThread::is_interp_only_mode()
On Fri, 30 Jun 2023 11:27:58 GMT, Serguei Spitsyn wrote:
> The JVMTI function `SetEventNotificationMode` can set notification mode
> globally (`event_thread == nullptr`) for all threads or for a specific thread
> (`event_thread != nullptr`). To get a stable mount/unmount vision of virtual
> threads a JvmtiVTMSTransitionDisabler helper object is created :
> `JvmtiVTMSTransitionDisabler disabler(event_thread);`
>
> In a case if `event_thread == nullptr` the VTMS transitions are disabled for
> all virtual thread,
> otherwise they are disabled for a specific thread if it is virtual.
> The call to `JvmtiEventController::set_user_enabled()` makes a call to
> `recompute_enabled()` at the end of its work to do a required bookkeeping. As
> part of this work, the `recompute_thread_enabled(state)` is called for each
> thread from the `ThreadsListHandle`, not only for the given `event_thread`:
>
> ThreadsListHandle tlh;
> for (; state != nullptr; state = state->next()) {
> any_env_thread_enabled |= recompute_thread_enabled(state);
> }
>
> This can cause crashes as VTMS transitions for other virtual threads are
> allowed.
> Crashes are observed in this small function:
>
> bool is_interp_only_mode() {
> return _thread == nullptr ? _saved_interp_only_mode != 0 :
> _thread->is_interp_only_mode();
> }
>
> In a case `_thread != nullptr` then the call needs to be executed:
> `_thread->is_interp_only_mode()`.
> But the filed `_thread` can be already changed to `nullptr` by a VTMS
> transition.
>
> The fix is to always disable all transitions.
> Thanks to Dan and Patricio for great analysis of this crash!
>
> Testing:
> - In progress: mach5 tiers 1-6
Looks good to me!
Patricio
-
Marked as reviewed by pchilanomate (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/14728#pullrequestreview-1507404209
Re: RFR: 8311043: Remove trailing blank lines in source files
On Wed, 28 Jun 2023 16:54:51 GMT, Leo Korinth wrote: > Remove trailing "blank" lines in source files. > > I like to use global-whitespace-cleanup-mode, but I can not use it if the > files are "dirty" to begin with. This fix will make more files "clean". I > also considered adding a check for this in jcheck for Skara, however it seems > jcheck code handling hunks does not track end-of-files. Thus I will only > clean the files. > > The fix removes trailing lines matching ^[[:space:]]*$ in > > - *.java > - *.cpp > - *.hpp > - *.c > - *.h > > I have applied the following bash script to each file: > > file="$1" > > while [[ $(tail -n 1 "$file") =~ ^[[:space:]]*$ ]]; do > truncate -s -1 "$file" > done > > `git diff --ignore-space-change --ignore-blank-lines master` displays no > changes > `git diff --ignore-blank-lines master` displays one change Ending the file with a blank line? I would not have expected that at all. I expect a single EOL at the end of the file; from a visual POV, the last line of non-blank text ends with an EOL. No more, no less. - PR Comment: https://git.openjdk.org/jdk/pull/14698#issuecomment-1614806396
Re: RFR: 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit [v8]
> The VMSupport class is required for translating an exception between the > HotSpot and libgraal heaps. > Loading it lazily can result in a loading exception, obscuring the exception > being translated. > To avoid this, VMSupport is loaded eagerly along with the other vmClasses. Doug Simon has updated the pull request incrementally with one additional commit since the last revision: fix warning about conversion from size_t to int - take 2 - Changes: - all: https://git.openjdk.org/jdk/pull/14641/files - new: https://git.openjdk.org/jdk/pull/14641/files/5bb3b529..11a2dad5 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=14641&range=07 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=14641&range=06-07 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/14641.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14641/head:pull/14641 PR: https://git.openjdk.org/jdk/pull/14641
RFR: 8311102: Write annotations in the classfile dumped by SA
Please review this PR that enables ClassWriter to write annotations to the class file being dumped. The fields annotations are stored in `Annotations::_fields_annotations` which is of type `Array*>`. There is no class in SA that can represent it. I have added ArrayOfU1Array to correspond to the type `Array*>` and it works. I believe there are better approaches but that would require a bit more refactoring of the classes representing Array types. I will leave that for future work for now. Testing: `test/hotspot/jtreg/serviceability/sa` and `test/jdk/sun/tools/jhsdb` Tested it manually by dumping j.l.String class and comparing the annotations with the original class using javap. The test case mentioned in [JDK-8311101](https://bugs.openjdk.org/browse/JDK-8311101) would provide better coverage. - Commit messages: - 8311102: Write annotations in the classfile dumped by SA Changes: https://git.openjdk.org/jdk/pull/14735/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=14735&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8311102 Stats: 379 lines in 9 files changed: 376 ins; 1 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/14735.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14735/head:pull/14735 PR: https://git.openjdk.org/jdk/pull/14735
Re: RFR: 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit [v6]
On Thu, 29 Jun 2023 20:06:19 GMT, Doug Simon wrote: >> The VMSupport class is required for translating an exception between the >> HotSpot and libgraal heaps. >> Loading it lazily can result in a loading exception, obscuring the exception >> being translated. >> To avoid this, VMSupport is loaded eagerly along with the other vmClasses. > > Doug Simon 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 six additional commits since > the last revision: > > - [skip ci] Merge remote-tracking branch 'openjdk-jdk/master' into > JDK-8310829 > - [skip ci] handle pending HotSpot exception closer to site causing exception > - revert to lazy loading of VMSupport > - each exception translation failure should trigger a JVMCI event > - try harder to show nested exception during exception translation > - resolve VMSupport at bootstrap to avoid nested exception in > ExceptionTranslation::doit I have fixed the warning on Windows: 5bb3b529d36c906ac861e5ebf1b861dbb35bfe2c > But, please, activate GHA testing for this branch. Isn't GHA a strict subset of or equal to tier1 mach5 testing? If so, what's the point of doing redundant testing? - PR Comment: https://git.openjdk.org/jdk/pull/14641#issuecomment-1614746764
Re: RFR: 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit [v7]
> The VMSupport class is required for translating an exception between the > HotSpot and libgraal heaps. > Loading it lazily can result in a loading exception, obscuring the exception > being translated. > To avoid this, VMSupport is loaded eagerly along with the other vmClasses. Doug Simon has updated the pull request incrementally with one additional commit since the last revision: fix warning about conversion from size_t to int - Changes: - all: https://git.openjdk.org/jdk/pull/14641/files - new: https://git.openjdk.org/jdk/pull/14641/files/e46a6a17..5bb3b529 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=14641&range=06 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=14641&range=05-06 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/14641.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14641/head:pull/14641 PR: https://git.openjdk.org/jdk/pull/14641
Re: RFR: 8310948: Fix ignored-qualifiers warning in Hotspot
On Tue, 27 Jun 2023 12:22:43 GMT, Daniel Jeliński wrote: > Please review this attempt to fix ignored-qualifiers warning. > > Example warnings: > > src/hotspot/share/oops/method.hpp:413:19: warning: 'volatile' type qualifier > on return type has no effect [-Wignored-qualifiers] >CompiledMethod* volatile code() const; >^ > > > src/hotspot/share/jfr/periodic/jfrModuleEvent.cpp:65:20: warning: type > qualifiers ignored on cast result type [-Wignored-qualifiers] > 65 | event.set_source((const ModuleEntry* const)from_module); >|^ > > > The proposed fix removes the ignored qualifiers. > In a few AD files I replaced `const` with `constexpr` where I noticed that > the method is returning a compile-time constant, and other platforms use > `constexpr` on the same method. > > Release, debug and slowdebug builds on Aarch64 / x64 and Mac / Linux complete > without errors. Cross-compile GHA builds also pass. Can I get another +1 on this? or should I proceed with splitting? - PR Comment: https://git.openjdk.org/jdk/pull/14674#issuecomment-1614738114
Re: RFR: 8311077: Fix -Wconversion warnings in jvmti code [v3]
On Fri, 30 Jun 2023 02:18:14 GMT, David Holmes wrote: >> Coleen Phillimore has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Add a comment about u1 cast to new_index for the ldc bytecode. > > Sorry Coleen but this raises a lot of questions for me. I expect there are a > few ways to silence these conversion warnings but many of the proposed > changes don't look semantically right to me. I realize that we have a lot of > inconsistencies in the way we declare and use types and that the proposed > changes may be the most minimal way to fix things, but it is better to type > them correctly from the start IMO and only cast at the "boundaries" if it > requires a change to propagate too far. @dholmes-ora The reason that these changes are broken up like this is because there are a huge amount of -Wconversion warnings and to do it right, we need to examine where the warnings are to see if there are bugs, and to correct the code to either use the right types or allow the cast with an assertion check if necessary. The changes are not designed to be completely minimal but balanced so that people can review them and that changes that aren't necessary aren't made. We pass around 'int' as a parameter type for values that are u2, for example. It really matters most when we're storing that value or using it in a context that requires the narrow type. If we're using it as an index which most constant pool and constant pool cache indices are used as, we can keep it as an int. A u2 parameter is promoted to int so it's the same thing. I hope I've answered your questions about these specific types in this change. - PR Comment: https://git.openjdk.org/jdk/pull/14710#issuecomment-1614635744
Re: RFR: 8311077: Fix -Wconversion warnings in jvmti code [v4]
On Fri, 30 Jun 2023 02:11:11 GMT, David Holmes wrote: >> I had to change these two lines because BytecodeStream::get_index_u2 returns >> an int, so got the warning and this didn't need to be declared with u2. >> get_index_u2() could be fixed to return a u2 but I didn't want to go that >> far as no casts were involved in this change. > > I think this change looks "wrong" - the indices are supposed to be u2's, if > the function returns an int that seems an error. Fixing these functions will have fallout further down in the code, so I didn't want to do this. These functions are used in a lot of places in the code where int is used for convenience. int is a convenient type. This didn't require a cast because the function klass_at_noresolve()'s parameter is an int. This isn't wrong. - PR Review Comment: https://git.openjdk.org/jdk/pull/14710#discussion_r1247802199
Re: RFR: 8311077: Fix -Wconversion warnings in jvmti code [v3]
On Fri, 30 Jun 2023 01:52:40 GMT, David Holmes wrote:
>> Coleen Phillimore has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Add a comment about u1 cast to new_index for the ldc bytecode.
>
> src/hotspot/share/oops/constMethod.hpp line 327:
>
>> 325:
>> 326: // code size
>> 327: u2 code_size() const { return _code_size; }
>
> The `code_length` of a `Code` attribute is `u4` - why is `u2` appropriate
> here?
The code_length attribute is defined in the class file specification as u4, but
further specifies that:
code_length
The value of the code_length item gives the number of bytes in the code array
for this method.
The value of code_length must be greater than zero (as the code array must not
be empty) and less than 65536.
So we store code_length as a u2 to save space in the ConstMethod. That's the
size of the field _code_size. There are checks in set_code_size and in the
ClassFileParser to verify this:
guarantee_property(code_length > 0 && code_length <= MAX_CODE_SIZE,
"Invalid method Code length %u in class file %s",
code_length, CHECK_NULL);
And further when assigning code_size:
void set_code_size(int size) {
assert(max_method_code_size < (1 << 16),
"u2 is too small to hold method code size in general");
assert(0 <= size && size <= max_method_code_size, "invalid code size");
_code_size = (u2)size;
}
At one point, there was discussion of expanding the code_size to u4. If that
decision is made, we should change these types. I made the return type match
the field type because it's copied when we create a new method. I did that to
avoid a cast.
> src/hotspot/share/oops/method.hpp line 257:
>
>> 255:
>> 256: // code size
>> 257: u2 code_size() const { return
>> constMethod()->code_size(); }
>
> Again why is this not u4?
see above.
> src/hotspot/share/oops/method.hpp line 269:
>
>> 267: // return original max stack size for method verification
>> 268: u2 verifier_max_stack() const { return
>> constMethod()->max_stack(); }
>> 269: int max_stack() const { return
>> constMethod()->max_stack() + extra_stack_entries(); }
>
> So this has to be greater than a `u2` because of the extra entries?
Yes.
> src/hotspot/share/prims/jvmtiClassFileReconstituter.cpp line 310:
>
>> 308: write_attribute_name_index("MethodParameters");
>> 309: write_u4(size);
>> 310: write_u1((u1)length);
>
> What is the rule for using a plain cast versus a checked_cast?
If the code above explicitly checks for the value of the int, then I don't use
checked_cast<>. The explicit checks are more descriptive of the expectations
and that assert would be better than the checked_cast<> assert which is
somewhat generic.
> src/hotspot/share/prims/jvmtiClassFileReconstituter.cpp line 402:
>
>> 400: length += sizeof(u2); // bootstrap_method_ref
>> 401: length += sizeof(u2); // num_bootstrap_arguments
>> 402: length += (int)sizeof(u2) * num_bootstrap_arguments; //
>> bootstrap_arguments[num_bootstrap_arguments]
>
> Not clear why the int cast is used given length is a u4?
I could change that to u4. It's the same thing. It just can't be size_t. I
really don't know why the C++ compiler gives this a Wconversion warning and not
the lines above it.
src/hotspot/share/prims/jvmtiClassFileReconstituter.cpp:402:12: warning:
conversion from 'long unsigned int' to 'u4' {aka 'unsigned int'} may change
value [-Wconversion]
402 | length += sizeof(u2) * num_bootstrap_arguments; //
bootstrap_arguments[num_bootstrap_arguments]
| ~~~^~~
> src/hotspot/share/prims/jvmtiRawMonitor.cpp line 83:
>
>> 81: bool
>> 82: JvmtiRawMonitor::is_valid() {
>> 83: uint64_t value = 0;
>
> Why `uint64_t` here when `JvmtiEnvBase::is_valid` uses `jlong`?
It's the same type. Since we're not already using a jlong in this function, I
used the preferable uint64_t. I can make it jlong if you prefer that.
> src/hotspot/share/prims/jvmtiRawMonitor.cpp line 385:
>
>> 383: OrderAccess::fence();
>> 384:
>> 385: int save = _recursions;
>
> `_recursions` is `intx`
JvmtiRawMonitor _recursions is an int. Maybe it shouldn't be. You could file
an RFE to change that if it's wrong.
volatile int _recursions; // recursion count, 0 for first entry
> src/hotspot/share/prims/jvmtiTrace.cpp line 205:
>
>> 203: _trace_flags[i] |= bits;
>> 204: } else {
>> 205: _trace_flags[i] &= (jbyte)~bits;
>
> Looks strange that only this use of bits needs the cast?
Yes the ~ operator promotes to int.
https://en.cppreference.com/w/cpp/language/operator_arithmetic
Conversions
If the operand passed to an arithmetic operator is integral or unscoped
enumeration type, then before any oth
Re: RFR: 8311077: Fix -Wconversion warnings in jvmti code [v3]
On Thu, 29 Jun 2023 17:29:50 GMT, Coleen Phillimore wrote: >> Please review change for mostly fixing return types in the constant pool and >> metadata to fix -Wconversion warnings in JVMTI code. The order of >> preference for changes are: 1. change the types to more distinct types >> (fields in the constant pool are u2 because that's their size in the >> classfile), 2. add direct int casts if the value has been checked in asserts >> above, and 3. checked_cast<> if not verified, and 4. added some >> pointer_delta_as_ints where needed. >> Tested with tier1-4. > > Coleen Phillimore has updated the pull request incrementally with one > additional commit since the last revision: > > Add a comment about u1 cast to new_index for the ldc bytecode. The task is NOT to just silence the conversion warnings but to correct the types to be more precise to not get conversion warnings. That's what this change does. I think I've answered your questions about the types chosen. - PR Review: https://git.openjdk.org/jdk/pull/14710#pullrequestreview-1507060957
Re: RFR: 8311077: Fix -Wconversion warnings in jvmti code [v4]
> Please review change for mostly fixing return types in the constant pool and > metadata to fix -Wconversion warnings in JVMTI code. The order of preference > for changes are: 1. change the types to more distinct types (fields in the > constant pool are u2 because that's their size in the classfile), 2. add > direct int casts if the value has been checked in asserts above, and 3. > checked_cast<> if not verified, and 4. added some pointer_delta_as_ints where > needed. > Tested with tier1-4. Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision: David's suggestions. - Changes: - all: https://git.openjdk.org/jdk/pull/14710/files - new: https://git.openjdk.org/jdk/pull/14710/files/42781421..d07a212c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=14710&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=14710&range=02-03 Stats: 6 lines in 4 files changed: 0 ins; 0 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/14710.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14710/head:pull/14710 PR: https://git.openjdk.org/jdk/pull/14710
[jdk21] RFR: 8310380: Handle problems in core-related tests on macOS when codesign tool does not work
8310380: Handle problems in core-related tests on macOS when codesign tool does not work - Commit messages: - Backport 39c104df44f17c1d65e35becd4272f73e2c6610c Changes: https://git.openjdk.org/jdk21/pull/87/files Webrev: https://webrevs.openjdk.org/?repo=jdk21&pr=87&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8310380 Stats: 56 lines in 4 files changed: 37 ins; 12 del; 7 mod Patch: https://git.openjdk.org/jdk21/pull/87.diff Fetch: git fetch https://git.openjdk.org/jdk21.git pull/87/head:pull/87 PR: https://git.openjdk.org/jdk21/pull/87
Integrated: JDK-8311026: Some G1 specific tests do not set -XX:+UseG1GC
On Fri, 30 Jun 2023 08:11:47 GMT, Matthias Baesken wrote: > Most G1 tests set -XX:+UseG1GC, but a few (e.g. > gc/g1/TestVerificationInConcurrentCycle.java) miss that. > This is usually just fine and no problem because G1 is the default anyway. > However in some cases, where a custom JVM changes the default GC, those tests > start to fail which is not really necessary. This pull request has now been integrated. Changeset: a7d168b5 Author:Matthias Baesken URL: https://git.openjdk.org/jdk/commit/a7d168b522bb05345a40ae1fb18942ba663d3182 Stats: 4 lines in 3 files changed: 0 ins; 0 del; 4 mod 8311026: Some G1 specific tests do not set -XX:+UseG1GC Reviewed-by: sspitsyn, tschatzl - PR: https://git.openjdk.org/jdk/pull/14722
Re: RFR: JDK-8311026: Some G1 specific tests do not set -XX:+UseG1GC
On Fri, 30 Jun 2023 08:11:47 GMT, Matthias Baesken wrote: > Most G1 tests set -XX:+UseG1GC, but a few (e.g. > gc/g1/TestVerificationInConcurrentCycle.java) miss that. > This is usually just fine and no problem because G1 is the default anyway. > However in some cases, where a custom JVM changes the default GC, those tests > start to fail which is not really necessary. Hi Thomas and Serguei, thanks for the reviews ! - PR Comment: https://git.openjdk.org/jdk/pull/14722#issuecomment-1614525324
Re: RFR: JDK-8310550: Adjust references to rt.jar [v3]
> There are a few references to rt.jar in comments and in the codebase itself. > Some of them might be removed or adjusted. Matthias Baesken has updated the pull request incrementally with one additional commit since the last revision: remove import - Changes: - all: https://git.openjdk.org/jdk/pull/14593/files - new: https://git.openjdk.org/jdk/pull/14593/files/6665f60b..9b2232a7 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=14593&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=14593&range=01-02 Stats: 2 lines in 1 file changed: 1 ins; 1 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/14593.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14593/head:pull/14593 PR: https://git.openjdk.org/jdk/pull/14593
RFR: 8303086: SIGSEGV in JavaThread::is_interp_only_mode()
The JVMTI function `SetEventNotificationMode` can set notification mode
globally (`event_thread == nullptr`) for all threads or for a specific thread
(`event_thread != nullptr`). To get a stable mount/unmount vision of virtual
threads a JvmtiVTMSTransitionDisabler helper object is created :
`JvmtiVTMSTransitionDisabler disabler(event_thread);`
In a case if `event_thread == nullptr` the VTMS transitions are disabled for
all virtual thread,
otherwise they are disabled for a specific thread if it is virtual.
The call to `JvmtiEventController::set_user_enabled()` makes a call to
`recompute_enabled()` at the end of its work to do a required bookkeeping. As
part of this work, the `recompute_thread_enabled(state)` is called for each
thread from the `ThreadsListHandle`, not only for the given specific
`event_thread`.
This can cause crashes as VTMS transitions for other virtual threads are
allowed.
Crashes are observed in this small function:
bool is_interp_only_mode() {
return _thread == nullptr ? _saved_interp_only_mode != 0 :
_thread->is_interp_only_mode();
}
In a case `_thread != nullptr` then the call needs to be executed:
`_thread->is_interp_only_mode()`.
But the filed `_thread` can be already changed to `nullptr` by a VTMS
transition.
The fix is to always disable all transitions.
Thanks to Dan and Patricio for great analysis of this crash!
Testing:
- In progress: mach5 tiers 1-6
-
Commit messages:
- 8303086: SIGSEGV in JavaThread::is_interp_only_mode()
Changes: https://git.openjdk.org/jdk/pull/14728/files
Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=14728&range=00
Issue: https://bugs.openjdk.org/browse/JDK-8303086
Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
Patch: https://git.openjdk.org/jdk/pull/14728.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/14728/head:pull/14728
PR: https://git.openjdk.org/jdk/pull/14728
Re: RFR: JDK-8311026: Some G1 specific tests do not set -XX:+UseG1GC
On Fri, 30 Jun 2023 08:11:47 GMT, Matthias Baesken wrote: > Most G1 tests set -XX:+UseG1GC, but a few (e.g. > gc/g1/TestVerificationInConcurrentCycle.java) miss that. > This is usually just fine and no problem because G1 is the default anyway. > However in some cases, where a custom JVM changes the default GC, those tests > start to fail which is not really necessary. Marked as reviewed by tschatzl (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/14722#pullrequestreview-1507020443
Re: RFR: JDK-8310550: Adjust references to rt.jar [v2]
> There are a few references to rt.jar in comments and in the codebase itself. > Some of them might be removed or adjusted. Matthias Baesken has updated the pull request incrementally with one additional commit since the last revision: Adjust comment in src/java.sql/share/classes/java/sql/DriverManager.java - Changes: - all: https://git.openjdk.org/jdk/pull/14593/files - new: https://git.openjdk.org/jdk/pull/14593/files/5d52b4cb..6665f60b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=14593&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=14593&range=00-01 Stats: 6 lines in 1 file changed: 1 ins; 2 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/14593.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14593/head:pull/14593 PR: https://git.openjdk.org/jdk/pull/14593
Re: RFR: JDK-8310550: Adjust references to rt.jar [v2]
On Wed, 28 Jun 2023 13:22:20 GMT, Matthias Baesken wrote:
>>> Hi Alan, regarding usage of class VM I get 'package jdk.internal.misc is
>>> declared in module java.base, which does not export it to module java.sql'
>>> Is there any concern to export it as well to module java.sql ? And btw did
>>> you mean to use it like this, in the if ?
>>>
>>> `if (callerCL == null || VM.isSystemDomainLoader(callerCL)) { callerCL =
>>> Thread.currentThread().getContextClassLoader(); }`
>>
>> It was just a passing comment, I didn't meant to suggest changing it as part
>> of this PR. We have always think twice before adding qualified exports from
>> java.base and this is case where java.sql is very "non-core", we don't want
>> to give it any access to java.base internals.
>
> Hi Alan, thanks for clarifying. So I should only adjust the comment, correct
> ?
Hi Alan, I adjusted the comment in DriverManager.java .
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14593#discussion_r1247686721
Re: RFR: 8310993: Missing @since tags in jdk.attach
On Fri, 30 Jun 2023 08:44:22 GMT, Kevin Walls wrote: > Simple since tag addition in selected files. Looks good. Thanks, Serguei - Marked as reviewed by sspitsyn (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14724#pullrequestreview-1506841290
RFR: 8310993: Missing @since tags in jdk.attach
Simple since tag addition in selected files. - Commit messages: - 8310993: Missing @since tags in jdk.attach Changes: https://git.openjdk.org/jdk/pull/14724/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=14724&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8310993 Stats: 11 lines in 4 files changed: 7 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/14724.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14724/head:pull/14724 PR: https://git.openjdk.org/jdk/pull/14724
Integrated: 8311000: missing @since info in jdk.management
On Thu, 29 Jun 2023 11:48:43 GMT, Kevin Walls wrote: > Simple addition of a doc tag. > > src/share/classes/com/sun/management/GarbageCollectionNotificationInfo.java > is introduced in jdk7 by > 7036199: Adding a notification to the implementation of > GarbageCollectorMXBeans This pull request has now been integrated. Changeset: c08c9831 Author:Kevin Walls URL: https://git.openjdk.org/jdk/commit/c08c9831df2428e9d083a56eda5ebf00623ba961 Stats: 3 lines in 1 file changed: 2 ins; 0 del; 1 mod 8311000: missing @since info in jdk.management Reviewed-by: rriggs, mli - PR: https://git.openjdk.org/jdk/pull/14708
Re: RFR: 8311000: missing @since info in jdk.management [v2]
> Simple addition of a doc tag. > > src/share/classes/com/sun/management/GarbageCollectionNotificationInfo.java > is introduced in jdk7 by > 7036199: Adding a notification to the implementation of > GarbageCollectorMXBeans Kevin Walls has updated the pull request incrementally with one additional commit since the last revision: (C) - Changes: - all: https://git.openjdk.org/jdk/pull/14708/files - new: https://git.openjdk.org/jdk/pull/14708/files/b4c36af7..30632c98 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=14708&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=14708&range=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/14708.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14708/head:pull/14708 PR: https://git.openjdk.org/jdk/pull/14708
Re: RFR: 8311000: missing @since info in jdk.management
On Thu, 29 Jun 2023 11:48:43 GMT, Kevin Walls wrote: > Simple addition of a doc tag. > > src/share/classes/com/sun/management/GarbageCollectionNotificationInfo.java > is introduced in jdk7 by > 7036199: Adding a notification to the implementation of > GarbageCollectorMXBeans Thanks for the reviews. - PR Comment: https://git.openjdk.org/jdk/pull/14708#issuecomment-1614313233
Re: RFR: 8310988: Missing @since tags in java.management.rmi [v2]
> Simple doc tag addition. > > These are files which describe an interface that has been with us since 1.5. > The files themselves were previously generated at build time, but have been > in the repo since jdk15. But the API is since 1.5. > > The other file mentioned in the bug is not a public class and has no javadoc > generated, ignoring that one. Kevin Walls has updated the pull request incrementally with two additional commits since the last revision: - (C) - (C) - Changes: - all: https://git.openjdk.org/jdk/pull/14714/files - new: https://git.openjdk.org/jdk/pull/14714/files/5bc9cc12..634cc78d Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=14714&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=14714&range=00-01 Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/14714.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14714/head:pull/14714 PR: https://git.openjdk.org/jdk/pull/14714
Integrated: 8310988: Missing @since tags in java.management.rmi
On Thu, 29 Jun 2023 16:08:15 GMT, Kevin Walls wrote: > Simple doc tag addition. > > These are files which describe an interface that has been with us since 1.5. > The files themselves were previously generated at build time, but have been > in the repo since jdk15. But the API is since 1.5. > > The other file mentioned in the bug is not a public class and has no javadoc > generated, ignoring that one. This pull request has now been integrated. Changeset: 660cd256 Author:Kevin Walls URL: https://git.openjdk.org/jdk/commit/660cd256b72154b966208174dbf9abe95c9bfd60 Stats: 6 lines in 2 files changed: 4 ins; 0 del; 2 mod 8310988: Missing @since tags in java.management.rmi Reviewed-by: rriggs, alanb, jjg, mli - PR: https://git.openjdk.org/jdk/pull/14714
Re: RFR: 8310988: Missing @since tags in java.management.rmi
On Thu, 29 Jun 2023 16:08:15 GMT, Kevin Walls wrote: > Simple doc tag addition. > > These are files which describe an interface that has been with us since 1.5. > The files themselves were previously generated at build time, but have been > in the repo since jdk15. But the API is since 1.5. > > The other file mentioned in the bug is not a public class and has no javadoc > generated, ignoring that one. Thanks for the reviews! - PR Comment: https://git.openjdk.org/jdk/pull/14714#issuecomment-1614310538
Re: RFR: JDK-8311026: Some G1 specific tests do not set -XX:+UseG1GC
On Fri, 30 Jun 2023 08:11:47 GMT, Matthias Baesken wrote: > Most G1 tests set -XX:+UseG1GC, but a few (e.g. > gc/g1/TestVerificationInConcurrentCycle.java) miss that. > This is usually just fine and no problem because G1 is the default anyway. > However in some cases, where a custom JVM changes the default GC, those tests > start to fail which is not really necessary. Looks good. Thanks, Serguei - Marked as reviewed by sspitsyn (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14722#pullrequestreview-1506747461
RFR: JDK-8311026: Some G1 specific tests do not set -XX:+UseG1GC
Most G1 tests set -XX:+UseG1GC, but a few (e.g. gc/g1/TestVerificationInConcurrentCycle.java) miss that. This is usually just fine and no problem because G1 is the default anyway. However in some cases, where a custom JVM changes the default GC, those tests start to fail which is not really necessary. - Commit messages: - JDK-8311026 Changes: https://git.openjdk.org/jdk/pull/14722/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=14722&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8311026 Stats: 4 lines in 3 files changed: 0 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/14722.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14722/head:pull/14722 PR: https://git.openjdk.org/jdk/pull/14722
