Re: RFR: 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit [v4]
On Tue, 27 Jun 2023 21:29:53 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: > > revert to lazy loading of VMSupport I don't think pushing it down that far improves things. encode always precedes decode so why not resolve it before the encode call. // Resolve VMSupport class explicitly as HotSpotJVMCI::compute_offsets // may not have been called. Klass* vmSupport = SystemDictionary::resolve_or_fail(vmSymbols::jdk_internal_vm_VMSupport(), true, THREAD); int res = 0; if (!HAS_PENDING_EXCEPTION) { res = encode(THREAD, vmSupport, buffer, buffer_size); } src/hotspot/share/jvmci/jvmciEnv.cpp line 402: > 400: } > 401: int res = encode(THREAD, buffer, buffer_size); > 402: if (_from_env != nullptr && !_from_env->is_hotspot() && > _from_env->has_pending_exception()) { Why is this check before the `HAS_PENDING_EXCEPTION` check? Couldn't you end up returning with a pending exception in this path? - PR Comment: https://git.openjdk.org/jdk/pull/14641#issuecomment-1610335668 PR Review Comment: https://git.openjdk.org/jdk/pull/14641#discussion_r1244461845
Re: RFR: 8310906: Fix -Wconversion warnings in runtime, oops and some code header files. [v3]
On Tue, 27 Jun 2023 20:48:27 GMT, Coleen Phillimore wrote: >> This is another version of PR https://github.com/openjdk/jdk/pull/14659 but >> I've added a pointer delta function in globalDefinitions.hpp to use for >> these pointer diff calculations that return int everywhere. If the name is >> agreeable, I'll fix the other cases of this like this. It's better than raw >> casts. >> Tested with tier1-4. > > Coleen Phillimore has updated the pull request incrementally with one > additional commit since the last revision: > > Fixed the comment. delta_as_int() could also be: delta2int() ptrdelta2int() ptrdiff2int() ptrsub2int() in the theme of shorter names. pointer_delta_as_int() ignoring it does something different than pointer_delta() (allows negative returns). Change them all to check_cast(a - b) - losing identifiable name ptrdiff_cast(a - b) where int ptrdiff_cast(ptrdiff_t val) { return check(val); } Brainstorming in a PR. Your suggestions welcome. - PR Comment: https://git.openjdk.org/jdk/pull/14675#issuecomment-1610330128
Re: RFR: JDK-8310066: Improve test coverage for JVMTI GetThreadState on carrier and mounted vthread [v2]
On Mon, 26 Jun 2023 20:57:44 GMT, Andrey Turbanov wrote: >> Alex Menkov has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Added spaces in synchronized > > test/hotspot/jtreg/serviceability/jvmti/vthread/GetThreadStateMountedTest/GetThreadStateMountedTest.java > line 85: > >> 83: } >> 84: }); >> 85: synchronized(syncObj) { > > Suggestion: > > synchronized (syncObj) { Fixed - PR Review Comment: https://git.openjdk.org/jdk/pull/14622#discussion_r1244420236
Re: RFR: 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit [v3]
On Mon, 26 Jun 2023 13:17:25 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: > > each exception translation failure should trigger a JVMCI event I've pushed the loading of `VMSupport` down into the 2 cases where it's actually needed and made it lazy again which should address all concerns about extra noise at VM startup. Thanks for the push back on this - I agree that it's the best solution. - PR Comment: https://git.openjdk.org/jdk/pull/14641#issuecomment-1610243648
Re: RFR: 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit [v4]
> 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: revert to lazy loading of VMSupport - Changes: - all: https://git.openjdk.org/jdk/pull/14641/files - new: https://git.openjdk.org/jdk/pull/14641/files/819a24fd..734903a8 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14641=03 - incr: https://webrevs.openjdk.org/?repo=jdk=14641=02-03 Stats: 35 lines in 5 files changed: 17 ins; 2 del; 16 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: JDK-8310066: Improve test coverage for JVMTI GetThreadState on carrier and mounted vthread [v2]
> This is follow-up JDK-8307153/JDK-8309612 (JVMTI GetThreadState on carrier > should return STATE_WAITING) > New test tests GetThreadState for different thread states. > The test detected a bug in the implementation, new issue is created: > JDK-8310584 > Corresponding testcases are commented now in the test, fix for JDK-8310584 > will uncomment them Alex Menkov has updated the pull request incrementally with one additional commit since the last revision: Added spaces in synchronized - Changes: - all: https://git.openjdk.org/jdk/pull/14622/files - new: https://git.openjdk.org/jdk/pull/14622/files/c5f669b7..b5b39bcd Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14622=01 - incr: https://webrevs.openjdk.org/?repo=jdk=14622=00-01 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/14622.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14622/head:pull/14622 PR: https://git.openjdk.org/jdk/pull/14622
Re: RFR: 8310906: Fix -Wconversion warnings in runtime, oops and some code header files. [v3]
> This is another version of PR https://github.com/openjdk/jdk/pull/14659 but > I've added a pointer delta function in globalDefinitions.hpp to use for these > pointer diff calculations that return int everywhere. If the name is > agreeable, I'll fix the other cases of this like this. It's better than raw > casts. > Tested with tier1-4. Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision: Fixed the comment. - Changes: - all: https://git.openjdk.org/jdk/pull/14675/files - new: https://git.openjdk.org/jdk/pull/14675/files/99a15d6e..858e9cfb Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14675=02 - incr: https://webrevs.openjdk.org/?repo=jdk=14675=01-02 Stats: 4 lines in 1 file changed: 1 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/14675.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14675/head:pull/14675 PR: https://git.openjdk.org/jdk/pull/14675
Integrated: 8310585: GetThreadState spec mentions undefined JVMTI_THREAD_STATE_MONITOR_WAITING
On Fri, 23 Jun 2023 22:59:54 GMT, Alex Menkov wrote: > Trivial fix in JVMTI spec. > As it's just a typo, CSR is not required This pull request has now been integrated. Changeset: a97f98fb Author:Alex Menkov URL: https://git.openjdk.org/jdk/commit/a97f98fb8a933b43cd4485c3791ac8ca016bc49f Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod 8310585: GetThreadState spec mentions undefined JVMTI_THREAD_STATE_MONITOR_WAITING Reviewed-by: sspitsyn, cjplummer - PR: https://git.openjdk.org/jdk/pull/14634
Integrated: 8294316: SA core file support is broken on macosx-x64 starting with macOS 12.x
On Tue, 20 Jun 2023 18:05:09 GMT, Tom Rodriguez wrote: > This is a minor fix to core file reading on macos x. I can confirm that > after this fix I can run the problem listed SA core file tests on Ventura. This pull request has now been integrated. Changeset: 269852b9 Author:Tom Rodriguez URL: https://git.openjdk.org/jdk/commit/269852b90634aa43d4d719c93563608e42792fc6 Stats: 17 lines in 2 files changed: 5 ins; 0 del; 12 mod 8294316: SA core file support is broken on macosx-x64 starting with macOS 12.x Reviewed-by: cjplummer, sspitsyn - PR: https://git.openjdk.org/jdk/pull/14569
Re: RFR: 8294316: SA core file support is broken on macosx-x64 starting with macOS 12.x [v4]
On Mon, 26 Jun 2023 18:09:24 GMT, Tom Rodriguez wrote: >> This is a minor fix to core file reading on macos x. I can confirm that >> after this fix I can run the problem listed SA core file tests on Ventura. > > Tom Rodriguez has updated the pull request incrementally with one additional > commit since the last revision: > > Update copyright Thanks! - PR Comment: https://git.openjdk.org/jdk/pull/14569#issuecomment-1610129056
Re: RFR: JDK-8310380: Handle problems in core-related tests on macOS when codesign tool does not work [v4]
On Tue, 27 Jun 2023 13:45:27 GMT, Matthias Baesken wrote: >> Currently, a number of tests fail on macOS because they miss the core file >> (e.g. serviceability/sa/TestJmapCore.java). >> The reason is that configure detects on some setups that codesign does not >> work ("checking if debug mode codesign is possible... no) . >> So adding the needed entitlement to generate cores is not done in the build. >> This is currently not checked later in the tests. >> But without the entitlement, a core is not generated. > > Matthias Baesken has updated the pull request incrementally with one > additional commit since the last revision: > > small adjustments Marked as reviewed by cjplummer (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/14562#pullrequestreview-1501681509
Re: jdwp virtual threads dump?
Thanks Alan! for now there's no special support for virtual threads in IDEA, that's why I'm asking :) "Get Thread Dump" action gathers the information from the threads list provided by com.sun.jdi.VirtualMachine#allThreads. -Egor On 27.06.2023 18:24, Alan Bateman wrote: On 27/06/2023 16:58, Egor Ushakov wrote: Hi all, is there a way for a debugger to create a virtual threads dump via jdwp? Right now, the only way to do this via JDWP is use the ObjectReference/InvokeMethod command (or JDI invokeMethod at the API level) to invoke HotSpotDiagnosticMXBean dumpThreads to generate a thread dump to a file. So not terrible when the debugger and target VM are on the same system or at least have access to the same file system. In the future, JDWP + JDI will require updates to define new commands/methods to find threads, the VirtualMachine/AllThreads command is not the right solution for virtual threads. BTW: What does IDEA "Get Thread Dump" (camera icon) use? -Alan
Re: jdwp virtual threads dump?
On 27/06/2023 16:58, Egor Ushakov wrote: Hi all, is there a way for a debugger to create a virtual threads dump via jdwp? Right now, the only way to do this via JDWP is use the ObjectReference/InvokeMethod command (or JDI invokeMethod at the API level) to invoke HotSpotDiagnosticMXBean dumpThreads to generate a thread dump to a file. So not terrible when the debugger and target VM are on the same system or at least have access to the same file system. In the future, JDWP + JDI will require updates to define new commands/methods to find threads, the VirtualMachine/AllThreads command is not the right solution for virtual threads. BTW: What does IDEA "Get Thread Dump" (camera icon) use? -Alan
jdwp virtual threads dump?
Hi all, is there a way for a debugger to create a virtual threads dump via jdwp? Thanks, Egor
Re: RFR: 8310906: Fix -Wconversion warnings in runtime, oops and some code header files. [v2]
On Tue, 27 Jun 2023 14:54:03 GMT, Coleen Phillimore wrote: >> src/hotspot/share/code/codeBlob.inline.hpp line 36: >> >>> 34: inline const ImmutableOopMap* CodeBlob::oop_map_for_slot(int slot, >>> address return_address) const { >>> 35: assert(_oop_maps != nullptr, "nope"); >>> 36: return _oop_maps->find_map_at_slot(slot, delta_as_int((intptr_t) >>> return_address, (intptr_t) code_begin())); >> >> Is this the only usage of `delta_as_int` that operates on `intptr_t`? If we >> remove the casts then all usages would operate on pointers. Maybe this is an >> indication that we only need a `pointer_delta_as_int` function, to go hand >> in hand with the other `pointer_delta` functions? > > Hm. Maybe this would be ok. Our original idea was to make it T* not T until > this cast. I don't know how many other cases there are that I haven't gotten > to yet. But it would eliminate a cast, so that's good (unless these aren't > the same). Some instances have ptr - constant that gets promoted I think. > The reason we didn't pick pointer_delta_as_int because pointer_delta has > different semantics. pointer_delta insists on positive results. Taking out that cast does work, so I've fixed that. - PR Review Comment: https://git.openjdk.org/jdk/pull/14675#discussion_r1243922324
Re: RFR: 8310906: Fix -Wconversion warnings in runtime, oops and some code header files. [v2]
> This is another version of PR https://github.com/openjdk/jdk/pull/14659 but > I've added a pointer delta function in globalDefinitions.hpp to use for these > pointer diff calculations that return int everywhere. If the name is > agreeable, I'll fix the other cases of this like this. It's better than raw > casts. > Tested with tier1-4. Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision: Make delta_as_int take pointers and fix cast. Missed one in stubCodeGenerator.hpp - Changes: - all: https://git.openjdk.org/jdk/pull/14675/files - new: https://git.openjdk.org/jdk/pull/14675/files/9b7fdef2..99a15d6e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14675=01 - incr: https://webrevs.openjdk.org/?repo=jdk=14675=00-01 Stats: 3 lines in 3 files changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/14675.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14675/head:pull/14675 PR: https://git.openjdk.org/jdk/pull/14675
Re: RFR: 8310906: Fix -Wconversion warnings in runtime, oops and some code header files.
On Tue, 27 Jun 2023 12:59:59 GMT, Stefan Karlsson wrote: >> This is another version of PR https://github.com/openjdk/jdk/pull/14659 but >> I've added a pointer delta function in globalDefinitions.hpp to use for >> these pointer diff calculations that return int everywhere. If the name is >> agreeable, I'll fix the other cases of this like this. It's better than raw >> casts. >> Tested with tier1-4. > > src/hotspot/share/code/codeBlob.inline.hpp line 36: > >> 34: inline const ImmutableOopMap* CodeBlob::oop_map_for_slot(int slot, >> address return_address) const { >> 35: assert(_oop_maps != nullptr, "nope"); >> 36: return _oop_maps->find_map_at_slot(slot, delta_as_int((intptr_t) >> return_address, (intptr_t) code_begin())); > > Is this the only usage of `delta_as_int` that operates on `intptr_t`? If we > remove the casts then all usages would operate on pointers. Maybe this is an > indication that we only need a `pointer_delta_as_int` function, to go hand in > hand with the other `pointer_delta` functions? Hm. Maybe this would be ok. Our original idea was to make it T* not T until this cast. I don't know how many other cases there are that I haven't gotten to yet. But it would eliminate a cast, so that's good (unless these aren't the same). Some instances have ptr - constant that gets promoted I think. The reason we didn't pick pointer_delta_as_int because pointer_delta has different semantics. pointer_delta insists on positive results. - PR Review Comment: https://git.openjdk.org/jdk/pull/14675#discussion_r1243892244
RFR: 8310948: Fix ignored-qualifiers warning in Hotspot
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. - Commit messages: - Fix other platforms - Fix shenandoah - Fix ignored-qualifiers warning Changes: https://git.openjdk.org/jdk/pull/14674/files Webrev: https://webrevs.openjdk.org/?repo=jdk=14674=00 Issue: https://bugs.openjdk.org/browse/JDK-8310948 Stats: 223 lines in 74 files changed: 0 ins; 0 del; 223 mod Patch: https://git.openjdk.org/jdk/pull/14674.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14674/head:pull/14674 PR: https://git.openjdk.org/jdk/pull/14674
Re: RFR: 8309979: BootstrapMethods attribute is missing in class files recreated by SA [v2]
On Fri, 16 Jun 2023 18:10:58 GMT, Chris Plummer wrote: >> Ashutosh Mehra has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Address review comments by plummercj >> >> Signed-off-by: Ashutosh Mehra > > Yes, it is already problem listed. How did you run the tests? If you use > "make test" it should be including the problem list automatically. Thanks @plummercj @kevinjwalls @sspitsyn for reviewing the PR. - PR Comment: https://git.openjdk.org/jdk/pull/14495#issuecomment-1609535045
Re: RFR: JDK-8310380: Handle problems in core-related tests on macOS when codesign tool does not work [v3]
On Thu, 22 Jun 2023 11:51:15 GMT, Matthias Baesken wrote: >> Currently, a number of tests fail on macOS because they miss the core file >> (e.g. serviceability/sa/TestJmapCore.java). >> The reason is that configure detects on some setups that codesign does not >> work ("checking if debug mode codesign is possible... no) . >> So adding the needed entitlement to generate cores is not done in the build. >> This is currently not checked later in the tests. >> But without the entitlement, a core is not generated. > > Matthias Baesken has updated the pull request incrementally with one > additional commit since the last revision: > > move javaPath into caller Hi Chris, I renamed the method and adjusted the comment. - PR Comment: https://git.openjdk.org/jdk/pull/14562#issuecomment-1609529567
Re: RFR: JDK-8310380: Handle problems in core-related tests on macOS when codesign tool does not work [v4]
> Currently, a number of tests fail on macOS because they miss the core file > (e.g. serviceability/sa/TestJmapCore.java). > The reason is that configure detects on some setups that codesign does not > work ("checking if debug mode codesign is possible... no) . > So adding the needed entitlement to generate cores is not done in the build. > This is currently not checked later in the tests. > But without the entitlement, a core is not generated. Matthias Baesken has updated the pull request incrementally with one additional commit since the last revision: small adjustments - Changes: - all: https://git.openjdk.org/jdk/pull/14562/files - new: https://git.openjdk.org/jdk/pull/14562/files/619b3578..09eb93c6 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14562=03 - incr: https://webrevs.openjdk.org/?repo=jdk=14562=02-03 Stats: 4 lines in 3 files changed: 0 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/14562.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14562/head:pull/14562 PR: https://git.openjdk.org/jdk/pull/14562
Re: RFR: 8309979: BootstrapMethods attribute is missing in class files recreated by SA [v3]
On Thu, 22 Jun 2023 15:37:11 GMT, Ashutosh Mehra wrote: >> Please review this PR that extends SA to write BootstrapMethods attribute >> when dumping the class files. >> >> Tested it by dumping the class file for java/lang/String and comparing the >> BootstrapMethods attribute shown by javap for the original and the dumped >> class. > > Ashutosh Mehra has updated the pull request incrementally with one additional > commit since the last revision: > > Add comment about the layout of operands array in constant pool > > Signed-off-by: Ashutosh Mehra (I intended to click 'approve' earlier!) - Marked as reviewed by kevinw (Committer). PR Review: https://git.openjdk.org/jdk/pull/14495#pullrequestreview-1500887539
Re: RFR: 8310906: Fix -Wconversion warnings in runtime, oops and some code header files.
On Tue, 27 Jun 2023 12:41:50 GMT, Coleen Phillimore wrote: > This is another version of PR https://github.com/openjdk/jdk/pull/14659 but > I've added a pointer delta function in globalDefinitions.hpp to use for these > pointer diff calculations that return int everywhere. If the name is > agreeable, I'll fix the other cases of this like this. It's better than raw > casts. > Tested with tier1-4. src/hotspot/share/code/codeBlob.inline.hpp line 36: > 34: inline const ImmutableOopMap* CodeBlob::oop_map_for_slot(int slot, > address return_address) const { > 35: assert(_oop_maps != nullptr, "nope"); > 36: return _oop_maps->find_map_at_slot(slot, delta_as_int((intptr_t) > return_address, (intptr_t) code_begin())); Is this the only usage of `delta_as_int` that operates on `intptr_t`? If we remove the casts then all usages would operate on pointers. Maybe this is an indication that we only need a `pointer_delta_as_int` function, to go hand in hand with the other `pointer_delta` functions? - PR Review Comment: https://git.openjdk.org/jdk/pull/14675#discussion_r1243696287
Re: RFR: 8309979: BootstrapMethods attribute is missing in class files recreated by SA [v2]
On Thu, 22 Jun 2023 16:10:13 GMT, Ashutosh Mehra wrote: > I am thinking of a comprehensive test that creates a classfile with specific > attribute, load it in the VM, dump that class file using SA, then disassemble > the generated class file to check for the presence of the attribute. We would > also need some mechanism to ensure all attributes and cp tags supported by > the VM level being tested are covered. Does that sound feasible? I am working on a test case that uses `buildreplayjars` command to dump the boot and app classfiles and reuse them to run the same application again. I think that would provide good coverage for classfile dumping functionality as well and would remove the requirement to check every other attribute in the dumped classfile. But the new test currently fails because we are not dumping many of the attributes in the classfile. So far I have added support for Annotations, NestMembers and NestHost. Once I have the test working I will open the PR to add it. As for this PR, we can either wait for the new test and other changes to be ready, or integrate it based on the manual testing. I don't mind either approach. - PR Comment: https://git.openjdk.org/jdk/pull/14495#issuecomment-1609484548
RFR: 8310906: Fix -Wconversion warnings in runtime, oops and some code header files.
This is another version of PR https://github.com/openjdk/jdk/pull/14659 but I've added a pointer delta function in globalDefinitions.hpp to use for these pointer diff calculations that return int everywhere. If the name is agreeable, I'll fix the other cases of this like this. It's better than raw casts. Tested with tier1-4. - Commit messages: - 8310906: Fix -Wconversion warnings in runtime, oops and some code header files. Changes: https://git.openjdk.org/jdk/pull/14675/files Webrev: https://webrevs.openjdk.org/?repo=jdk=14675=00 Issue: https://bugs.openjdk.org/browse/JDK-8310906 Stats: 59 lines in 31 files changed: 8 ins; 0 del; 51 mod Patch: https://git.openjdk.org/jdk/pull/14675.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14675/head:pull/14675 PR: https://git.openjdk.org/jdk/pull/14675