Closed Bug 1472291 Opened 6 years ago Closed 6 years ago

BytecodeEmitter does not emit columns for if/switch/with/break/continue statements

Categories

(Core :: JavaScript Engine, defect)

Product:
Core
Component:
JavaScript Engine
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: loganfsmyth, Assigned: loganfsmyth)

Details

Attachments

(2 files, 1 obsolete file)

Bug 1472291 - Ensure that if, switch, do-while, with, break, and continue statements have column offsets.
59 bytes, text/x-review-board-request
jorendorff
: review+
Details
Bug 1472291 - Fix two small typos.
59 bytes, text/x-review-board-request
jorendorff
: review+
Details 
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/67.0.3396.99 Safari/537.36

Steps to reproduce:

I'm going to post a patch, but essentially loading up some code with these statements and using `dbg.getAllColumnOffsets` will show that the columns are all `0` generally since the line was incremented, but no column was selected.

https://bugzilla.mozilla.org/show_bug.cgi?id=1436407 explains a potential overall solution to try to address this.
Assignee: nobody → loganfsmyth
Status: UNCONFIRMED → ASSIGNED
Component: Untriaged → JavaScript Engine
Ever confirmed: true
Product: Firefox → Core
Comment on attachment 8988855 [details]
Bug 1472291 - Ensure that if, switch, do-while, with, break, and continue statements have column offsets.

https://reviewboard.mozilla.org/r/254014/#review261336

Great patch. Thanks very much for the fixes in Opcodes.h. Please land them in a separate changeset, r=me.

I'd prefer for the location code to stay centralized

I just sunk some work into improving these tests. Not a great use of time, maybe, but we might as well use it!

```js
// Set breakpoints "everywhere" in a function, then call the function and check that
// the columns where breakpoints were hit are as expected.
//
// `code` is a JS Script, all on one line, that defines a function `f`.
// `expected` is a string of spaces and carets ('^'). Throws if we don't hit
// breakpoints on exactly the columns indicated by the carets.  Doesn't test
// the order they're hit or how many times.
function testColumnOffsets(code, expected) {
    // Define the function `f` in a new global.
    let global = newGlobal();
    global.eval(code);

    // Set breakpoints everywhere and call the function.
    let dbg = new Debugger;
    let script = dbg.addDebuggee(global).makeDebuggeeValue(global.f).script;
    let hits = [];
    for (let offset of script.getAllColumnOffsets()) {
        assertEq(offset.lineNumber, 1);
        assertEq(offset.columnNumber < code.length, true);
        script.setBreakpoint(offset.offset, {
            hit(frame) {
                hits[offset.columnNumber] = true;
            }
        });
    }
    global.f();

    // Make a map of where we hit.
    let actual = "";
    for (let i = 0; i < code.length; i++)
        actual += i in hits ? "^" : " ";

    if (actual.trimEnd() != expected.trimEnd()) {
        throw new Error(`Assertion failed:
                     code: ${code}
            expected hits: ${expected}
              actual hits: ${actual}\n`);
    }
}
```

Please drop that into a file in js/src/jit-test/lib/ and then merge all your new tests into a single test file, like this:


```
// Test column offsets for the various parts of JS statements.

load(libdir + "debug-whatever.js");

testColumnOffsets(
    "function f(n) { switch(n) { default: print(n); } }",
    "                ^                    ^           ^");

testColumnOffsets(
    "function f(n) { do { print(n); break; } while(false); }",
    "                ^    ^         ^                      ^");
```

...and so on.

Not all of the existing tests can use this, but all your new ones can.

::: js/src/frontend/BytecodeEmitter.cpp:7030
(Diff revision 1)
>  {
>      IfEmitter ifThenElse(this);
>  
>    if_again:
> +    // Make sure this code is attributed to the "if" so that it gets a useful
> +    // column number, instead of the default 0 value.

The fact that this comment is necessary kind of bugs me, but I don't see what to do about it.

Renaming the method doesn't seem sufficient. Moving all the updateSourceCoordNotes calls to a single place at the top of emitTree() might be clearer (at least there would be a single comment), but then `else if` chaining requires us to have a special udpateSourceCoordNotes call here anyway.

Oh well. I'm fine with this as written.

::: js/src/frontend/BytecodeEmitter.cpp:7032
(Diff revision 1)
>  
>    if_again:
> +    // Make sure this code is attributed to the "if" so that it gets a useful
> +    // column number, instead of the default 0 value.
> +    if (!updateSourceCoordNotes(pn->pn_pos.begin))
> +        return false;

Please test that this works as desired when there's `else if` chaining in the JS code (i.e. when the `goto if_again` is hit. That has to be the most egregiously unnecessary `goto` in our codebase...)
Comment on attachment 8988855 [details]
Bug 1472291 - Ensure that if, switch, do-while, with, break, and continue statements have column offsets.

https://reviewboard.mozilla.org/r/254014/#review261358

r=me with the tests updated. Feel free to rephrase existing tests to use the new lib function, or not.
Attachment #8988855 - Flags: review?(jorendorff) → review+
Comment on attachment 8989593 [details]
Bug 1472291 - Fix two small typos.

https://reviewboard.mozilla.org/r/254614/#review261448
Attachment #8989593 - Flags: review?(jorendorff) → review+
Comment on attachment 8989594 [details]
Bug 1472291 - Refactor the column offset tests to use a simple helper.

https://reviewboard.mozilla.org/r/254616/#review261450

Perfect. I didn't understand what you meant by "separate patch" when we chatted about this on IRC. This patch and the previous should be combined ("squashed") before landing.

(`hg histedit` is the equivalent of `git rebase -i`, in case that helps; you may have to turn on an extension to get that command, but if you did `mach setup` then it's probably done for you... maybe??)

::: js/src/jit-test/tests/debug/Script-getAllColumnOffsets.js:5
(Diff revision 1)
> +load(libdir + "assert-offset-columns.js");
> +
> +// getColumnOffsets correctly places the various parts of a ForStatement.
> +assertOffsetColumns(
> +    "function f(n) { for (var i = 0; i < n; ++i) hits.push('.'); hits.push('!'); } debugger;",

Please remove the debugger statements from these, since they're not being used anymore.
Attachment #8989594 - Flags: review?(jorendorff) → review+
Attachment #8989594 - Attachment is obsolete: true
Needs proper mozreview approval before it can be landed.
Flags: needinfo?(loganfsmyth)
Keywords: checkin-needed
Comment on attachment 8988855 [details]
Bug 1472291 - Ensure that if, switch, do-while, with, break, and continue statements have column offsets.

https://reviewboard.mozilla.org/r/254014/#review262634

APPROVED
Comment on attachment 8989593 [details]
Bug 1472291 - Fix two small typos.

https://reviewboard.mozilla.org/r/254614/#review262638

APPROVED AGAIN IN CASE IT HELPS
Sorry, didn't mean to be snarky there, I was just kind of stupidly playing with mozreview to see what it would do ... anyway, I imagine this will do the trick, but please ni?me if not.
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/b4a8e7ca6bcd
Fix two small typos. r=jorendorff
https://hg.mozilla.org/integration/autoland/rev/a1757187c5a9
Ensure that if, switch, do-while, with, break, and continue statements have column offsets. r=jorendorff
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b4a8e7ca6bcd
https://hg.mozilla.org/mozilla-central/rev/a1757187c5a9
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Flags: needinfo?(loganfsmyth)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: