Last Comment Bug 754713 - IonMonkey: Assertion failure: [infer failure] Missing type pushed 0: void, at js/src/jsinfer.cpp:353
: IonMonkey: Assertion failure: [infer failure] Missing type pushed 0: void, at...
Status: RESOLVED FIXED
[jsbugmon:update]
: assertion, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: x86_64 Linux
: -- major (vote)
: ---
Assigned To: Jan de Mooij [:jandem] (PTO until July 31)
:
Mentors:
: 741203 (view as bug list)
Depends on:
Blocks: langfuzz IonFuzz
  Show dependency treegraph
 
Reported: 2012-05-13 15:06 PDT by Christian Holler (:decoder)
Modified: 2013-01-14 08:18 PST (History)
8 users (show)
choller: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected


Attachments
Patch (7.22 KB, patch)
2012-05-16 10:36 PDT, Jan de Mooij [:jandem] (PTO until July 31)
no flags Details | Diff | Splinter Review
Patch (7.15 KB, patch)
2012-05-16 11:15 PDT, Jan de Mooij [:jandem] (PTO until July 31)
dvander: review+
Details | Diff | Splinter Review

Description Christian Holler (:decoder) 2012-05-13 15:06:48 PDT
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());
Comment 1 Jan de Mooij [:jandem] (PTO until July 31) 2012-05-15 05:32:48 PDT
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.
Comment 2 Jan de Mooij [:jandem] (PTO until July 31) 2012-05-16 06:54:22 PDT
*** Bug 741203 has been marked as a duplicate of this bug. ***
Comment 3 Jan de Mooij [:jandem] (PTO until July 31) 2012-05-16 06:59:45 PDT
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.
Comment 4 Jan de Mooij [:jandem] (PTO until July 31) 2012-05-16 10:36:07 PDT
Created attachment 624442 [details] [diff] [review]
Patch

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.
Comment 5 Jan de Mooij [:jandem] (PTO until July 31) 2012-05-16 11:15:20 PDT
Created attachment 624454 [details] [diff] [review]
Patch
Comment 6 David Anderson [:dvander] 2012-05-16 11:50:04 PDT
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?
Comment 7 Jan de Mooij [:jandem] (PTO until July 31) 2012-05-16 13:19:43 PDT
(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
Comment 8 Christian Holler (:decoder) 2013-01-14 08:18:29 PST
A testcase for this bug was automatically identified at js/src/jit-test/tests/ion/bug754713-2.js.

Note You need to log in before you can comment on or make changes to this bug.