950458 - Assertion failure: MIR instruction returned object with unexpected type, at jit/IonMacroAssembler.cpp:1219

Status: VERIFIED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Christian Holler (:decoder)]

The following testcase asserts on mozilla-central revision c049cb230d77 (run with --fuzzing-safe --ion-check-range-analysis --ion-compile-try-catch --ion-eager):


var N = 100;
var T = TypedObject;
var Point = new T.StructType({x: T.uint32, y: T.uint32, z: T.uint32});
var PointArray = Point.array();
var array = new PointArray(N);
for (var i = 0; i < N; i++) {
  array[i].x = i + 0;
  try {} catch(e) {}
}

Comment 1

[Christian Holler (:decoder)]

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

Comment 2

[Jan de Mooij [:jandem]]

Niko, this is failing the new type checks I added in bug 949475.

It looks like we have a MNewDerivedTypedObject with an empty resultTypeSet, but it's returning something.

Comment 3

[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/1b91cf5c8407
user:        Jan de Mooij
date:        Sat Dec 14 14:32:35 2013 +0100
summary:     Bug 949475 - Add some debug-only sanity checks. r=bhackett

This iteration took 346.742 seconds to run.

Comment 4

[Niko Matsakis [:nmatsakis]]

This is related to bug 943852, I believe.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Bug 953373 may be related.

Comment 6

[Jan de Mooij [:jandem]]

Niko, can we get somebody assigned here? This is a very generic assert and fixing it makes it easier for the fuzzers to find other, unrelated bugs.

Comment 7

[Niko Matsakis [:nmatsakis]]

Jan -- yes, pnkfelix and I have been discussing the right fix off and on, I will attempt to tackle soon. I'm leaving the needinfo to keep this on my radar.

Comment 8

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

Okay so I cannot replicate via the command line flags that were originally posted on the bug, but I can if I add |--ion-parallel-compile=off| in addition to |--ion-eager|

I am looking further now.

Comment 9

[Niko Matsakis [:nmatsakis]]

I am able to reproduce using --ion-parallel-compile=off. I have been doing some extensive refactorings and I think I have the ability to do the "right" fix here -- which (I believe) is for us to compute the complete result type set and use that, rather than using the observed type set, which can be incomplete.

Comment 10

[Niko Matsakis [:nmatsakis]]

Actually, so long as the prototypes on type descriptors are user mutable (which they currently are) I think my plan won't work -- if the user mutates the prototype for a type descriptor, this will cause a new TI type object to be used for later instances (since the TI type object is keyed by class/prototype). Would be nicer if the prototypes for type descriptors were immutable, though I don't know if this is in the cards.

I have therefore experimented with various possible fixes, all of which seem to work.

1. Add a type barrier whenever we create a derived typed object. This is unfortunate but perhaps of little consequence in real code -- first, I expect most derived typed objects to be optimized away (and hopefully the barriers would be optimized away too -- I haven't checked yet), but second I would expect that in 'normal' code these type sets will have only a single entry.

2. Do not set resultTypeSet. This solves the problem but (I believe) impedes further optimization because further uses of this derived typed object are not aware of its type. I am conducting further experiments, because I'm not sure I 100% understand how IonBuilder/TI/etc interact. It might be possible to reclaim some of the optimizations by modifying the routine that identifies the TypeRepresentationSet from an MDefinition to have special treatment for a MDerivedTypedObject. This at least recovers optimal behavior for a sequence of accessors like `a.b.c`, but (I believe) it will optimize more poorly in the face of loops or phis.

For the time being, I am inclined to leave in the type barrier until perf measurements suggest it is a problem.

Comment 11

[Niko Matsakis [:nmatsakis]]

OK, I understand better now and can clarify the impact on optimization of removing the resultTypeSet:

As I initially thought, if we do not set the resultTypeSet at all, then we are giving less information to consumers of the derived typed object and this can hinder optimization. I wasn't observing this at first because the routine which goes from a MDefinition* to a TypeRepresentationSet already had an optimization which, if its input was a MNewDerivedTypedObject, extracted the TypeRepresentationSet directly. Otherwise, it would derive the TypeRepresentationSet from the resultTypeSet. This is in fact the optimization I was referring to when I said "it might be possible to reclaim some of the optimizations".

Long story short, I'm inclined to add the type barrier for now.

Comment 12

[Niko Matsakis [:nmatsakis]]

Attachment: Bug950458.diff

1. Consolidate some common code paths
2. Insert a type barrier after creating derived typed objects

Comment 13

[Jan de Mooij [:jandem]]

Comment on attachment 8368809 [details] [diff] [review]
Bug950458.diff

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

Thanks!

::: js/src/jit/IonBuilder.cpp
@@ +6729,5 @@
>      if (!checkTypedObjectIndexInBounds(elemSize, obj, index, &indexAsByteOffset, objTypeReprs))
>          return false;
>  
> +    return pushScalarLoadFromTypedObject(emitted, obj,
> +                                         indexAsByteOffset, elemTypeRepr);

Nit: fits on one line.

Comment 14

[Niko Matsakis [:nmatsakis]]

https://tbpl.mozilla.org/?tree=Try&rev=aa0e7c6b93b9

Comment 15

[Niko Matsakis [:nmatsakis]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1c9290033b33

Comment 16

[Christian Holler (:decoder)]

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

Comment 17

[Carsten Book [:Tomcat]]

landed on central as https://hg.mozilla.org/mozilla-central/rev/1c9290033b33

Comment 18

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.

Comment 19

[Hannes Verschore [:h4writer]]

I assume we want to uplift this? Can you look into creating an aurora fix and ask for approval?

Comment 20

[Jan de Mooij [:jandem]]

TypedObject is only enabled on Nightly I think. Maybe Aurora too but not beta/release.

Comment 21

[Hannes Verschore [:h4writer]]

(In reply to Jan de Mooij [:jandem] from comment #20)
> TypedObject is only enabled on Nightly I think. Maybe Aurora too but not
> beta/release.

According to flags: disabled on FF27/FF28, affected FF29 and fixed FF30. So aurora is still affected.

Comment 22

[Niko Matsakis [:nmatsakis]]

I don't believe aurora is affected -- Typed Objects can only be created in Nightly builds.

Comment 23

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

(In reply to Niko Matsakis [:nmatsakis] from comment #22)
> I don't believe aurora is affected -- Typed Objects can only be created in
> Nightly builds.

Setting flag for aurora builds to disabled.