Bug #1797

trace instruction not generated by compiler

Added by mark-moseley (Mark Moseley) almost 3 years ago. Updated about 1 year ago.

[ruby-core:24463]
Status:Closed Start date:07/21/2009
Priority:Normal Due date:
Assignee:- % Done:

100%

Category:-
Target version:-
ruby -v:1.9.1

Description

With the following code:

1: def foo
2:   a = 5
3:   return a
4: end

No trace instruction is generated for line 3.

If line 3 is just "a", without the return, then it functions as expected.

Associated revisions

Revision 25537
Added by yugui over 2 years ago

merges r24579 and r24581 from trunk into ruby_1_9_1. -- * compile.c (NODE_RETURN): fire return event at explicit return. [ruby-dev:38701] -- * test/ruby/test_settracefunc.rb (test_return, test_return2): add two tests for [ruby-dev:38701] and [ruby-core:24463]. -- * parse.y (reduce_nodes_gen): preserve NODE_FL_NEWLINE flag during node reducing. [ruby-core:24463]

History

Updated by mark-moseley (Mark Moseley) almost 3 years ago

Here's another one:

1: i = 0
2: 1.upto(2) {
3: i += 1
4: }

No trace instruction is generated for line 3. Add more statements in the {}, and it works fine.

Updated by mark-moseley (Mark Moseley) almost 3 years ago

Re: Jacky Cheung

> in the ruby  can doesn't has line 3
> your code can wirite like this:
> def foo
>   a = 5
> end
>
> that's ok!

Jacky, I understand that functionally it is the same. Look at this as an issue of release vs. debug builds. If Ruby is going to generate trace messages, then I would think that those messages would correspond to all lines of valid code, and not omit the ones that were optimized away.

And I also don't understand why the trace message is generated when line 3 is just "a", instead of "return a". Clearly, something is wrong; there should be no difference between the two.

Updated by mame (Yusuke Endoh) almost 3 years ago

Hi,

2009/7/21 Mark Moseley <redmine@ruby-lang.org>:
> With the following code:
>
> 1: def foo
> 2:   a = 5
> 3:   return a
> 4: end
>
> No trace instruction is generated for line 3.
>
> If line 3 is just "a", without the return, then it functions as expected.


I think it is a parser's fault, not compiler's.
reduce_nodes (in parse.y) may incorrectly eliminate some nodes marked as
NODE_FL_NEWLINE.
The following patch preserves and propagates the mark.


Index: parse.y
===================================================================
--- parse.y	(revision 24580)
+++ parse.y	(working copy)
@@ -8406,6 +8406,7 @@
      (reduce_nodes(&node->n1), body = &node->n2, 1))

     while (node) {
+	int newline = node->flags & NODE_FL_NEWLINE;
 	switch (nd_type(node)) {
 	  end:
 	  case NODE_NIL:
@@ -8413,9 +8414,11 @@
 	    return;
 	  case NODE_RETURN:
 	    *body = node = node->nd_stts;
+	    if (newline && node) node->flags |= NODE_FL_NEWLINE;
 	    continue;
 	  case NODE_BEGIN:
 	    *body = node = node->nd_body;
+	    if (newline && node) node->flags |= NODE_FL_NEWLINE;
 	    continue;
 	  case NODE_BLOCK:
 	    body = &node->nd_end->nd_head;
@@ -8439,6 +8442,7 @@
 	    return;
 	}
 	node = *body;
+	if (newline && node) node->flags |= NODE_FL_NEWLINE;
     }

 #undef subnodes


I did not completely understand your patch (http://gist.github.com/166330).
`last_line_traced' seems to be a workaround against the above bug, then
to be no longer needed.
But I couldn't understand why change in `compile_massign_lhs' was needed.

Is my above patch enough for you?  If not, could you elaborate the
situation you are facing?

-- 
Yusuke ENDOH <mame@tsg.ne.jp>

Updated by mark-moseley (Mark Moseley) almost 3 years ago

I agree that it would be better to stay within the current NODE_FL_NEWLINE solution and use this patch. I was trying to come up with a solution that also fixed bug #1676, but that appears to be closed now. I'll apply the patches for both and verify the ruby-debug test cases.

Updated by matz (Yukihiro Matsumoto) almost 3 years ago

Hi,

In message "Re: [ruby-core:24969] Re: [Bug #1797] trace instruction not generated by 	compiler"
    on Wed, 19 Aug 2009 02:41:49 +0900, Yusuke ENDOH <mame@tsg.ne.jp> writes:

|I think it is a parser's fault, not compiler's.
|reduce_nodes (in parse.y) may incorrectly eliminate some nodes marked as
|NODE_FL_NEWLINE.
|The following patch preserves and propagates the mark.

Could you check in?

							matz.

Updated by mark-moseley (Mark Moseley) almost 3 years ago

I've verified that both of my test cases listed above now pass.

However, #1676 still fails. My patch fixes it, although I understand if you want to take a different approach.

And to answer the question about the change that my patch makes to compile_massign_lhs(), that's due to this kind of code:

x = [ 1, 2, 3, 4, 5, 6 ]
a, b, c,
d, e, f = x

Currently compile_massign_lhs() removes a "putnil" inserted prematurely due to the statement spread across two lines. I had to modify it to also remove a "trace" put there as well.

Updated by yugui (Yuki Sonoda) over 2 years ago

  • Status changed from Open to Closed
  • % Done changed from 0 to 100
This issue was solved with changeset r25537.
Mark, thank you for reporting this issue.
Your contribution to Ruby is greatly appreciated.
May Ruby be with you.

Also available in: Atom PDF