Re: RFR: 8330171: Lazy W^X switch implementation
On Fri, 12 Apr 2024 14:40:05 GMT, Sergey Nazarkin wrote: > An alternative for preemptively switching the W^X thread mode on macOS with > an AArch64 CPU. This implementation triggers the switch in response to the > SIGBUS signal if the *si_addr* belongs to the CodeCache area. With this > approach, it is now feasible to eliminate all WX guards and avoid potentially > costly operations. However, no significant improvement or degradation in > performance has been observed. Additionally, considering the issue with > AsyncGetCallTrace, the patched JVM has been successfully operated with > [asgct_bottom](https://github.com/parttimenerd/asgct_bottom) and > [async-profiler](https://github.com/async-profiler/async-profiler). > > Additional testing: > - [x] MacOS AArch64 server fastdebug *gtets* > - [ ] MacOS AArch64 server fastdebug *jtreg:hotspot:tier4* > - [ ] Benchmarking > > @apangin and @parttimenerd could you please check the patch on your > scenarios?? What about granting `WXWrite` only if the current thread is in `_thread_in_vm`? That would be more restrictive and roughly equivalent how it currently works. Likely there are some places then that should be granted `WXWrite` eagerly because they need `WXWrite` without `_thread_in_vm`. E.g. the JIT compiler threads should have `WXWrite` and never `WXExec` (I assume) which should be checked in the signal handler. - PR Comment: https://git.openjdk.org/jdk/pull/18762#issuecomment-2065983074
Integrated: 8327990: [macosx-aarch64] Various tests fail with -XX:+AssertWXAtThreadSync
On Tue, 12 Mar 2024 15:05:00 GMT, Richard Reingruber wrote: > Updated (2024-03-20): > > This PR adds switching to `WXWrite` mode before entering the vm where it is > missing. > > With the changes the following jtreg tests succeed with AssertWXAtThreadSync > enabled. > > * hotspot tier 1-4 > * jdk tier 1-4 > * langtools > * jaxp This pull request has now been integrated. Changeset: e41bc42d Author:Richard Reingruber URL: https://git.openjdk.org/jdk/commit/e41bc42deb22615c9b93ee639d04e9ed2bd57f64 Stats: 13 lines in 8 files changed: 11 ins; 0 del; 2 mod 8327990: [macosx-aarch64] Various tests fail with -XX:+AssertWXAtThreadSync Reviewed-by: dholmes, stuefe, mdoerr, tholenstein, aph - PR: https://git.openjdk.org/jdk/pull/18238
Re: RFR: 8327990: [macosx-aarch64] Various tests fail with -XX:+AssertWXAtThreadSync [v4]
On Wed, 20 Mar 2024 14:06:04 GMT, Richard Reingruber wrote: >> Updated (2024-03-20): >> >> This PR adds switching to `WXWrite` mode before entering the vm where it is >> missing. >> >> With the changes the following jtreg tests succeed with AssertWXAtThreadSync >> enabled. >> >> * hotspot tier 1-4 >> * jdk tier 1-4 >> * langtools >> * jaxp > > Richard Reingruber 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 8327990_jfr_enters_vm_without_wxwrite > - 2 more locations > - Merge branch 'master' into 8327990_jfr_enters_vm_without_wxwrite > - Set WXWrite at more locations > - Switch to WXWrite before entering the vm Tests with AssertWXAtThreadSync are clean now. Thanks! - PR Comment: https://git.openjdk.org/jdk/pull/18238#issuecomment-2012399791
Re: RFR: 8327990: [macosx-aarch64] Various tests fail with -XX:+AssertWXAtThreadSync [v4]
> This pr changes `JfrJvmtiAgent::retransform_classes()` and `jfr_set_enabled` > to switch to `WXWrite` before transitioning to the vm. > > Testing: > make test TEST=jdk/jfr/event/runtime/TestClassLoadEvent.java > TEST_VM_OPTS=-XX:+AssertWXAtThreadSync > make test TEST=compiler/intrinsics/klass/CastNullCheckDroppingsTest.java > TEST_VM_OPTS=-XX:+AssertWXAtThreadSync > > More tests are pending. Richard Reingruber 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 8327990_jfr_enters_vm_without_wxwrite - 2 more locations - Merge branch 'master' into 8327990_jfr_enters_vm_without_wxwrite - Set WXWrite at more locations - Switch to WXWrite before entering the vm - Changes: - all: https://git.openjdk.org/jdk/pull/18238/files - new: https://git.openjdk.org/jdk/pull/18238/files/1b4d7606..8239638b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18238=03 - incr: https://webrevs.openjdk.org/?repo=jdk=18238=02-03 Stats: 6136 lines in 243 files changed: 2379 ins; 2538 del; 1219 mod Patch: https://git.openjdk.org/jdk/pull/18238.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18238/head:pull/18238 PR: https://git.openjdk.org/jdk/pull/18238
Re: RFR: 8327990: [macosx-aarch64] Various tests fail with -XX:+AssertWXAtThreadSync [v3]
On Tue, 19 Mar 2024 17:46:31 GMT, Tobias Holenstein wrote: >> I've asked this myself (after making the change). >> Being in `WXWrite` mode would be wrong if the thread would execute >> dynamically generated code. There's not too much happening outside the scope >> of the `ThreadInVMfromNative` instances. I see jni calls >> (`GetObjectArrayElement`, `ExceptionOccurred`) and a jvmti call >> (`RetransformClasses`) but these are safe because the callees enter the vm >> right away. We even avoid switching to `WXWrite` and back there. >> So I thought it would be ok to coarsen the WXMode switching. >> But maybe it's still better to avoid any risk especially since there's >> likely no performance effect. > > Or could the `ThreadInVMfromNative tvmfn(THREAD);` in > `check_exception_and_log` be move out to > `JfrJvmtiAgent::retransform_classes`? And then only use one > `ThreadInVMfromNative` in `JfrJvmtiAgent::retransform_classes` I think this would require hoisting the `ThreadInVMfromNative` out of the loop with the `check_exception_and_log` call but then the thread would be in `_thread_in_vm` when doing the `GetObjectArrayElement` jni call which would be wrong. - PR Review Comment: https://git.openjdk.org/jdk/pull/18238#discussion_r1532121487
Re: RFR: 8327990: [macosx-aarch64] Various tests fail with -XX:+AssertWXAtThreadSync [v3]
On Tue, 19 Mar 2024 12:17:22 GMT, David Holmes wrote: > But all this discussion suggests to me that this PR is not really worth > pursuing at this time - IIUC no actual failures are observed other than those > pertaining to AssertWXAtThreadSync and that flag will be gone if we do decide > to be more fine-grained about WX management. I see it differently. This PR is just a simple attempt to get clean test runs with AssertWXAtThreadSync (after fixing an actual crash https://bugs.openjdk.org/browse/JDK-8327036). While the violating locations in this PR might be unlikely to produce actual crashes I think it is worthwhile to have clean testing with AssertWXAtThreadSync because this will help prevent regressions that are more likely. Beyond the trivial fixes of this PR I'm very much in favor of further enhancements as the aforementioned https://bugs.openjdk.org/browse/JDK-8307817. My recommendation would be to remove as much non-constant data from the code cache as possible. - PR Comment: https://git.openjdk.org/jdk/pull/18238#issuecomment-2007709981
Re: RFR: 8327990: [macosx-aarch64] Various tests fail with -XX:+AssertWXAtThreadSync [v3]
> This pr changes `JfrJvmtiAgent::retransform_classes()` and `jfr_set_enabled` > to switch to `WXWrite` before transitioning to the vm. > > Testing: > make test TEST=jdk/jfr/event/runtime/TestClassLoadEvent.java > TEST_VM_OPTS=-XX:+AssertWXAtThreadSync > make test TEST=compiler/intrinsics/klass/CastNullCheckDroppingsTest.java > TEST_VM_OPTS=-XX:+AssertWXAtThreadSync > > More tests are pending. Richard Reingruber 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 three additional commits since the last revision: - Merge branch 'master' into 8327990_jfr_enters_vm_without_wxwrite - Set WXWrite at more locations - Switch to WXWrite before entering the vm - Changes: - all: https://git.openjdk.org/jdk/pull/18238/files - new: https://git.openjdk.org/jdk/pull/18238/files/9075f4be..1b4d7606 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18238=02 - incr: https://webrevs.openjdk.org/?repo=jdk=18238=01-02 Stats: 293771 lines in 434 files changed: 6197 ins; 6221 del; 281353 mod Patch: https://git.openjdk.org/jdk/pull/18238.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18238/head:pull/18238 PR: https://git.openjdk.org/jdk/pull/18238
Re: RFR: 8327990: [macosx-aarch64] Various tests fail with -XX:+AssertWXAtThreadSync [v2]
On Fri, 15 Mar 2024 13:58:15 GMT, Tobias Holenstein wrote: > > As I wrote in JBS, shouldn't this be handled by `ThreadInVMfromNative`? > > I agree. This is something I am investigating at the moment. Ideally, > AssertWXAtThreadSync would also be true by default. I've added a bunch more locations we've seen when testing with AssertWXAtThreadSync. @tobiasholenstein would you think that this PR is actually not needed because you are going to push a better way of handling the WXMode in the near future? How should be handle the issues in released versions (jdk 21, 17, ...)? Will it be possible to backport your work? - PR Comment: https://git.openjdk.org/jdk/pull/18238#issuecomment-2003496165
Re: RFR: 8327990: [macosx-aarch64] JFR enters VM without WXWrite [v2]
> This pr changes `JfrJvmtiAgent::retransform_classes()` and `jfr_set_enabled` > to switch to `WXWrite` before transitioning to the vm. > > Testing: > make test TEST=jdk/jfr/event/runtime/TestClassLoadEvent.java > TEST_VM_OPTS=-XX:+AssertWXAtThreadSync > make test TEST=compiler/intrinsics/klass/CastNullCheckDroppingsTest.java > TEST_VM_OPTS=-XX:+AssertWXAtThreadSync > > More tests are pending. Richard Reingruber has updated the pull request incrementally with one additional commit since the last revision: Set WXWrite at more locations - Changes: - all: https://git.openjdk.org/jdk/pull/18238/files - new: https://git.openjdk.org/jdk/pull/18238/files/b34ad5f3..9075f4be Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18238=01 - incr: https://webrevs.openjdk.org/?repo=jdk=18238=00-01 Stats: 6 lines in 4 files changed: 5 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/18238.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18238/head:pull/18238 PR: https://git.openjdk.org/jdk/pull/18238
Re: RFR: 8327990: [macosx-aarch64] JFR enters VM without WXWrite
On Thu, 14 Mar 2024 14:49:12 GMT, Matthias Baesken wrote: >> This pr changes `JfrJvmtiAgent::retransform_classes()` and >> `jfr_set_enabled` to switch to `WXWrite` before transitioning to the vm. >> >> Testing: >> make test TEST=jdk/jfr/event/runtime/TestClassLoadEvent.java >> TEST_VM_OPTS=-XX:+AssertWXAtThreadSync >> make test TEST=compiler/intrinsics/klass/CastNullCheckDroppingsTest.java >> TEST_VM_OPTS=-XX:+AssertWXAtThreadSync >> >> More tests are pending. > > I noticed a few more asserts (assert(_wx_state == expected) failed: wrong > state) in the jfr area (jdk tier3 jfr tests). > E.g. > > > V [libjvm.dylib+0x8a5d94] JavaThread::check_for_valid_safepoint_state()+0x0 > V [libjvm.dylib+0x3e95b4] > ThreadStateTransition::transition_from_native(JavaThread*, JavaThreadState, > bool)+0x174 > V [libjvm.dylib+0x3e93d0] > ThreadInVMfromNative::ThreadInVMfromNative(JavaThread*)+0x70 > V [libjvm.dylib+0x91a578] JfrRecorderService::emit_leakprofiler_events(long > long, bool, bool)+0xcc > > > > and > > > V [libjvm.dylib+0x8a5d94] JavaThread::check_for_valid_safepoint_state()+0x0 > V [libjvm.dylib+0x3e95b4] > ThreadStateTransition::transition_from_native(JavaThread*, JavaThreadState, > bool)+0x174 > V [libjvm.dylib+0x3e93d0] > ThreadInVMfromNative::ThreadInVMfromNative(JavaThread*)+0x70 > V [libjvm.dylib+0x8d7f74] JfrJavaEventWriter::flush(_jobject*, int, int, > JavaThread*)+0xf8 > j jdk.jfr.internal.JVM.flush(Ljdk/jfr/internal/event/EventWriter;II)V+0 > jdk.jfr@23-internal > j jdk.jfr.internal.event.EventWriter.flush(II)V+3 jdk.jfr@23-internal > > @MBaesken found 2 more locations in jvmti that need switching to `WXWrite` > > ``` > > JvmtiExport::get_jvmti_interface > > GetCarrierThread > > ``` > > Both use `ThreadInVMfromNative`. > > Should we address those 2 more findings in this PR ? Or open a separate JBS > issue ? > I'm leaning towards fixing all locations in this PR even though this will prevent clean backports. Would that be ok? - PR Comment: https://git.openjdk.org/jdk/pull/18238#issuecomment-1999168860
Re: RFR: 8327990: [macosx-aarch64] JFR enters VM without WXWrite
On Tue, 12 Mar 2024 15:05:00 GMT, Richard Reingruber wrote: > This pr changes `JfrJvmtiAgent::retransform_classes()` and `jfr_set_enabled` > to switch to `WXWrite` before transitioning to the vm. > > Testing: > make test TEST=jdk/jfr/event/runtime/TestClassLoadEvent.java > TEST_VM_OPTS=-XX:+AssertWXAtThreadSync > make test TEST=compiler/intrinsics/klass/CastNullCheckDroppingsTest.java > TEST_VM_OPTS=-XX:+AssertWXAtThreadSync > > More tests are pending. @MBaesken found 2 more locations in jvmti that need switching to `WXWrite` JvmtiExport::get_jvmti_interface GetCarrierThread Both use `ThreadInVMfromNative`. - PR Comment: https://git.openjdk.org/jdk/pull/18238#issuecomment-1994930681
Re: RFR: 8327990: [macosx-aarch64] JFR enters VM without WXWrite
On Wed, 13 Mar 2024 12:19:34 GMT, David Holmes wrote: >> This pr changes `JfrJvmtiAgent::retransform_classes()` and >> `jfr_set_enabled` to switch to `WXWrite` before transitioning to the vm. >> >> Testing: >> make test TEST=jdk/jfr/event/runtime/TestClassLoadEvent.java >> TEST_VM_OPTS=-XX:+AssertWXAtThreadSync >> make test TEST=compiler/intrinsics/klass/CastNullCheckDroppingsTest.java >> TEST_VM_OPTS=-XX:+AssertWXAtThreadSync >> >> More tests are pending. > > src/hotspot/share/jfr/instrumentation/jfrJvmtiAgent.cpp line 160: > >> 158: ResourceMark rm(THREAD); >> 159: // WXWrite is needed before entering the vm below and in callee >> methods. >> 160: MACOS_AARCH64_ONLY(ThreadWXEnable __wx(WXWrite, THREAD)); > > I understand you placed this here to cover the transition inside > `create_classes_array` and the immediate one at line 170, but doesn't this > risk having the wrong WX state for code in between? I've asked this myself (after making the change). Being in `WXWrite` mode would be wrong if the thread would execute dynamically generated code. There's not too much happening outside the scope of the `ThreadInVMfromNative` instances. I see jni calls (`GetObjectArrayElement`, `ExceptionOccurred`) and a jvmti call (`RetransformClasses`) but these are safe because the callees enter the vm right away. We even avoid switching to `WXWrite` and back there. So I thought it would be ok to coarsen the WXMode switching. But maybe it's still better to avoid any risk especially since there's likely no performance effect. - PR Review Comment: https://git.openjdk.org/jdk/pull/18238#discussion_r1523305040
Re: RFR: 8327990: [macosx-aarch64] JFR enters VM without WXWrite
On Wed, 13 Mar 2024 09:49:19 GMT, David Holmes wrote: > As I wrote in JBS, shouldn't this be handled by `ThreadInVMfromNative`? (I wanted to publish the PR before answering your comment) This would be reasonable in my opinion. I've hoisted setting `WXWrite` mode in `JfrJvmtiAgent::retransform_classes()` because multiple instances of `ThreadInVMfromNative` are reached. This is likely not even necessary. Still exceptions could be made if there are usages of `ThreadInVMfromNative` where it is needed. While I agree I'd prefer to do it as a separate enhancement. - PR Comment: https://git.openjdk.org/jdk/pull/18238#issuecomment-1994000304
RFR: 8327990: [macosx-aarch64] JFR enters VM without WXWrite
This pr changes `JfrJvmtiAgent::retransform_classes()` and `jfr_set_enabled` to switch to `WXWrite` before transitioning to the vm. Testing: make test TEST=jdk/jfr/event/runtime/TestClassLoadEvent.java TEST_VM_OPTS=-XX:+AssertWXAtThreadSync make test TEST=compiler/intrinsics/klass/CastNullCheckDroppingsTest.java TEST_VM_OPTS=-XX:+AssertWXAtThreadSync More tests are pending. - Commit messages: - Switch to WXWrite before entering the vm Changes: https://git.openjdk.org/jdk/pull/18238/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18238=00 Issue: https://bugs.openjdk.org/browse/JDK-8327990 Stats: 5 lines in 2 files changed: 4 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/18238.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18238/head:pull/18238 PR: https://git.openjdk.org/jdk/pull/18238
Re: RFR: 8318895: Deoptimization results in incorrect lightweight locking stack [v2]
On Thu, 9 Nov 2023 15:54:13 GMT, Roman Kennke wrote: >> See JBS issue for details. >> >> I basically: >> - took the test-modification and turned it into its own test-case >> - added test runners for lightweight- and legacy-locking, so that we keep >> testing both, no matter what is the default >> - added Axels fix (mentioned in the JBS issue) with the modification to >> only inflate when exec_mode == Unpack_none, as explained by Richard. >> >> Testing: >> - [x] EATests.java >> - [x] tier1 >> - [ ] tier2 > > Roman Kennke has updated the pull request incrementally with one additional > commit since the last revision: > > Add @reinrich's test-case Fix and new test case look good to me. Local testing was clean. Thanks, Richard. test/jdk/com/sun/jdi/EATests.java line 1755: > 1753: > / > 1754: > 1755: // The debugger reads and publishes an object with eliminated locking > to a static variable. Suggestion: // The debugger reads and publishes an object with eliminated locking to an instance field. - Marked as reviewed by rrich (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16568#pullrequestreview-1724167589 PR Review Comment: https://git.openjdk.org/jdk/pull/16568#discussion_r1389009395
Re: RFR: 8318895: Deoptimization results in incorrect lightweight locking stack
On Wed, 8 Nov 2023 19:00:53 GMT, Roman Kennke wrote: > See JBS issue for details. > > I basically: > - took the test-modification and turned it into its own test-case > - added test runners for lightweight- and legacy-locking, so that we keep > testing both, no matter what is the default > - added Axels fix (mentioned in the JBS issue) with the modification to only > inflate when exec_mode == Unpack_none, as explained by Richard. > > Testing: > - [x] EATests.java > - [x] tier1 > - [ ] tier2 Hi Roman, thanks for opening the pr. I've implemented [another test case](https://github.com/reinrich/jdk/commit/b72b3b3d7d1b5927811ae49e3ddea01d298dcb85) that demonstrates why relocking should be done before an object reference with eliminated locking is passed to a JVMTI agent. Would it be ok to include it in your pr? ([This is a version](https://github.com/reinrich/jdk/commit/f7a90c13e27e9fe38c892f069bd8d58484f59445) where relocking is delayed until the compiled frame is deoptimized. The new test fails with -XX:+UseNewCode). I will put the change through our CI testing. Cheers, Richard. - PR Comment: https://git.openjdk.org/jdk/pull/16568#issuecomment-1803985944
Re: RFR: 8301995: Move invokedynamic resolution information out of ConstantPoolCacheEntry [v14]
On Tue, 28 Mar 2023 16:33:09 GMT, Martin Doerr wrote: >> src/hotspot/cpu/ppc/templateInterpreterGenerator_ppc.cpp line 652: >> >>> 650: // Scale the index to be the entry index * >>> sizeof(ResolvedInvokeDynamicInfo) >>> 651: __ sldi(size, size, log2i_exact(sizeof(ResolvedIndyEntry))); >>> 652: __ add(cache, cache, size); >> >> @reinrich Is there any specific reason, why you're not calling >> load_resolved_indy_entry() method here. On s390x build/changes are stable >> even with calling that helper method. > > It should work if we move the addition of > `Array::base_offset_in_bytes()` into the other caller. > @reinrich Is there any specific reason, why you're not calling > load_resolved_indy_entry() method here. On s390x build/changes are stable > even with calling that helper method. Looks like this was changed on x86_64 after I ported it to ppc. Thanks for making me aware of it. - PR Review Comment: https://git.openjdk.org/jdk/pull/12778#discussion_r1151498444
Re: RFR: 8301995: Move invokedynamic resolution information out of ConstantPoolCacheEntry [v9]
On Tue, 21 Mar 2023 10:49:32 GMT, Andrew Haley wrote: >> Matias Saavedra Silva has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Fix riscv interpreter mistake and acquire semantics > > src/hotspot/cpu/aarch64/templateTable_aarch64.cpp line 2337: > >> 2335: // Load-acquire the adapter method >> 2336: __ lea(method, Address(cache, >> in_bytes(ResolvedIndyEntry::method_offset(; >> 2337: __ ldar(method, method); > > What reordering are we trying to prevent here? The loads of the data stored in `ResolvedIndyEntry::fill_in()` must not be reordered with loading `ResolvedIndyEntry::_method`. - PR Review Comment: https://git.openjdk.org/jdk/pull/12778#discussion_r1143781928
Re: RFR: 8301995: Move invokedynamic resolution information out of ConstantPoolCacheEntry [v6]
On Wed, 15 Mar 2023 18:45:00 GMT, Matias Saavedra Silva wrote: >> The current structure used to store the resolution information for >> invokedynamic, ConstantPoolCacheEntry, is difficult to interpret due to its >> ambigious fields f1 and f2. This structure can hold information for fields, >> methods, and invokedynamics and each of its fields can hold different types >> of values depending on the entry. >> >> This enhancement proposes a new structure to exclusively contain >> invokedynamic information in a manner that is easy to interpret and easy to >> extend. Resolved invokedynamic entries will be stored in an array in the >> constant pool cache and the operand of the invokedynamic bytecode will be >> rewritten to be the index into this array. >> >> Any areas that previously accessed invokedynamic data from >> ConstantPoolCacheEntry will be replaced with accesses to this new array and >> structure. Verified with tier1-9 tests. >> >> The PPC was provided by @reinrich and the RISCV port was provided by >> @DingliZhang and @zifeihan. >> >> This change supports the following platforms: x86, aarch64, PPC, and RISCV > > Matias Saavedra Silva has updated the pull request incrementally with one > additional commit since the last revision: > > Fixed aarch64 interpreter mistake src/hotspot/cpu/aarch64/templateTable_aarch64.cpp line 2335: > 2333: > 2334: __ load_resolved_indy_entry(cache, index); > 2335: __ ldr(method, Address(cache, > in_bytes(ResolvedIndyEntry::method_offset(; Should this load have acquire semantics? Like [here in template interpreter](https://github.com/openjdk/jdk/blob/2f23c80e0de44815d26a7d541701e16c9c1d32bc/src/hotspot/cpu/aarch64/interp_masm_aarch64.cpp#L239) and [here for the zero interpreter](https://github.com/openjdk/jdk/blob/2f23c80e0de44815d26a7d541701e16c9c1d32bc/src/hotspot/share/oops/cpCache.inline.hpp#L33)? Call stack for zero interpreter is ConstantPoolCacheEntry::indices_ord() ConstantPoolCacheEntry::bytecode_1() ConstantPoolCacheEntry::is_resolved(enum Bytecodes::Code) BytecodeInterpreter::run(interpreterState) - PR: https://git.openjdk.org/jdk/pull/12778
Re: RFR: 8301995: Move invokedynamic resolution information out of ConstantPoolCacheEntry [v6]
On Wed, 15 Mar 2023 18:45:00 GMT, Matias Saavedra Silva wrote: >> The current structure used to store the resolution information for >> invokedynamic, ConstantPoolCacheEntry, is difficult to interpret due to its >> ambigious fields f1 and f2. This structure can hold information for fields, >> methods, and invokedynamics and each of its fields can hold different types >> of values depending on the entry. >> >> This enhancement proposes a new structure to exclusively contain >> invokedynamic information in a manner that is easy to interpret and easy to >> extend. Resolved invokedynamic entries will be stored in an array in the >> constant pool cache and the operand of the invokedynamic bytecode will be >> rewritten to be the index into this array. >> >> Any areas that previously accessed invokedynamic data from >> ConstantPoolCacheEntry will be replaced with accesses to this new array and >> structure. Verified with tier1-9 tests. >> >> The PPC was provided by @reinrich and the RISCV port was provided by >> @DingliZhang and @zifeihan. >> >> This change supports the following platforms: x86, aarch64, PPC, and RISCV > > Matias Saavedra Silva has updated the pull request incrementally with one > additional commit since the last revision: > > Fixed aarch64 interpreter mistake src/hotspot/cpu/ppc/templateTable_ppc_64.cpp line 3398: > 3396: const Bytecodes::Code code = bytecode(); > 3397: const bool is_invokeinterface = code == Bytecodes::_invokeinterface; > 3398: const bool is_invokedynamic= code == false; // should not reach > here with invokedynamic This is what I meant. Suggestion: const bool is_invokedynamic= false; // should not reach here with invokedynamic Thanks! - PR: https://git.openjdk.org/jdk/pull/12778
Re: RFR: 8301995: Move invokedynamic resolution information out of ConstantPoolCacheEntry [v6]
On Thu, 16 Mar 2023 09:21:26 GMT, Martin Doerr wrote: >> Basically I kept the local variable as a name for the (now) constant value >> passed in the call at L3409. >> >> The parameter cannot be eliminated since `load_invoke_cp_cache_entry()` is >> declared in a shared header. >> >> I could replace the variable reference in the call with `false /* >> is_invokedynamic */` if you like that better. Personally I'd prefer the >> current version. > > I meant `code == false`. That was probably not intended. Oh my ... Your are right of course. - PR: https://git.openjdk.org/jdk/pull/12778
Re: RFR: 8301995: Move invokedynamic resolution information out of ConstantPoolCacheEntry [v6]
On Wed, 15 Mar 2023 19:04:41 GMT, Martin Doerr wrote: >> Matias Saavedra Silva has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Fixed aarch64 interpreter mistake > > src/hotspot/cpu/ppc/templateTable_ppc_64.cpp line 3398: > >> 3396: const Bytecodes::Code code = bytecode(); >> 3397: const bool is_invokeinterface = code == Bytecodes::_invokeinterface; >> 3398: const bool is_invokedynamic= code == false; // should not reach >> here with invokedynamic > > This looks strange! I guess you wanted to delete more? Basically I kept the local variable as a name for the (now) constant value passed in the call at L3409. The parameter cannot be eliminated since `load_invoke_cp_cache_entry()` is declared in a shared header. I could replace the variable reference in the call with `false /* is_invokedynamic */` if you like that better. Personally I'd prefer the current version. - PR: https://git.openjdk.org/jdk/pull/12778
Re: RFR: 8301995: Move invokedynamic resolution information out of ConstantPoolCacheEntry [v2]
On Tue, 14 Mar 2023 17:01:20 GMT, Matias Saavedra Silva wrote: > > @matias9927 can I ask you to merge master? There seem to be conflicts (at > > least I see a message "This branch has conflicts that must be resolved"). > > I'd like to give the change a spin in our CI testing. This requires that it > > can be applied on master. > > I saw that merge error but nothing came up when I tried to merge locally. The > branch is updated nonetheless, so you should be able to test it now @reinrich > ! Thanks. The testing didn't reveal anything. - PR: https://git.openjdk.org/jdk/pull/12778
Re: RFR: 8301995: Move invokedynamic resolution information out of ConstantPoolCacheEntry [v2]
On Tue, 14 Mar 2023 13:18:40 GMT, Coleen Phillimore wrote: > Ok, never mind, I saw s390 port but it doesn't seem to be in these changes (?) It is not in these changes. @offamitkumar is working on s390x. It is not yet finished though. (I wasn't aware that putting the URL of this PR into a comment somewhere else adds a comment to this PR) - PR: https://git.openjdk.org/jdk/pull/12778
Re: RFR: 8301995: Move invokedynamic resolution information out of ConstantPoolCacheEntry [v2]
On Thu, 9 Mar 2023 21:18:19 GMT, Matias Saavedra Silva wrote: >> The current structure used to store the resolution information for >> invokedynamic, ConstantPoolCacheEntry, is difficult to interpret due to its >> ambigious fields f1 and f2. This structure can hold information for fields, >> methods, and invokedynamics and each of its fields can hold different types >> of values depending on the entry. >> >> This enhancement proposes a new structure to exclusively contain >> invokedynamic information in a manner that is easy to interpret and easy to >> extend. Resolved invokedynamic entries will be stored in an array in the >> constant pool cache and the operand of the invokedynamic bytecode will be >> rewritten to be the index into this array. >> >> Any areas that previously accessed invokedynamic data from >> ConstantPoolCacheEntry will be replaced with accesses to this new array and >> structure. Verified with tier1-9 tests. >> >> The PPC was provided by @reinrich and the RISCV port was provided by >> @DingliZhang and @zifeihan. >> >> This change supports the following platforms: x86, aarch64, PPC, and RISCV > > Matias Saavedra Silva has updated the pull request incrementally with one > additional commit since the last revision: > > Interpreter optimization and comments @matias9927 can I ask you to merge master? There seem to be conflicts (at least I see a message "This branch has conflicts that must be resolved"). I'd like to give the change a spin in our CI testing. This requires that it can be applied on master. - PR: https://git.openjdk.org/jdk/pull/12778
Re: RFR: 8301995: Move invokedynamic resolution information out of ConstantPoolCacheEntry
On Thu, 9 Mar 2023 16:01:21 GMT, Coleen Phillimore wrote: >> This change should be in a further RFE though (and you can explain it there >> so we can maybe use it in the other platforms too). Does it affect >> performance when generating the template interpreter? Do you need to have >> hsdis in the LD_LIBRARY_PATH environment variable to use this? I see it's >> already used by default in one place. > > This looks cool. > This change should be in a further RFE though (and you can explain it there > so we can maybe use it in the other platforms too). Ok. > Does it affect performance when generating the template interpreter? I didn't think it would affect performance if the interpreter is not printed. I have not measured it though. > Do you need to have hsdis in the LD_LIBRARY_PATH environment variable to use > this? I see it's already used by default in one place. Yes you do. It is not working with the AbstractDisassembler which produces a hex dump of the machine code. - PR: https://git.openjdk.org/jdk/pull/12778
Re: RFR: 8301995: Move invokedynamic resolution information out of ConstantPoolCacheEntry
On Tue, 7 Mar 2023 13:30:50 GMT, Coleen Phillimore wrote: >> The current structure used to store the resolution information for >> invokedynamic, ConstantPoolCacheEntry, is difficult to interpret due to its >> ambigious fields f1 and f2. This structure can hold information for fields, >> methods, and invokedynamics and each of its fields can hold different types >> of values depending on the entry. >> >> This enhancement proposes a new structure to exclusively contain >> invokedynamic information in a manner that is easy to interpret and easy to >> extend. Resolved invokedynamic entries will be stored in an array in the >> constant pool cache and the operand of the invokedynamic bytecode will be >> rewritten to be the index into this array. >> >> Any areas that previously accessed invokedynamic data from >> ConstantPoolCacheEntry will be replaced with accesses to this new array and >> structure. Verified with tier1-9 tests. >> >> The PPC was provided by @reinrich and the RISCV port was provided by >> @DingliZhang and @zifeihan. >> >> This change supports the following platforms: x86, aarch64, PPC, and RISCV > > src/hotspot/cpu/ppc/templateTable_ppc_64.cpp line 53: > >> 51: >> 52: #undef __ >> 53: #define __ Disassembler::hook(__FILE__, >> __LINE__, _masm)-> > > What is this? Is this something useful for debugging the template > interpreter? Probably doesn't belong with this change but might be nice to > have (?) @reinrich Yes this is really useful when debugging the template interpreter. It annotates the disassembly with the generator source code. It helped tracking down a bug in the ppc part oft this pr. Other platforms have it too. Example: invokedynamic 186 invokedynamic [0x3fff80075a00, 0x3fff80075dc8] 968 bytes 0x3fff80075a00: std r17,0(r15) ;;@FILE: src/hotspot/cpu/ppc/templateInterpreterGenerator_ppc.cpp ;; 2185: aep = __ pc(); __ push_ptr(); __ b(L); 0x3fff80075a04: addir15,r15,-8 0x3fff80075a08: b 0x3fff80075a40 ;; 2185: aep = __ pc(); __ push_ptr(); __ b(L); 0x3fff80075a0c: stfsf15,0(r15) ;; 2186: fep = __ pc(); __ push_f();__ b(L); 0x3fff80075a10: addir15,r15,-8 0x3fff80075a14: b 0x3fff80075a40 ;; 2186: fep = __ pc(); __ push_f();__ b(L); 0x3fff80075a18: stfdf15,-8(r15) ;; 2187: dep = __ pc(); __ push_d();__ b(L); 0x3fff80075a1c: addir15,r15,-16 0x3fff80075a20: b 0x3fff80075a40 ;; 2187: dep = __ pc(); __ push_d();__ b(L); 0x3fff80075a24: li r0,0;; 2188: lep = __ pc(); __ push_l();__ b(L); 0x3fff80075a28: std r0,0(r15) 0x3fff80075a2c: std r17,-8(r15) 0x3fff80075a30: addir15,r15,-16 0x3fff80075a34: b 0x3fff80075a40 ;; 2188: lep = __ pc(); __ push_l();__ b(L); 0x3fff80075a38: stw r17,0(r15) ;; 2189: __ align(32, 12, 24); // align L ;; 2191: iep = __ pc(); __ push_i(); 0x3fff80075a3c: addir15,r15,-8 0x3fff80075a40: li r21,1 ;; 2192: vep = __ pc(); ;; 2193: __ bind(L); ;;@FILE: src/hotspot/share/interpreter/templateInterpreterGenerator.cpp ;; 366: __ verify_FPU(1, t->tos_in()); ;;@FILE: src/hotspot/cpu/ppc/templateTable_ppc_64.cpp ;; 2293: __ load_resolved_indy_entry(cache, index); 0x3fff80075a44: lwaxr21,r14,r21 0x3fff80075a48: nandr21,r21,r21 0x3fff80075a4c: ld r31,40(r27) 0x3fff80075a50: rldicr r21,r21,4,59 0x3fff80075a54: addir21,r21,8 0x3fff80075a58: add r31,r31,r21 0x3fff80075a5c: ld r22,0(r31) ;; 2294: __ ld_ptr(method, in_bytes(ResolvedIndyEntry::method_offset()), cache); 0x3fff80075a60: cmpdi r22,0 ;; 2297: __ cmpdi(CCR0, method, 0); 0x3fff80075a64: bne-0x3fff80075b94 ;; 2298: __ bne(CCR0, resolved);,bo=0b00100[no_hint] 0x3fff80075a68: li r4,186 ;; 2304: __ li(R4_ARG2, code); 0x3fff80075a6c: ld r11,0(r1) ;; 2305: __ call_VM(noreg, entry, R4_ARG2, true); - PR: https://git.openjdk.org/jdk/pull/12778
Re: RFR: JDK-8302320: AsyncGetCallTrace obtains too few frames in sanity test [v5]
On Mon, 20 Feb 2023 09:18:46 GMT, Johannes Bechberger wrote: >> Extends the existing AsyncGetCallTrace test case and fixes the issue. > > Johannes Bechberger has updated the pull request incrementally with one > additional commit since the last revision: > > Improve condition again Looks good to me. Richard. - Marked as reviewed by rrich (Reviewer). PR: https://git.openjdk.org/jdk/pull/12535
Re: RFR: JDK-8302320: AsyncGetCallTrace obtains too few frames in sanity test [v3]
On Mon, 20 Feb 2023 09:04:39 GMT, Johannes Bechberger wrote:
>> The sanity test worked :)
>
> ... and no regression in the other serviceability tests.
Probably it's better to adapt the limit though (sorry).
Problem with this version is that it allows `unextended_sp` outside the address
range reserved for the stack. Let's see what other reviewers think.
Suggestion:
if (!thread->is_in_stack_range_incl(unextended_sp, sp -
Interpreter::stackElementSize)) {
-
PR: https://git.openjdk.org/jdk/pull/12535
Re: RFR: JDK-8302320: AsyncGetCallTrace obtains too few frames in sanity test [v3]
On Fri, 17 Feb 2023 14:47:56 GMT, Johannes Bechberger wrote:
>> Extends the existing AsyncGetCallTrace test case and fixes the issue.
>
> Johannes Bechberger has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Revert previous change
src/hotspot/cpu/x86/frame_x86.cpp line 75:
> 73: // interpreted -> interpreted calls that go through a method handle
> linker,
> 74: // since those pop the last argument (the appendix) from the stack.
> 75: if (!thread->is_in_full_stack_checked(unextended_sp)) {
The condition can remain stricter. The following should work too. Could you
please check?
Suggestion:
if (!thread->is_in_stack_range_incl(unextended_sp +
Interpreter::stackElementSize, sp)) {
-
PR: https://git.openjdk.org/jdk/pull/12535
Re: RFR: JDK-8302320: AsyncGetCallTrace obtains too few frames in sanity test [v2]
On Thu, 16 Feb 2023 14:25:15 GMT, Johannes Bechberger wrote: >> Extends the existing AsyncGetCallTrace test case and fixes the issue by >> modifying `MethodHandles` code. > > Johannes Bechberger has updated the pull request incrementally with one > additional commit since the last revision: > > Implement addptr suggestion by @JohnVernee and @reinrich Java stack # # A fatal error has been detected by the Java Runtime Environment: # # SIGSEGV (0xb) at pc=0x0315, pid=41551, tid=41572 # # JRE version: OpenJDK Runtime Environment (21.0) (fastdebug build 21-internal-adhoc.openjdk.jdk-dev) # Java VM: OpenJDK 64-Bit Server VM (fastdebug 21-internal-adhoc.openjdk.jdk-dev, mixed mode, sharing, tiered, compressed oops, compressed class ptrs, g1 gc, linux-amd64) # Problematic frame: # C 0xe755a950 # # Core dump will be written. Default location: .../jtreg_hotspot_tier4_work/JTwork/scratch/0 # # If you would like to submit a bug report, please visit: # https://bugreport.java.com/bugreport/crash.jsp # --- S U M M A R Y Command Line: -Dtest.vm.opts=-Xmx768m -Djava.awt.headless=true -Djava.util.prefs.userRoot=.../jtreg_hotspot_tier4_work/tmp -Djava.io.tmpdir=.../jtreg_hotspot_tier4_work/tmp -Dtest.getfreeport.max.tries=40 -ea -esa -Dtest.tool.vm.opts=-J-Xmx768m -J-Djava.awt.headless=true -J-Djava.util.prefs.userRoot=.../jtreg_hotspot_tier4_work/tmp -J-Djava.io.tmpdir=.../jtreg_hotspot_tier4_work/tmp -J-Dtest.getfreeport.max.tries=40 -J-ea -J-esa -Dtest.compiler.opts= -Dtest.java.opts= -Dtest.jdk=.../sapjvm_21 -Dcompile.jdk=.../sapjvm_21 -Dtest.timeout.factor=6.0 -Dtest.nativepath=.../grmpf/testdata/jtreg/jtreg_test_21/test/hotspot/jtreg/native -Dtest.root=.../grmpf/testdata/jtreg/jtreg_test_21/test/hotspot/jtreg -Dtest.name=vmTestbase/vm/mlvm/indy/func/jvmti/stepBreakPopReturn/INDIFY_Test.java -Dtest.file=.../grmpf/testdata/jtreg/jtreg_test_21/test/hotspot/jtreg/vmTestbase/vm/mlvm/indy/func/jvmti/stepBreakPopReturn/INDIFY_Test.java -Dtest.src=.../grmpf/testdata/jtreg/jtreg_test_21/test/hotspot/jtreg/ vmTestbase/vm/mlvm/indy/func/jvmti/stepBreakPopReturn -Dtest.src.path=.../grmpf/testdata/jtreg/jtreg_test_21/test/hotspot/jtreg/vmTestbase/vm/mlvm/indy/func/jvmti/stepBreakPopReturn:.../grmpf/testdata/jtreg/jtreg_test_21/test/hotspot/jtreg/vmTestbase:.../grmpf/testdata/jtreg/jtreg_test_21/test/lib -Dtest.classes=.../jtreg_hotspot_tier4_work/JTwork/classes/vmTestbase/vm/mlvm/indy/func/jvmti/stepBreakPopReturn/INDIFY_Test.d -Dtest.class.path=.../jtreg_hotspot_tier4_work/JTwork/classes/vmTestbase/vm/mlvm/indy/func/jvmti/stepBreakPopReturn/INDIFY_Test.d:.../jtreg_hotspot_tier4_work/JTwork/classes/vmTestbase:.../jtreg_hotspot_tier4_work/JTwork/classes/test/lib -Dtest.class.path.prefix=.../jtreg_hotspot_tier4_work/JTwork/classes/vmTestbase/vm/mlvm/indy/func/jvmti/stepBreakPopReturn/INDIFY_Test.d:.../grmpf/testdata/jtreg/jtreg_test_21/test/hotspot/jtreg/vmTestbase/vm/mlvm/indy/func/jvmti/stepBreakPopReturn:.../jtreg_hotspot_tier4_work/JTwork/classes/vmTestbase:.../jtreg_hotspot_tier4_work/ JTwork/classes/test/lib -Xmx768m -Djava.awt.headless=true -Djava.util.prefs.userRoot=.../jtreg_hotspot_tier4_work/tmp -Djava.io.tmpdir=.../jtreg_hotspot_tier4_work/tmp -Dtest.getfreeport.max.tries=40 -ea -esa -Djava.library.path=.../grmpf/testdata/jtreg/jtreg_test_21/test/hotspot/jtreg/native -agentlib:stepBreakPopReturn=verbose= com.sun.javatest.regtest.agent.MainWrapper .../jtreg_hotspot_tier4_work/JTwork/vmTestbase/vm/mlvm/indy/func/jvmti/stepBreakPopReturn/INDIFY_Test.d/main.0.jta Host: ..., Intel(R) Xeon(R) Platinum 8260M CPU @ 2.40GHz, 8 cores, 23G, SUSE Linux Enterprise Server 15 SP3 Time: Fri Feb 17 01:34:06 2023 CET elapsed time: 2.781186 seconds (0d 0h 0m 2s) --- T H R E A D --- Current thread (0x7fbc94422600): JavaThread "MainThread" [_thread_in_Java, id=41572, stack(0x7fbc6af26000,0x7fbc6b027000)] Stack: [0x7fbc6af26000,0x7fbc6b027000], sp=0x7fbc6b025118, free space=1020k Native frames: (J=compiled Java code, j=interpreted, Vv=VM code, C=native code) C 0xe755a950 j java.lang.invoke.LambdaForm$MH+0x000801018400.linkToTargetMethod(Ljava/lang/Object;Ljava/lang/Object;ILjava/lang/Object;)I+7 java.base@21-internal j vm.mlvm.indy.func.jvmti.stepBreakPopReturn.INDIFY_Test.run()Z+52 j vm.mlvm.share.MlvmTestExecutor.runMlvmTestInstance(Lvm/mlvm/share/MlvmTest;)Z+82 j vm.mlvm.share.MlvmTestExecutor.runMlvmTest(Ljava/lang/Class;[Ljava/lang/Object;)Z+21 j vm.mlvm.share.MlvmTestExecutor.launch(Ljava/lang/Class;[Ljava/lang/Object;)V+28 j vm.mlvm.share.MlvmTestExecutor.launch([Ljava/lang/Object;)V+20 j vm.mlvm.share.MlvmTestExecutor.launch([Ljava/lang/String;[Ljava/lang/Object;)V+5 j vm.mlvm.share.MlvmTest.launch([Ljava/lang/String;)V+2 j vm.mlvm.indy.func.jvmti.stepBreakPopReturn.INDIFY_Test.main([Ljava/lang/String;)V+1 j
Re: RFR: JDK-8302320: AsyncGetCallTrace obtains too few frames in sanity test
On Thu, 16 Feb 2023 00:36:03 GMT, Jorn Vernee wrote: > So... my understanding is that a c2i adapter is used when a callee is > interpreted, so its `from_compiled_entry` points to the c2i adapter. A > compiled caller calls the `from_compiled_entry`, so it ends up going through > the `c2i` adapter of an interpreted callee. However, for method handle > linkers, the `from_compiled_entry` never points to a c2i adapter, but to the > MH linker compiler entry generated in `gen_special_dispatch` in e.g > `sharedRuntime_x86_64`. So, in other words, the 3. and 4. scenarios above are > not possible, AFAICS. I see. Thanks! That's what I've been missing. It's implemented in `SystemDictionary::find_method_handle_intrinsic()` I think. I agree that it will be ok to modify r13 as you suggested. Maybe explaining why we never reach here coming from a c2i adapter: because native wrappers are generated eagerly in `SystemDictionary::find_method_handle_intrinsic()` unless -Xint is given but then the caller cannot be compiled. - PR: https://git.openjdk.org/jdk/pull/12535
Re: RFR: JDK-8302320: AsyncGetCallTrace obtains too few frames in sanity test
On Wed, 15 Feb 2023 21:39:52 GMT, Jorn Vernee wrote: > The entry generated by generate_method_handle_interpreter_entry is > essentially the from_interpreted_entry, AFAIU. But it is also stored in `Method::_i2i_entry` which is [used by c2i adapters](https://github.com/openjdk/jdk/blob/3ba156082b73c4a8e9d890a57a42fb68df2bf98f/src/hotspot/cpu/x86/sharedRuntime_x86_64.cpp#L748-L750). (Maybe too) long version: The interpreter has entries depending on the `MethodKind`. See `AbstractInterpreter::entry_for_kind()`. The entries are stored in `AbstractInterpreter::_entry_table`. In `MethodHandlesAdapterGenerator::generate()` the interpreter entries for method handle intrinsics MethodKinds are generated using the generator `MethodHandles::generate_method_handle_interpreter_entry()`. In `Method::link_method()` the entry points are set depending on the `MethodKind`. The interpreter entry (see above) for the method's `MethodKind` is looked up and stored in `Method::_i2i_entry` which is also [used by c2i adapters](https://github.com/openjdk/jdk/blob/3ba156082b73c4a8e9d890a57a42fb68df2bf98f/src/hotspot/cpu/x86/sharedRuntime_x86_64.cpp#L748-L750). So to me it looks as if the entry generated by `generate_method_handle_interpreter_entry()` can be reached coming from a compiled caller. The generator for ordinary methods is `TemplateInterpreterGenerator::generate_normal_entry()`. The entries it generates are surely reached by compiled callers. I might be missing something in the case of MH intrinsics but right now it looks to me as if the caller can be compiled too. - PR: https://git.openjdk.org/jdk/pull/12535
Re: RFR: JDK-8302320: AsyncGetCallTrace obtains too few frames in sanity test
On Wed, 15 Feb 2023 21:12:19 GMT, Jorn Vernee wrote: >>> I think your conclusion is correct wrt `unextended_sp < sp` in that case. >>> For certain MH linker calls, an additional `MemberName` 'appendix' is >>> passed as the last argument. The MH adapter/trampoline stub pops the >>> appendix argument from the stack for an interpreted caller, and uses it to >>> find out the actual callee. But that means that, the sp saved into `r13` >>> will be one slot too 'large' (small? accounting for one too many >>> arguments). In other words, the MethodHandle adapter for interpreted >>> callers 'consumes' the appendix argument, but doesn't adjust `r13`. I think >>> this could be done though, i.e. just add: >>> >>> ``` >>> __ addptr(r13, Interpreter::stackElementSize); // adjust unexteded_sp >>> for callee, for proper stack walking >>> ``` >> >> I don't think you can do that. Modifying the unextended_sp would break >> accesses to compiled frames (gc, locals). The original unextended_sp is also >> needed to undo the c2i extension. >> >>> In the linked code in >>> `MethodHandles::generate_method_handle_interpreter_entry` to make this work. >>> >>> (note that we can not just leave the appendix on the stack, since the >>> callee doesn't expect it, so we would break argument oop GC if we left it >>> there) >>> >>> If that works, I think doing that would be preferable to adjusting >>> `safe_for_sender`. That seems like it would uphold the `sp <= >>> unextended_sp` invariant as well (maybe an assert to that effect in the >>> constructor of `frame` wouldn't be a bad idea as well). >>> >>> It seems like PPC is not modifying the stack in this case (?): >>> >>> ``` >>> Register R19_member = R19_method; // MemberName ptr; incoming method >>> ptr is dead now >>> __ ld(R19_member, RegisterOrConstant((intptr_t)8), R15_argbase); >>> __ addi(R15_argbase, R15_argbase, Interpreter::stackElementSize); >>> generate_method_handle_dispatch(_masm, iid, tmp_recv, R19_member, >>> not_for_compiler_entry); >>> ``` >> >> Only the expression stack pointer (R15) is modified but not the sp (R1). >> >> unextended_sp < sp is possible on PPC because the expression stack is >> allocated with its maximum size and it's trimmed to the used part when >> making a call. The sender sp is the original sp. > >> I don't think you can do that. Modifying the unextended_sp would break >> accesses to compiled frames (gc, locals). > > This is for interpreter entries, so the caller is definitely interpreted. If > the callee is compiled, it shouldn't care about the contents of `r13` right? > And if the callee is interpreted, it will get the correct `r13`. > >> The original unextended_sp is also needed to undo the c2i extension. > > Can you explain this? We shouldn't have a c2i adapters in this case. since > the caller is interpreted. ~If the callee is compiled, we will go through a > c2i adapter for that, but in that case the adapter overwrites `r13` again.~ It's to long since I was working on low level MH stuff. If an ordinary interpreter entry is used for a call this means the callee will be interpreted. The caller is not necessarily interpreted. It can be compiled. Is this different here? - PR: https://git.openjdk.org/jdk/pull/12535
Re: RFR: JDK-8302320: AsyncGetCallTrace obtains too few frames in sanity test
On Wed, 15 Feb 2023 20:38:04 GMT, Johannes Bechberger wrote: >>> I think `unextended_sp < sp` is possible on x86 when interpreter entries >>> generated by `MethodHandles::generate_method_handle_interpreter_entry()` >>> are executed. They modify `sp` ([here for >>> example](https://github.com/openjdk/jdk/blob/7f71a1040d9c03f72d082e329ccaf2c4a3c060a6/src/hotspot/cpu/x86/methodHandles_x86.cpp#L310-L314)) >>> but not `sender_sp` (`InterpreterMacroAssembler ::_bcp_register`). >> >> I think we're talking about a case where an interpreted caller calls an >> interpreted callee through a MethodHandle. If my understanding is correct >> (looking at `frame::sender_for_interpreter_frame`), in that case there are 2 >> sender sps, one that is computed based on the frame pointer of the callee. >> This just adds 2 words (skipping the return address), and the other is the >> sp which is saved into `r13` in >> `InterpreterMacroAssembler::prepare_to_jump_from_interpreted`, and before >> e.g. a c2i adapter extends the stack >> [here](https://github.com/openjdk/jdk/blob/50dcc2aec5b16c0826e27d58e49a7f55a5f5ad38/src/hotspot/cpu/x86/sharedRuntime_x86_64.cpp#L631) >> Which then seems to be saved in the interpreted callee's frame as well (I'm >> not super familiar with interpreter frame layouts though...). The latter >> being what ends up as `unextended_sp`. >> >> I think @reinrich's conclusion is correct wrt `unextended_sp < sp` in that >> case. For certain MH linker calls, an additional `MemberName` 'appendix' is >> passed as the last argument. The MH adapter/trampoline stub pops the >> appendix argument from the stack for an interpreted caller, and uses it to >> find out the actual callee. But that means that, the sp saved into `r13` >> will be one slot too 'large' (small? accounting for one too many arguments). >> In other words, the MethodHandle adapter for interpreted callers 'consumes' >> the appendix argument, but doesn't adjust `r13`. I think this could be done >> though, i.e. just add: >> >> >> __ addptr(r13, Interpreter::stackElementSize); // adjust unexteded_sp for >> callee, for proper stack walking >> >> >> In the linked code in >> `MethodHandles::generate_method_handle_interpreter_entry` to make this work. >> >> (note that we can not just leave the appendix on the stack, since the callee >> doesn't expect it, so we would break argument oop GC if we left it there) >> >> If that works, I think doing that would be preferable to adjusting >> `safe_for_sender`. That seems like it would uphold the `sp <= unextended_sp` >> invariant as well (maybe an assert to that effect in the constructor of >> `frame` wouldn't be a bad idea as well). >> >> It seems like PPC is not modifying the stack in this case (?): >> >> >> Register R19_member = R19_method; // MemberName ptr; incoming method >> ptr is dead now >> __ ld(R19_member, RegisterOrConstant((intptr_t)8), R15_argbase); >> __ addi(R15_argbase, R15_argbase, Interpreter::stackElementSize); >> generate_method_handle_dispatch(_masm, iid, tmp_recv, R19_member, >> not_for_compiler_entry); > > This is an interesting insight, I'm going to test your fix idea tomorrow. > I think your conclusion is correct wrt `unextended_sp < sp` in that case. For > certain MH linker calls, an additional `MemberName` 'appendix' is passed as > the last argument. The MH adapter/trampoline stub pops the appendix argument > from the stack for an interpreted caller, and uses it to find out the actual > callee. But that means that, the sp saved into `r13` will be one slot too > 'large' (small? accounting for one too many arguments). In other words, the > MethodHandle adapter for interpreted callers 'consumes' the appendix > argument, but doesn't adjust `r13`. I think this could be done though, i.e. > just add: > > ``` > __ addptr(r13, Interpreter::stackElementSize); // adjust unexteded_sp for > callee, for proper stack walking > ``` I don't think you can do that. Modifying the unextended_sp would break accesses to compiled frames (gc, locals). The original unextended_sp is also needed to undo the c2i extension. > In the linked code in > `MethodHandles::generate_method_handle_interpreter_entry` to make this work. > > (note that we can not just leave the appendix on the stack, since the callee > doesn't expect it, so we would break argument oop GC if we left it there) > > If that works, I think doing that would be preferable to adjusting > `safe_for_sender`. That seems like it would uphold the `sp <= unextended_sp` > invariant as well (maybe an assert to that effect in the constructor of > `frame` wouldn't be a bad idea as well). > > It seems like PPC is not modifying the stack in this case (?): > > ``` > Register R19_member = R19_method; // MemberName ptr; incoming method ptr > is dead now > __ ld(R19_member, RegisterOrConstant((intptr_t)8), R15_argbase); > __ addi(R15_argbase, R15_argbase,
Re: RFR: JDK-8302320: AsyncGetCallTrace obtains too few frames in sanity test
On Tue, 14 Feb 2023 07:38:21 GMT, Johannes Bechberger wrote: >> The test fails without this relaxation and the relaxation has been used on >> PPC to solve a similar issue a few years back. > > The original assumption does not always hold anymore and should therefore > relaxed. I think `unextended_sp < sp` is possible on x86 when interpreter entries generated by `MethodHandles::generate_method_handle_interpreter_entry()` are executed. They modify `sp` ([here for example](https://github.com/openjdk/jdk/blob/7f71a1040d9c03f72d082e329ccaf2c4a3c060a6/src/hotspot/cpu/x86/methodHandles_x86.cpp#L310-L314)) but not `sender_sp` (`InterpreterMacroAssembler ::_bcp_register`). - PR: https://git.openjdk.org/jdk/pull/12535
Re: RFR: 8297082: Remove sun/tools/jhsdb/BasicLauncherTest.java from problem list
On Thu, 24 Nov 2022 10:59:41 GMT, Martin Doerr wrote: > Related issue is fixed with > [JDK-8228649](https://bugs.openjdk.org/browse/JDK-8228649) > Test has passed. Lgtm - Marked as reviewed by rrich (Reviewer). PR: https://git.openjdk.org/jdk/pull/11351
Re: RFR: 8296323: JVMTI can_support_virtual_threads not available for agents loaded into running VM [v7]
On Tue, 22 Nov 2022 05:57:37 GMT, Serguei Spitsyn wrote: >> The can_support_virtual_thread was initially implemented as an onload >> capability. >> It is why this capability does not work for the agents loaded into running >> VM. >> The fix is to move it from `onload` to `always`capabilities list. >> >> Testing: >> New test is added: VirtualStartThreadTest. >> TBD: mach5 jvmti, jdi and tier1-6 tests. > > Serguei Spitsyn 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 eight additional > commits since the last revision: > > - Merge branch 'master' into br19 >Merge > - removed thread->vthread() != NULL from JvmtiVirtualThreadEventMark > constructor > - minor update for unnamed threads in jvmti_common.h > - fixed a trailing white space issue > - extended VirtualThreadStartTest to support more configs; fixed issue in > jvmtiExport.cpp > - roll back unintended VirtualThread.java file update > - simplified VirtualThreadStartTest > - 8296323: JVMTI can_support_virtual_threads not available for agents loaded > into running VM Changes look good to me. Thanks, Richard. - Marked as reviewed by rrich (Reviewer). PR: https://git.openjdk.org/jdk/pull/11246
Re: RFR: 8296323: JVMTI can_support_virtual_threads not available for agents loaded into running VM [v4]
On Mon, 21 Nov 2022 19:08:16 GMT, Serguei Spitsyn wrote:
>> src/hotspot/share/prims/jvmtiExport.cpp line 201:
>>
>>> 199: JvmtiVirtualThreadEventMark(JavaThread *thread) :
>>> 200: JvmtiEventMark(thread) {
>>> 201: if (thread->vthread() != NULL) {
>>
>> Can this condition ever be false?
>
> Yes, this condition can be false for platform threads.
The
[comment](https://github.com/openjdk/jdk/blob/master/src/hotspot/share/runtime/javaThread.hpp#L88)
suggests that `vthread()` cannot evaluate to NULL. Otherwise
`Thread.currentThread()` would return null too.
I've experimented a little bit and found that `thread->vthread()` in fact can
be NULL during initialization but then `thread->threadObj()` is also not yet
initialized. E.g. in `Threads::initialize_java_lang_classes()` class events are
generated before `create_initial_thread()` is reached. The constructor of
`JvmtiClassEventMark` calls its parent class' constructor, that is
`JvmtiVirtualThreadEventMark`, with a not yet fully initialized initial thread.
I found that `jtreg:test/hotspot/jtreg:hotspot_serviceability` succeeds with
`assert(thread->vthread() != NULL || thread->threadObj() == NULL, "");`
So you could unconditionally use `thread->vthread()`.
>> test/hotspot/jtreg/serviceability/jvmti/vthread/VirtualThreadStartTest/libVirtualThreadStartTest.cpp
>> line 54:
>>
>>> 52: started_thread_cnt++;
>>> 53: }
>>> 54: deallocate(jvmti, jni, (void*)tname);
>>
>> This will crash if `get_thread_name()` returns the string constant
>> `""`
>
> Nice catch.
> It is strange I've never seen any related crashes.
> This could be addressed separately but I've fixed it now.
> Please, let me know if you are okay with it.
> Will submit more mach5 runs to be safe.
The fix looks good to me. Thanks for taking care of the issue right away.
-
PR: https://git.openjdk.org/jdk/pull/11246
Re: RFR: 8296323: JVMTI can_support_virtual_threads not available for agents loaded into running VM [v4]
On Sun, 20 Nov 2022 08:48:16 GMT, Serguei Spitsyn wrote:
>> The can_support_virtual_thread was initially implemented as an onload
>> capability.
>> It is why this capability does not work for the agents loaded into running
>> VM.
>> The fix is to move it from `onload` to `always`capabilities list.
>>
>> Testing:
>> New test is added: VirtualStartThreadTest.
>> TBD: mach5 jvmti, jdi and tier1-6 tests.
>
> Serguei Spitsyn has updated the pull request incrementally with one
> additional commit since the last revision:
>
> fixed a trailing white space issue
Hi Serguei,
besides minor comments the changes look fine to me.
Best regards,
Richard.
src/hotspot/share/prims/jvmtiExport.cpp line 201:
> 199: JvmtiVirtualThreadEventMark(JavaThread *thread) :
> 200: JvmtiEventMark(thread) {
> 201: if (thread->vthread() != NULL) {
Can this condition ever be false?
test/hotspot/jtreg/serviceability/jvmti/vthread/VirtualThreadStartTest/libVirtualThreadStartTest.cpp
line 54:
> 52: started_thread_cnt++;
> 53: }
> 54: deallocate(jvmti, jni, (void*)tname);
This will crash if `get_thread_name()` returns the string constant `""`
-
PR: https://git.openjdk.org/jdk/pull/11246
Re: RFR: 8296875: Generational ZGC: Refactor loom code [v4]
On Thu, 17 Nov 2022 09:24:04 GMT, Erik Österlund wrote: >> The current loom code makes some assumptions about GC that will not work >> with generational ZGC. We should make this code more GC agnostic, and >> provide a better interface for talking to the GC. >> >> In particular, >> 1) All GCs have a way of encoding oops inside of the heap differently to >> oops outside of the heap. For non-ZGC collectors, that is compressed oops. >> For ZGC, that is colored pointers. With generational ZGC, pointers on-heap >> will be colored and pointers off-heap will be "colorless". So we need to >> generalize encoding and decoding of oops in the heap, for loom. >> >> 2) The cont_oop is located on a stack. In order to access it we need to >> start_processing on that thread, if it isn't the current thread. This >> happened to work so far for ZGC, because the stale pointers had enough >> colors. But with generational ZGC, these on-stack oops will be colorless, so >> we have to be more accurate here and ensure processing really has started on >> any thread that cont_oop is used on. To make life a bit easier, I'm moving >> the oop processing responsibility for these oops to the thread instead. >> Currently there is no more than one of these, so doing it lazily per frame >> seems a bit overkill. >> >> 3) Refactoring the stack chunk allocation code >> >> Tested with tier1-5 and manually running Skynet. No regressions detected. We >> have also been running with this (yet a slightly different backend) in the >> generational ZGC repo for a while now. > > Erik Österlund has updated the pull request incrementally with one additional > commit since the last revision: > > Fix Richard comments Not an expert of every aspect but the changes look good to me. Thanks, Richard. Marked as reviewed by rrich (Reviewer). - PR: https://git.openjdk.org/jdk/pull/1
Re: RFR: 8296875: Generational ZGC: Refactor loom code [v3]
On Thu, 17 Nov 2022 09:23:48 GMT, Erik Österlund wrote:
>> src/hotspot/share/gc/shared/barrierSetStackChunk.cpp line 68:
>>
>>> 66:
>>> 67: virtual void do_oop(oop* p) override {
>>> 68: if (UseCompressedOops) {
>>
>> Wouldn't it be better to hoist the check for `UseCompressedOops`?
>
> The compiler should be able to do that already. We devirtualize calls into
> oop closures, and the closure is stack allocated. So the compiler should be
> able to do that if it finds that it is a good idea. I'd prefer to leave that
> to the compiler.
`CompressOopsOopClosure::do_oop()` and `FrameOopIterator::oops_do()` are
defined in different compilation units. So calls to `do_oop()` cannot be
devirtualized or am I missing something?
Mistaken or not, I'm ok with this version.
>> src/hotspot/share/gc/shenandoah/shenandoahBarrierSetStackChunk.cpp line 30:
>>
>>> 28:
>>> 29: void ShenandoahBarrierSetStackChunk::encode_gc_mode(stackChunkOop
>>> chunk, OopIterator* oop_iterator) {
>>> 30: // Nothing to do
>>
>> Shenandoah allows `UseCompressedOops` enabled, doesn't it? Isn't it
>> necessary then to do the encoding as in the super class?
>
> No we don't convert the oops for Shenandoah. Instead, Shenandoah's closures
> know how to deal with both oop* and narrowOop* on the heap, and will get
> passed the appropriate type of pointer. So it doesn't use the bitmap. I have
> tested that it works with Shenandoah as well.
Interesting and good to know.
-
PR: https://git.openjdk.org/jdk/pull/1
Re: RFR: 8296875: Generational ZGC: Refactor loom code [v3]
On Mon, 14 Nov 2022 16:07:34 GMT, Erik Österlund wrote:
>> The current loom code makes some assumptions about GC that will not work
>> with generational ZGC. We should make this code more GC agnostic, and
>> provide a better interface for talking to the GC.
>>
>> In particular,
>> 1) All GCs have a way of encoding oops inside of the heap differently to
>> oops outside of the heap. For non-ZGC collectors, that is compressed oops.
>> For ZGC, that is colored pointers. With generational ZGC, pointers on-heap
>> will be colored and pointers off-heap will be "colorless". So we need to
>> generalize encoding and decoding of oops in the heap, for loom.
>>
>> 2) The cont_oop is located on a stack. In order to access it we need to
>> start_processing on that thread, if it isn't the current thread. This
>> happened to work so far for ZGC, because the stale pointers had enough
>> colors. But with generational ZGC, these on-stack oops will be colorless, so
>> we have to be more accurate here and ensure processing really has started on
>> any thread that cont_oop is used on. To make life a bit easier, I'm moving
>> the oop processing responsibility for these oops to the thread instead.
>> Currently there is no more than one of these, so doing it lazily per frame
>> seems a bit overkill.
>>
>> 3) Refactoring the stack chunk allocation code
>>
>> Tested with tier1-5 and manually running Skynet. No regressions detected. We
>> have also been running with this (yet a slightly different backend) in the
>> generational ZGC repo for a while now.
>
> Erik Österlund has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Indentation fix
Hi @fisk, I've skimmed the changes. They look good to me. I do have a few
comments/questions also.
src/hotspot/cpu/riscv/sharedRuntime_riscv.cpp line 876:
> 874:
> 875: OopMap* map = new OopMap(((int)ContinuationEntry::size() + wordSize) /
> VMRegImpl::stack_slot_size, 0 /* arg_slots*/);
> 876: ContinuationEntry::setup_oopmap(map);
I'd suggest to add a comment where the oops are handled.
src/hotspot/share/gc/shared/barrierSetStackChunk.cpp line 68:
> 66:
> 67: virtual void do_oop(oop* p) override {
> 68: if (UseCompressedOops) {
Wouldn't it be better to hoist the check for `UseCompressedOops`?
src/hotspot/share/gc/shenandoah/shenandoahBarrierSetStackChunk.cpp line 30:
> 28:
> 29: void ShenandoahBarrierSetStackChunk::encode_gc_mode(stackChunkOop chunk,
> OopIterator* oop_iterator) {
> 30: // Nothing to do
Shenandoah allows `UseCompressedOops` enabled, doesn't it? Isn't it necessary
then to do the encoding as in the super class?
-
PR: https://git.openjdk.org/jdk/pull/1
Integrated: 8295413: com/sun/jdi/EATests.java fails with compiler flag -XX:+StressReflectiveCode
On Wed, 19 Oct 2022 15:43:17 GMT, Richard Reingruber wrote: > With `StressReflectiveCode` C2 has inexact type information which can prevent > ea > based optimizations (see `ConnectionGraph::add_call_node()`) > > This pr changes the test jdk/com/sun/jdi/EATests.java to read the flag > `StressReflectiveCode`. If enabled it shall neither expect ea based > optimizations > of allocations nor deoptimization of corresponding frames upon debugger > access. > > Tested on the standard platforms with fastdebug and release builds. > > > make test TEST=test/jdk/com/sun/jdi/EATests.java > TEST_VM_OPTS="-XX:+IgnoreUnrecognizedVMOptions -XX:+StressReflectiveCode" This pull request has now been integrated. Changeset: 08d3ef4f Author:Richard Reingruber URL: https://git.openjdk.org/jdk/commit/08d3ef4fe60460d94b0a2db0b6671adc56a6653c Stats: 16 lines in 1 file changed: 16 ins; 0 del; 0 mod 8295413: com/sun/jdi/EATests.java fails with compiler flag -XX:+StressReflectiveCode Reviewed-by: lmesnik, kvn, sspitsyn - PR: https://git.openjdk.org/jdk/pull/10769
Re: RFR: 8295413: com/sun/jdi/EATests.java fails with compiler flag -XX:+StressReflectiveCode [v2]
On Thu, 20 Oct 2022 20:55:12 GMT, Richard Reingruber wrote: >> With `StressReflectiveCode` C2 has inexact type information which can >> prevent ea >> based optimizations (see `ConnectionGraph::add_call_node()`) >> >> This pr changes the test jdk/com/sun/jdi/EATests.java to read the flag >> `StressReflectiveCode`. If enabled it shall neither expect ea based >> optimizations >> of allocations nor deoptimization of corresponding frames upon debugger >> access. >> >> Tested on the standard platforms with fastdebug and release builds. >> >> >> make test TEST=test/jdk/com/sun/jdi/EATests.java >> TEST_VM_OPTS="-XX:+IgnoreUnrecognizedVMOptions -XX:+StressReflectiveCode" > > Richard Reingruber has updated the pull request incrementally with one > additional commit since the last revision: > > Skip all test cases if StressReflectiveCode is enabled Thanks for reviewing. Richard. - PR: https://git.openjdk.org/jdk/pull/10769
Re: RFR: 8295413: com/sun/jdi/EATests.java fails with compiler flag -XX:+StressReflectiveCode [v2]
> With `StressReflectiveCode` C2 has inexact type information which can prevent > ea > based optimizations (see `ConnectionGraph::add_call_node()`) > > This pr changes the test jdk/com/sun/jdi/EATests.java to read the flag > `StressReflectiveCode`. If enabled it shall neither expect ea based > optimizations > of allocations nor deoptimization of corresponding frames upon debugger > access. > > Tested on the standard platforms with fastdebug and release builds. > > > make test TEST=test/jdk/com/sun/jdi/EATests.java > TEST_VM_OPTS="-XX:+IgnoreUnrecognizedVMOptions -XX:+StressReflectiveCode" Richard Reingruber has updated the pull request incrementally with one additional commit since the last revision: Skip all test cases if StressReflectiveCode is enabled - Changes: - all: https://git.openjdk.org/jdk/pull/10769/files - new: https://git.openjdk.org/jdk/pull/10769/files/9071e5b4..c8c346f8 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=10769=01 - incr: https://webrevs.openjdk.org/?repo=jdk=10769=00-01 Stats: 16 lines in 1 file changed: 15 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/10769.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10769/head:pull/10769 PR: https://git.openjdk.org/jdk/pull/10769
Re: RFR: 8295413: com/sun/jdi/EATests.java fails with compiler flag -XX:+StressReflectiveCode
On Thu, 20 Oct 2022 19:52:39 GMT, Vladimir Kozlov wrote: > Should we add check for `StressReflectiveCode` to all `shouldSkip()` methods > too? Similar to `DeoptimizeObjectsALot` flag. I'd be ok to skip all testcases (by checking for `StressReflectiveCode` in the base classes' `shouldSkip()`). The tests' purpose is to check if ea based optimizations are reverted appropriately. Doesn't help much to run them with `StressReflectiveCode` except for random fuzzing. Would you like me to skip all test cases if running with `StressReflectiveCode`? - PR: https://git.openjdk.org/jdk/pull/10769
Re: RFR: 8295413: com/sun/jdi/EATests.java fails with compiler flag -XX:+StressReflectiveCode
On Wed, 19 Oct 2022 15:43:17 GMT, Richard Reingruber wrote: > With `StressReflectiveCode` C2 has inexact type information which can prevent > ea > based optimizations (see `ConnectionGraph::add_call_node()`) > > This pr changes the test jdk/com/sun/jdi/EATests.java to read the flag > `StressReflectiveCode`. If enabled it shall neither expect ea based > optimizations > of allocations nor deoptimization of corresponding frames upon debugger > access. > > Tested on the standard platforms with fastdebug and release builds. > > > make test TEST=test/jdk/com/sun/jdi/EATests.java > TEST_VM_OPTS="-XX:+IgnoreUnrecognizedVMOptions -XX:+StressReflectiveCode" The tests haven't finished in 12h. Not sure if they ever will... Local testing (see above) was good though. - PR: https://git.openjdk.org/jdk/pull/10769
RFR: 8295413: com/sun/jdi/EATests.java fails with compiler flag -XX:+StressReflectiveCode
With `StressReflectiveCode` C2 has inexact type information which can prevent ea based optimizations (see `ConnectionGraph::add_call_node()`) This pr changes the test jdk/com/sun/jdi/EATests.java to read the flag `StressReflectiveCode`. If enabled it shall neither expect ea based optimizations of allocations nor deoptimization of corresponding frames upon debugger access. Tested on the standard platforms with fastdebug and release builds. make test TEST=test/jdk/com/sun/jdi/EATests.java TEST_VM_OPTS="-XX:+IgnoreUnrecognizedVMOptions -XX:+StressReflectiveCode" - Commit messages: - Adapt test to -XX:+StressReflectiveCode Changes: https://git.openjdk.org/jdk/pull/10769/files Webrev: https://webrevs.openjdk.org/?repo=jdk=10769=00 Issue: https://bugs.openjdk.org/browse/JDK-8295413 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/10769.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10769/head:pull/10769 PR: https://git.openjdk.org/jdk/pull/10769
