344501 - Can't step into/put breakpoint in functions that are tail called in the debugger

Status: RESOLVED FIXED

(Rhino Graveyard :: Core, defect)

Description

[Gil Tayar]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.4) Gecko/20060508 Firefox/1.5.0.4
Build Identifier: 1.6R2

When running a js program using the debugger, and putting a breakpoint or trying to step into a function which is called by another function, the source line in which the debugger _thinks_ it is in is stuck in the call to the nested function.

I have downloaded 1.6R2 and 1.6R1 and it happens on both of them. It
does _not_ happen on 1.5R5. 

Reproducible: Always

Steps to Reproduce:
Given this simple program (in foo.js):
function foo (page)
{
  var x = 1;
  ++x;
  ++x;
  ++x;
  return true;

}

  function zoo(page)
  {
    var z = 1;
    z++;
    z++;
    return foo(page);
  }

  zoo('ssss');

I run the Rhino debugger using:

java -cp js.jar org.mozilla.javascript.tools.debugger.Main foo.js

Now I click on step into repeatedly. The first one enters zoo, which is
good. Once I get to the

return foo(page);

line and continue clicking on step into, then the debugger does the
step, but the current line stays on the "return foo..." line. The same thing happens if I try and put a breakpoint in the function foo and click "Go" - it gets stuck on the "return foo..." line.

Actual Results:  
The debugger does the
step, but the current line stays on the "return foo..." line. The same thing happens if I try and put a breakpoint in the function foo and click "Go" - it gets stuck on the "return foo..." line.

Expected Results:  
The debugger should step into the function, or if a breakpoint is set, should stand on that line.

Comment 1

[James Ross]

What does this have to do with JSD or Venkman?

Comment 2

[Adam Guthrie]

Hrm... maybe this bug /is/ about Rhino.

Comment 3

[Gil Tayar]

(In reply to comment #2)
> Hrm... maybe this bug /is/ about Rhino.
> 

It is.

Comment 4

[Gil Tayar]

I found the problem, although fixing it is above my means:

the problem is in functions which are called via tail calls (Icode_TAIL_CALL in the code).

In the code that executes the Icode_TAIL_CALL ("Interpreter.interpret"), it says this:

                if (op == Icode_TAIL_CALL) {
                    // Release the parent
                    exitFrame(cx, frame, null);

Since you do tail call elimination, the debugger also exits the frame, and thus does not "know" about the entry into the function.

I worked around this by eliminating tail call optimization (by commenting out the assigment in the code below, in the function Interpreter visitExpression):

                    if (type == Token.CALL) {
                        if ((contextFlags & ECF_TAIL) != 0) {
                            //type = Icode_TAIL_CALL;
                        }
                    }

Comment 5

[Gil Tayar]

Just to make sure I am understood about the nature of the bug. If you change function zoo in the example code in the first comment to:

function zoo(page)
  {
    var z = 1;
    z++;
    z++;
    foo(page);
    z++
  }

then the debugger will gladly step into the foo function. No problem.

The change is that now the call to foo is not a tail call, and thus no problem.

Comment 6

[Adam Guthrie]

I'm going to mark this as invalid since it wasn't a bug in our code. ->INVALID

Comment 7

[Gil Tayar]

(In reply to comment #6)
> I'm going to mark this as invalid since it wasn't a bug in our code. ->INVALID
> 

Could you explain why this is invalid and why it isn't a bug in the code? My last comment showed where in the code _exactly_ the problem is. It may be that I wasn't clear enough, in which case I would gladly try again.

Comment 8

[Hannes Wallnoefer]

Attachment: Fixes this bug without disabling tail call optimization by calling exitFrame() before initFrame()

I looked into this a bit. It is true that disabling tail call optimization solves the problem. However, the problem is not with tail call optimization itself, but a tiny bug in how it is done. The problem is that the new stack frame is created before the old one is popped, so the debugger first sees the onEnter() for the new frame, and immediately afterwards the onExit() for the old one, which results in the new frame being popped and the old one being kept as current.

This simple patch for Interpreter.java fixes the problem without disabling tail call optimization by calling exitFrame() before initFrame(). 

However, this still results in a "hole" in the stack frame context combobox, which may be confusing. Maybe the right thing to do is to disable tail call optimization when the debugger is on, but I don't know if this information is always easily available at compile time.

Comment 9

[Attila Szegedi]

Comitted Hannes' patch to CVS HEAD.

Comment 10

[Norris Boyd]

Adding target milestone of 1.6R6 based on the date this bug was resolved FIXED.