1147216 - Assertion failure: i <= 65535, at frontend/BytecodeEmitter.cpp

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

The upcoming hard-to-reduce testcase asserts js debug shell on m-c changeset 2e2244031032 with --fuzzing-safe --no-threads --no-ion at Assertion failure: i <= 65535, at frontend/BytecodeEmitter.cpp.

Configure options:

CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar AUTOCONF=/usr/local/Cellar/autoconf213/2.13/bin/autoconf213 sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=x86_64-apple-darwin12.5.0 --enable-debug --enable-nspr-build --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

python -u ~/fuzzing/js/compileShell.py -b "--enable-debug --enable-more-deterministic --enable-nspr-build" -r 2e2244031032

=== Treeherder Build Bisection Results by autoBisect ===

The "good" changeset has the timestamp "20150321073953" and the hash "a6e177495aa9".
The "bad" changeset has the timestamp "20150321082224" and the hash "787394ba34a2".

Likely regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=a6e177495aa9&tochange=787394ba34a2

Jan, is bug 1143704 a likely regressor?

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: hard-to-reduce testcase

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: stack

(lldb) bt 5
* thread #1: tid = 0x43a3e, 0x0000000100188882 js-dbg-64-dm-nsprBuild-darwin-2e2244031032`js::frontend::BytecodeEmitter::emitCallOrNew(js::frontend::ParseNode*) [inlined] js::frontend::BytecodeEmitter::emitUint16Operand(this=<unavailable>, op=<unavailable>, i=<unavailable>) + 67 at BytecodeEmitter.cpp:540, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
  * frame #0: 0x0000000100188882 js-dbg-64-dm-nsprBuild-darwin-2e2244031032`js::frontend::BytecodeEmitter::emitCallOrNew(js::frontend::ParseNode*) [inlined] js::frontend::BytecodeEmitter::emitUint16Operand(this=<unavailable>, op=<unavailable>, i=<unavailable>) + 67 at BytecodeEmitter.cpp:540
    frame #1: 0x000000010018883f js-dbg-64-dm-nsprBuild-darwin-2e2244031032`js::frontend::BytecodeEmitter::emitCallOrNew(this=<unavailable>, pn=<unavailable>) + 1711 at BytecodeEmitter.cpp:6221
    frame #2: 0x000000010016fe6f js-dbg-64-dm-nsprBuild-darwin-2e2244031032`js::frontend::BytecodeEmitter::emitTree(this=0x00007fff5fbfacf0, pn=0x00000001053f8d40) + 575 at BytecodeEmitter.cpp:7223
    frame #3: 0x0000000100187328 js-dbg-64-dm-nsprBuild-darwin-2e2244031032`js::frontend::BytecodeEmitter::emitStatement(this=0x00007fff5fbfacf0, pn=<unavailable>) + 696 at BytecodeEmitter.cpp:5913
    frame #4: 0x000000010016ffab js-dbg-64-dm-nsprBuild-darwin-2e2244031032`js::frontend::BytecodeEmitter::emitTree(this=0x00007fff5fbfacf0, pn=0x00000001053f8cd0) + 891 at BytecodeEmitter.cpp:7107
(lldb)

Comment 3

[Jan de Mooij [:jandem]]

Awesome. I added this i <= UINT16_MAX assertion in bug 1143704 after I converted this code from a macro to a proper method, and the assert caught a real bug.

Basically the JSOP_LINENO op we emit after an eval has an uint16 operand but the line number does not necessarily fit in that. That's why your testcase is hard to reduce because you need a line number > 65536 :)

Below is a testcase that fails in opt builds (and also failed before my patches) because we get a bogus line number.

Yay, proper asserts :) Not security sensitive and not a major bug, but we should fix this.

function f() {
    var s = "";
    var x, stack;
    for (var i=0; i<70000; i++) {
	s += "x = 0;\n";
	if (i === 68000)
	    s += "eval('stack = Error().stack');";
    }
    eval(s);
    assertEq(stack.indexOf("line 68002") > 0, true);
}
f();

Comment 4

[Tom Tromey :tromey]

This prevents diagnosing bug 1143449, as firefox crashes trying to load the referenced page.

Comment 5

[Jan de Mooij [:jandem]]

(In reply to Tom Tromey :tromey from comment #4)
> This prevents diagnosing bug 1143449, as firefox crashes trying to load the
> referenced page.

Great! So before I added this assert this was a correctness bug that wouldn't be that easy to track down, but now we get a nice assertion failure and the cause of bug 1143449 is obvious :) (looks like it's the same issue, big line numbers)

I'll get to this tomorrow.

Comment 6

[Tom Tromey :tromey]

FWIW I took a quick look at it.  Changing JSOP_LINENO to take an int32 seems doable.
But on the other hand, JSOP_LINENO could also just be removed.  It's only used in one spot.

Comment 7

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #5)
> but now we get a nice assertion failure
> and the cause of bug 1143449 is obvious :) (looks like it's the same issue,
> big line numbers)

Well I may be a bit too optimistic; it could be completely unrelated of course.

(In reply to Tom Tromey :tromey from comment #6)
> FWIW I took a quick look at it.  Changing JSOP_LINENO to take an int32 seems
> doable.
> But on the other hand, JSOP_LINENO could also just be removed.  It's only
> used in one spot.

Yes I'm not sure why we use this path for direct eval but not for the other callers of DescribeScriptedCallerForCompilation. We should probably do some bug/hg archaeology; the code doesn't explain why we have that special CALLED_FROM_JSOP_EVAL path...

Comment 8

[Tom Tromey :tromey]

(In reply to Jan de Mooij [:jandem] from comment #7)
> (In reply to Jan de Mooij [:jandem] from comment #5)
> > but now we get a nice assertion failure
> > and the cause of bug 1143449 is obvious :) (looks like it's the same issue,
> > big line numbers)
> 
> Well I may be a bit too optimistic; it could be completely unrelated of
> course.

Yeah -- that bug is about a failure of the debugger to stop at the right spot;
it's just that when I tried to investigate that bug, I couldn't due to this one.
So this is a prerequisite but unfortunately not a solution :)

Comment 9

[Douglas Crosher [:dougc]]

Hitting this one too on larger asm.js applications.

Assertion failure: i <= (65535), at src/frontend/BytecodeEmitter.cpp:528

Program received signal SIGSEGV, Segmentation fault.
0x00000000005a2281 in js::frontend::BytecodeEmitter::emitUint16Operand (this=this@entry=0x7fffffffc4c0, op=op@entry=JSOP_LINENO, i=<optimized out>) at src/js/src/frontend/BytecodeEmitter.cpp:528
528	    MOZ_ASSERT(i <= UINT16_MAX);
Missing separate debuginfos, use: debuginfo-install glibc-2.18-19.fc20.x86_64 libgcc-4.8.3-7.fc20.x86_64 libstdc++-4.8.3-7.fc20.x86_64 nspr-4.10.8-1.fc20.x86_64
(gdb) back
#0  0x00000000005a2281 in js::frontend::BytecodeEmitter::emitUint16Operand (this=this@entry=0x7fffffffc4c0, op=op@entry=JSOP_LINENO, i=<optimized out>) at src/js/src/frontend/BytecodeEmitter.cpp:528
#1  0x00000000005c7e64 in js::frontend::BytecodeEmitter::emitCallOrNew (this=this@entry=0x7fffffffc4c0, pn=pn@entry=0x7fffeb302f28) at src/js/src/frontend/BytecodeEmitter.cpp:6173

Comment 10

[Jan de Mooij [:jandem]]

Attachment: Patch

Give JSOP_LINENO an uint32 (instead of uint16) operand.

Comment 11

[Luke Wagner [:luke]]

Comment on attachment 8587975 [details] [diff] [review]
Patch

Review of attachment 8587975 [details] [diff] [review]:
-----------------------------------------------------------------

Nice!

Comment 12

[Jan de Mooij [:jandem]]

Attachment: Another fix

I noticed another bug in DescribeScriptedCallerForCompilation. It does:

MOZ_ASSERT(*(pc + (isSpread ? JSOP_SPREADEVAL_LENGTH : JSOP_EVAL_LENGTH)) == JSOP_LINENO);

*linenop = GET_UINT32(pc + (JSOp(*pc) == JSOP_EVAL ? JSOP_EVAL_LENGTH
                                                   : JSOP_SPREADEVAL_LENGTH));

So the assert is correct but the code after it is wrong: if *pc == JSOP_STRICTEVAL, we add JSOP_SPREADEVAL_LENGTH instead of JSOP_EVAL_LENGTH.

This patch refactors the code a bit to avoid the duplication and fixes the bug.

But this means our line numbers for eval in strict mode have been wrong since JSOP_STRICTEVAL was added :(

Comment 13

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/de8a57b0f68f
https://hg.mozilla.org/integration/mozilla-inbound/rev/f334f70a115c

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/de8a57b0f68f
https://hg.mozilla.org/mozilla-central/rev/f334f70a115c

Comment 15

[Jan de Mooij [:jandem]]

Comment on attachment 8587975 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/regressing bug #]: Bug 1143704 exposed this, actual bug is much older.
[User impact if declined]: Assertion failures in debug builds, or incorrect line numbers in stack traces.
[Describe test coverage new/current, TreeHerder]: Tested on TreeHerder.
[Risks and why]: Very low.
[String/UUID change made/needed]: None.

Comment 16

[Jan de Mooij [:jandem]]

Comment on attachment 8587984 [details] [diff] [review]
Another fix

Approval Request Comment
[Feature/regressing bug #]: Bug 1101905.
[User impact if declined]: Bogus line numbers in stack traces for eval in strict mode.
[Describe test coverage new/current, TreeHerder]: Tested on TreeHerder.
[Risks and why]: Simple patch, no risk.
[String/UUID change made/needed]: None.

Comment 17

[Liz Henry (:lizzard)]

Comment on attachment 8587984 [details] [diff] [review]
Another fix

Looks like this fix will help with diagnosing crashes and the tests look good on m-c. Approved for uplift to aurora.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

This has Xdr.h conflicts on Aurora, and I'm not sure what the rules are for bytecode version bumps on uplifts.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Got my questions answered on IRC.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2deec68d84bb
https://hg.mozilla.org/releases/mozilla-aurora/rev/729003f39455

Comment 21

[Liz Henry (:lizzard)]

Comment on attachment 8587975 [details] [diff] [review]
Patch

Marking this approved too. Looks like it was already uplifted. Thanks RyanVM.