82188 - source notes broken for toplevel scripts

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Robert Ginda]

I think the problem is larger than the current summary, "can't set breakpoints
if debugger keyword is used", but I can't think of sentence to describe what's
going on.  Olaf Meincke reported in .jseng that he couldn't set breakpoints in
the jsds console debugger anymore.  I verified this, and noticed that removing
the debugger keyword made it possible to set breakpoints again.  Then, Olaf noticed:

    When I run the following script

    var a = 1;
    var c = a+2;
    debugger;

    I can neither step through the script nor can I use breakpoint to stop at a
    given source line. However, I am able to display the value of a variable
    with the show() command.

    When I run the script

    foo();
    function foo() {
      var a = 1;
      var c = a+2;
      debugger;
    }

    step() and breakpoints work fine. Unfortunately, I am no longer able to
    display variales. For example, show('c') returns "undefined" as result!

Something is not right.

Comment 1

[Robert Ginda]

Comments from [e-mail] on n.p.m.jseng...

I've documented below a sequence of JSSrcNoteType's
received via jssrcnote (sn) in js_GetScriptLineExtent()
for a simple script containing 2 functions, and comparing
rc2 and rc3a. In rc3a, js_GetScriptLineExtent() actually
picks up the correct line extent when in step 10, but then
bails out (in step 11) with lineno=0 after it receives a final
SRC_SETLINE.
I don't know much about the JS bytecode generation code in
jsemit.c, but either the srcnotes are being generated incorrectly
or js_GetScriptLineExtent() is incorrectly getting the line extent
number for a script in rc3a. The debugger is pretty much
broken because of that.

Celso Aguiar

----------------------------------------
in rc2
1. SRC_SETLINE
2. SRC_NEWLINE
3. SRC_FUNCDEF
4. SRC_SETLINE
5. SRC_FUNCDEF
6. SRC_SETLINE
7. SRC_NEWLINE
8. SRC_VAR
9. SRC_NEWLINE
10. SRC_XDELTA
11. SRC_PCBASE
12. SRC_NEWLINE

in rc3a
1. SRC_FUNCDEF
2. SRC_SETLINE
3. SRC_FUNCDEF
4. SRC_SETLINE
5. SRC_NEWLINE
6. SRC_VAR
7. SRC_NEWLINE
8. SRC_XDELTA
9. SRC_PCBASE
10. SRC_NEWLINE
11. SRC_SETLINE
 

Comment 2

[Robert Ginda]

I write a simple driver to compile the following script, and use the SrcNotes
function from the js shell to dump the notes and line extent...

test.js:
function f()
{
}

x = 1;
<EOF>

Source notes:
  0:     0 [   0] newline 
  1:     0 [   0] funcdef  atom 0 (function f() {})
  3:     1 [   1] setline  lineno 5
  5:     9 [   8] xdelta  
  6:     9 [   0] setline  lineno 0
ScriptExtent: 0

I don't know much about source notes myself, but I'm betting that the final
setline 0 is a Bad Thing.

Comment 3

[Robert Ginda]

Same driver on the rc2 source produces...

Source notes:
  0:     0 [   0] setline  lineno 0
  2:     0 [   0] setline  lineno 2
  4:     0 [   0] funcdef  atom 0 (function f() {})
  6:     1 [   1] setline  lineno 5
ScriptExtent: 5

Comment 4

[Brendan Eich [:brendan]]

How many CVS revisions to jsemit.c between RC2 and 3?  I'll take a look when I
get some time; cc'ing shaver.

/be

Comment 5

[Robert Ginda]

16 fun-filled check-ins between rc3 and rc3.  rc2 used r3.37, rc3 used r3.53,
the trunk is at r3.54.

http://bonsai.mozilla.org/cvsview2.cgi?command=DIFF&subdir=mozilla%2Fjs%2Fsrc&file=jsemit.c&rev1=3.37&rev2=3.54&whitespace_mode=ignore&diff_mode=context

I'll try to narrow it down a bit with cvs co -D.

Comment 6

[Robert Ginda]

cvs co mozilla/js/src -D 1/27/01 leaves you with a working revision 3.48 of
jsemit.c, while -D 1/28/01 shows that 3.49 introduced the problem.  Revision
3.49 was part of a large patch checked in for bug 3390, "memory exhausted with
javascript array."

Comment 7

[Robert Ginda]

um, that's bug 33390, not 3390

Comment 8

[Robert Ginda]

Here comes a patch that fixes the symptoms.  I don't know this stuff well enough
to know for sure if it's a good patch, but it passes all the js engine tests,
and I can start the browser, open a second window, open mailnews and read my pop
mail.

In jsparse.c, we called js_EmitTree after calling Statements from the top level,
this seems to do nothing but set our setline 0 sourcenote (a STMT_BLOCK is
pushed and popped to the tree context, but nothing is ever emitted.)    I'm
guessing there is no reason for this extra js_EmitTree call.

Second, in UPDATE_LINENO_NOTES macro from jsemit.c, it looks to me like our
subtraction is backwards.

With these changes, my test driver produces...
Source notes:
  0:     0 [   0] setline  lineno 2
  2:     0 [   0] funcdef  atom 0 (function f() {})
  4:     1 [   1] setline  lineno 5
ScriptExtent: 5

Brendan, please take a look at this patch and lemme know what you think.

Comment 9

[Robert Ginda]

Attachment: js.diff; proposed patch

Comment 10

[Robert Ginda]

Nevermind the backwards subtraction comment (or patch), I'm the one who was
backwards.

Comment 11

[Brendan Eich [:brendan]]

Ah, yes: TCF_COMPILING implies that the tc handed to Statements is really a cg,
and that Statements should therefore compile each statement in turn.  And we
know that TCF_COMPILING is set in the cg/tc passed from js_CompileTokenStream.
sr=[e-mail] on the jsparse.c patch.

The jsemit.c patch seems wrong -- cg->currentLine equals or lags pn->pn_pos,
IIRC.  How can that jsemit.c patch work for you?

/be

Comment 12

[Brendan Eich [:brendan]]

Hold the phone -- I have a better jsparse.c patch coming up.  Phil, can you test
the heck out of it?  Thanks,

/be

Comment 13

[Brendan Eich [:brendan]]

Attachment: proposed jsparse.c fix

Comment 14

[Brendan Eich [:brendan]]

Attachment: better comment, prompted by rginda over IRC

Comment 15

[Brendan Eich [:brendan]]

Attachment: best patch yet -- like, it actually works (fer sure!)

Comment 16

[Brendan Eich [:brendan]]

Cc'ing jband for sr= love, in case shaver is traveling.  rginda can r= my patch,
and even check it in (I'm happy to do so, too).

/be

Comment 17

[Robert Ginda]

r=rginda, and I'll check it in.  For the record, this patch passes all of the js
engine tests that unpatched source does.  Specifically, the following three
cases fail, and nothing else.

ecma_3/RegExp/regress-78156.js
ecma_3/RegExp/regress-87231.js
ecma_3/String/regress-83293.js

Comment 18

[olaf.meincke]

I have applied the patch which solves half of the problem. Stepping through the 
code and setting breakpoints now works fine. However, under certain 
circumstances I still cannot display the content of variables using the show 
command in the console debugger.

Running the following script:

var a = 42;
debugger;

I can display the value of 'a' by typing: show('a')
However, when doing the same thing in the following script:

foo();

function foo() {
  var a = 42;
  debugger;
}


the console debugger displays 'undefined' for the value of 'a'.

Olaf

Comment 19

[Brendan Eich [:brendan]]

olaf: that sounds like a different bug -- please open one, unless rginda knows
of an existing report.

/be

Comment 20

[Robert Ginda]

I don't know of an existing report, please make sure you have the latest CVS
sources (with this patch applied.)  There was a similar problem that "fixed
itself" no too long ago.  If you see the problem with the latest source, please
open a new bug.

Comment 21

[Phil Schwartau]

Just ran the JS test suite on Linux with patch (id=42509) applied.
No new regressions introduced. All failures are previously known
and due to other bugs that are still being worked on -

Comment 22

[Robert Ginda]

jband/shaver spare some time for an sr=?

Comment 23

[John Bandhauer]

sr=jband

Comment 24

[Robert Ginda]

thanks, fix checked in.

Comment 25

[Phil Schwartau]

Marking Verified -