313922 - js_LineNumberToPC returns prolog PC not main PC

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Mike Moening]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7.12) Gecko/20050915 Firefox/1.0.7
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7.12) Gecko/20050915 Firefox/1.0.7

Given the sample code below, js_LineNumberToPC will always return the PC for
line 10 when called with line 8 as the target.
js_LineNumberToPC searches through the sourcenotes and sees the
predefining prolog entry for the DEFVAR op code and mistakenly returns line
10, since 10 is greater than 8 (the function thinks 10 is the closest line
to 8).

js_LineNumberToPC should skip the prolog source notes.

The results of this behavior make it impossible to set a breakpoint
using JS_SetTrap (which requires a PC) on line 8.
If you remove line 10 from the source it works since I assume the prolog stops
at line 3 (the last DEFVAR).  The venkman debugger also suffers from this problem for the same reason.

01: function DoNothing(n)
02: {
03:   var iStupidCounter;
04:   for(i=0;i<n;i++)
05:     iStupidCounter++;
06: }
07:
08: DoNothing(5);
09:
10: var oCantSetBreakBeforeThisLineUnlessIRemoveIt;


Reproducible: Always

Steps to Reproduce:
1. Call js_LineNumberToPC using line 8 and source provided.
2.
3.

Actual Results:  
Returns PC for line 10

Expected Results:  
Should return PC for line 8

The js_LineNumberToPC function is found in jsscript.c
Please see below for the fix.  Sorry I can't get CVS to work

Patched code below to ignore the prolog.
----------
js_LineNumberToPC(JSScript *script, uintN target)
{
    ptrdiff_t offset;
    uintN lineno;
    jssrcnote *sn;
    JSSrcNoteType type;

    offset = 0;
    lineno = script->lineno;
    for (sn = SCRIPT_NOTES(script); !SN_IS_TERMINATOR(sn); sn = SN_NEXT(sn)) {
-       if (lineno >= target)
+       if (lineno >= target && (script->code+offset >= script->main))
            break;
----------

Comment 1

[Brendan Eich [:brendan]]

Sorry I missed this -- it should be fixed soon, and could be for js1.6rc1.

/be

Comment 2

[Brendan Eich [:brendan]]

Fixed already by patch for bug 111352.

Here's a shell testcase:

s = Script( "function DoNothing(n)\n" +
            "{\n" +
            "  var iStupidCounter;\n" +
            "  for(i=0;i<n;i++)\n" +
            "    iStupidCounter++;\n" +
            "}\n" +
            "\n" +
            "DoNothing(5);\n" +
            "\n" +
            "var oCantSetBreakBeforeThisLineUnlessIRemoveIt;\n");
print(s);
dis(s);
print(line2pc(s, 8));

/be

*** This bug has been marked as a duplicate of 111352 ***

Comment 3

[Mike Moening]

I'm not sure the fix for that other bug will do it.

We should still be checking that (script->code+offset >= script->main)
Otherwise we are looking at the prolog and not the main line script.
Why would we want to check any PC's before script->main?

Comment 4

[Brendan Eich [:brendan]]

Mike: did you read the patch, or test the testcase I included in comment 2?

The fix works because it picks the nearest pc not after any pc associated with the given line number.  It's needed for other cases, and it's efficient.  There is no need for a redundant check that covers only this bug's case when the full solution works here too.

/be

Comment 5

[Bob Clary [:bc] (inactive)]

Removing js16 blocking as fix in bug 111352 is already in the 1.5 release.

Comment 6

[Mike Moening]

Brendan, This is not fixed.

Here's a simple 6 line script to illustrate the problem.

01:  var sVar2;
02:  sVar2 = '';
03:  for(var i=1;i<=10;i++){
04:     sVar2 += i;
05:  }
06:  sVar2='';

If you use js_LineNumberToPC() on line 3 it returns the PC for the prolog.

Here's a jsshell test case using the same script to illustrate the problem.

Please check out the PC returned from the calls to line2pc().
The return PC value should increase for lines 2, 3 and 4.
However, line 3 returns a PC that is less than line 2!
Obviously this is the prolog PC.

We still need the fix I suggested in Comment# 3 above.

The downstream impact is that if you use this PC in JS_SetTrap() when the breakHandler fires you are looking at prolog code instead of the mainline script.

------------
s = Script( "var sVar2;\n" +
            "sVar2 = '';\n" +
            "for(var i=1;i<=10;i++){\n" + 
            "   sVar2 += i;\n" +
            "}\n" +
            "sVar2='';\n")
print(s);
dis(s);
print(line2pc(s, 2));
print(line2pc(s, 3));
print(line2pc(s, 4));
--------

Mike M.

Comment 7

[Mike Moening]

I tried the following quick hack shown below:
It fixes the bug, however I think it would be better to 
scan forward in the source notes until we are greater 
than or equal to script->main before checking line numbers at all.

Someone smarter than me needs to turn this into a better patch.

----------
jsbytecode *
js_LineNumberToPC(JSScript *script, uintN target)
{
    ptrdiff_t offset, best;
    uintN lineno, bestdiff, diff;
    jssrcnote *sn;
    JSSrcNoteType type;

    offset = 0;
    best = -1;
    lineno = script->lineno;
    bestdiff = SN_LINE_LIMIT;
    for (sn = SCRIPT_NOTES(script); !SN_IS_TERMINATOR(sn); sn = SN_NEXT(sn)) {
        if (lineno == target && (script->code+offset >= script->main))
            goto out;
-       if (lineno >= target)
+       if (lineno >= target && (script->code+offset >= script->main))
            break;
----------

Comment 8

[Mike Moening]

Arrg!
I there was extra crap in my patch.  Here it is again:

----------
jsbytecode *
js_LineNumberToPC(JSScript *script, uintN target)
{
    ptrdiff_t offset, best;
    uintN lineno, bestdiff, diff;
    jssrcnote *sn;
    JSSrcNoteType type;

    offset = 0;
    best = -1;
    lineno = script->lineno;
    bestdiff = SN_LINE_LIMIT;
    for (sn = SCRIPT_NOTES(script); !SN_IS_TERMINATOR(sn); sn = SN_NEXT(sn)) {
-       if (lineno >= target)
+       if (lineno == target && (script->code+offset >= script->main))
            goto out;
        if (lineno > target) {
----------

Comment 9

[Brendan Eich [:brendan]]

If you never want to return a prolog pc, ok. But how can you be sure there are main bytecodes related to the target line in question? Some lines cause *only* prolog bytecodes to be emitted. Counterexample:

"var x = 42\n")

function f() {
}

var x = 42;

js> line2pc(s,1)
0

Should be 1.

/be

Comment 10

[Brendan Eich [:brendan]]

I would say we need a better API -- one to which you can give advice about what section you want the pc to be in, or perhaps one that returns a set of bytecode addresses (efficiently represented) for the given line. Morph this bug into that RFE, if you can specify the new API completely and cleanly.

/be

Comment 11

[Mike Moening]

I would agree with your comment in #9.  However, I would add the following question.  What good is setting a breakpoint in the prolog for debugging purposes?  Most people when they hit a breakpoint on a line far down in the code, assume the got there by executing the code above it.  When they look at objects and properties for that stack frame they will look all messed up since nothing has been executed except variable definitions. Maybe breakpoints on these lines should not be allowed, period. Yes I know that may confuse people, but which confusion is worse?
I'm gonna sleep on this and post back here later.
Feel free to add more to this in the meantime...

Comment 12

[Mike Moening]

Instead of creating a new API would it be better to add another JSOPTION_ or #ifdef to the code?

The code change is so small and the two API's would share 99% of the same code.
A reply to questions raised in #11 would be nice too.

Mike M

Comment 13

[Brendan Eich [:brendan]]

I will attach a patch in a few, that simply prefers main to prolog pc, but will find the uninitialized var's prolog bytecode address instead of returning null.

/be

Comment 14

[Brendan Eich [:brendan]]

Wait, I already wrote and landed this patch to js_LineNumberToPC -- two years ago! See bug 111352. MikeM, why don't you have up to date source here?

/be

Comment 15

[Mike Moening]

Brendan,

I DO have the latest source.
Please try the jsshell test case I wrote in #6.  Do you see the issue?
It should still be present.

Comment 16

[Mike Moening]

This really isn't fixed.  Please try jsshell test case from comment #6.
If you can't repro it...then I'll shut up. :-)

Comment 17

[Brendan Eich [:brendan]]

Attachment: fix, sorry it took so long

Mike, please try to break this. Thanks,

/be

Comment 18

[Brendan Eich [:brendan]]

Attachment: patch for checkin

Rearranged to avoid a PTRDIFF use-case, and commented. Waiting for tree to open.

/be

Comment 19

[Brendan Eich [:brendan]]

Fixed on trunk:

js/src/jsscript.c 3.139

/be