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

Fix break not turning into LocalJumpError soon enough #4694

Merged
merged 6 commits into from Jun 28, 2017

Conversation

Projects
None yet
2 participants
@headius
Member

headius commented Jun 28, 2017

For #4686 and #4577.

The strategy here is as it was in 1.7: mark literal blocks as "escaped" when the call they were originally passed to returns. Break operations can then immediately trigger a LocalJumpError and be rescued in all the various ways reported in #4686 and #4577.

This does not fix super calls because the additional work is nontrivial (pushing literal-closure param through a lot of code in interpreter and JIT) and the exposure surface is pretty small for super PLUS literal closure PLUS break PLUS escaping of that block.

headius added some commits Jun 27, 2017

Mark blocks as escaped and raise LJE for break when appropriate.
Fixes #4686.
Fixes #4577.

The logic here adds finally wrappers around all call paths that
receive a block. When the call exits, the block is marked
"escaped" since it no longer has a method activation to go with
it. This indicates that non-local flow, like break, should
immediately trigger a LocalJumpError.

This passes specs but has not been tested extensively with other
types of call forms that receive blocks.

@headius headius requested review from enebo and subbuss Jun 28, 2017

@headius headius added this to the JRuby 9.2.0.0 milestone Jun 28, 2017

@enebo

enebo approved these changes Jun 28, 2017

I think I would like the comments I made in interpreter addressed but can easily be talked out of wanting them. The null check is not used by all instrs there and duplication of block + blockType looks icky...It is a minor comment so don't let it hold us up. Otherwise it all makes sense.

@@ -433,7 +433,7 @@ protected static IRubyObject processReturnOp(ThreadContext context, Block block,
// This assumes that scopes with break instr. have a frame / dynamic scope
// pushed so that we can get to its static scope. For-loops now always have
// a dyn-scope pushed onto stack which makes this work in all scenarios.
return IRRuntimeHelpers.initiateBreak(context, currDynScope, rv, blockType);
return IRRuntimeHelpers.initiateBreak(context, currDynScope, rv, block, blockType);

This comment has been minimized.

@enebo

enebo Jun 28, 2017

Member

Looking at this code I wonder if we shouldn't just pass block. I see also we pass blockType for nonlocal returns but at top of this method we endlessly do block == null check and we may as well push that into initiateBreak and handleNonlocalReturn.

This comment has been minimized.

@headius

headius Jun 28, 2017

Member

Turns out blockType was only used by the two cases that always have a non-null block (as we discussed on IRC, non-local return and break should only be emitted for block bodies) so I just removed that null check and made each method look at block.type directly. Addresses this and other comment.

// Wrap the return value in an exception object and push it through the break exception
// paths so that ensures are run, frames/scopes are popped from runtime stacks, etc.
if (inLambda(blockType)) throw new IRWrappedLambdaReturnValue(breakValue);
IRScopeType scopeType = ensureScopeIsClosure(context, dynScope);
DynamicScope parentScope = dynScope.getParentScope();
if (block.isEscaped()) {

This comment has been minimized.

@enebo

enebo Jun 28, 2017

Member

Based on my previous comment I now wonder if block can be null here ever? Perhaps this is only called in cases where block is guaranteed non-null?

@@ -589,9 +589,9 @@ public void invokeOtherOneFloat(String file, int line, String name, double flote
if (!MethodIndex.hasFastFloatOps(name)) {
pushFloat(flote);
if (callType == CallType.NORMAL) {
invokeOther(file, line, name, 1, false, false);
invokeOther(file, line, name, 1, false, false, false);

This comment has been minimized.

@enebo

enebo Jun 28, 2017

Member

Not a review comment but just a lament that Java has no keyword arguments :P

This comment has been minimized.

@headius

headius Jun 28, 2017

Member

Yeah I don't stacking booleans like this. I might combine the two into an enum representing what exactly was passed for block argument.

This comment has been minimized.

@enebo

enebo Jun 28, 2017

Member

Hmmm I wonder how many people static final booleans for this sort of issue? I don't think I have ever seen that done but it is probably a pattern?

headius added some commits Jun 28, 2017

Final tweaks to LJE break fixes.
* Add literal closure method to IR closure-accepting interface
* Replace double boolean in JIT code with enum
* Check for null in non-local return logic
@headius

This comment has been minimized.

Member

headius commented Jun 28, 2017

I've made all suggested tweaks and I'm happy with it.

Note that I had to re-add a null check in IRRuntimeHelpers.initiateNonLocalReturn because it appears that that instruction is also used to return from a singleton class body. I saw an error in the JRuby suite, running without that null check, that looked like this:

[exec] Error: test_sclass_return(TestClass): Java::JavaLang::NullPointerException:
[exec] org.jruby.ir.runtime.IRRuntimeHelpers.initiateNonLocalReturn(IRRuntimeHelpers.java:99)

We may want to see if this really needs to be a non-local return (@enebo).

I will open a separate issue for the known-unfixed super behavior.

@headius headius merged commit f7b21a0 into master Jun 28, 2017

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details

@headius headius deleted the fix_break_lje branch Jun 28, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment