1174547 - Assertion failure: type == MIRType_Object, at jit/IonTypes.h

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine: JIT, defect)

Description

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

f = (function() {
         "use asm"
         function f() {
             var i = {};
             (_ ? 2() : (0 ? [] : w)) | 0
         }
         return f
 })()
 for (var j = 0; j < 999; j++) {
     try {
         f()
     } catch (e) {}
 }

asserts js debug shell on m-c changeset 203e1025a826 with --fuzzing-safe --no-threads --baseline-eager at Assertion failure: type == MIRType_Object, at jit/IonTypes.h.

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 203e1025a826

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

The "good" changeset has the timestamp "20150611043342" and the hash "fbb2e7d00e46".
The "bad" changeset has the timestamp "20150611053142" and the hash "46acf3627306".

Likely regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=fbb2e7d00e46&tochange=46acf3627306

Setting s-s because MIR type assertion failures can be bad.

Nicolas, is bug 1166711 a likely regressor?

Comment 1

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

Attachment: stack

(lldb) bt 5
* thread #1: tid = 0x1d1518, 0x00000001006f2435 js-dbg-64-dm-nsprBuild-darwin-203e1025a826`js::jit::CodeGeneratorX64::visitBox(js::jit::LBox*) + 52 at IonTypes.h:450, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
  * frame #0: 0x00000001006f2435 js-dbg-64-dm-nsprBuild-darwin-203e1025a826`js::jit::CodeGeneratorX64::visitBox(js::jit::LBox*) + 52 at IonTypes.h:450
    frame #1: 0x00000001006f2401 js-dbg-64-dm-nsprBuild-darwin-203e1025a826`js::jit::CodeGeneratorX64::visitBox(this=<unavailable>, box=<unavailable>) + 593 at CodeGenerator-x64.cpp:81
    frame #2: 0x00000001004c2b79 js-dbg-64-dm-nsprBuild-darwin-203e1025a826`js::jit::CodeGenerator::generateBody(this=0x0000000101fe4000) + 985 at CodeGenerator.cpp:4111
    frame #3: 0x00000001004db9da js-dbg-64-dm-nsprBuild-darwin-203e1025a826`js::jit::CodeGenerator::generate(this=0x0000000101fe4000) + 458 at CodeGenerator.cpp:7787
    frame #4: 0x0000000100532c6e js-dbg-64-dm-nsprBuild-darwin-203e1025a826`js::jit::GenerateCode(mir=0x000000010391a258, lir=0x000000010391f188) + 302 at Ion.cpp:1729
(lldb)

Comment 2

[Nicolas B. Pierron [:nbp]]

This issue is caused by the fact that we re-specialized a Phi after removing a branch due to GVN / UCE optimizations.  The problem is that the first time we specialized the Phi to be an Object, and the type analysis inserted an MBox of that Phi.  The second iteration, as none of the result type sets are known, the phi is specialized to a Value, but the MBox is kept in place.

I see 2 options:

The first solution would be to flag any instructions added by the ApplyType phase, and the second time we run the ApplyType phase, we can either get rid of all of them, or only the one which are working against the Phi specialization.

The second solution would be to not specialize the Phi the second time.


I like the first solution as an optimization, but it sounds that it might be more likely introduce additional issues.  I will attempt the second solution, and see if it works well.

Comment 3

[Nicolas B. Pierron [:nbp]]

Attachment: ApplyType: Prevent re-entry of specialization phases.

Comment 4

[Nicolas B. Pierron [:nbp]]

(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #0)
> Likely regression window:
> https://hg.mozilla.org/integration/mozilla-inbound/
> pushloghtml?fromchange=fbb2e7d00e46&tochange=46acf3627306
> 
> Nicolas, is bug 1166711 a likely regressor?

No, this is Bug 1165348, as Bug 1166711 only extend Scalar Replacement, and this issue is about re-executing the Phi specialization after running UCE.

Comment 8

[Nicolas B. Pierron [:nbp]]

Do you have any other test case with the same signature?

Apparently the current fix does not fix all web-site, so I suspect there is another one with a similar signature.

Comment 9

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

(In reply to Nicolas B. Pierron [:nbp] from comment #8)
> Do you have any other test case with the same signature?

Nope I don't, yet.

Comment 10

[Christian Holler (:decoder)]

JSBugMon: The testcase found in this bug no longer reproduces (tried revision be81b8d6fae9).

Comment 11

[Jan de Mooij [:jandem]]

Comment on attachment 8623618 [details] [diff] [review]
ApplyType: Prevent re-entry of specialization phases.

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

::: js/src/jit/IonAnalysis.h
@@ +46,5 @@
>  
>  bool
>  EliminateDeadCode(MIRGenerator* mir, MIRGraph& graph);
>  
> +// This state variable is used to prevent optimization which might not be safe

Nit: optimizations

@@ +49,5 @@
>  
> +// This state variable is used to prevent optimization which might not be safe
> +// to re-enter.
> +//
> +// Fox example, UCE can remove branches which cause the Phi specialization to

Nit: causes

@@ +51,5 @@
> +// to re-enter.
> +//
> +// Fox example, UCE can remove branches which cause the Phi specialization to
> +// change from a more specific MIRType_Object to a less specific
> +// MIRType_Value. The last type specialization might have add a MBox after the

Nit: added

@@ +54,5 @@
> +// change from a more specific MIRType_Object to a less specific
> +// MIRType_Value. The last type specialization might have add a MBox after the
> +// Phi, but this MBox is not removed during the second iteration, and after the
> +// re-specialization.
> +enum ApplyTypeState {

You can use an enum class to avoid the ApplyType_ prefix:

enum class ApplyTypeState {
    FirstPass,
    FixupPass
};

Comment 12

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Nicolas, I noticed that this patch was r+'d on 6/24 but it hasn't been checked in anywhere. Could you please follow up?

Comment 13

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Adding a tracking flag for FF41 so we can make sure the fix for this sec-high bugs gets landed in Aurora.

Comment 14

[Nicolas B. Pierron [:nbp]]

(In reply to Ritu Kothari (:ritu) from comment #12)
> Nicolas, I noticed that this patch was r+'d on 6/24 but it hasn't been
> checked in anywhere. Could you please follow up?

Bug 1165348 got backed out, so we should no longer be able to reproduce this issue (see comment 2).
I kept this bug open as I still need to land this fix when I reland Bug 1165348.

Comment 16

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Setting FF41 status to fixed based on comment 14 and removing the tracking flag for 41. Adding a tracking flag for FF42 as Nicolas wants this bug to remain open.

Comment 17

[Nicolas B. Pierron [:nbp]]

(In reply to Ritu Kothari (:ritu) from comment #13)
> Adding a tracking flag for FF41 so we can make sure the fix for this
> sec-high bugs gets landed in Aurora.

The faulty patch got backed out (Bug 1165348 comment 14), so this bug no longer need to be a s-s bug, and we do not have to track any version anymore.

Comment 18

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

(In reply to Nicolas B. Pierron [:nbp] from comment #14)
> I kept this bug open as I still need to land this fix when I reland Bug
> 1165348.

If bug 1165348 is now WONTFIX, should this fix also be WONTFIX'ed?

Comment 19

[Nicolas B. Pierron [:nbp]]

(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #18)
> (In reply to Nicolas B. Pierron [:nbp] from comment #14)
> > I kept this bug open as I still need to land this fix when I reland Bug
> > 1165348.
> 
> If bug 1165348 is now WONTFIX, should this fix also be WONTFIX'ed?

Yes, indeed.  Branch Pruning is the new alternative to moving Scalar Replacement later in the pipeline, which does not require any ApplyType fixes.