849781 - IonMonkey: Assertion failure: obj->isTypedArray(), at jstypedarrayinlines.h:80

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

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


function assertArraysFirstEqual(a, b) {
    if (a.length != b.length) {}
}
function check(b) {
    var a = deserialize(serialize(b));
    assertArraysFirstEqual(a, b);
}
check(new Int8Array(1));
evaluate("check(['a', 'b']);");

Comment 1

[Christian Holler (:decoder)]

Not sure if this could lead to some sort of data structure/type confusion, marking s-s to be safe.

Comment 2

[Christian Holler (:decoder)]

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

Comment 3

[Christian Holler (:decoder)]

JSBugMon: Bisection requested, result:
Due to skipped revisions, the first bad revision could be any of:
changeset:   122584:b831500ca4be
user:        David Anderson
date:        Thu Feb 21 13:52:09 2013 -0800
summary:     Prevent GC from occuring during IC linking (bug 837714, r=bhackett).

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

changeset:   122586:5054f997ef77
user:        Gregory Szorc
date:        Thu Feb 21 14:11:54 2013 -0800
summary:     Bug 841074 - Statically declare fields on FHR measurements; r=rnewman

changeset:   122587:6c126d076b0d
user:        Phil Ringnalda
date:        Thu Feb 21 14:26:04 2013 -0800
summary:     Back out b831500ca4be (bug 837714) for bustage

This iteration took 15.084 seconds to run.

Comment 4

[Christian Holler (:decoder)]

David, Nicolas, can you take a look which bug from the bisect in comment 3 is the right one?

Comment 5

[Hannes Verschore [:h4writer]]

Again type mismatch and doesn't trigger when --ion-inlining=off. Therefore supposedly Bug 796114.

Comment 6

[Hannes Verschore [:h4writer]]

Attachment: Make sure caller arguments are subset of callee arguments

The fix in bug 851067 doesn't cover this testcase. (type is MIRType_Object for caller and callee, but at the caller side it is TypedArray and not at the callee side)

Therefore I just went ahead and added code to disable inlining if the types didn't match. I already had this code for JSOP_FUNAPPLY. Now I enable it for all calls. This patch is mostly refactoring to have a better system.

Comment 7

[Nicolas B. Pierron [:nbp]]

Comment on attachment 727138 [details] [diff] [review]
Make sure caller arguments are subset of callee arguments

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

I like your approach, as this would not inline function unless they have already been monitored with such arguments.
Still I feel bad that we are recovering arguments typeset from multiple locations, we should only do that in CallInfo class.

::: js/src/ion/IonBuilder.cpp
@@ -3063,5 @@
> -
> -        // Non-matching types are boxed such as the MIRType does not conflict
> -        // with the inferred type.
> -        if (callerObs->getKnownTypeTag() != calleeObs->getKnownTypeTag() &&
> -            ins->type() != MIRType_Value)

nit: assert that known types are matching.

::: js/src/ion/TypeOracle.cpp
@@ +664,5 @@
>  
> +    size_t nargs = Min<size_t>(callee->nargs, argc);
> +    for (size_t i = 0; i < nargs; i++) {
> +        calleeType = types::TypeScript::ArgTypes(targetScript, i);
> +        callerType = getCallArg(callerScript, argc, i+1, pc);

This information should also be extracted for constructing the CallInfo structure.  Can't we construct the CallInfo before doing any of these and get this from the callInfo instead of asking again the oracle?

We would probably have to move the CallInfo construction before the inlining decision, and this should be safe as the callInfo is needed for inlining. (see initCallType & initFunApplyArguments)

@@ +666,5 @@
> +    for (size_t i = 0; i < nargs; i++) {
> +        calleeType = types::TypeScript::ArgTypes(targetScript, i);
> +        callerType = getCallArg(callerScript, argc, i+1, pc);
> +
> +        if (!callerType->isSubset(calleeType))

Thanks for adding isSubset function, I wish I knew about it before.

Comment 8

[Hannes Verschore [:h4writer]]

Good feedback, I was forgotten I wanted to do that all along. But looks like a lot to do in a security bug. I think we first should land this and file a follow up bug. I will normally incorporate this in bug 837312...

Comment 9

[Nicolas B. Pierron [:nbp]]

(In reply to Hannes Verschore [:h4writer] from comment #8)
> Good feedback, I was forgotten I wanted to do that all along. But looks like
> a lot to do in a security bug. I think we first should land this and file a
> follow up bug. I will normally incorporate this in bug 837312...

The main reason I mentioned that is because I think there might be a security issue in your fix or in the current code, as you are not getting arguments the same way as done in CallInfo.

Comment 10

[Hannes Verschore [:h4writer]]

Attachment: Make sure caller arguments are subset of callee arguments

The initCallType or initfunapply happens already before doing makeInliningDecisions. I use the types from callInfo now, but I didn't put callInfo as an argument in TypeOracle, since I thought dvander wanted it to be local in IonBuilder only.

Comment 11

[Nicolas B. Pierron [:nbp]]

Comment on attachment 727616 [details] [diff] [review]
Make sure caller arguments are subset of callee arguments

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

The current patch disable the inlining of any call-site with an excluded type.  We still want to inline functions where the given argument has an unstable type (when inferred by TI), but is still stable when used in practice.

::: js/src/ion/IonBuilder.cpp
@@ -3062,5 @@
>          current->add(barrier);
> -
> -        // Non-matching types are boxed such as the MIRType does not conflict
> -        // with the inferred type.
> -        if (callerObs->getKnownTypeTag() != calleeObs->getKnownTypeTag() &&

I guess you should still need this barrier for cases where the type vary.  such as:

  var arr = ["", {}, "", {}, "", …];
  for (var i = 0; i < arr.length; i++) {
    var x = arr[i];
    if (i % 2)
      foo(x)
    else
      bar(x)
  }

and make sure foo & bar are inlined.

::: js/src/ion/TypeOracle.cpp
@@ +657,5 @@
>  }
>  
> +bool
> +TypeInferenceOracle::callArgsTypeSetMatches(types::StackTypeSet *thisType, Vector<types::StackTypeSet *> &argvType, RawFunction callee)
> +{

This functions check that all callers types are subset of all callee types.  Doing so does will not inline cases where we have excluded types which have a non-empty intersection.

@@ +666,5 @@
> +
> +    // This
> +    if (nargs > 0) {
> +        calleeType = types::TypeScript::ArgTypes(targetScript, 0);
> +        if (!thisType->isSubset(calleeType))

I think you want to check something like:

  thisType->hasCommonTypesWith(calleeType)

Comment 12

[Hannes Verschore [:h4writer]]

Attachment: Make sure caller arguments and callee arguments intersect

When fixing this, I found multiple issues. This patch now has the same performance as before, maybe even a little improvement over v8.

* added "callArgsTypeSetIntersects"
* fixed issues in "callArgsTypeSetMatches", TypeScript::ArgTypes starts at 0 with args
* fixed issues in "initFunApplyArguments", thisTypes and argumentTypes were plain wrong
* fixed issue in "initCallType", thisType was plain wrong
* removed addTypeBarrier for constructing calls. The |this| type is still old one.
* made sure canInlineTarget doesn't test |this| type for constructing calls

Comment 13

[Hannes Verschore [:h4writer]]

Comment on attachment 728301 [details] [diff] [review]
Make sure caller arguments and callee arguments intersect

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

::: js/src/ion/IonBuilder.cpp
@@ -3068,5 @@
> -        {
> -            MBox *box = MBox::New(ins);
> -            current->add(box);
> -            ins = box;
> -        }

I know you still had a comment about this. I'll check it this weekend and if it is an issue I won't remove this and add the testcase. I still offered this review, because now you can review it already tonight if you find time. That way I maybe can finish this in the weekend.

Comment 14

[Hannes Verschore [:h4writer]]

(In reply to Hannes Verschore [:h4writer] from comment #13)
> Comment on attachment 728301 [details] [diff] [review]
> Make sure caller arguments and callee arguments intersect
> 
> Review of attachment 728301 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/ion/IonBuilder.cpp
> @@ -3068,5 @@
> > -        {
> > -            MBox *box = MBox::New(ins);
> > -            current->add(box);
> > -            ins = box;
> > -        }
> 
> I know you still had a comment about this. I'll check it this weekend and if
> it is an issue I won't remove this and add the testcase. I still offered
> this review, because now you can review it already tonight if you find time.
> That way I maybe can finish this in the weekend.

I tried to get a testcase asserting, but failed. And also thought a long time about it, and I think these lines can indeed get removed.

Currently only the following is possible:

Caller type / Callee type
-----------------------------------
1) MIRType_Specific / MIRType_Specific (when the caller sees the same type as callee)
2) MIRType_Value / MIRType_Specific (when the caller sees more types than callee)
3) MIRType_Specific / MIRType_Value (when caller sees less types than callee)
4) MIRType_Value / MIRType_Value

1) Is not an issue, since the types intersect eachother, the specific type should be the same.
2) There never was a problem in this case. I.e value is already boxed.
3) Our type system will solve this and box it
4) Not an issue.

I think the problems we had, were because there was no intersection of the caller and callee types while inlining...

Comment 15

[Nicolas B. Pierron [:nbp]]

Comment on attachment 728301 [details] [diff] [review]
Make sure caller arguments and callee arguments intersect

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

Can you clarify the following details for me, and ask again for review if I am wrong or once fixed.

::: js/src/ion/IonBuilder.cpp
@@ +235,5 @@
> +            return false;
> +        }
> +    } else if (JSOp(*pc) == JSOP_FUNAPPLY) {
> +        // For fun.apply() the typeset of the caller should be a subset of the callee,
> +        // since we omitted a call and when bailing for a new type that call won't see that type.

I am not sure to fully understand this case, can you provide an example?

::: js/src/ion/IonBuilder.h
@@ +635,2 @@
>          argsBarriers_ = oracle->callArgsBarrier(script, pc);
> +        thisType_ = oracle->getCallArg(script, 2, 1, pc);

???

Fun, This, LazyArguments
 -1,   0 ,  1

@@ +637,4 @@
>          if (!argsType_.reserve(nargs))
>              return false;
>          for (uint32_t i = 0; i < nargs; i++)
> +            argsType_.infallibleAppend(oracle->getCallArg(parent, nargs, i + 1, parentPc));

getCallArg is only true for Calls, not for JSOP_FUNAPPLY, which is a call with only 3 arguments, where the first one is the function, the new this and the lazy argument object.

getCallArgs returns the popped types which is not the same thing at all. You just cannot use this one here.

Comment 16

[Hannes Verschore [:h4writer]]

Comment on attachment 728301 [details] [diff] [review]
Make sure caller arguments and callee arguments intersect

Question 1:

JSOP_FUNAPPLY does the following:

Some kind of callstack:
1) fun(arg1, arg2, arg3)
2) inlined.apply(this1, arguments);
3) inlined(arg1, arg2, arg3)

With inlining we remove the native call, i.e. the apply:

1) fun(arg1, arg2, arg3)
2) inlined(arg1, arg2, arg3)

If the 1_arg1 has an extra type, there will be a problem.
Using "argsBarriers" in CallInfo will never do addTypeBarrier, since it sees only |target|, |this| and |arguments array|. I.e. the types are correct. But that means we only see the type when it is observed in the inlined call. Than we will bail, but TI will report an error, because the new type wasn't observed at the start of the inlined() function, but only in the middle.

Example that will assert in debug build (is in the testsuite)
function dumpArgs6(i) { 
  if (i == 90)
    return funapply6.arguments.length;
  return [i];
}
function funapply6() {
  return dumpArgs6.apply({}, arguments);
}                                                                                                     
function test6(i) {
  return funapply6(i,1,2,3);
}
test6(89)[0]
test6(0.2)

As a result we don't want to see new types in the inlined call and I added the extra rule that we only inline a funapply when the caller typeset is a subset of the callee typeset. (Another solution would be to compute the correct argsBarriers information. But I don't know how hard that is, if possible)

Question 2:

For fun.apply we take the actual arguments in the CallInfo class.
So the CallInfo of the callstack I gave will be:
thisArg: this1
argv[0]: arg1
argv[1]: arg2
argv[2]: arg3

NOT:
thisArg: fun.this
argv[0]: this1
argv[1]: arguments vector

That also means the types should be of the actual function:

fun.apply(this1, arguments);
Fun, This of FUN, this1,  LazyArguments
 -1,       0,       1,         2

=> thisType_ = oracle->getCallArg(script, 2, 1, pc);

Question 3:

Here again. We need the types of the inlined arguments, not the real arguments to fun.apply(). That's why we get them from the call arguments of the caller.

function parent() {
  fun(1,2,3); <------------ HERE
}
function fun() {
  inlined.apply(this, arguments);
}
function inlined(a,b,c) {

}

=> argsType_.infallibleAppend(oracle->getCallArg(parent, nargs, i + 1, parentPc));

Comment 17

[Nicolas B. Pierron [:nbp]]

(In reply to Hannes Verschore [:h4writer] from comment #16)
> Comment on attachment 728301 [details] [diff] [review]
> Make sure caller arguments and callee arguments intersect
> 
> Question 1:
> 
> […]
> 
> As a result we don't want to see new types in the inlined call and I added
> the extra rule that we only inline a funapply when the caller typeset is a
> subset of the callee typeset. (Another solution would be to compute the
> correct argsBarriers information. But I don't know how hard that is, if
> possible)

Ok, so the problem is that TI does not monitor type barriers at the call boundary of fun.apply, and I guess this might be the same for fun.call (or not).  So if we were to bailout at these locations we should resume before the call such as the interpreter can monitor the unexpected inputs, and thus without invalidating at the time of the bailout.

> Question 2:

Ok.

> Question 3:
> 
> Here again. We need the types of the inlined arguments, not the real
> arguments to fun.apply(). That's why we get them from the call arguments of
> the caller.
> 
> function parent() {
>   fun(1,2,3); <------------ HERE
> }
> function fun() {
>   inlined.apply(this, arguments);
> }
> function inlined(a,b,c) {
> 
> }
> 
> => argsType_.infallibleAppend(oracle->getCallArg(parent, nargs, i + 1,
> parentPc));

Ok,

I was just taking the formals of the current function instead of taking the actual of the caller of the function inlining fun.apply.

I got confused by getCallArg, but you you it with the caller pc.  Ideally you should use the oracle of the caller² instead of the oracle the caller¹ which contains the fun.apply.

Comment 18

[Nicolas B. Pierron [:nbp]]

Comment on attachment 728301 [details] [diff] [review]
Make sure caller arguments and callee arguments intersect

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

All points have been addressed in Bugzilla comments.

Might be worth adding similar comments into the code where the code might be confusing otherwise.  Such as uses of getCallArg for fun apply arguments.

::: js/src/ion/IonBuilder.cpp
@@ +4033,5 @@
>      RootedScript scriptRoot(cx, script());
> +    RootedScript parentScriptRoot(cx, callerBuilder_->script());
> +    if (!callInfo.initFunApplyArguments(oracle, scriptRoot, pc,
> +                                        parentScriptRoot, callerBuilder_->pc,
> +                                        inlinedArguments_.length()))

note: if the caller² does not have any formal arguments, then the problem becomes identical to when we were using getParameter on caller¹.

function c0(i) { … }
function c1() { c0.apply(this, arguments); }
function c2() { c1.apply(this, arguments); }
c2(1);
c2("");

This optimization is only valid if we know the non-funapply caller, which here is assumed to be the direct caller.

I guess it would be best to capture such state as part of the inlinedArguments or aside to it and use them directly here. Even keeping a CallInfo instead of the inlinedArguments might be worth looking into.

Comment 19

[Hannes Verschore [:h4writer]]

Thanks for testcase. I forgot that corner-case. In that case we didn't inline, because we got (wrongly) the TypeSet LazyArgs. I fixed it by keeping the types, just like the inlinedArguments. 

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

Comment 20

[Ryan VanderMeulen [:RyanVM]]

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

Comment 21

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.