959119 - Assertion failure: store->index()->type() == MIRType_Int32, at jit/TypePolicy.cpp:783 or Crash on Heap near [@ EnterIon] with StructType

Status: VERIFIED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Christian Holler (:decoder)]

The following testcase asserts on mozilla-central revision 12d3ba62a599 (run with --fuzzing-safe --ion-eager):


var StructType = TypedObject.StructType;
var uint8 = TypedObject.uint8;
var float32 = TypedObject.float32;
var RgbColor1 = new StructType({r: uint8, g: uint8, b: uint8});
var RgbColor2 = new StructType({r: uint8, g: float32, b: uint8});
var Fade1 = new StructType({from: RgbColor1, to: RgbColor1});
var Fade2 = new StructType({from: RgbColor2, to: RgbColor2});
RgbColor2.prototype.add = function(c) {
  this.g += c;
  this.b += c;
};
var black = new RgbColor1({r: 0, g: 0, b: 0});
var gray = new RgbColor2({r: 129, g: 128, b: 127});
gray.add(1);
var fade1 = new Fade1({from: black, to: gray});
var fade2 = new Fade2(fade1);
fade2.from.add(1);

Comment 1

[Christian Holler (:decoder)]

Opt-crash trace:

Program received signal SIGSEGV, Segmentation fault.
0xf799dd2e in ?? ()
#0  0xf799dd2e in ?? ()
#1  0x081772c6 in EnterIon (data=..., cx=0x9015950) at js/src/jit/Ion.cpp:2259
#2  js::jit::IonCannon (cx=0x9015950, state=...) at js/src/jit/Ion.cpp:2339
#3  0x083ba417 in js::RunScript (cx=0x9015950, state=...) at js/src/vm/Interpreter.cpp:399
#4  0x083bc22c in RunScript (state=..., cx=0x9015950) at js/src/vm/Interpreter.cpp:389
#5  js::Invoke (cx=0x9015950, args=..., construct=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:482
#6  0x083bcc2d in js::Invoke (cx=0x9015950, thisv=..., fval=..., argc=1, argv=0xffffc81c, rval=...) at js/src/vm/Interpreter.cpp:519
#7  0x081868b6 in js::jit::DoCallFallback (cx=0x9015950, frame=0xffffc84c, stub=0x9029690, argc=1, vp=0xffffc80c, res=...) at js/src/jit/BaselineIC.cpp:8097
edx     0x902aa70       151169648
=> 0xf799dd2e:  movss  %xmm1,(%edx,%edx,4)


Marking s-s based on opt-crash.

Comment 2

[Christian Holler (:decoder)]

Attachment: [crash-signature] Machine-readable crash signature

Comment 3

[Christian Holler (:decoder)]

JSBugMon: Cannot process bug: Error: Failed to compile specified revision 12d3ba62a599 (maybe try another?)

Comment 4

[Christian Holler (:decoder)]

JSBugMon: Bisection requested, failed due to error: Error: Failed to compile specified revision 12d3ba62a599 (maybe try another?)

Comment 5

[Niko Matsakis [:nmatsakis]]

I started investigating this. I have not yet reproduced the problem. I have some suspicions as to its cause but I wasn't able to confirm them yet. I'll leave the needinfo flag for the moment.

Comment 6

[Felix S. Klock II [:pnkfelix, :fklock]]

I cannot replicate with my local build of 12d3ba62a599  when run with --fuzzing-safe --ion-eager on my OS X machine.

decoder: can you tell me the configure flags that were used for the build?

(I can try to replicate atop a Linux VM, but that may introduce its own difficulties...)

If there is a reference build with associated debug info I should be downloading instead of trying to rebuild locally, feel free to clue me into where I should find that.

Comment 7

[Christian Holler (:decoder)]

The flags should be --enable-debug --enable-optimize --enable-valgrind --disable-threadsafe and it's a 32 bit Linux build.

Comment 8

[Niko Matsakis [:nmatsakis]]

I'll try again to reproduce given those flags, I have a 32 bit linux system :)

Comment 9

[Niko Matsakis [:nmatsakis]]

I haven't been able to reproduce.

Comment 10

[Niko Matsakis [:nmatsakis]]

However, given the assertion, I have suspicions that this may be related to bug 943852

Comment 11

[Niko Matsakis [:nmatsakis]]

The assertion seems to be that somehow the index was not an int32. By inspection, I cannot find a current code path where this could be the case.

Comment 12

[Christian Holler (:decoder)]

Let's see if JSBugMon can reproduce this on recent m-c.

Comment 13

[Christian Holler (:decoder)]

This still reproduces for me, tested on m-c rev ecf20a2484b6, linux 32-bit build (--enable-debug --enable-optimize --enable-valgrind --disable-threadsafe), with runtime options --ion-eager.

Comment 14

[Niko Matsakis [:nmatsakis]]

Huh! OK, I'll try a clean build.

Comment 15

[Niko Matsakis [:nmatsakis]]

Incidentally, I cannot even *build* with --disable-threadsafe (I get unexplained build errors). Perhaps that is the difference between our setups.

Comment 16

[Niko Matsakis [:nmatsakis]]

OK, I was able to reproduce! Horray.

Comment 17

[Niko Matsakis [:nmatsakis]]

Not sure if this is security sensitive; probably not? The problem is that divisions which are already specialized to int can be "re-specialized" to float32 by the float32 process.

Comment 18

[Niko Matsakis [:nmatsakis]]

Attachment: Bug959119.diff

Avoid specialized to float32 if already specialized to int32. The typed object code generates special purpose MDiv instructions for address arithmetic and so on that later get converted from int32 to float32 by TypeAnalysis, foiling various assertions.

Comment 19

[Niko Matsakis [:nmatsakis]]

(I decided it may be security sensitive after all -- without the assertion, who knows what the generated code would do)

Comment 20

[Jan de Mooij [:jandem]]

Comment on attachment 8373962 [details] [diff] [review]
Bug959119.diff

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

This patch only contains the test?

Comment 21

[Niko Matsakis [:nmatsakis]]

Attachment: Bug959119.diff

This time with code changes (I think...)

Comment 22

[Jan de Mooij [:jandem]]

Comment on attachment 8374155 [details] [diff] [review]
Bug959119.diff

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

Isn't this also a problem for normal int32 divs in JS? Forwarding to bbouvier as he's more familiar with the float32 optimization.

::: js/src/jit/MIR.cpp
@@ +1386,5 @@
>      MDefinition *left = lhs();
>      MDefinition *right = rhs();
>  
> +    if (specialization() == MIRType_Int32
> +        || !left->canProduceFloat32()

Style nit: || goes at the end of the previous line.

Comment 23

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8374155 [details] [diff] [review]
Bug959119.diff

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

::: js/src/jit-test/tests/TypedObject/bug959119.js
@@ +1,3 @@
> +// This test exposed a bug in float32 optimization.
> +// The (inlined and optimized) code for `add()` created
> +// MDiv instructions specialized to integers, which was

I see the assertion indeed occurs in a MDiv node, but this happens in some internal hosted function, if I understand correctly. Could you add a note for that, please?

::: js/src/jit/MIR.cpp
@@ +1385,5 @@
>  {
>      MDefinition *left = lhs();
>      MDefinition *right = rhs();
>  
> +    if (specialization() == MIRType_Int32

Actually, I think that's not the right fix. For instance, adding two constant integers and storing them into a float32 array would imply a conversion with this fix, whereas we could just convert the two constants into floats at compile time and then store the result directly. This is just an example but there is a lot of potential other cases.

Here, CheckUsesAreFloat32Consumers should fail: StoreTypedArrayElement is a Float32 consumer as the typed array has the Float32 type. The problem is that in this case, the *index* is being optimized, not the value we want to store. canConsumeFloat32 should take the input as an argument and check which use is made in the first place. That's a separate issue I will take care of.

I was wondering why it wouldn't happen with regular stores (i.e. not on typed objects). The reason is that in IonBuilder ( http://mxr.mozilla.org/mozilla-central/source/js/src/jit/IonBuilder.cpp#7800 precisely ), we ensure that the id (key) is an integer, so there's no chance this bad case happens. In StoreScalarTypedObjectValue, there is no such coercion, hence the result. As a matter of fact, you can either add the ToInt32 in this function, make StoreTypedArrayElement a MixPolicy< currentPolicy, NoFloatPolicy<0> >, or wait for me to change the consumer checking rule, as you prefer.

Comment 24

[Niko Matsakis [:nmatsakis]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #23)
> Comment on attachment 8374155 [details] [diff] [review]
> Bug959119.diff
> 
> Review of attachment 8374155 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit-test/tests/TypedObject/bug959119.js
> @@ +1,3 @@
> > +// This test exposed a bug in float32 optimization.
> > +// The (inlined and optimized) code for `add()` created
> > +// MDiv instructions specialized to integers, which was
> 
> I see the assertion indeed occurs in a MDiv node, but this happens in some
> internal hosted function, if I understand correctly. Could you add a note
> for that, please?

I will expand the comment, but what you wrote isn't strictly correct. The error occurs when we generate an (optimized) version of the add function -- the access to the fields generates MIR nodes doing address computations, some of which are MDiv nodes. There is no internal hosted function involved.

> Actually, I think that's not the right fix. For instance, adding two
> constant integers and storing them into a float32 array would imply a
> conversion with this fix, whereas we could just convert the two constants
> into floats at compile time and then store the result directly. This is just
> an example but there is a lot of potential other cases.

I considered this, but I thought that in that case the MAdd would not be specialized (yet). Perhaps I am wrong about this. In any case, your fix sounds better, the one I did seemed kind of simplistic.

Comment 25

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Niko Matsakis [:nmatsakis] from comment #24)
> 
> I will expand the comment, but what you wrote isn't strictly correct. The
> error occurs when we generate an (optimized) version of the add function --
> the access to the fields generates MIR nodes doing address computations,
> some of which are MDiv nodes. There is no internal hosted function involved.

You are right. I wrote this part of the comment before finding out the real cause and forgot to update it. I'll take care of the fix.

Comment 26

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Use the index of the use to determine whether a node can consume a float32

Sean, some context: the current canConsumeFloat32() function can be wrong for MInstructions that aren't unary instructions, as it doesn't take into account which operand we consider. For instance, MStoreTypedArray can consume a float32 when the elements node is a Float32Array, but if you've used a float32 as the index, this is wrong! This never happened as there were tricks in IonBuilder to avoid this situation (adds of ToInt32 on the index). This patch adds a MUse argument to canConsumeFloat32 so that we can retrieve the index of the use and thus forbid some uses.

nbp told me that we can directly get the index' use, so it seems useful to have the MUse in the canConsumeFloat32 function. That modifies the API of the debug function isConsistentFloat32Use() as well, but it's for the good. We could directly use the index instead of the MUse but that would makes the API weirder in my opinion, and further modifications might also take advantage of other MUse's fields.

Comment 27

[Christian Holler (:decoder)]

JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/e31629cfa31c
user:        Nicholas D. Matsakis
date:        Mon Nov 11 20:29:53 2013 -0500
summary:     Bug 937391 - Refactor prop/elem optimizations r=jandem

This iteration took 1.803 seconds to run.

Comment 28

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Updated patch

Carrying forward r+ from sstangl.

Updated patch contains a one-liner fix for MSetElementCache (that used the 2nd input (index) instead of the 3rd (value) to check whether the use is legit). Now passes all tests locally on x86 and x64, should be good.

https://hg.mozilla.org/integration/mozilla-inbound/rev/9058ac74ec28

Comment 29

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9058ac74ec28

Comment 30

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.

Comment 31

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8380640 [details] [diff] [review]
Updated patch

This is affecting gecko since bug 888109 landed, but there are no cases so far that haven't been caught by the fuzzers. Asking for approval though, as it might fix correctness issues, which are more difficult to catch.

[Approval Request Comment]
Regression caused by (bug #): 888109
User impact if declined: correctness issues, maybe crashes
Testing completed (on m-c, etc.): m-i and m-c
Risk to taking this patch (and alternatives if risky): low risk
String or IDL/UUID changes made by this patch: N/A

Comment 32

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8380640 [details] [diff] [review]
Updated patch

Will leave the m-r nom in case we want to pick this up as a ride-along, though at this point there is no indication we would be doing a 27.0.2

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8035c44ef2df
https://hg.mozilla.org/releases/mozilla-beta/rev/07e9d1f64d55

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/07e9d1f64d55

Comment 35

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8380640 [details] [diff] [review]
Updated patch

It is in 29 and we are going to release it next week...