Closed
Bug 754713
Opened 12 years ago
Closed 12 years ago
IonMonkey: Assertion failure: [infer failure] Missing type pushed 0: void, at js/src/jsinfer.cpp:353
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
firefox-esr10 | --- | unaffected |
People
(Reporter: decoder, Assigned: jandem)
References
Details
(Keywords: assertion, testcase, Whiteboard: [jsbugmon:update])
Attachments
(1 file, 1 obsolete file)
7.15 KB,
patch
|
dvander
:
review+
|
Details | Diff | Splinter Review |
The following testcase asserts on ionmonkey revision e8de64e7e9fe (run with --ion -n): var t = 100; function foo(ox) { var x = ox; var n = 0; for (var i = 0; i < 90; i++) { n += x.f.g; if (i >= 80) t; } } function f() {} function g() {} f.prototype = {f:g}; foo(new f());
Assignee | ||
Comment 1•12 years ago
|
||
The problem is that IonBuilder::jsop_getprop pushes a constant value for "x.f" and therefore "x" has no SSA uses. IsPhiObservable does not like this: // Note that this skips reading resume points, which we don't count as // actual uses. This is safe as long as the SSA still mimics the actual // bytecode, i.e. no elimination has occurred. If the only uses are resume // points, then the SSA name is never consumed by the program. So it's basically the same issue as bug 724517. Requiring SSA uses to mimic bytecode uses seems too error-prone. I will change IsPhiObservable to not skip resume points and see if it hurts benchmark performance.
Assignee: general → jdemooij
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•12 years ago
|
||
Bug 741203 has another great testcase, I reduced it to: function f(x) { var y = (x < 0) ? 1 : 2; Math.floor(0); // bailout } Math.floor(0); f(1); Here we eliminate the phi for "y", then bailout and resume at SETLOCAL y. The type check fails because we set y to |undefined| instead of 2.
Assignee | ||
Comment 4•12 years ago
|
||
This is one of the simpler solutions we discussed yesterday. My initial patch fixed all places manually and modified traverseBytecode to ensure that if there's a bytecode use there must be at least one SSA use. The final patch is much simpler though: if a bytecode instruction uses a phi, it marks that phi as having bytecode uses and we always consider such phis observable. It fixes all testcases I found and does not noticeably regress performance.
Attachment #624442 -
Flags: review?(dvander)
Assignee | ||
Updated•12 years ago
|
Attachment #624442 -
Flags: review?(dvander)
Assignee | ||
Comment 5•12 years ago
|
||
Attachment #624442 -
Attachment is obsolete: true
Attachment #624454 -
Flags: review?(dvander)
Comment on attachment 624454 [details] [diff] [review] Patch Review of attachment 624454 [details] [diff] [review]: ----------------------------------------------------------------- Nice - glad to see there was both a simple and generic fix. ::: js/src/ion/IonBuilder.cpp @@ +622,5 @@ > > +void > +IonBuilder::markPhiBytecodeUses(JSOp op) > +{ > + unsigned nuses = analyze::GetUseCount(script, pc - script->code); What's the difference between this and js_CodeSpec[op].nuses?
Attachment #624454 -
Flags: review?(dvander) → review+
Assignee | ||
Comment 7•12 years ago
|
||
(In reply to David Anderson [:dvander] from comment #6) > > What's the difference between this and js_CodeSpec[op].nuses? js_CodeSpec[op].nuses is -1 if the number of uses is dynamic (JSOP_CALL, JSOP_POPN etc). GetUseCount returns the right value in these cases. http://hg.mozilla.org/projects/ionmonkey/rev/46eda2edc3b7
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Updated•12 years ago
|
status-firefox-esr10:
--- → unaffected
Assignee | ||
Updated•12 years ago
|
Group: core-security
Reporter | ||
Comment 8•11 years ago
|
||
A testcase for this bug was automatically identified at js/src/jit-test/tests/ion/bug754713-2.js.
Flags: in-testsuite+
You need to log in
before you can comment on or make changes to this bug.
Description
•