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).
jruby · Dec 22, 2017 · 7c60d07 · 7c60d07
1 parent f30ef2e
commit 7c60d07
Showing 7 changed files with 48 additions and 28 deletions.
diff --git a/core/src/main/java/org/jruby/ast/ClassNode.java b/core/src/main/java/org/jruby/ast/ClassNode.java
@@ -45,18 +45,19 @@ public class ClassNode extends Node implements IScopingNode {
     private final StaticScope scope;
     private final Node bodyNode;
     private final Node superNode;
+    private final int endLine;
 
-    public ClassNode(ISourcePosition position, Colon3Node cpath, StaticScope scope, Node bodyNode, Node superNode) {
+    public ClassNode(ISourcePosition position, Colon3Node cpath, StaticScope scope, Node bodyNode, Node superNode, int endLine) {
         super(position, cpath.containsVariableAssignment() || bodyNode.containsVariableAssignment() || superNode != null && superNode.containsVariableAssignment());
-
-        assert cpath != null : "cpath is not null";
+
         assert scope != null : "scope is not null";
         assert bodyNode != null : "bodyNode is not null";
 
         this.cpath = cpath;
         this.scope = scope;
         this.bodyNode = bodyNode;
         this.superNode = superNode;
+        this.endLine = endLine;
     }
 
     public NodeType getNodeType() {
@@ -79,6 +80,13 @@ public <T> T accept(NodeVisitor<T> iVisitor) {
     public Node getBodyNode() {
         return bodyNode;
     }
+
+    /**
+     * Gets line where the 'end' was for this module.
+     */
+    public int getEndLine() {
+        return endLine;
+    }
 
     /**
      * Get the static scoping information.

diff --git a/core/src/main/java/org/jruby/ast/ModuleNode.java b/core/src/main/java/org/jruby/ast/ModuleNode.java
@@ -44,17 +44,18 @@ public class ModuleNode extends Node implements IScopingNode {
     private final Colon3Node cpath;
     private final StaticScope scope;
     private final Node bodyNode;
+    private final int endLine;
 
-    public ModuleNode(ISourcePosition position, Colon3Node cpath, StaticScope scope, Node bodyNode) {
+    public ModuleNode(ISourcePosition position, Colon3Node cpath, StaticScope scope, Node bodyNode, int endLine) {
         super(position, cpath.containsVariableAssignment() || bodyNode.containsVariableAssignment());
 
-        assert cpath != null : "cpath is not null";
         assert scope != null : "scope is not null";
         assert bodyNode != null : "bodyNode is not null";
 
         this.cpath = cpath;
         this.scope = scope;
         this.bodyNode = bodyNode;
+        this.endLine = endLine;
     }
 
     public NodeType getNodeType() {
@@ -77,7 +78,14 @@ public <T> T accept(NodeVisitor<T> iVisitor) {
     public Node getBodyNode() {
         return bodyNode;
     }
-
+
+    /**
+     * Gets line where the 'end' was for this module.
+     */
+    public int getEndLine() {
+        return endLine;
+    }
+
     /**
      * Get the static scoping information.
      * 

diff --git a/core/src/main/java/org/jruby/ast/SClassNode.java b/core/src/main/java/org/jruby/ast/SClassNode.java
@@ -50,16 +50,17 @@ public class SClassNode extends Node {
     private final Node receiverNode;
     private final StaticScope scope;
     private final Node bodyNode;
+    private final int endLine;
 
-    public SClassNode(ISourcePosition position, Node recvNode, StaticScope scope, Node bodyNode) {
+    public SClassNode(ISourcePosition position, Node recvNode, StaticScope scope, Node bodyNode, int endLine) {
         super(position, recvNode.containsVariableAssignment() || bodyNode.containsVariableAssignment());
 
         assert scope != null : "scope is not null";
-        assert recvNode != null : "receiverNode is not null";
 
         this.receiverNode = recvNode;
         this.scope = scope;
         this.bodyNode = bodyNode;
+        this.endLine = endLine;
     }
 
     public NodeType getNodeType() {
@@ -82,6 +83,13 @@ public <T> T accept(NodeVisitor<T> iVisitor) {
     public Node getBodyNode() {
         return bodyNode;
     }
+
+    /**
+     * Gets line where the 'end' was for this module.
+     */
+    public int getEndLine() {
+        return endLine;
+    }
 
     /**
      * Gets the scope of this class

diff --git a/core/src/main/java/org/jruby/ir/IRBuilder.java b/core/src/main/java/org/jruby/ir/IRBuilder.java
@@ -1375,7 +1375,7 @@ public Operand buildClass(ClassNode classNode) {
         Variable classVar = addResultInstr(new DefineClassInstr(createTemporaryVariable(), body, container, superClass));
 
         Variable processBodyResult = addResultInstr(new ProcessModuleBodyInstr(createTemporaryVariable(), classVar, NullBlock.INSTANCE));
-        newIRBuilder(manager, body).buildModuleOrClassBody(classNode.getBodyNode(), classNode.getLine());
+        newIRBuilder(manager, body).buildModuleOrClassBody(classNode.getBodyNode(), classNode.getLine(), classNode.getEndLine());
         return processBodyResult;
     }
 
@@ -1389,7 +1389,7 @@ public Operand buildSClass(SClassNode sclassNode) {
 
         // sclass bodies inherit the block of their containing method
         Variable processBodyResult = addResultInstr(new ProcessModuleBodyInstr(createTemporaryVariable(), sClassVar, scope.getYieldClosureVariable()));
-        newIRBuilder(manager, body).buildModuleOrClassBody(sclassNode.getBodyNode(), sclassNode.getLine());
+        newIRBuilder(manager, body).buildModuleOrClassBody(sclassNode.getBodyNode(), sclassNode.getLine(), sclassNode.getEndLine());
         return processBodyResult;
     }
 
@@ -3164,7 +3164,7 @@ public Operand buildModule(ModuleNode moduleNode) {
         Variable moduleVar = addResultInstr(new DefineModuleInstr(createTemporaryVariable(), body, container));
 
         Variable processBodyResult = addResultInstr(new ProcessModuleBodyInstr(createTemporaryVariable(), moduleVar, NullBlock.INSTANCE));
-        newIRBuilder(manager, body).buildModuleOrClassBody(moduleNode.getBodyNode(), moduleNode.getLine());
+        newIRBuilder(manager, body).buildModuleOrClassBody(moduleNode.getBodyNode(), moduleNode.getLine(), moduleNode.getEndLine());
         return processBodyResult;
     }
 
@@ -4077,9 +4077,9 @@ private Operand[] adjustVariableDepth(Operand[] args, int depthFromSuper) {
         return newArgs;
     }
 
-    private InterpreterContext buildModuleOrClassBody(Node bodyNode, int linenumber) {
+    private InterpreterContext buildModuleOrClassBody(Node bodyNode, int startLine, int endLine) {
         if (RubyInstanceConfig.FULL_TRACE_ENABLED) {
-            addInstr(new TraceInstr(RubyEvent.CLASS, null, getFileName(), linenumber));
+            addInstr(new TraceInstr(RubyEvent.CLASS, null, getFileName(), startLine));
         }
 
         prepareImplicitState();                                    // recv_self, add frame block, etc)
@@ -4088,11 +4088,10 @@ private InterpreterContext buildModuleOrClassBody(Node bodyNode, int linenumber)
         Operand bodyReturnValue = build(bodyNode);
 
         if (RubyInstanceConfig.FULL_TRACE_ENABLED) {
-            // FIXME: We also should add a line number instruction so that backtraces
-            // inside the trace function get the correct line. Unfortunately, we don't
-            // have one here and we can't do it dynamically like TraceInstr does.
-            // See https://github.com/jruby/jruby/issues/4051
-            addInstr(new TraceInstr(RubyEvent.END, null, getFileName(), -1));
+            // This is only added when tracing is enabled because an 'end' will normally have no other instrs which can
+            // raise after this point.  When we add trace we need to add one so backtrace generated shows the 'end' line.
+            addInstr(manager.newLineNumber(endLine));
+            addInstr(new TraceInstr(RubyEvent.END, null, getFileName(), endLine));
         }
 
         addInstr(new ReturnInstr(bodyReturnValue));

diff --git a/core/src/main/java/org/jruby/ir/instructions/TraceInstr.java b/core/src/main/java/org/jruby/ir/instructions/TraceInstr.java
@@ -72,10 +72,7 @@ public static TraceInstr decode(IRReaderDecoder d) {
     @Override
     public Object interpret(ThreadContext context, StaticScope currScope, DynamicScope currDynScope, IRubyObject self, Object[] temp) {
         if (context.runtime.hasEventHooks()) {
-            // FIXME: Try and statically generate END linenumber instead of hacking it.
-            int linenumber = getLinenumber() == -1 ? context.getLine()+1 : getLinenumber();
-
-            context.trace(getEvent(), getName(), context.getFrameKlazz(), getFilename(), linenumber);
+            context.trace(getEvent(), getName(), context.getFrameKlazz(), getFilename(), getLinenumber());
         }
 
         return null;

diff --git a/core/src/main/java/org/jruby/parser/RubyParser.java b/core/src/main/java/org/jruby/parser/RubyParser.java
@@ -3830,7 +3830,7 @@ public Object yyparse (RubyLexer yyLex) throws java.io.IOException {
   @Override public Object execute(ParserSupport support, RubyLexer lexer, Object yyVal, Object[] yyVals, int yyTop) {
                     Node body = support.makeNullNil(((Node)yyVals[-1+yyTop]));
 
-                    yyVal = new ClassNode(((ISourcePosition)yyVals[-5+yyTop]), ((Colon3Node)yyVals[-4+yyTop]), support.getCurrentScope(), body, ((Node)yyVals[-3+yyTop]));
+                    yyVal = new ClassNode(((ISourcePosition)yyVals[-5+yyTop]), ((Colon3Node)yyVals[-4+yyTop]), support.getCurrentScope(), body, ((Node)yyVals[-3+yyTop]), lexer.getRubySourceline());
                     support.popCurrentScope();
     return yyVal;
   }
@@ -3854,7 +3854,7 @@ public Object yyparse (RubyLexer yyLex) throws java.io.IOException {
   @Override public Object execute(ParserSupport support, RubyLexer lexer, Object yyVal, Object[] yyVals, int yyTop) {
                     Node body = support.makeNullNil(((Node)yyVals[-1+yyTop]));
 
-                    yyVal = new SClassNode(((ISourcePosition)yyVals[-7+yyTop]), ((Node)yyVals[-5+yyTop]), support.getCurrentScope(), body);
+                    yyVal = new SClassNode(((ISourcePosition)yyVals[-7+yyTop]), ((Node)yyVals[-5+yyTop]), support.getCurrentScope(), body, lexer.getRubySourceline());
                     support.popCurrentScope();
                     support.setInDef(((Boolean)yyVals[-4+yyTop]).booleanValue());
                     support.setInSingle(((Integer)yyVals[-2+yyTop]).intValue());
@@ -3874,7 +3874,7 @@ public Object yyparse (RubyLexer yyLex) throws java.io.IOException {
   @Override public Object execute(ParserSupport support, RubyLexer lexer, Object yyVal, Object[] yyVals, int yyTop) {
                     Node body = support.makeNullNil(((Node)yyVals[-1+yyTop]));
 
-                    yyVal = new ModuleNode(((ISourcePosition)yyVals[-4+yyTop]), ((Colon3Node)yyVals[-3+yyTop]), support.getCurrentScope(), body);
+                    yyVal = new ModuleNode(((ISourcePosition)yyVals[-4+yyTop]), ((Colon3Node)yyVals[-3+yyTop]), support.getCurrentScope(), body, lexer.getRubySourceline());
                     support.popCurrentScope();
     return yyVal;
   }

diff --git a/core/src/main/java/org/jruby/parser/RubyParser.y b/core/src/main/java/org/jruby/parser/RubyParser.y
@@ -1575,7 +1575,7 @@ primary         : literal
                 } bodystmt keyword_end {
                     Node body = support.makeNullNil($5);
 
-                    $$ = new ClassNode($1, $<Colon3Node>2, support.getCurrentScope(), body, $3);
+                    $$ = new ClassNode($1, $<Colon3Node>2, support.getCurrentScope(), body, $3, lexer.getRubySourceline());
                     support.popCurrentScope();
                 }
                 | keyword_class tLSHFT expr {
@@ -1588,7 +1588,7 @@ primary         : literal
                 } bodystmt keyword_end {
                     Node body = support.makeNullNil($7);
 
-                    $$ = new SClassNode($1, $3, support.getCurrentScope(), body);
+                    $$ = new SClassNode($1, $3, support.getCurrentScope(), body, lexer.getRubySourceline());
                     support.popCurrentScope();
                     support.setInDef($<Boolean>4.booleanValue());
                     support.setInSingle($<Integer>6.intValue());
@@ -1601,7 +1601,7 @@ primary         : literal
                 } bodystmt keyword_end {
                     Node body = support.makeNullNil($4);
 
-                    $$ = new ModuleNode($1, $<Colon3Node>2, support.getCurrentScope(), body);
+                    $$ = new ModuleNode($1, $<Colon3Node>2, support.getCurrentScope(), body, lexer.getRubySourceline());
                     support.popCurrentScope();
                 }
                 | keyword_def fname {