-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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
8316746: Top of lock-stack does not match the unlocked object #16345
Conversation
👋 Welcome back mdoerr! A progress list of the required criteria for merging this PR into |
@TheRealMDoerr The following label will be automatically applied to this pull request:
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. |
What's the best way to reproduce the problem on x64? |
// from the interpreter frame. Using map()->peek... may return them | ||
// in wrong order. We should use the popped obj. | ||
if (LockingMode == LM_LEGACY) { | ||
// May potentially be the wrong one in case of OSR, but we need both, box and obj. |
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.
This appears to mask the real problem, which, unless I missed it, has not been explained yet.
This is my understanding of the problem: The locked object can be found at two places in the OSR buffer:
Normally, both ways should deliver the right object because the interpreter handles both structures like a kind of stack if the bytecode is well-formed regarding the monitors (which is checked by the JIT compilers). However, I believe that it's possible to mess things up by using a debugger. Note that the failing test is vmTestbase/nsk/jdi/StepEvent. The interpreter uses the 2nd variant for unlocking (atos is the object to unlock) and doesn't care about the order of 1. C1 does the same by The idea of "JDK-8316746-reproducer.patch" (in the JBS issue) is to show that the objects in 1. are not ordered the way as C2 needs them on x64. The question is if we can somehow make the order of 1. correct, but I think it will be more robust to follow the interpreter and C1. |
Btw. thanks a lot for looking at my reproducer and your time! I've fixed it as you suggested and now it's only failing on PPC64. I'll check the interpreter. In the long term, I'd still appreciate to get this changed and the BoxLock removed. |
Thanks for your feedback! We can fix it in the PPC64 interpreter. We may get back to improving C2 later. |
In case of OSR compilations,
map()->peek_monitor_obj()
may return the objects in wrong order. We should use the popped object. I'm not changing the code forLM_LEGACY
. That may be done separately.Note that C1 uses the popped object, too:
case Bytecodes::_monitorexit : monitorexit (apop(), s.cur_bci());
This PR is not complete: A lot of C2 code uses the BoxLock. Ideas to solve that are welcome! I think we should get rid of these nodes in the long term because they are only really needed by
LM_LEGACY
.The vmTestbase/nsk/jdi/StepEvent tests are passing stable on x64 and ppc64.
Progress
Issue
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/16345/head:pull/16345
$ git checkout pull/16345
Update a local copy of the PR:
$ git checkout pull/16345
$ git pull https://git.openjdk.org/jdk.git pull/16345/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 16345
View PR using the GUI difftool:
$ git pr show -t 16345
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/16345.diff
Webrev
Link to Webrev Comment