839315 - IonMonkey: Assertion failure: def->getOperand(1)->type() == MIRType_String, at ion/TypePolicy.cpp:155

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

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


try {
function f(x) {
    switch(x) {
    case 0:
    case C1:
    }
}
f(uneval(this));
} catch(exc1) {}
evaluate('f({})', { noScriptRval : true });

Comment 1

[Christian Holler (:decoder)]

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

The first bad revision is:
changeset:   120569:f3b3be0822c4
user:        Hannes Verschore
date:        Fri Feb 01 11:39:02 2013 +0100
summary:     Bug 828119: IonMonkey: Add fastpath for strict string comparison, r=jandem

This iteration took 98.039 seconds to run.

Comment 2

[Christian Holler (:decoder)]

Ccing Hannes based on comment 1.

Comment 3

[Hannes Verschore [:h4writer]]

Looks like there isn't a fault in my code, but getting wrong types from the oracle.
Since bug 825599 we add type information to the compare operation in JSOP_CONDSWITCH. Therefore adding Nicolas, but probably it goes further back than that and is a fault in TI, therefore adding Brian

The first time we do the condswitch the compare is  MIRType_String === MIRType_Int32.
The second time it should be MIRType_Value (as object is added) === MIRType_Int32, but it is again MIRType_String === MIRType_Int32, therefore we get the assert, because we assumed the type was MIRType_String, but is actually MIRType_Object ...

@Nicalas, will you look into this? Next week I'm on holiday ... Afterwards I can take it back if needed.

Better testcase:

function f(x) {
    switch(x) {
      case 0:
         // This compare get's inferred
      case 100: 
         // Make sure we have condswitch and not tableswitch. 
         // FYI we don't generate (compare) code for this "case" 
    }
}

// String === Int32
f("");

// Object === Int32
// FYI: haven't been able to reproduce without evaluate nor noScriptRval:true. 
evaluate('f({})', { noScriptRval : true });

Comment 4

[Hannes Verschore [:h4writer]]

To remove all blame of bug 828119, here a similar testcase that will also fail before the bisected revision. Bisecting this testcase will give you the mentioned revision where bug 825599 landed:

function f(x) {
    switch(x) {
      case 0.1:
      case 100:
    }
}

f(false);
evaluate('f({})', { noScriptRval : true });

Comment 5

[Hannes Verschore [:h4writer]]

FYI I think this is an inlining problem. TI still has the old types "Boolean === Double", while due to inlining we have already newer information, because we known x will be "Object"

We specialize to Compare_Boolean, but the types are of the MIRs are "Object === Double"

Comment 6

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

(In reply to Hannes Verschore [:h4writer] from comment #4)
> To remove all blame of bug 828119, here a similar testcase that will also
> fail before the bisected revision. Bisecting this testcase will give you the
> mentioned revision where bug 825599 landed:
> 
> function f(x) {
>     switch(x) {
>       case 0.1:
>       case 100:
>     }
> }
> 
> f(false);
> evaluate('f({})', { noScriptRval : true });

Using this testcase:

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   117701:e6e5fca3acb3
user:        Nicolas B. Pierron
date:        Thu Jan 03 09:57:41 2013 -0800
summary:     Bug 825599 - Specialize type of switch's conditions. r=h4writer

Comment 7

[Hannes Verschore [:h4writer]]

Attachment: Patch

Bug 766218 is causing the regression. That is FF18+. It is caused by a bug in the TypeOracle. We look at TI to decide which compare optimization to do. In case of CompareType_Boolean, we know the rhs is a Boolean due to TI information. BUT IM can have newer/better information. In this case a NewObject is passed to the rhs. Therefore in that case we should box and unbox it to have correctly running IM code.

NOTE: before and after this patch this code always bails. And we will never execute that generated code. But now the asserts are gone AND the IM code is actually correct...

Comment 8

[Hannes Verschore [:h4writer]]

Attachment: Right patch

Oops, attached the wrong patch

Comment 9

[Jan de Mooij [:jandem]]

Comment on attachment 715155 [details] [diff] [review]
Right patch

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

Good catch. Please also add a testcase, r=me with that.

Comment 10

[Hannes Verschore [:h4writer]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/95e67683fe4e

Comment 11

[Gregory Szorc [:gps]]

https://hg.mozilla.org/mozilla-central/rev/95e67683fe4e