851067 - IonMonkey: Assertion failure: isInt32(), at ./dist/include/js/Value.h:1087

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase asserts on mozilla-central revision b672877ed046 (run with --ion-eager):


function toPrinted(value) {
  value = String(value);
}
String = Array;
toPrinted(123);
evaluate('toPrinted("foo");');

Comment 1

[Christian Holler (:decoder)]

Filing s-s because it seems that int and string are confused. The test doesn't crash though, so it might be that the situation is handled properly anyway.

Comment 2

[Christian Holler (:decoder)]

JSBugMon: Bisection requested, failed due to error (try manually).

Comment 3

[Christian Holler (:decoder)]

Trying this once more with a fix.

Comment 4

[Christian Holler (:decoder)]

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

The first bad revision is:
changeset:   122585:437c955ff06d
user:        Nicolas B. Pierron
date:        Wed Jan 30 07:41:01 2013 -0800
summary:     Bug 796114 - Inline with type-checked arguments. r=h4writer

This iteration took 111.518 seconds to run.

Comment 5

[Christian Holler (:decoder)]

Nicolas, can you take a look based on comment 4? Thanks.

Comment 6

[Nicolas B. Pierron [:nbp]]

This test case looks similar to one that I added recently in the test suite which was also a regression on the same patch.  The previous one was a problem in the TypePolicy, where some instruction can be used with arguments which are not accounted by the type inference which put Ion in an ambiguous state.

Comment 7

[Nicolas B. Pierron [:nbp]]

Attachment: Only inline arrays constructors with a correct input type.

Add a guard to prevent inlining cases.

Strangely I feel like Bug 796114 has open a new class of errors to fuzzers, because this error pre-dates Bug 796114 and this should probably be backported to all Ion branches.

Comment 8

[Nicolas B. Pierron [:nbp]]

(In reply to Nicolas B. Pierron [:nbp] from comment #7)
> Strangely I feel like Bug 796114 has open a new class of errors to fuzzers,
> because this error pre-dates Bug 796114 and this should probably be
> backported to all Ion branches.

I haven't been able to extract a test case showing an original issue, so I will just add a simplified version of the test case with the patch.

Comment 9

[Hannes Verschore [:h4writer]]

Not sure this is the right fix.

It would fix this particular issue, but I think there is a bigger error here. I think we assume all over the place that if TI says it is a MIRType_... and it is a isContstant(), that the type of the constant is MIRType_... That is not correct ?anymore? after your patch.

So from what I understand these errors are because the type we see in IonBuilder (on constant) is not present in the TI information. This can only happen during inlining. Wouldn't it make sense to disable an inline when the inputs of the function we are inlining are not in the subset of observed types of the function? I thought all these issues have a MConstant at the base. 
1) Just checking the type of the MConstant before inlining should work or
2) It would be possible to Box the MConstant when it doesn't correspond with the observed types. That way it would be a MIRType_Value.

Comment 10

[Nicolas B. Pierron [:nbp]]

Attachment: Box non-matching argument types.

Note: doing so also fix bug 852140, because it prevent the original input type to flow into the compare adjust type.

Comment 11

[Hannes Verschore [:h4writer]]

Comment on attachment 726814 [details] [diff] [review]
Box non-matching argument types.

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

With the adjustments talked on IRC.

Comment 12

[Nicolas B. Pierron [:nbp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cf0ec8e2c809

Comment 13

[Christian Holler (:decoder)]

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

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/cf0ec8e2c809

Comment 15

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.