8350208: CTW: GraphKit::add_safepoint_edges asserts "not enough operands for reexecution"

[merykitty]

Hi,

This PR fixes the issue of the compiler crashing with "not enough operands for reexecution". The issue here is that during Parse::catch_inline_exceptions, the old stack is gone, and we cannot reexecute the current bytecode anymore. However, there are some places where we try to insert safepoints into the graph, such as if the handler is a backward jump, or if one of the exceptions in the handlers is not loaded. Since the _reexecute state of the current jvms is "undefined", it is inferred automatically that it should reexecute for some bytecodes such as putfield. The solution then is to explicitly set _reexecute to false.

I can manage to write a unit test for the case of a backward handler, for the other cases, since the exceptions that can be thrown for a bytecode that is inferred to reexecute are NullPointerException, ArrayIndexOutOfBoundsException, and ArrayStoreException. I find it hard to construct such a test in which one of them is not loaded.

Please kindly review, thanks a lot.

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8350208: CTW: GraphKit::add_safepoint_edges asserts "not enough operands for reexecution" (Bug - P3)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/28597/head:pull/28597
$ git checkout pull/28597

Update a local copy of the PR:
$ git checkout pull/28597
$ git pull https://git.openjdk.org/jdk.git pull/28597/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 28597

View PR using the GUI difftool:
$ git pr show -t 28597

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/28597.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back qamai! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

[openjdk]

@merykitty The following labels will be automatically applied to this pull request:

• core-libs
• hotspot-compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 00: Full (89edd0e8)

[dean-long]

I agree there are situations where we need to set the reexecute flag explicitly and not base it on the bytecode. I recently fixed JDK-8370766 and filed JDK-8372846 as a followup for similar issues. I need to try out your test to understand this better. Does it cause a backwards-branch safepoint? I suspect that it may not be safe to set rexeecute to false here. If reexecute is false and -XX:+VerifyStack is set, deoptimization may fail if the operands are not on the stack.

[merykitty]

Yes, it is a backwards-branch safepoint.

Tbh, after looking deeper, I don't really understand what is happening here. I modified the test a little bit so the final compiled code does not elide the safepoint in the loop, and ran with -XX:+VerifyStack -XX:+DeoptimizeALot -XX:+SafepointALot, but the test still passed after 100 repeats. I think that the state is correct, but I don't see how the compiled code notifies the deoptimizater and the interpreter that it is in an exception state, and the interpreter needs to find an exception handler instead of continuing with the next bytecode. My guess is that the compiled code should store the exception into Thread::_pending_exception, or the deoptimizer needs to do so, and the interpreter needs to check that when being handed the control. But I have not yet found that.

[dean-long]

It seems to be very difficult to force the back-edge safepoint to deoptimize. I tried creating a thread that calls System.gc(), but so far no crash. Still, I think the state is incorrect if reexecute=false. Setting reexecute to false means it will skip the current instruction. To correctly handle a deoptimization on the backwards branch, the debug state, bci, and exception location should match. I think we have 3 choices to prepare for maybe_add_safepoint():

1. preserve stack inputs, use original bci, do not push exception oop, let interpreter reexecute and throw the exception (reexecute=true)
   This might be as simple as reversing the order of calls to push_ex_oop and maybe_add_safepoint.
2. trim stack, push exception object, use bci of exception handler (reexecute=true)
   This would require temporarily changing the bci for the maybe_add_safepoint call.
3. trim stack, throw exception (move to Thread) (reexecute=true)
   This requires extra unconditional overhead even though safepoint rarely happens.

[eme64]

Suggested change

	* @run main/othervm compiler.exceptions.TestDebugDuringExceptionCatching
	* @run main/othervm ${test.main.class}

Drive-by comment.

[dean-long]

@merykitty , I tried solution 1) and it seems to work, but I think I prefer solution 2) because it aligns better with my idea from JDK-8372846 of canonicalized exception states. If you like, I can take over this bug.

[merykitty]

@dean-long Thanks a lot, please take over this bug then.

trim stack, throw exception (move to Thread) (reexecute=true)
This requires extra unconditional overhead even though safepoint rarely happens.

I have been thinking about this approach, we can have additional parameter in the SafePointNode to notify that it is in an exception state, then the deoptimizer can take the responsibility of throwing the actual exception.

[bridgekeeper]

@merykitty This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply issue a /touch or /keepalive command to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!