Skip to content

Commit

Permalink
HEADS UP! Removed all trace of setWithinDefined and isWithinDefined.
Browse files Browse the repository at this point in the history
* Tested on rubyspecs in 1.8 and 1.9 mode -- AST, JIT, IR.  No
  change in failures after this change.

* Removed this from AST interpreter, JIT, IR

* setWithinDefined was set by all of these before entering defined?
  checks and cleared on exit.  isWithinDefined was only used by
  RaiseException to decide whether to set $! or not -- effectively
  $! was not being set for exceptions raised by code executing
  by the call tree rooted in the defined? checks.  However, this
  resulted in buggy behavior because this call tree could do a ton
  of work (including loading files, requires, setting up classes,
  etc.) any of which can raise exceptions and might have to be
  handled, which require access to $!.  The workaround in IR and
  AST interpreters was to set "$!" whenever an exception was
  received (effectively bypassing the condition that RaiseException
  was guarding).  But, since I removed both these hacky checks as
  part of a previous commit (20632af),
  we had code failures in Rails (bug JRUBY-6705) because of files
  not getting loaded properly. So, given that isWithinDefined is only
  used by RaiseException, which was anyway getting bypassed, there is
  no reason to keep that check around.  Once I removed that check,
  since there was no other use of isWithinDefined in the codebase,
  I also removed setWithinDefined, which led to a bunch of dead code
  which I purged.

* NOTE: JIT is probably yet to remove the $! hack that I removed in
  the other commit.  It would be best to remove that hack as well
  so we know for sure that these two commits are working together
  well without bugs.
  • Loading branch information
subbuss committed Jun 22, 2012
1 parent b830efc commit 7c3f642
Show file tree
Hide file tree
Showing 11 changed files with 7 additions and 186 deletions.
9 changes: 2 additions & 7 deletions src/org/jruby/ast/DefinedNode.java
Original file line number Diff line number Diff line change
Expand Up @@ -83,12 +83,7 @@ public List<Node> childNodes() {

@Override
public IRubyObject interpret(Ruby runtime, ThreadContext context, IRubyObject self, Block aBlock) {
try {
context.setWithinDefined(true);
ByteList definition = expressionNode.definition(runtime, context, self, aBlock);
return definition != null ? RubyString.newStringShared(runtime, definition) : runtime.getNil();
} finally {
context.setWithinDefined(false);
}
ByteList definition = expressionNode.definition(runtime, context, self, aBlock);
return definition != null ? RubyString.newStringShared(runtime, definition) : runtime.getNil();
}
}
7 changes: 1 addition & 6 deletions src/org/jruby/ast/OpAsgnOrNode.java
Original file line number Diff line number Diff line change
Expand Up @@ -104,12 +104,7 @@ public IRubyObject interpret(Ruby runtime, ThreadContext context, IRubyObject se
}

private boolean defined(Ruby runtime, ThreadContext context, Node node, IRubyObject self, Block aBlock) {
try {
context.setWithinDefined(true);
return node.definition(runtime, context, self, aBlock) != null;
} finally {
context.setWithinDefined(false);
}
return node.definition(runtime, context, self, aBlock) != null;
}

@Override
Expand Down
15 changes: 1 addition & 14 deletions src/org/jruby/compiler/ASTCompiler.java
Original file line number Diff line number Diff line change
Expand Up @@ -1489,20 +1489,7 @@ public void compileGetDefinitionBase(final Node node, BodyCompiler context) {
compileGetDefinitionBase(((NewlineNode)node).getNextNode(), context);
break;
default:
BranchCallback reg = new BranchCallback() {

public void branch(BodyCompiler context) {
context.inDefined();
compileGetDefinition(node, context);
}
};
BranchCallback out = new BranchCallback() {

public void branch(BodyCompiler context) {
context.outDefined();
}
};
context.protect(reg, out, ByteList.class);
compileGetDefinition(node, context);
}
}

Expand Down
2 changes: 0 additions & 2 deletions src/org/jruby/compiler/BodyCompiler.java
Original file line number Diff line number Diff line change
Expand Up @@ -535,8 +535,6 @@ public void defineNewMethod(String name, int methodArity, StaticScope scope,
public void performRescue(BranchCallback regularCode, BranchCallback rubyCatchCode, BranchCallback rubyElseCode, boolean needsRetry);
public void performRescueLight(BranchCallback regularCode, BranchCallback rubyCatchCode, BranchCallback rubyElseCode, boolean needsRetry);
public void performEnsure(BranchCallback regularCode, BranchCallback ensuredCode);
public void inDefined();
public void outDefined();
public void stringOrNil();
public void pushNull();
public void pushString(String strVal);
Expand Down
14 changes: 1 addition & 13 deletions src/org/jruby/compiler/impl/BaseBodyCompiler.java
Original file line number Diff line number Diff line change
Expand Up @@ -1852,18 +1852,6 @@ public void wrapJavaObject() {
method.invokestatic(p(JavaUtil.class), "convertJavaToUsableRubyObject", sig(IRubyObject.class, Ruby.class, Object.class));
}

public void inDefined() {
method.aload(StandardASMCompiler.THREADCONTEXT_INDEX);
method.iconst_1();
invokeThreadContext("setWithinDefined", sig(void.class, params(boolean.class)));
}

public void outDefined() {
method.aload(StandardASMCompiler.THREADCONTEXT_INDEX);
method.iconst_0();
invokeThreadContext("setWithinDefined", sig(void.class, params(boolean.class)));
}

public void stringOrNil() {
loadThreadContext();
invokeUtilityMethod("stringOrNil", sig(IRubyObject.class, ByteList.class, ThreadContext.class));
Expand Down Expand Up @@ -2880,4 +2868,4 @@ public void definedNot() {
method.swap();
invokeUtilityMethod("getDefinedNot", sig(ByteList.class, Ruby.class, ByteList.class));
}
}
}
4 changes: 1 addition & 3 deletions src/org/jruby/exceptions/RaiseException.java
Original file line number Diff line number Diff line change
Expand Up @@ -236,9 +236,7 @@ private void doCallEventHook(ThreadContext context) {
}

private void doSetLastError(ThreadContext context) {
if (!context.isWithinDefined()) {
context.runtime.getGlobalVariables().set("$!", exception);
}
context.runtime.getGlobalVariables().set("$!", exception);
}

/**
Expand Down
58 changes: 1 addition & 57 deletions src/org/jruby/ir/IRBuilder.java
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
import org.jruby.ir.instructions.defined.MethodDefinedInstr;
import org.jruby.ir.instructions.defined.MethodIsPublicInstr;
import org.jruby.ir.instructions.defined.RestoreErrorInfoInstr;
import org.jruby.ir.instructions.defined.SetWithinDefinedInstr;
import org.jruby.ir.instructions.defined.SuperMethodBoundInstr;
import org.jruby.ir.instructions.ruby18.ReceiveOptArgInstr18;
import org.jruby.ir.instructions.ruby18.ReceiveRestArgInstr18;
Expand Down Expand Up @@ -1123,43 +1122,6 @@ interface CodeBlock {
public Operand run(Object[] args);
}

private Variable protectCodeWithEnsure(IRScope s, CodeBlock protectedCode, Object[] protectedCodeArgs, CodeBlock ensureCode, Object[] ensureCodeArgs) {
// This effectively mimics a begin-ensure-end code block
// Except this silently swallows all exceptions raised by the protected code

Variable ret = s.getNewTemporaryVariable();

// Push a new ensure block info node onto the stack of ensure block
EnsureBlockInfo ebi = new EnsureBlockInfo(s, null, getCurrentLoop());
_ensureBlockStack.push(ebi);
Label rBeginLabel = ebi.regionStart;
Label rEndLabel = ebi.end;

// Protected region code
s.addInstr(new LabelInstr(rBeginLabel));
s.addInstr(new ExceptionRegionStartMarkerInstr(rBeginLabel, rEndLabel, ebi.dummyRescueBlockLabel, ebi.dummyRescueBlockLabel));
Operand v1 = protectedCode.run(protectedCodeArgs); // YIELD: Run the protected code block
s.addInstr(new CopyInstr(ret, v1));
s.addInstr(new JumpInstr(ebi.start));
s.addInstr(new ExceptionRegionEndMarkerInstr());

// Rescue block code
// SSS FIXME: How do we get this to catch all exceptions, not just Ruby exceptions?
s.addInstr(new LabelInstr(ebi.dummyRescueBlockLabel));
s.addInstr(new CopyInstr(ret, manager.getNil()));

_ensureBlockStack.pop();

// Ensure block code -- this should not throw exceptions
s.addInstr(new LabelInstr(ebi.start));
ensureCode.run(ensureCodeArgs); // YIELD: Run the ensure code block

// End
s.addInstr(new LabelInstr(rEndLabel));

return ret;
}

private Operand protectCodeWithRescue(IRScope m, CodeBlock protectedCode, Object[] protectedCodeArgs, CodeBlock rescueBlock, Object[] rescueBlockArgs) {
// This effectively mimics a begin-rescue-end code block
// Except this catches all exceptions raised by the protected code
Expand Down Expand Up @@ -1202,25 +1164,7 @@ private Operand protectCodeWithRescue(IRScope m, CodeBlock protectedCode, Object
}

protected Operand buildGenericGetDefinitionIR(Node node, IRScope s) {
s.addInstr(new SetWithinDefinedInstr(manager.getTrue()));

// Protected code
CodeBlock protectedCode = new CodeBlock() {
public Operand run(Object[] args) {
return buildGetDefinition((Node)args[0], (IRScope)args[1]);
}
};

// Ensure code
CodeBlock ensureCode = new CodeBlock() {
public Operand run(Object[] args) {
IRScope m = (IRScope)args[0];
m.addInstr(new SetWithinDefinedInstr(manager.getFalse()));
return manager.getNil();
}
};

return protectCodeWithEnsure(s, protectedCode, new Object[] {node, s}, ensureCode, new Object[] {s});
return buildGetDefinition(node, s);
}

protected Operand buildVersionSpecificGetDefinitionIR(Node node, IRScope s) {
Expand Down
2 changes: 0 additions & 2 deletions src/org/jruby/ir/IRVisitor.java
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
import org.jruby.ir.instructions.defined.MethodDefinedInstr;
import org.jruby.ir.instructions.defined.MethodIsPublicInstr;
import org.jruby.ir.instructions.defined.RestoreErrorInfoInstr;
import org.jruby.ir.instructions.defined.SetWithinDefinedInstr;
import org.jruby.ir.instructions.defined.SuperMethodBoundInstr;
import org.jruby.ir.instructions.ruby18.ReceiveOptArgInstr18;
import org.jruby.ir.instructions.ruby18.ReceiveRestArgInstr18;
Expand Down Expand Up @@ -168,7 +167,6 @@ private void error(Object object) {
public void MethodDefinedInstr(MethodDefinedInstr methoddefinedinstr) { error(methoddefinedinstr); }
public void MethodIsPublicInstr(MethodIsPublicInstr methodispublicinstr) { error(methodispublicinstr); }
public void RestoreErrorInfoInstr(RestoreErrorInfoInstr restoreerrorinfoinstr) { error(restoreerrorinfoinstr); }
public void SetWithinDefinedInstr(SetWithinDefinedInstr setwithindefinedinstr) { error(setwithindefinedinstr); }
public void SuperMethodBoundInstr(SuperMethodBoundInstr supermethodboundinstr) { error(supermethodboundinstr); }

// ruby 1.8 specific
Expand Down
55 changes: 0 additions & 55 deletions src/org/jruby/ir/instructions/defined/SetWithinDefinedInstr.java

This file was deleted.

6 changes: 0 additions & 6 deletions src/org/jruby/ir/targets/JVMVisitor.java
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@
import org.jruby.ir.instructions.defined.MethodDefinedInstr;
import org.jruby.ir.instructions.defined.MethodIsPublicInstr;
import org.jruby.ir.instructions.defined.RestoreErrorInfoInstr;
import org.jruby.ir.instructions.defined.SetWithinDefinedInstr;
import org.jruby.ir.instructions.defined.SuperMethodBoundInstr;
import org.jruby.ir.instructions.ruby18.ReceiveOptArgInstr18;
import org.jruby.ir.instructions.ruby18.ReceiveRestArgInstr18;
Expand Down Expand Up @@ -1025,11 +1024,6 @@ public void RestoreErrorInfoInstr(RestoreErrorInfoInstr restoreerrorinfoinstr) {
super.RestoreErrorInfoInstr(restoreerrorinfoinstr); //To change body of overridden methods use File | Settings | File Templates.
}

@Override
public void SetWithinDefinedInstr(SetWithinDefinedInstr setwithindefinedinstr) {
super.SetWithinDefinedInstr(setwithindefinedinstr); //To change body of overridden methods use File | Settings | File Templates.
}

@Override
public void SuperMethodBoundInstr(SuperMethodBoundInstr supermethodboundinstr) {
super.SuperMethodBoundInstr(supermethodboundinstr); //To change body of overridden methods use File | Settings | File Templates.
Expand Down
21 changes: 0 additions & 21 deletions src/org/jruby/runtime/ThreadContext.java
Original file line number Diff line number Diff line change
Expand Up @@ -89,9 +89,6 @@ public static ThreadContext newContext(Ruby runtime) {
// Is this thread currently with in a function trace?
private boolean isWithinTrace;

// Is this thread currently doing an defined? defined should set things like $!
private boolean isWithinDefined;

private RubyThread thread;
private Fiber fiber;
// Cache format string because it is expensive to create on demand
Expand Down Expand Up @@ -1182,24 +1179,6 @@ public void setWithinTrace(boolean isWithinTrace) {
this.isWithinTrace = isWithinTrace;
}

/**
* Is this thread actively in defined? at the moment.
*
* @return true if within defined?
*/
public boolean isWithinDefined() {
return isWithinDefined;
}

/**
* Set whether we are actively within defined? or not.
*
* @param isWithinDefined true if so
*/
public void setWithinDefined(boolean isWithinDefined) {
this.isWithinDefined = isWithinDefined;
}

/**
* Return a binding representing the current call's state
* @return the current binding
Expand Down

0 comments on commit 7c3f642

Please sign in to comment.