Skip to content

Commit

Permalink
Speculative fix to crash for inlining:
Browse files Browse the repository at this point in the history 
  def self.new(*)
    super
  end

This introduces a problem with ScopeModule not really having a scope to
figure out what its module is anymore (the scope was inlined away).  The
answer is to use the inlining candidate type and for non-zero ScopeModules it
is to subtract one from scopeDepth? (I am less sure about this).  With this
change, self.new still does not properly inline but this seems like a step
in the right direction (and I want @subbuss to see the approach).
@enebo
enebo committed Dec 23, 2015
1 parent 3d1b500 commit 61f8da4
Showing 2 changed files with 9 additions and 1 deletion.
8 changes: 8 additions & 0 deletions core/src/main/java/org/jruby/ir/operands/ScopeModule.java
View file
Original file line number Diff line number Diff line change
@@ -4,6 +4,7 @@
import org.jruby.ir.persistence.IRReaderDecoder;
import org.jruby.ir.persistence.IRWriterEncoder;
import org.jruby.ir.transformations.inlining.CloneInfo;
import org.jruby.ir.transformations.inlining.InlineCloneInfo;
import org.jruby.parser.StaticScope;
import org.jruby.runtime.DynamicScope;
import org.jruby.runtime.Helpers;
@@ -45,6 +46,13 @@ public void addUsedVariables(List<Variable> l) {

@Override
public Operand cloneForInlining(CloneInfo ii) {
if (ii instanceof InlineCloneInfo) {
if (scopeModuleDepth == 0) { // replace scope with inlined candidate receiver
return ((InlineCloneInfo) ii).getRenamedSelfVariable(null);

This comment has been minimized.

Sign in to view

Copy link
@subbuss

subbuss Dec 23, 2015

Contributor

Can this happen? ClassBody is the only non-singleton class body => for all other scope types that is being inlined into the host scope, ScopeModule will always have scopeModuleDepth > 1. So, this scenario should not happen and you should assert.

This comment has been minimized.

Sign in to view

Copy link
@subbuss

subbuss Dec 23, 2015

Contributor

That assumption holds because you are never inlining a class body into something else.

This comment has been minimized.

Sign in to view

Copy link
@subbuss

subbuss Dec 23, 2015

Contributor

There are 2 other places in IRBuilder where ScopeModule comes from. But, both of them reference IRScope.getNearestModuleReferencingScopeDepth and for all non-module-body scopes, you have depth > 0. So, once again since you are inlining methods and blocks, depth cannot be zero in the scopes that are being inlined.

This comment has been minimized.

Sign in to view

Copy link
@enebo

enebo Dec 23, 2015

Author Member

Yet I can see it: https://gist.github.com/enebo/737ed265e936898f83ee
This is IR from a Foo.new method in:

class Foo
  def self.new(*)
    super
  end
end

def foo
  i = 1_000
  while true
    Foo.new
    i -= 1
    return 0 if i < 0
  end
end

3000.times do
  foo
end

This comment has been minimized.

Sign in to view

Copy link
@subbuss

subbuss Dec 23, 2015

Contributor

Aha .. ok .. I missed the direct reference to SCOPE_MODULE[0].

Actually, this shows that we both misunderstood 'scope' here. This is lexical scope (StaticScope / IRScope), not variable scope (DynScope). So, the -1 is actually incorrect in both scenarios. So, ScopeModule implicitly walks lexical scope stack from the scope in which the instruction exists.

So, there is implicit state / reference to the StaticScope/IRScope that is not represented in ScopeModule. If explicitly represented, inlining would not change ScopeModule since the StaticScope/IRScope reference is unchanged. You can see that as the currScope that seeds the search here. The same problem affects the CurrentScope operand. So, both need an explicit reference to the implicit starting scope (GetClassVarContainerModuleInstr has an operand for the starting scope, for ex.)

This problem is similar to the explicit self I added to WrappedIRClosure to solve a similar inlining problem.

But, I guess one problem here is that if StaticScope is considered non-static / runtime state, you need some symbolic way of referrring to it (for IR persistence, for ex.)
} else { // otherwise we only lost one scope from inlining...subtract one.
return ModuleFor(scopeModuleDepth - 1);
}
}
return this;
}

2 changes: 1 addition & 1 deletion core/src/main/java/org/jruby/ir/transformations/inlining/InlineCloneInfo.java
View file
Original file line number Diff line number Diff line change
@@ -150,7 +150,7 @@ protected Label getRenamedLabelSimple(Label l) {
return getHostScope().getNewLabel();
}

protected Variable getRenamedSelfVariable(Variable self) {
public Variable getRenamedSelfVariable(Variable self) {
return callReceiver;
}

0 comments on commit 61f8da4

Please sign in to comment.