source notes broken for toplevel scripts

VERIFIED FIXED

Status

()

VERIFIED FIXED
18 years ago
17 years ago

People

(Reporter: rginda, Assigned: rginda)

Tracking

Trunk
x86
All
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments)

(Assignee)

Description

18 years ago
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.
(Assignee)

Comment 1

18 years ago
Comments from caguiar@adobe.com 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
 

Status: NEW → ASSIGNED
(Assignee)

Comment 2

18 years ago
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.
(Assignee)

Comment 3

18 years ago
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
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
(Assignee)

Comment 5

18 years ago
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.
(Assignee)

Comment 6

18 years ago
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."
(Assignee)

Comment 7

18 years ago
um, that's bug 33390, not 3390
(Assignee)

Comment 8

18 years ago
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.
(Assignee)

Comment 9

18 years ago
Created attachment 42315 [details] [diff] [review]
js.diff; proposed patch
(Assignee)

Updated

18 years ago
Component: JavaScript Debugger → Javascript Engine
Summary: can't set breakpoints if debugger keyword is used → source notes broken for toplevel scripts
(Assignee)

Comment 10

18 years ago
Nevermind the backwards subtraction comment (or patch), I'm the one who was
backwards.
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=brendan@mozilla.org 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
Hold the phone -- I have a better jsparse.c patch coming up.  Phil, can you test
the heck out of it?  Thanks,

/be
Created attachment 42499 [details] [diff] [review]
proposed jsparse.c fix
Created attachment 42506 [details] [diff] [review]
better comment, prompted by rginda over IRC
Created attachment 42509 [details] [diff] [review]
best patch yet -- like, it actually works (fer sure!)
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
(Assignee)

Comment 17

18 years ago
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

18 years ago
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
olaf: that sounds like a different bug -- please open one, unless rginda knows
of an existing report.

/be
(Assignee)

Comment 20

18 years ago
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.
QA Contact: rginda → pschwartau

Comment 21

18 years ago
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 -
(Assignee)

Comment 22

18 years ago
jband/shaver spare some time for an sr=?

Comment 23

18 years ago
sr=jband
(Assignee)

Comment 24

18 years ago
thanks, fix checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED

Comment 25

18 years ago
Marking Verified -
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.