Skip to content

Commit 7c60d07

Browse files
eneboenebo
committedDec 22, 2017
Fixes #4060. Wrong line numbers inside set_trace_func for end events.
Fixes #4051. Wrong line numbers in Kernel#caller inside set_trace_func (with code to reproduce) #4051 was largely already fixed but the backtrace elements themselves had the wrong line (which was why #4060 was opened). The problem was mri changes their position of class/module/sclass from start line to end line. We just store start line. They will also look at first instr/node within the class/module for start line. That solution is memory efficient but then makes those nodes report wrong lines with -S ast. Our solution was to just add an endLine field to those nodes. The memory increase from that is very small so I think it is ok. The second part of this was that we build backtrace from setting line number via a line number instr. We had no end line so we couldn't emit one. Now that we have one we do....but only if --debug is supplied since the only instr after this point is return and that cannot raise (so we cannot generate a backtrace).
·
10.0.0.09.2.0.0
1 parent f30ef2e commit 7c60d07

File tree

7 files changed

+48
-28
lines changed

7 files changed

+48
-28
lines changed
 

‎core/src/main/java/org/jruby/ast/ClassNode.java

+11-3
Original file line numberDiff line numberDiff line change
@@ -45,18 +45,19 @@ public class ClassNode extends Node implements IScopingNode {
4545
private final StaticScope scope;
4646
private final Node bodyNode;
4747
private final Node superNode;
48+
private final int endLine;
4849

49-
public ClassNode(ISourcePosition position, Colon3Node cpath, StaticScope scope, Node bodyNode, Node superNode) {
50+
public ClassNode(ISourcePosition position, Colon3Node cpath, StaticScope scope, Node bodyNode, Node superNode, int endLine) {
5051
super(position, cpath.containsVariableAssignment() || bodyNode.containsVariableAssignment() || superNode != null && superNode.containsVariableAssignment());
51-
52-
assert cpath != null : "cpath is not null";
52+
5353
assert scope != null : "scope is not null";
5454
assert bodyNode != null : "bodyNode is not null";
5555

5656
this.cpath = cpath;
5757
this.scope = scope;
5858
this.bodyNode = bodyNode;
5959
this.superNode = superNode;
60+
this.endLine = endLine;
6061
}
6162

6263
public NodeType getNodeType() {
@@ -79,6 +80,13 @@ public <T> T accept(NodeVisitor<T> iVisitor) {
7980
public Node getBodyNode() {
8081
return bodyNode;
8182
}
83+
84+
/**
85+
* Gets line where the 'end' was for this module.
86+
*/
87+
public int getEndLine() {
88+
return endLine;
89+
}
8290

8391
/**
8492
* Get the static scoping information.

‎core/src/main/java/org/jruby/ast/ModuleNode.java

+11-3
Original file line numberDiff line numberDiff line change
@@ -44,17 +44,18 @@ public class ModuleNode extends Node implements IScopingNode {
4444
private final Colon3Node cpath;
4545
private final StaticScope scope;
4646
private final Node bodyNode;
47+
private final int endLine;
4748

48-
public ModuleNode(ISourcePosition position, Colon3Node cpath, StaticScope scope, Node bodyNode) {
49+
public ModuleNode(ISourcePosition position, Colon3Node cpath, StaticScope scope, Node bodyNode, int endLine) {
4950
super(position, cpath.containsVariableAssignment() || bodyNode.containsVariableAssignment());
5051

51-
assert cpath != null : "cpath is not null";
5252
assert scope != null : "scope is not null";
5353
assert bodyNode != null : "bodyNode is not null";
5454

5555
this.cpath = cpath;
5656
this.scope = scope;
5757
this.bodyNode = bodyNode;
58+
this.endLine = endLine;
5859
}
5960

6061
public NodeType getNodeType() {
@@ -77,7 +78,14 @@ public <T> T accept(NodeVisitor<T> iVisitor) {
7778
public Node getBodyNode() {
7879
return bodyNode;
7980
}
80-
81+
82+
/**
83+
* Gets line where the 'end' was for this module.
84+
*/
85+
public int getEndLine() {
86+
return endLine;
87+
}
88+
8189
/**
8290
* Get the static scoping information.
8391
*

‎core/src/main/java/org/jruby/ast/SClassNode.java

+10-2
Original file line numberDiff line numberDiff line change
@@ -50,16 +50,17 @@ public class SClassNode extends Node {
5050
private final Node receiverNode;
5151
private final StaticScope scope;
5252
private final Node bodyNode;
53+
private final int endLine;
5354

54-
public SClassNode(ISourcePosition position, Node recvNode, StaticScope scope, Node bodyNode) {
55+
public SClassNode(ISourcePosition position, Node recvNode, StaticScope scope, Node bodyNode, int endLine) {
5556
super(position, recvNode.containsVariableAssignment() || bodyNode.containsVariableAssignment());
5657

5758
assert scope != null : "scope is not null";
58-
assert recvNode != null : "receiverNode is not null";
5959

6060
this.receiverNode = recvNode;
6161
this.scope = scope;
6262
this.bodyNode = bodyNode;
63+
this.endLine = endLine;
6364
}
6465

6566
public NodeType getNodeType() {
@@ -82,6 +83,13 @@ public <T> T accept(NodeVisitor<T> iVisitor) {
8283
public Node getBodyNode() {
8384
return bodyNode;
8485
}
86+
87+
/**
88+
* Gets line where the 'end' was for this module.
89+
*/
90+
public int getEndLine() {
91+
return endLine;
92+
}
8593

8694
/**
8795
* Gets the scope of this class

‎core/src/main/java/org/jruby/ir/IRBuilder.java

+9-10
Original file line numberDiff line numberDiff line change
@@ -1375,7 +1375,7 @@ public Operand buildClass(ClassNode classNode) {
13751375
Variable classVar = addResultInstr(new DefineClassInstr(createTemporaryVariable(), body, container, superClass));
13761376

13771377
Variable processBodyResult = addResultInstr(new ProcessModuleBodyInstr(createTemporaryVariable(), classVar, NullBlock.INSTANCE));
1378-
newIRBuilder(manager, body).buildModuleOrClassBody(classNode.getBodyNode(), classNode.getLine());
1378+
newIRBuilder(manager, body).buildModuleOrClassBody(classNode.getBodyNode(), classNode.getLine(), classNode.getEndLine());
13791379
return processBodyResult;
13801380
}
13811381

@@ -1389,7 +1389,7 @@ public Operand buildSClass(SClassNode sclassNode) {
13891389

13901390
// sclass bodies inherit the block of their containing method
13911391
Variable processBodyResult = addResultInstr(new ProcessModuleBodyInstr(createTemporaryVariable(), sClassVar, scope.getYieldClosureVariable()));
1392-
newIRBuilder(manager, body).buildModuleOrClassBody(sclassNode.getBodyNode(), sclassNode.getLine());
1392+
newIRBuilder(manager, body).buildModuleOrClassBody(sclassNode.getBodyNode(), sclassNode.getLine(), sclassNode.getEndLine());
13931393
return processBodyResult;
13941394
}
13951395

@@ -3164,7 +3164,7 @@ public Operand buildModule(ModuleNode moduleNode) {
31643164
Variable moduleVar = addResultInstr(new DefineModuleInstr(createTemporaryVariable(), body, container));
31653165

31663166
Variable processBodyResult = addResultInstr(new ProcessModuleBodyInstr(createTemporaryVariable(), moduleVar, NullBlock.INSTANCE));
3167-
newIRBuilder(manager, body).buildModuleOrClassBody(moduleNode.getBodyNode(), moduleNode.getLine());
3167+
newIRBuilder(manager, body).buildModuleOrClassBody(moduleNode.getBodyNode(), moduleNode.getLine(), moduleNode.getEndLine());
31683168
return processBodyResult;
31693169
}
31703170

@@ -4077,9 +4077,9 @@ private Operand[] adjustVariableDepth(Operand[] args, int depthFromSuper) {
40774077
return newArgs;
40784078
}
40794079

4080-
private InterpreterContext buildModuleOrClassBody(Node bodyNode, int linenumber) {
4080+
private InterpreterContext buildModuleOrClassBody(Node bodyNode, int startLine, int endLine) {
40814081
if (RubyInstanceConfig.FULL_TRACE_ENABLED) {
4082-
addInstr(new TraceInstr(RubyEvent.CLASS, null, getFileName(), linenumber));
4082+
addInstr(new TraceInstr(RubyEvent.CLASS, null, getFileName(), startLine));
40834083
}
40844084

40854085
prepareImplicitState(); // recv_self, add frame block, etc)
@@ -4088,11 +4088,10 @@ private InterpreterContext buildModuleOrClassBody(Node bodyNode, int linenumber)
40884088
Operand bodyReturnValue = build(bodyNode);
40894089

40904090
if (RubyInstanceConfig.FULL_TRACE_ENABLED) {
4091-
// FIXME: We also should add a line number instruction so that backtraces
4092-
// inside the trace function get the correct line. Unfortunately, we don't
4093-
// have one here and we can't do it dynamically like TraceInstr does.
4094-
// See https://github.com/jruby/jruby/issues/4051
4095-
addInstr(new TraceInstr(RubyEvent.END, null, getFileName(), -1));
4091+
// This is only added when tracing is enabled because an 'end' will normally have no other instrs which can
4092+
// raise after this point. When we add trace we need to add one so backtrace generated shows the 'end' line.
4093+
addInstr(manager.newLineNumber(endLine));
4094+
addInstr(new TraceInstr(RubyEvent.END, null, getFileName(), endLine));
40964095
}
40974096

40984097
addInstr(new ReturnInstr(bodyReturnValue));

‎core/src/main/java/org/jruby/ir/instructions/TraceInstr.java

+1-4
Original file line numberDiff line numberDiff line change
@@ -72,10 +72,7 @@ public static TraceInstr decode(IRReaderDecoder d) {
7272
@Override
7373
public Object interpret(ThreadContext context, StaticScope currScope, DynamicScope currDynScope, IRubyObject self, Object[] temp) {
7474
if (context.runtime.hasEventHooks()) {
75-
// FIXME: Try and statically generate END linenumber instead of hacking it.
76-
int linenumber = getLinenumber() == -1 ? context.getLine()+1 : getLinenumber();
77-
78-
context.trace(getEvent(), getName(), context.getFrameKlazz(), getFilename(), linenumber);
75+
context.trace(getEvent(), getName(), context.getFrameKlazz(), getFilename(), getLinenumber());
7976
}
8077

8178
return null;

‎core/src/main/java/org/jruby/parser/RubyParser.java

+3-3
Original file line numberDiff line numberDiff line change
@@ -3830,7 +3830,7 @@ public Object yyparse (RubyLexer yyLex) throws java.io.IOException {
38303830
@Override public Object execute(ParserSupport support, RubyLexer lexer, Object yyVal, Object[] yyVals, int yyTop) {
38313831
Node body = support.makeNullNil(((Node)yyVals[-1+yyTop]));
38323832

3833-
yyVal = new ClassNode(((ISourcePosition)yyVals[-5+yyTop]), ((Colon3Node)yyVals[-4+yyTop]), support.getCurrentScope(), body, ((Node)yyVals[-3+yyTop]));
3833+
yyVal = new ClassNode(((ISourcePosition)yyVals[-5+yyTop]), ((Colon3Node)yyVals[-4+yyTop]), support.getCurrentScope(), body, ((Node)yyVals[-3+yyTop]), lexer.getRubySourceline());
38343834
support.popCurrentScope();
38353835
return yyVal;
38363836
}
@@ -3854,7 +3854,7 @@ public Object yyparse (RubyLexer yyLex) throws java.io.IOException {
38543854
@Override public Object execute(ParserSupport support, RubyLexer lexer, Object yyVal, Object[] yyVals, int yyTop) {
38553855
Node body = support.makeNullNil(((Node)yyVals[-1+yyTop]));
38563856

3857-
yyVal = new SClassNode(((ISourcePosition)yyVals[-7+yyTop]), ((Node)yyVals[-5+yyTop]), support.getCurrentScope(), body);
3857+
yyVal = new SClassNode(((ISourcePosition)yyVals[-7+yyTop]), ((Node)yyVals[-5+yyTop]), support.getCurrentScope(), body, lexer.getRubySourceline());
38583858
support.popCurrentScope();
38593859
support.setInDef(((Boolean)yyVals[-4+yyTop]).booleanValue());
38603860
support.setInSingle(((Integer)yyVals[-2+yyTop]).intValue());
@@ -3874,7 +3874,7 @@ public Object yyparse (RubyLexer yyLex) throws java.io.IOException {
38743874
@Override public Object execute(ParserSupport support, RubyLexer lexer, Object yyVal, Object[] yyVals, int yyTop) {
38753875
Node body = support.makeNullNil(((Node)yyVals[-1+yyTop]));
38763876

3877-
yyVal = new ModuleNode(((ISourcePosition)yyVals[-4+yyTop]), ((Colon3Node)yyVals[-3+yyTop]), support.getCurrentScope(), body);
3877+
yyVal = new ModuleNode(((ISourcePosition)yyVals[-4+yyTop]), ((Colon3Node)yyVals[-3+yyTop]), support.getCurrentScope(), body, lexer.getRubySourceline());
38783878
support.popCurrentScope();
38793879
return yyVal;
38803880
}

‎core/src/main/java/org/jruby/parser/RubyParser.y

+3-3
Original file line numberDiff line numberDiff line change
@@ -1575,7 +1575,7 @@ primary : literal
15751575
} bodystmt keyword_end {
15761576
Node body = support.makeNullNil($5);
15771577

1578-
$$ = new ClassNode($1, $<Colon3Node>2, support.getCurrentScope(), body, $3);
1578+
$$ = new ClassNode($1, $<Colon3Node>2, support.getCurrentScope(), body, $3, lexer.getRubySourceline());
15791579
support.popCurrentScope();
15801580
}
15811581
| keyword_class tLSHFT expr {
@@ -1588,7 +1588,7 @@ primary : literal
15881588
} bodystmt keyword_end {
15891589
Node body = support.makeNullNil($7);
15901590

1591-
$$ = new SClassNode($1, $3, support.getCurrentScope(), body);
1591+
$$ = new SClassNode($1, $3, support.getCurrentScope(), body, lexer.getRubySourceline());
15921592
support.popCurrentScope();
15931593
support.setInDef($<Boolean>4.booleanValue());
15941594
support.setInSingle($<Integer>6.intValue());
@@ -1601,7 +1601,7 @@ primary : literal
16011601
} bodystmt keyword_end {
16021602
Node body = support.makeNullNil($4);
16031603

1604-
$$ = new ModuleNode($1, $<Colon3Node>2, support.getCurrentScope(), body);
1604+
$$ = new ModuleNode($1, $<Colon3Node>2, support.getCurrentScope(), body, lexer.getRubySourceline());
16051605
support.popCurrentScope();
16061606
}
16071607
| keyword_def fname {

0 commit comments

Comments
 (0)
Please sign in to comment.