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 Marked as reviewed by cjplummer (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/14495#pullrequestreview-1499916073
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 It may be in the noise but noise adds up over time. It just seems to me that the simplest fix here would have been to convert Klass* vmSupport = SystemDictionary::resolve_or_fail(vmSymbols::jdk_internal_vm_VMSupport(), true, THREAD); guarantee(!HAS_PENDING_EXCEPTION, ""); to Klass* vmSupport = SystemDictionary::resolve_or_fail(vmSymbols::jdk_internal_vm_VMSupport(), true, CHECK); and just return on exception. A very isolated change with zero impact on anything else. - PR Comment: https://git.openjdk.org/jdk/pull/14641#issuecomment-1608455121
Re: RFR: 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit [v3]
On Mon, 26 Jun 2023 21:56:21 GMT, David Holmes wrote: > I would think it is likely to fail with OOME under the same GC stress test > conditions. > > But if this is just a stress test issue then bailing out when the loading > fails would be far simpler than pre-loading the class. You've added to the VM > startup cost just to avoid a stress test failure. Any app running near the GC limit can cause this and we've seen this with libgraal in practice. When the VM is near the GC limit, any compilation can cause an OOME and then when that OOME wants to be translated back to libgraal, it can be the first time VMSupport is used. The time for loading `VMSupport` eagerly is in the noise. For example, `-verbose:class` shows that about 30 classes (including `VMSupport`) are loaded in 1ms: [0.011s][info][class,load] java.lang.reflect.Executable source: shared objects file [0.012s][info][class,load] java.lang.reflect.Method source: shared objects file [0.012s][info][class,load] java.lang.reflect.Constructor source: shared objects file [0.012s][info][class,load] jdk.internal.vm.ContinuationScope source: shared objects file [0.012s][info][class,load] jdk.internal.vm.Continuation source: shared objects file [0.012s][info][class,load] jdk.internal.vm.StackChunk source: shared objects file [0.012s][info][class,load] jdk.internal.vm.VMSupport source: shared objects file [0.012s][info][class,load] jdk.internal.reflect.MagicAccessorImpl source: shared objects file [0.012s][info][class,load] jdk.internal.reflect.MethodAccessor source: shared objects file [0.012s][info][class,load] jdk.internal.reflect.MethodAccessorImpl source: shared objects file [0.012s][info][class,load] jdk.internal.reflect.DelegatingClassLoader source: shared objects file [0.012s][info][class,load] jdk.internal.reflect.ConstantPool source: shared objects file [0.012s][info][class,load] java.lang.annotation.Annotation source: shared objects file [0.012s][info][class,load] jdk.internal.reflect.CallerSensitive source: shared objects file [0.012s][info][class,load] jdk.internal.reflect.ConstructorAccessor source: shared objects file [0.012s][info][class,load] jdk.internal.reflect.ConstructorAccessorImpl source: shared objects file [0.012s][info][class,load] jdk.internal.reflect.DirectConstructorHandleAccessor$NativeAccessor source: shared objects file [0.012s][info][class,load] jdk.internal.reflect.SerializationConstructorAccessorImpl source: shared objects file [0.012s][info][class,load] java.lang.invoke.MethodHandle source: shared objects file [0.012s][info][class,load] java.lang.invoke.DirectMethodHandle source: shared objects file [0.012s][info][class,load] java.lang.invoke.VarHandle source: shared objects file [0.012s][info][class,load] java.lang.invoke.MemberName source: shared objects file [0.012s][info][class,load] java.lang.invoke.ResolvedMethodName source: shared objects file [0.012s][info][class,load] java.lang.invoke.MethodHandleNatives source: shared objects file [0.012s][info][class,load] java.lang.invoke.LambdaForm source: shared objects file [0.012s][info][class,load] java.lang.invoke.TypeDescriptor$OfMethod source: shared objects file [0.012s][info][class,load] java.lang.invoke.MethodType source: shared objects file [0.012s][info][class,load] java.lang.BootstrapMethodError source: shared objects file [0.012s][info][class,load] java.lang.invoke.CallSite source: shared objects file [0.012s][info][class,load] jdk.internal.foreign.abi.NativeEntryPoint source: shared objects file [0.012s][info][class,load] jdk.internal.foreign.abi.ABIDescriptor source: shared objects file [0.012s][info][class,load] jdk.internal.foreign.abi.VMStorage source: shared objects file [0.013s][info][class,load] jdk.internal.foreign.abi.UpcallLinker$CallRegs source: shared objects file - PR Comment: https://git.openjdk.org/jdk/pull/14641#issuecomment-1608388679
Re: RFR: 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit [v3]
On Mon, 26 Jun 2023 21:47:08 GMT, Doug Simon wrote: > I doubt VMSupport. would ever fail in practice. I would think it is likely to fail with OOME under the same GC stress test conditions. But if this is just a stress test issue then bailing out when the loading fails would be far simpler than pre-loading the class. You've added to the VM startup cost just to avoid a stress test failure. - PR Comment: https://git.openjdk.org/jdk/pull/14641#issuecomment-1608371203
Re: RFR: 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit [v3]
On Mon, 26 Jun 2023 21:41:25 GMT, David Holmes wrote: > Then why did you not simply handle the exception from the loading of > VMSupport the same way? The only actual case seen of the original way failing is due to OOME in GC stress tests. If loading of VMSupport is done during VM startup, then no stress test code can cause an OOME while loading VMSupport. I doubt `VMSupport.` would ever fail in practice. - PR Comment: https://git.openjdk.org/jdk/pull/14641#issuecomment-1608330027
Re: RFR: 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit [v3]
On Mon, 26 Jun 2023 13:06:46 GMT, Doug Simon wrote: > That's fine - if VMSupport fails class initialization, then no "rich" > decoding can occur (by definition) and so throwing the clinit error is the > best we can do. Then why did you not simply handle the exception from the loading of VMSupport the same way? - PR Comment: https://git.openjdk.org/jdk/pull/14641#issuecomment-1608306762
Re: RFR: 8310906: Fix -Wconversion warnings in runtime, oops and some code header files.
On Mon, 26 Jun 2023 17:47:25 GMT, Coleen Phillimore wrote:
> More casts for size of address differences in same code space,
> checked_casts<> and type changes to fix -Wconversion warnings. See CR for
> details.
> Tested with tier1-4.
After some offline conversation with Ioi, we thought of a better way to express
these pointer subtractions returning int, so I'm going to close this PR and
open a new one with at least these changed with that new function.
// delta usually called with pointers or intptr_t that returns an int
// used for code buffer and other nearby address calculations.
// This allows the left side to be less than the right side.
template
inline int delta_as_int(const volatile T left, const volatile T right) {
return checked_cast(left - right);
}
-
PR Comment: https://git.openjdk.org/jdk/pull/14659#issuecomment-1608292000
Withdrawn: 8310906: Fix -Wconversion warnings in runtime, oops and some code header files.
On Mon, 26 Jun 2023 17:47:25 GMT, Coleen Phillimore wrote: > More casts for size of address differences in same code space, > checked_casts<> and type changes to fix -Wconversion warnings. See CR for > details. > Tested with tier1-4. This pull request has been closed without being integrated. - PR: https://git.openjdk.org/jdk/pull/14659
Re: RFR: 8310906: Fix -Wconversion warnings in runtime, oops and some code header files.
On Mon, 26 Jun 2023 20:19:47 GMT, Frederic Parain wrote: >> More casts for size of address differences in same code space, >> checked_casts<> and type changes to fix -Wconversion warnings. See CR for >> details. >> Tested with tier1-4. > > src/hotspot/cpu/aarch64/continuationFreezeThaw_aarch64.inline.hpp line 220: > >> 218: // If caller is interpreted it already made room for the callee >> arguments >> 219: int overlap = caller.is_interpreted_frame() ? >> ContinuationHelper::InterpretedFrame::stack_argsize(hf) : 0; >> 220: const int fsize = >> checked_cast(ContinuationHelper::InterpretedFrame::frame_bottom(hf) - >> hf.unextended_sp() - overlap); > > This line computes the size of the unextended frame minus the overlap that > exists if the callee is using arguments in the caller’s frame. So this value > is lower or equal to the frame’ size which is encoded with an int. I don't > think a checked_cast is required here. This applies to the x86 version too. I'll make this change in the new PR. I'm going to redo the address calculations to call a function rather than the casts. - PR Review Comment: https://git.openjdk.org/jdk/pull/14659#discussion_r1242788203
Re: RFR: JDK-8310066: Improve test coverage for JVMTI GetThreadState on carrier and mounted vthread
On Fri, 23 Jun 2023 01:57:56 GMT, Alex Menkov wrote:
> 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
test/hotspot/jtreg/serviceability/jvmti/vthread/GetThreadStateMountedTest/GetThreadStateMountedTest.java
line 85:
> 83: }
> 84: });
> 85: synchronized(syncObj) {
Suggestion:
synchronized (syncObj) {
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14622#discussion_r1242759252
Re: RFR: 8310906: Fix -Wconversion warnings in runtime, oops and some code header files.
On Mon, 26 Jun 2023 17:47:25 GMT, Coleen Phillimore wrote: > More casts for size of address differences in same code space, > checked_casts<> and type changes to fix -Wconversion warnings. See CR for > details. > Tested with tier1-4. Changes requested by fparain (Reviewer). src/hotspot/cpu/aarch64/continuationFreezeThaw_aarch64.inline.hpp line 220: > 218: // If caller is interpreted it already made room for the callee > arguments > 219: int overlap = caller.is_interpreted_frame() ? > ContinuationHelper::InterpretedFrame::stack_argsize(hf) : 0; > 220: const int fsize = > checked_cast(ContinuationHelper::InterpretedFrame::frame_bottom(hf) - > hf.unextended_sp() - overlap); This line computes the size of the unextended frame minus the overlap that exists if the callee is using arguments in the caller’s frame. So this value is lower or equal to the frame’ size which is encoded with an int. I don't think a checked_cast is required here. This applies to the x86 version too. - PR Review: https://git.openjdk.org/jdk/pull/14659#pullrequestreview-1499357905 PR Review Comment: https://git.openjdk.org/jdk/pull/14659#discussion_r1242721648
Re: RFR: 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 Marked as reviewed by cjplummer (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/14634#pullrequestreview-1499146614
Re: RFR: 8294316: SA core file support is broken on macosx-x64 starting with macOS 12.x [v4]
> 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 - Changes: - all: https://git.openjdk.org/jdk/pull/14569/files - new: https://git.openjdk.org/jdk/pull/14569/files/bd45a311..1eed618c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14569=03 - incr: https://webrevs.openjdk.org/?repo=jdk=14569=02-03 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/14569.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14569/head:pull/14569 PR: https://git.openjdk.org/jdk/pull/14569
RFR: 8310906: Fix -Wconversion warnings in runtime, oops and some code header files.
More casts for size of address differences in same code space, checked_casts<> and type changes to fix -Wconversion warnings. See CR for details. Tested with tier1-4. - Commit messages: - Fix -Wconversion warnings in runtime, oops and some code header files. Changes: https://git.openjdk.org/jdk/pull/14659/files Webrev: https://webrevs.openjdk.org/?repo=jdk=14659=00 Issue: https://bugs.openjdk.org/browse/JDK-8310906 Stats: 71 lines in 36 files changed: 0 ins; 0 del; 71 mod Patch: https://git.openjdk.org/jdk/pull/14659.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14659/head:pull/14659 PR: https://git.openjdk.org/jdk/pull/14659
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
Changes requested by cjplummer (Reviewer).
test/lib/jdk/test/lib/Platform.java line 276:
> 274: }
> 275:
> 276: public static boolean hasPlistEntriesOSX() throws IOException {
Although I understand why you chose this API name (it mimics the form used for
`isHardenedOSX`), I don't think it is a good choice. `isHardenedOSX` is short
for "is this a hardened OSX system". That mapping does not work with
`hasPlistEntriesOSX`, which is more like "does this OSX system have plist
entries". I would suggest `hasOSXPlistEntries`.
test/lib/jdk/test/lib/util/CoreUtils.java line 154:
> 152: }
> 153: } else {
> 154: // codesign has to add entitlements using the plist, if
> this is not present we might not generate a core file
Suggestion:
// codesign has to add entitlements using the plist. If this is
not present we might not generate a core file.
-
PR Review: https://git.openjdk.org/jdk/pull/14562#pullrequestreview-1499040669
PR Review Comment: https://git.openjdk.org/jdk/pull/14562#discussion_r1242519320
PR Review Comment: https://git.openjdk.org/jdk/pull/14562#discussion_r1242520818
Re: RFR: 8294316: SA core file support is broken on macosx-x64 starting with macOS 12.x [v3]
On Mon, 26 Jun 2023 17:02:22 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: > > Adjust for review Marked as reviewed by cjplummer (Reviewer). Please update the copyright before integrating. - PR Review: https://git.openjdk.org/jdk/pull/14569#pullrequestreview-1499010655 Marked as reviewed by cjplummer (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14569#pullrequestreview-1499012965
Re: RFR: 8294316: SA core file support is broken on macosx-x64 starting with macOS 12.x [v2]
On Mon, 26 Jun 2023 05:06:13 GMT, Chris Plummer wrote:
>> Tom Rodriguez has updated the pull request incrementally with three
>> additional commits since the last revision:
>>
>> - Flatten nested ifs
>> - Adjust ordering of message
>> - Adjust printing
>
> src/jdk.hotspot.agent/macosx/native/libsaproc/ps_core.c line 302:
>
>> 300: // The base of the library is offset by a random amount which
>> ends up as a load command with a
>> 301: // filesize of 0. This must be ignored otherwise the base
>> address of the library is wrong.
>> 302: if (segcmd.filesize != 0 && add_map_info(ph, fd, segcmd.fileoff,
>> segcmd.vmaddr, segcmd.vmsize, segcmd.flags) == NULL) {
>
> I actually preferred this part with two `if` statements. Seems easier to
> read that way.
Ok. I reverted it to 2 ifs.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14569#discussion_r1242483550
Re: RFR: 8294316: SA core file support is broken on macosx-x64 starting with macOS 12.x [v3]
> 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: Adjust for review - Changes: - all: https://git.openjdk.org/jdk/pull/14569/files - new: https://git.openjdk.org/jdk/pull/14569/files/2ebe4d09..bd45a311 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14569=02 - incr: https://webrevs.openjdk.org/?repo=jdk=14569=01-02 Stats: 5 lines in 1 file changed: 2 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/14569.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14569/head:pull/14569 PR: https://git.openjdk.org/jdk/pull/14569
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 looks ok to me. - Marked as reviewed by never (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14641#pullrequestreview-1498967746
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 BTW, I've manually tested this with libgraal. It's not possible to add a jtreg test until libgraal in part of OpenJDK. - PR Comment: https://git.openjdk.org/jdk/pull/14641#issuecomment-1607466267
Re: RFR: 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit [v3]
> 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 - Changes: - all: https://git.openjdk.org/jdk/pull/14641/files - new: https://git.openjdk.org/jdk/pull/14641/files/614e1e9f..819a24fd Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14641=02 - incr: https://webrevs.openjdk.org/?repo=jdk=14641=01-02 Stats: 4 lines in 2 files changed: 3 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/14641.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14641/head:pull/14641 PR: https://git.openjdk.org/jdk/pull/14641
Re: RFR: 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit [v2]
On Mon, 26 Jun 2023 07:59:17 GMT, David Holmes wrote: > if the first decode encounters a class initialization error then it will just > return with the exception pending and no decoding will actually have occurred That's fine - if VMSupport fails class initialization, then no "rich" decoding can occur (by definition) and so throwing the clinit error is the best we can do. > If we get to the encode first and it encounters an initialization error it > will still attempt to do a decode that must in turn fail You have to keep in mind that `encode` and `decode` are called in different runtimes. If encoding an exception in the HotSpot runtime fails (e.g. due to an OOME calling `VMSupport.` in the HotSpot heap), then it's still fine to try call `VMSupport.decode` in the libgraal runtime. If `VMSupport.decode` in the libgraal runtime also fails, then it throw the exception describing the secondary failure. There's simply no way to guarantee all info is shown when secondary issues occur during exception handling. - PR Comment: https://git.openjdk.org/jdk/pull/14641#issuecomment-1607433634
Re: RFR: 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit [v2]
> 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: try harder to show nested exception during exception translation - Changes: - all: https://git.openjdk.org/jdk/pull/14641/files - new: https://git.openjdk.org/jdk/pull/14641/files/fafc0aae..614e1e9f Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14641=01 - incr: https://webrevs.openjdk.org/?repo=jdk=14641=00-01 Stats: 51 lines in 4 files changed: 21 ins; 4 del; 26 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: 8298047: Remove all non-significant trailing whitespace from properties files [v2]
On Sat, 24 Jun 2023 06:47:48 GMT, Vladimir Sitnikov wrote: > I've posted the suggetion to add .editorconfig on ide-support-dev, however, > the list does not seem to be active: > https://mail.openjdk.org/pipermail/ide-support-dev/2023-June/000281.html I think that sounds like a reasonable idea. I'm not following that mailing list. I think you can just post a PR with your suggested contents and target build-dev (through the `build` label). I'm not familiar with this kind of file, so an explanation of which IDEs support it would be good to include. - PR Comment: https://git.openjdk.org/jdk/pull/11491#issuecomment-1607073914
Re: RFR: 8310816: GcInfoBuilder float/double signature mismatch
On Fri, 23 Jun 2023 18:10:59 GMT, Kevin Walls wrote: > Simple typo in a signature which is passed to JNU_NewObjectByName. The > method clearly intentds to pass Float, but uses Double. > > This code is probably not invoked, unless there is a GC MXBean with such > fields. I see no straightforward way of testing this explicitly, but the > change is tiny. > > All tests in test/jdk/com/sun/management still pass. Thanks! - PR Comment: https://git.openjdk.org/jdk/pull/14631#issuecomment-1606975619
Integrated: 8310816: GcInfoBuilder float/double signature mismatch
On Fri, 23 Jun 2023 18:10:59 GMT, Kevin Walls wrote: > Simple typo in a signature which is passed to JNU_NewObjectByName. The > method clearly intentds to pass Float, but uses Double. > > This code is probably not invoked, unless there is a GC MXBean with such > fields. I see no straightforward way of testing this explicitly, but the > change is tiny. > > All tests in test/jdk/com/sun/management still pass. This pull request has now been integrated. Changeset: a96e92c8 Author:Kevin Walls URL: https://git.openjdk.org/jdk/commit/a96e92c83dd3cb36c10282724466e6d1339f58f6 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod 8310816: GcInfoBuilder float/double signature mismatch Reviewed-by: mchung, dholmes - PR: https://git.openjdk.org/jdk/pull/14631
Re: RFR: 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit
On Sun, 25 Jun 2023 06:58:14 GMT, Doug Simon wrote: > The VMSupport class is required for translating an exception between the > HotSpot and libgraal heaps. > Loading it lazily can result in a loading exception, obscuring the exception > being translated. > To avoid this, VMSupport is loaded eagerly along with the other vmClasses. Edited above to correct: I think it is just highlighting that this code **not** is prepared for "system" errors like this. - PR Comment: https://git.openjdk.org/jdk/pull/14641#issuecomment-1606923008
Re: RFR: 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit
On Mon, 26 Jun 2023 07:46:38 GMT, Doug Simon wrote: > The usages of `vmSupport` below all use `JavaCalls:call_static` which takes > care of linking and initializing the class. Sure but my point is that initialization can fail and the code does not seem to directly take that into account - if the first `decode` encounters a class initialization error then it will just return with the exception pending and no decoding will actually have occurred. If we get to the `encode` first and it encounters an initialization error it will still attempt to do a `decode` that must in turn fail and again we return with an exception pending and no encoding/decoding occurring. This code does not seem to be written in a way that allows for an error initializing `VMSupport`. So while the fix side-steps the problematic guarantee, I think it is just highlighting that this code is prepared for "system" errors like this. - PR Comment: https://git.openjdk.org/jdk/pull/14641#issuecomment-1606915981
Re: RFR: 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit
On Mon, 26 Jun 2023 07:42:48 GMT, David Holmes wrote:
> The eager loading seems reasonable, but I do not understand the details here.
> In what way was loading failing? You still have to initialize `VMSupport`
> before you can call methods on it, so that could also fail - though the code
> does not seem to notice/handle this. ??
The usages of `vmSupport` below all use `JavaCalls:call_static` which takes
care of linking and initializing the class.
> src/hotspot/share/jvmci/jvmciCompilerToVM.cpp line 585:
>
>> 583:
>> 584: if (class_name->utf8_length() <= 1) {
>> 585: JVMCI_THROW_MSG_0(InternalError, err_msg("Primitive type %s should
>> be handled in Java code", str));
>
> Seems unrelated to the fix at hand.
Yes, it's a minor fix up I noticed while making changes a few lines below. It
just avoids a conversion of a `Symbol` back to a C string when the original C
string is available.
-
PR Comment: https://git.openjdk.org/jdk/pull/14641#issuecomment-1606899412
PR Review Comment: https://git.openjdk.org/jdk/pull/14641#discussion_r1241766196
Re: RFR: 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit
On Sun, 25 Jun 2023 06:58:14 GMT, Doug Simon wrote:
> The VMSupport class is required for translating an exception between the
> HotSpot and libgraal heaps.
> Loading it lazily can result in a loading exception, obscuring the exception
> being translated.
> To avoid this, VMSupport is loaded eagerly along with the other vmClasses.
The eager loading seems reasonable, but I do not understand the details here.
In what way was loading failing? You still have to initialize `VMSupport`
before you can call methods on it, so that could also fail - though the code
does not seem to notice/handle this. ??
src/hotspot/share/jvmci/jvmciCompilerToVM.cpp line 585:
> 583:
> 584: if (class_name->utf8_length() <= 1) {
> 585: JVMCI_THROW_MSG_0(InternalError, err_msg("Primitive type %s should
> be handled in Java code", str));
Seems unrelated to the fix at hand.
-
PR Review: https://git.openjdk.org/jdk/pull/14641#pullrequestreview-1497840467
PR Review Comment: https://git.openjdk.org/jdk/pull/14641#discussion_r1241749015
