Always treat non-full IC as not having protocol#6288
Merged
headius merged 1 commit intojruby:jruby-9.2from Jun 18, 2020
Merged
Conversation
enebo
reviewed
Jun 18, 2020
core/src/main/java/org/jruby/runtime/InterpretedIRBlockBody.java
Outdated
Show resolved
Hide resolved
The logic here used to go to interpreterContext to check for the call protocol logic. The hasExplicitCallProtocol method returns a cached value acquired from the encapsulated scope, via its flags. However the enum of flags in IRScope may be mutated concurrently, leading to a situation where we have seen the scope flip to having call protocols installed, but the InterpreterContext in hand is still the one without protocol instructions. As a result, we may start interpreting using an unoptimized IR along the "direct" path which does not push any scope state. This lead to the AIOOB reported later in jruby#6282. The NPE reported in jruby#6282 has not reproduced for me on the jruby-9.2 branch, and may have been fixed through other means, but this modified logic should also help any such cases that remain. Instead of overwriting the two InterpreterContext fields, the non- full version will always remain the non-full version, and the full version will only be set into that field. Once set, neither are ever cleared or re-set to a new value, avoiding a transient null in the instructionContext field. I ran the new reproduction from jruby#6282 for a few hours and saw no errors. Adding a test that guarantees this is fixed may be difficult, but I believe we've got it here. This logic will be overhauled for 9.3, so we'll go with this workaround for now and call it fixed. Fixes jruby#6282.
headius
force-pushed
the
more_atomic_full_build
branch
from
3e760d4 to
b0a3990
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The logic here used to go to interpreterContext to check for the
call protocol logic. The hasExplicitCallProtocol method returns a
cached value acquired from the encapsulated scope, via its flags.
However the enum of flags in IRScope may be mutated concurrently,
leading to a situation where we have seen the scope flip to having
call protocols installed, but the InterpreterContext in hand is
still the one without protocol instructions. As a result, we may
start interpreting using an unoptimized IR along the "direct" path
which does not push any scope state. This lead to the AIOOB
reported later in #6282.
The NPE reported in #6282 has not reproduced for me on the
jruby-9.2 branch, and may have been fixed through other means, but
this modified logic should also help any such cases that remain.
Instead of overwriting the two InterpreterContext fields, the non-
full version will always remain the non-full version, and the full
version will only be set into that field. Once set, neither are
ever cleared or re-set to a new value, avoiding a transient null
in the instructionContext field.
I ran the new reproduction from #6282 for a few hours and saw no
errors. Adding a test that guarantees this is fixed may be
difficult, but I believe we've got it here.
This logic will be overhauled for 9.3, so we'll go with this
workaround for now and call it fixed.
Fixes #6282.