-
Notifications
You must be signed in to change notification settings - Fork 6.2k
8353565: Javac throws "inconsistent stack types at join point" exception #24617
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
Conversation
|
👋 Welcome back jlahoda! A progress list of the required criteria for merging this PR into |
|
@lahodaj 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: 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 362 new commits pushed to the
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 |
Webrevs
|
vicente-romero-oracle
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
| types.isSubtype(t, tother) ? tother : | ||
| types.isSubtype(tother, t) ? t : | ||
| error(); | ||
| commonSuperClass(t, tother); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are the isSubtype() or t==tother checks above still necessary? Like do those fast paths not exist in types::lub?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just checked, lub is a varargs call and is slow. However, I think the fast path merge logic t==tother? ... should be in commonSuperClass.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The isSubtype serve two purposes - they handle null/BOT type (which, I think, lub normally does not handle), and they are cheaper to run.
I've moved them into commonSuperClass, as suggested, here:
bb08b49
vicente-romero-oracle
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lg
liach
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for updating per my review!
mcimadamore
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems a nice generalization of what the code used to do. One possible problematic aspect is non-denotable types. E.g. if A and B both implements two interfaces like I1 and I2. In that case the lub will be I1 & I2 which erased will lead you to I1. I think this is still correct because the verifier doesn't concern with interface types -- but would probably be better to double check.
Another alternative would be to use Object as the stackmap type is the join is non-denotable. But then I'm not sure what would be the ramifications of doing that. Using the erasure seems safer because javac will insert synthetic cast if e.g. accessing a member on something that doesn't look like the erasure (e.g. calling a method in I2 on a I1 & I2).
mcimadamore
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On a second look, the test for intersection type is already there. No further comments from me!
|
/integrate |
|
Going to push as commit a5f4366.
Your commit was automatically rebased without conflicts. |
Consider code like:
Compiling this leads to a crash:
The reason is that all of the yields "jump" to the same point in the bytecode, and javac is unable to compute the top-of-stack type for that point. javac expects that there's subtyping relation for the types on the join point.
In a little bit more detail:
(Object)cast is important, as that makes the switch expressions standalone expressions, whose types are inferred from the content, not from the target type-if we had only:
then this would compile, as the type of the switch expression would be
Object, and insidevisitYield, we useforceStackTopto change the top-stack type to the type of the switch expression. This works on one level, but does not work well for nested level switch expressions - while the outer switch expression in the original example has typeObject, the nested ones haveI1andI2, respectively, and the nestedyieldstatements setI1orI2as the stack-top type. And neither ofI1orI2is a subtype of the other, leading to the failure.The proposal here is to use
Types.lubto compute the common supertype of the types that join. The subtyping checks are kept, to help with some corner cases, in particular withnull/BOTtype handling. And also to limit potential performance impact.I also think the
forceStackTopis more a workaround that permitted the use of simply subtyping rather thanlubat the join points. And it only works for one level of nesting for switch expressions. The proposal here is to drop theforceStackTopfor both switch expressions and conditional expressions, so that if there are some cases where the new code would not work, we would find out faster/easier. But I can keep theforceStackTopcalls in that is preferred.(In any case, there's one call to
forceStackTopinGen.visitAssign. That one seems legitimate to me, so that one is kept.)Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/24617/head:pull/24617$ git checkout pull/24617Update a local copy of the PR:
$ git checkout pull/24617$ git pull https://git.openjdk.org/jdk.git pull/24617/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 24617View PR using the GUI difftool:
$ git pr show -t 24617Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/24617.diff
Using Webrev
Link to Webrev Comment