1201459 - Crash [@ js::jit::MUnbox::New] or MOZ_CRASH(Given MIRType cannot be unboxed.)

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

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

function f() {
    (x ? Math.fround(0) : x ? a : x) && b;
}
f(Math.fround);

crashes js debug shell on m-c changeset a6786bf8d71d with --fuzzing-safe --no-threads --ion-eager at MOZ_CRASH(Given MIRType cannot be unboxed.)

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 ~/funfuzz/js/compileShell.py -b "--enable-debug --enable-more-deterministic --enable-nspr-build" -r a6786bf8d71d

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

The "good" changeset has the timestamp "20150814012939" and the hash "0760af2a400f9bb1b86449d52bcf41926ab58288".
The "bad" changeset has the timestamp "20150814025439" and the hash "2db399cd414f8ca2e70b41d4c36d50fa0a9a8d87".

Likely regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=0760af2a400f9bb1b86449d52bcf41926ab58288&tochange=2db399cd414f8ca2e70b41d4c36d50fa0a9a8d87

Hannes, is bug 1193112 a likely regressor?

Comment 1

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

Attachment: stack

(lldb) bt 5
* thread #1: tid = 0x43eb50, 0x000000010072984c js-dbg-64-dm-nsprBuild-darwin-a6786bf8d71d`js::jit::MUnbox::New(alloc=<unavailable>, ins=<unavailable>, type=<unavailable>, mode=Infallible) + 172 at MIR.h:4409, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
  * frame #0: 0x000000010072984c js-dbg-64-dm-nsprBuild-darwin-a6786bf8d71d`js::jit::MUnbox::New(alloc=<unavailable>, ins=<unavailable>, type=<unavailable>, mode=Infallible) + 172 at MIR.h:4409
    frame #1: 0x00000001007510f0 js-dbg-64-dm-nsprBuild-darwin-a6786bf8d71d`js::jit::FilterTypeSetPolicy::adjustInputs(this=<unavailable>, alloc=0x00000001028c7020, ins=0x00000001028cb0e0) + 704 at TypePolicy.cpp:1117
    frame #2: 0x000000010059ed9e js-dbg-64-dm-nsprBuild-darwin-a6786bf8d71d`js::jit::ApplyTypeInformation(js::jit::MIRGenerator*, js::jit::MIRGraph&) [inlined] (anonymous namespace)::TypeAnalyzer::adjustInputs(def=0x00000001028cb0e0) + 8 at IonAnalysis.cpp:1110
    frame #3: 0x000000010059ed96 js-dbg-64-dm-nsprBuild-darwin-a6786bf8d71d`js::jit::ApplyTypeInformation(js::jit::MIRGenerator*, js::jit::MIRGraph&) + 1491 at IonAnalysis.cpp:1170
    frame #4: 0x000000010059e7c3 js-dbg-64-dm-nsprBuild-darwin-a6786bf8d71d`js::jit::ApplyTypeInformation(js::jit::MIRGenerator*, js::jit::MIRGraph&) + 103 at IonAnalysis.cpp:1414
(lldb)

Comment 2

[Hannes Verschore [:h4writer]]

Attachment: Adjust type policy.

Comment 3

[Hannes Verschore [:h4writer]]

So I forgot that if there are no uses to an instruction the float32 optimization is executed eagerly. So we need to make sure that we handle that during type analysis. (I didn't consider this case, when trying to find ways how we could have a non float32 before the MFilterTypeSet and float32 after).

This is sub-optimal actually. Do we really want to convert to float32 when there are no uses? In this case it generates an extra instruction LValuetoFloat32. Which without the float32 optimization wouldn't get executed ...

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8657168 [details] [diff] [review]
Adjust type policy.

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

(In reply to Hannes Verschore [:h4writer] from comment #3)
> This is sub-optimal actually. Do we really want to convert to float32 when
> there are no uses? In this case it generates an extra instruction
> LValuetoFloat32. Which without the float32 optimization wouldn't get
> executed ...

Oh no, we definitely don't want to specialize any operation as float32 if it would slow down the operation. In that case, I guess you can just special-case FilterTypeSet::trySpecializeFloat32(), so that we don't specialize if there aren't any uses. Clearing the review flag if that sounds right to you too; otherwise, please re-ask for review :) (the patch looks good, although, with the comment below)

Anyways, if that ToFloat32 is actually not needed, shouldn't that be eliminated during GVN? Or is it because the ToFloat32 operation might be effectful, that we don't eliminate it? (calling toNumber(), etc.) There might be a different issue here...

::: js/src/jit/TypePolicy.cpp
@@ +1090,5 @@
>          ins->replaceOperand(0, BoxAt(alloc, ins, ins->getOperand(0)));
>          return true;
>      }
>  
> +    // Output is a float32, Unbox doesn't cover that.

Shouldn't this block be added after the |if (inputType != MIRType_Value)| block? Otherwise it seems we're adding a ToFloat32 that might not be necessary?

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

Answered in comment 4.

Comment 6

[Hannes Verschore [:h4writer]]

Attachment: Adjust type policy

I thought that "trySpecializeFloat32" was used on the right types. As a result I thought if I disabled any inputs that doesn't have MIRType_Float32 as input, we will only see MIRType_Float32 flow into the type analysis pass of MFilterTypeSet.
That statement is however wrong. It seems that MPhi will report they are MIRType_Float32, as a result MFilterTypeSet will make itself a MIRType_Float32 type. Whereafter the MPhi is brought back to a MIRType_Value type. As a result MFilterTypeSet sees MIRType_Value as input type and MIRType_Float32 as output type.

As a fix I added support for MIRType_Float32 in MFilterTypeSet typepolicy. Though instead to adjust inputs (like with MUnbox). I had to do it afterwards, to make sure the MToFloat32 has the correct resultTypeSet to decide if it needs to get guared or can get removed.

Comment 7

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8658163 [details] [diff] [review]
Adjust type policy

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

Just a question below, pending the review.

::: js/src/jit/MIR.cpp
@@ -2328,5 @@
>  {
> -    MDefinition* in = input();
> -    if (in->type() != MIRType_Float32)
> -        return;
> -

Was that intended?

@@ +2366,5 @@
>      // Do not use Float32 if we can use int32.
>      if (specialization_ == MIRType_Int32)
>          return;
> +    if (specialization_ == MIRType_None)
> +        return;

That's another patch, I think.

::: js/src/jit/TypePolicy.cpp
@@ +1079,5 @@
>      MOZ_ASSERT(ins->numOperands() == 1);
>      MIRType inputType = ins->getOperand(0)->type();
>      MIRType outputType = ins->type();
>  
> +    // Special case when output is a Float32, but input isn't.

Not sure to follow here, to be honest. If the MFilterTypeSet is supposed to behave like a no-op, having the MToFloat32 before or after shouldn't change a thing; and as a matter of fact, it could, and should (for simplicitly) be added for the input, not after the instruction. Am I missing something here?

@@ +1136,5 @@
>      }
>  
> +
> +
> +

nit: trailing space

Comment 8

[Hannes Verschore [:h4writer]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #7)
> Not sure to follow here, to be honest. If the MFilterTypeSet is supposed to
> behave like a no-op, having the MToFloat32 before or after shouldn't change
> a thing; and as a matter of fact, it could, and should (for simplicitly) be
> added for the input, not after the instruction. Am I missing something here?

I complained that the MToFloat32 wasn't GVN'ed. This is caused by the position of the MToFloat32
and the logic in the constructor of MToFloat32. If the MToFloat32 can be an object or symbol it will get guarded and cannot get removed anymore:

Before the filtertypeset
> MFoo                       (typeset: none)
> MToFloat32 foo1            (guard: YES)
> MFilterTypeSet tofloat322  (typeset: int double)

After the filtertypeset
MFoo                         (typeset: none)
MFilterTypeSet foo1          (typeset: int double)
MToFloat32 filtertypeset2    (guard: NO)

As a result in the later case the MToFloat32 and MFilterTypeSet gets deleted. In the before case not.

Comment 9

[Hannes Verschore [:h4writer]]

Comment on attachment 8658163 [details] [diff] [review]
Adjust type policy

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

::: js/src/jit/MIR.cpp
@@ -2328,5 @@
>  {
> -    MDefinition* in = input();
> -    if (in->type() != MIRType_Float32)
> -        return;
> -

Was on purpose, but on second thought I'll indeed leave that part ;)

@@ +2366,5 @@
>      // Do not use Float32 if we can use int32.
>      if (specialization_ == MIRType_Int32)
>          return;
> +    if (specialization_ == MIRType_None)
> +        return;

Correct. Sorry

Comment 10

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8658163 [details] [diff] [review]
Adjust type policy

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

After too much thinking, that seems to be the most reasonable solution. As I don't know all the arcanes of the MIR graph (uses e.g.), I'd really like another pair of eyes to look at this, just to be sure.

::: js/src/jit/MIR.cpp
@@ +614,5 @@
> +    MOZ_ASSERT(dom != nullptr);
> +    MOZ_ASSERT(dom != this);
> +
> +    // Move all uses to new dom. Save the use of the exception.
> +    MUse *except_use = nullptr;

nit: we don't do snake_casing, do we? exceptUse looks fine

::: js/src/jit/MIR.h
@@ +735,5 @@
>      void replaceAllUsesWith(MDefinition* dom);
>  
>      // Like replaceAllUsesWith, but doesn't set UseRemoved on |this|'s operands.
>      void justReplaceAllUsesWith(MDefinition* dom);
> +    void justReplaceAllUsesWithExcept(MDefinition* dom, MDefinition* except);

I'd change the API to just use a single argument, as I can't see any other use cases where dom != except (and we didn't seem to have them in the tree beforehand, as this function didn't even exist).

::: js/src/jit/TypePolicy.cpp
@@ +1094,5 @@
> +        //       equals the original type.
> +        ins->setResultType(ins->resultTypeSet()->getKnownMIRType());
> +
> +        // Do the type analysis
> +        ins->block()->insertAfter(ins, replace);

Can you move up this line, and put it right below the justReplaceAllUsesWithExcept? The setResultType and adjustInputs are more related to types, while the insertAfter is related to the new instruction, as is the justReplace...

@@ +1136,5 @@
>      }
>  
> +
> +
> +

(and by trailing space, I meant "three blank lines")

Comment 11

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8658163 [details] [diff] [review]
Adjust type policy

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

I am not sure I understand this patch, as the MFilterTypeSet should be transparent for the float32 optimization.
Can you explain me why the following line is correct:

    https://dxr.mozilla.org/mozilla-central/source/js/src/jit/MIR.cpp#2321

Why don't we check if the input is a valid flot32 producer instead of checking the MIRType?

Comment 12

[Hannes Verschore [:h4writer]]

(In reply to Nicolas B. Pierron [:nbp] from comment #11)
> Comment on attachment 8658163 [details] [diff] [review]
> Adjust type policy
> 
> Review of attachment 8658163 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I am not sure I understand this patch, as the MFilterTypeSet should be
> transparent for the float32 optimization.

Yes we want it to be transparent for the float32 optimization. The easy case for MFilterTypeSet would be to mimic "type() == MIRType_Float32" or "canProduceFloat32" of the input. Which was what I did. As a result MFilterTypeSet typepolicy doesn't need to worry about MIRType_Any -> MIRType_Float32. Since theoretically only MIRType_Float32 -> MIRType_Float32 can happen.

Though due to decisions made in how the algorithm work, we have a problem. We fake most MPhi to be MIRType_Float32 and canProduceFloat32. Then iterate all instructions and afterwards fix the MPhi's and decide if it actually will use MIRType_Float32.
=> trySpecializeFloat32 decides to use MIRType_Float32 (due to MPhi as input), but during type policy we see MIRType_Value -> MIRType_Float32, since it was decided against typing MPhi MIRType_Float32. As a result we now have to handle NonFloat32 input to MIRType_Float32 in the typepolicy of MFilterTypeSet. And that is what this patch fixes.

> Can you explain me why the following line is correct:
> 
>     https://dxr.mozilla.org/mozilla-central/source/js/src/jit/MIR.cpp#2321
> 
> Why don't we check if the input is a valid float32 producer instead of
> checking the MIRType?

Since testing for float32 producer isn't good enough. Float32 producer is only given to instructions that produce the float32 themself. Not to instructions where after the computation the result is Float32. Given that we iterate in RPO, that means an instruction will have corrected itself w.r.t. output type. As a result it is fine to test if the input is Float32. Secondly we want the MFilterTypeSet to be a NOP. As a result if the input has decided it wants to be Float32, MFilterTypeSet needs to be a Float32 too.
(Currently this distinction plays only a role in the MMinMax trySpecializeFloat32 function.)

Comment 13

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8658163 [details] [diff] [review]
Adjust type policy

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

Add a comment in [1] to mention that the graph can be mutated before and after the current instruction, and thus the iterator should not be incremented before the adjustInput function returns.

[1] https://dxr.mozilla.org/mozilla-central/source/js/src/jit/IonAnalysis.cpp#1169

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9f8526509f5a

Comment 15

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/9f8526509f5a