Navigation Menu

Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8275638: GraphKit::combine_exception_states fails with "matching stack sizes" assert #6572

Closed
wants to merge 6 commits into from

Conversation

rwestrel
Copy link
Contributor

@rwestrel rwestrel commented Nov 26, 2021

Root cause is identical to 8273165 AFIU: late inline of a virtual call
can throw from 2 different paths (null check and the call
itself). That breaks because the logic for exceptions expects the
stack for all paths that throw exceptions to have the same stack size.

AFAIU, the stack doesn't matter exception handling: either the
exception is caught by a exception handler and then the stack is
popped and the exception is pushed or, the exception is rethrown to
the caller in which case the current stack is also popped (that is the
jvm state for the current method). As a consequence the fix I propose
is to ignore the stack in GraphKit::combine_exception_states().

AFAIU, the same fix would work for 8273165 but I left the current work
around as is: not sure if we want to be conservative for now or not


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8275638: GraphKit::combine_exception_states fails with "matching stack sizes" assert

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/6572/head:pull/6572
$ git checkout pull/6572

Update a local copy of the PR:
$ git checkout pull/6572
$ git pull https://git.openjdk.java.net/jdk pull/6572/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 6572

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/6572.diff

@bridgekeeper
Copy link

bridgekeeper bot commented Nov 26, 2021

👋 Welcome back roland! 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 openjdk bot added the rfr Pull request is ready for review label Nov 26, 2021
@openjdk
Copy link

openjdk bot commented Nov 26, 2021

@rwestrel The following label will be automatically applied to this pull request:

  • hotspot-compiler

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

@openjdk openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Nov 26, 2021
@mlbridge
Copy link

mlbridge bot commented Nov 26, 2021

Webrevs

@dean-long
Copy link
Member

The existing code is confusing to me. Are these exception states only used when we are actually throwing an exception, or can do they affect the state when deoptimizing? Why does GraphKit::add_exception_state() have a comment about arguments and compares stacks? Why doesn't GraphKit::make_exception_state() just reset the stack using push_ex_oop() instead of preserving the stack wtih set_saved_ex_oop()? (The names for these two functions are terrible, because "push_ex_oop" resets the stack first and "set_saved_ex_oop" does not...)

@dean-long
Copy link
Member

I tried calling

  set_sp(0);
  clean_stack(0);

in GraphKit::make_exception_state() and it seems to work, but I still don't understand why the stack size mismatch only shows up with late inlining.

@rwestrel
Copy link
Contributor Author

Thanks for looking at this @dean-long

The existing code is confusing to me. Are these exception states only used when we are actually throwing an exception, or can do they affect the state when deoptimizing?

They are only for throwing exceptions

Why does GraphKit::add_exception_state() have a comment about arguments and compares stacks?

That makes little sense to me too.

Why doesn't GraphKit::make_exception_state() just reset the stack using push_ex_oop() instead of preserving the stack wtih set_saved_ex_oop()?

I'm not sure about that either.

@rwestrel
Copy link
Contributor Author

in GraphKit::make_exception_state() and it seems to work, but I still don't understand why the stack size mismatch only shows up with late inlining.

At parse time, exception throwing goes through Parse::do_exceptions() which either:

  • pops the current frame in Parse::throw_to_exit() so there can't be a stack size mismatch anymore if "exception states" are combined later on
    or
  • push the exception on the stack in Parse::catch_inline_exceptions() which causes the stack to be resized. When that code path is taken there can be 2 "exception states" for a single bci (the null check and the exception from the call in the case of a virtual call) and they are not "combined" (the explicit test in GraphKit::add_exception_state() prevents that).

I agree that this all feels tortuous but not sure if cleaning it up as part of this change is the best thing to do.

@dean-long
Copy link
Member

If we want to be conservative, then perhaps we should do a bailout if the stack size don't match. Ignoring the stack in one place, but being careful to preserve it in others makes me think we are missing something. For example, this troubling comment from Parse::catch_inline_exceptions():

911 // Start executing from the given throw state. (Keep its stack, for now.)

I think we need a followup RFE to clean this all up once and for all.

@vnkozlov, do we have someone who really understands what this exceptions code is doing in regards to stack sizes?

@rwestrel
Copy link
Contributor Author

If we want to be conservative, then perhaps we should do a bailout if the stack size don't match. Ignoring the stack in one place, but being careful to preserve it in others makes me think we are missing something. For example, this troubling comment from Parse::catch_inline_exceptions():

911 // Start executing from the given throw state. (Keep its stack, for now.)

If we're missing something, wouldn't testing catch it? Wouldn't running that patch through extensive testing help then?

@dean-long
Copy link
Member

If we're missing something, wouldn't testing catch it? Wouldn't running that patch through extensive testing help then?

Normally, I would say yes, but my impression is that we don't have great test coverage for this issue, and I'm concerned that this close to the RDP1 date we should go with the most conservative fix.

Out of curiosity, I checked when the "Keep its stack, for now" comment was added, and it was for JDK-4432078. The comment in the bug says:

"Missing stack contents in graphkit-based exceptions make it impossible
to re-run the trapping bytecode in the interpreter. Fix is to retain
stack information a little longer."

which again brings up my concern that this exception information may be used for deoptimization. Prior to JDK-4432078, it appears that we did truncate the stack size before pushing the exception object, and that apparently led to problems. Given this new information, this change makes me especially nervous:

-    // Skip everything in the JVMS after tos.  (The ex_oop follows.)
-    if (i == tos)  i = ex_jvms->monoff();
+    // Skip everything in the JVMS after the stack (included).  (The ex_oop follows.)
+    if (i == ex_jvms->stkoff())  i = ex_jvms->monoff();

@rwestrel
Copy link
Contributor Author

rwestrel commented Dec 1, 2021

Out of curiosity, I checked when the "Keep its stack, for now" comment was added, and it was for JDK-4432078. The comment in the bug says:

"Missing stack contents in graphkit-based exceptions make it impossible to re-run the trapping bytecode in the interpreter. Fix is to retain stack information a little longer."

Thanks for investigating that further.
Where would deoptimization occur then? In Parse::catch_inline_exceptions() when the right exception handlers is picked with subtype checks? I don't see any uncommon trap there. Maybe the requirement to keep the stack is no longer necessary?

The reason I didn't go with Vladimir's work around for 8273165 is that I think it could have a performance impact that would be more likely to be noticed than in the case of 8273165 (because late inlining of method handle has been around for many releases and is likely something that's relied on). We could extend Vladimir's work around by checking that the receiver may be null and that the null check would cause the exception to be thrown rather than deoptimization.

Another way to deal with this could be to pop the stack if the current method has no exception handlers because then the exception is passed on to the caller and the entire frame is popped anyway. That would work nicely for this case as AFAIU, the method handle invoker can only be inlined from a lambda form that wouldn't have exception handlers.

@dean-long
Copy link
Member

Where would deoptimization occur then? In Parse::catch_inline_exceptions() when the right exception handlers is picked with subtype checks? I don't see any uncommon trap there. Maybe the requirement to keep the stack is no longer necessary?

At the end of the method I see:

  1. a runtime call to the rethrow stub. The comment says "must not deoptimize", but I'm not sure what prevents that
  2. catch_call_exceptions(), which does an uncommon trap for unloaded exception classes

The reason I didn't go with Vladimir's work around for 8273165 is that I think it could have a performance impact that would be more likely to be noticed than in the case of 8273165 (because late inlining of method handle has been around for many releases and is likely something that's relied on). We could extend Vladimir's work around by checking that the receiver may be null and that the null check would cause the exception to be thrown rather than deoptimization.

But with possible performance impact, right?

Another way to deal with this could be to pop the stack if the current method has no exception handlers because then the exception is passed on to the caller and the entire frame is popped anyway. That would work nicely for this case as AFAIU, the method handle invoker can only be inlined from a lambda form that wouldn't have exception handlers.

That sounds promising.

@rwestrel
Copy link
Contributor Author

rwestrel commented Dec 2, 2021

1. a runtime call to the rethrow stub.  The comment says "must not deoptimize", but I'm not sure what prevents that

That one is a leaf call and doesn't capture debug info so I think it's safe to assume it can't deoptimize.

2. catch_call_exceptions(), which does an uncommon trap for unloaded exception classes

There's a push_ex_oop() right because the uncommon trap and that one clears the stack so it can't be the reason to preserve the stack.

The reason I didn't go with Vladimir's work around for 8273165 is that I think it could have a performance impact that would be more likely to be noticed than in the case of 8273165 (because late inlining of method handle has been around for many releases and is likely something that's relied on). We could extend Vladimir's work around by checking that the receiver may be null and that the null check would cause the exception to be thrown rather than deoptimization.

But with possible performance impact, right?

Right.

Another way to deal with this could be to pop the stack if the current method has no exception handlers because then the exception is passed on to the caller and the entire frame is popped anyway. That would work nicely for this case as AFAIU, the method handle invoker can only be inlined from a lambda form that wouldn't have exception handlers.

That sounds promising.

So you agree that if there's no handler then there's no need to preserve the stack?

@dean-long
Copy link
Member

So you agree that if there's no handler then there's no need to preserve the stack?

Yes, if we are blowing away the frame and there's no chance the state will be used by an uncommon trap. As far as I can tell from SCCS comments for JDK-4432078 and related duplicate bugs, the original problem was re-executing the trapping bytecode with an uncommon trap. And one way that could have happened was removed between jdk5 and jdk6 with this change:

http://hg.openjdk.java.net/jdk6/jdk6/hotspot/rev/fdd57634910e

If that was the only uncommon trap causing a problem, then there's a good chance that preserving the stack is no longer needed at all.

@rwestrel
Copy link
Contributor Author

rwestrel commented Dec 7, 2021

I pushed a new change that pops the stack when there's no handler in the method.

@dean-long
Copy link
Member

Roland, this change seems to work, but I feel like it is only luck that the stack is empty when GraphKit::make_slow_call_ex() pushes the 2nd exception state. Isn't it possible to construct a test case where the stack is not empty when the call returns?

@rwestrel
Copy link
Contributor Author

rwestrel commented Dec 8, 2021

Roland, this change seems to work, but I feel like it is only luck that the stack is empty when GraphKit::make_slow_call_ex() pushes the 2nd exception state. Isn't it possible to construct a test case where the stack is not empty when the call returns?

Given the call doesn't return but throws, can there be anything on the stack?
Late inlining in this case occurs in a lambda form that's generated by the system so I have no control over the byte code at the call site.
Do you think it would better to clear the stack in make_exception_state() which is taken for both exception paths?

@dean-long
Copy link
Member

Given the call doesn't return but throws, can there be anything on the stack?

Maybe not in your test case, but in general, I believe there can be something on expression stack when the null check is peformed or when the method returns. I tried to change your test to have the method return "int" and use it in an expression, but that didn't help.

Late inlining in this case occurs in a lambda form that's generated by the system so I have no control over the byte code at the call site.

Can this code get inlined into the caller? Then there could be values on the stack from the caller.

Do you think it would better to clear the stack in make_exception_state() which is taken for both exception paths?

It might be the correct fix, but it increases the risk. That would basically revert the behavior to how it was before JDK-4432078. There may not be a path to an uncommon trap that needs the exception state expression stack anymore, but how would be future-proof the code to protect against a developer adding an uncommon trap that breaks this assumption?

@rwestrel
Copy link
Contributor Author

rwestrel commented Dec 8, 2021

Given the call doesn't return but throws, can there be anything on the stack?

Maybe not in your test case, but in general, I believe there can be something on expression stack when the null check is peformed or when the method returns. I tried to change your test to have the method return "int" and use it in an expression, but that didn't help.

AFAIU, the return of the method only affects the case where there's no exception. If there's an exception, then the callee doesn't exit normally and can't push anything on the stack.

Late inlining in this case occurs in a lambda form that's generated by the system so I have no control over the byte code at the call site.

Can this code get inlined into the caller? Then there could be values on the stack from the caller.

Yes, it can but can there be anything on the stack across a call? Isn't everything on the stack at the call expected to be arguments to the call?

Do you think it would better to clear the stack in make_exception_state() which is taken for both exception paths?

It might be the correct fix, but it increases the risk. That would basically revert the behavior to how it was before JDK-4432078. There may not be a path to an uncommon trap that needs the exception state expression stack anymore, but how would be future-proof the code to protect against a developer adding an uncommon trap that breaks this assumption?

That wouldn't revert to the behavior before JDK-4432078 because the change is only in the case where there's no handler in the method.

@dean-long
Copy link
Member

Yes, it can but can there be anything on the stack across a call? Isn't everything on the stack at the call expected to be arguments to the call?

The call can be part of an expression, or its result used as arguments to another call.

That wouldn't revert to the behavior before JDK-4432078 because the change is only in the case where there's no handler in the method.

Are you proposing to clear the stack in make_exception_state but conditional on !method()->has_exception_handlers()? That is probably OK for your test case, but I don't see why we can't run into the same problem when there are exception handlers and late inlining.

@rwestrel
Copy link
Contributor Author

rwestrel commented Dec 9, 2021

That wouldn't revert to the behavior before JDK-4432078 because the change is only in the case where there's no handler in the method.

Are you proposing to clear the stack in make_exception_state but conditional on !method()->has_exception_handlers()? That is probably OK for your test case, but I don't see why we can't run into the same problem when there are exception handlers and late inlining.

Sure there are issues but they should be addressed by Vladimir's fix/workaround for 8273165.

@dean-long
Copy link
Member

@roland, I've asked John Rose to take a look at this too.

I'm still staring at the code trying to figure out why parse-time inlining can avoid with this problem, but late inlining can't.

@rose00
Copy link
Contributor

rose00 commented Dec 14, 2021

It is true that when an exception is being thrown the stack is clear.

The only reason C2 sometimes keeps (or used to keep) stuff on the stack along an exception path is that, when an exception in being thrown by a bytecode that is re-executable, C2 might cleverly issue an uncommon trap that re-executes the throwing bytecode. In that case, having the stack unchanged is obviously important. But call instructions are not re-executable, and any exception coming from a call (directly or indirectly) must clear the stack.

Even if there is a matching local handler in the method making the call, such as a catch (NullPointerException _) where a call might throw NPE, the correct move is to clear the stack when the throw is started (by the call), and then thread the JVMS (with empty stack) into the handler.

@rwestrel
Copy link
Contributor Author

I'm still staring at the code trying to figure out why parse-time inlining can avoid with this problem, but late inlining can't.

With parse time inlining, when the call is processed, 2 exception states are added. One for the null check before the call and one for exceptions thrown after the call. They have different sp so they can't be combined. Either the exception is caught in the current method in which case, the 2 exception states are processed independently in Parse::catch_inline_exceptions() at which point the expression stacks are popped for each one of them. Or the exception is passed on the caller in which case, the expression stacks are also popped in Parse::throw_to_exit().

With late inlining, the call to Parse::catch_inline_exceptions() or Parse::throw_to_exit() happened before the call site is processed and that one assumed a single exception state. So when late inlining occurs, exception states must be combined while with parse time inlining they don't need to be combined.

FWIW, my understanding of what happens comes from running the tests of this PR with and without late inlining and stepping through the code.

I'm unclear where we go from here. My opinion is that, by going over the history of the code, you figured out why expression handling preserves the stack. And AFAICT, you also found that the code that needed the stack preserved was removed. So I would go with a fix that pops the stack when an exception is thrown, have it go through as much testing as possible and integrate that fix if testing is clean. Whether we want to have the exception on stack in exception states or as an extra edge (as it is today) is something that could be revisited later.

@dean-long
Copy link
Member

Yes, I agree that's the current difference between parse-time inlining and late inlining as implemented today. I'd just like to know why late inlining must combine the exceptions states into a single state. But that's not a blocker for your fix.

John wrote me and said your fix looks OK (thought not a general solution). For your test, he suggested adding a 3rd case with extra call depth: "test3 -> mcaller -> m". Also, I believe @vnkozlov will take a look too.

It would be nice if some of the assumptions the code is making could be checked, which could be done in a separate RFE with perhaps a more general solution:

  • an exception state with cleared stack won't be used to reexecute a bytecode (uncommon trap)
  • an exception state with uncleared stack must be used to reexecute
  • safepoint stack size matches computed interpreter oopmap stack size (VerifyStack logic)

@openjdk
Copy link

openjdk bot commented Dec 14, 2021

@rwestrel This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8275638: GraphKit::combine_exception_states fails with "matching stack sizes" assert

Reviewed-by: dlong

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 130 new commits pushed to the master branch:

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Dec 14, 2021
@rwestrel
Copy link
Contributor Author

John wrote me and said your fix looks OK (thought not a general solution). For your test, he suggested adding a 3rd case with extra call depth: "test3 -> mcaller -> m". Also, I believe @vnkozlov will take a look too.

Thanks @dean-long. I need to open a PR for jdk 18 and close that one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot-compiler hotspot-compiler-dev@openjdk.org ready Pull request is ready to be integrated rfr Pull request is ready for review
3 participants