Last Comment Bug 716624 - IonMonkey: Assertion failure: unexpected type, at ../ion/LIR.h:568
: IonMonkey: Assertion failure: unexpected type, at ../ion/LIR.h:568
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Hannes Verschore [:h4writer]
:
Mentors:
Depends on: 715111
Blocks: 677337
  Show dependency treegraph
 
Reported: 2012-01-09 11:56 PST by Hannes Verschore [:h4writer]
Modified: 2012-01-31 04:30 PST (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Undefined get property access (946 bytes, patch)
2012-01-10 05:28 PST, Hannes Verschore [:h4writer]
no flags Details | Diff | Splinter Review
Undefined get property access (1.85 KB, patch)
2012-01-11 12:29 PST, Hannes Verschore [:h4writer]
no flags Details | Diff | Splinter Review
Undefined get property access (3.79 KB, patch)
2012-01-15 12:35 PST, Hannes Verschore [:h4writer]
dvander: review+
Details | Diff | Splinter Review
Add testcase (2.73 KB, patch)
2012-01-23 13:45 PST, Hannes Verschore [:h4writer]
hv1989: review+
Details | Diff | Splinter Review

Description Hannes Verschore [:h4writer] 2012-01-09 11:56:12 PST
Minimized the v8 deltablue test to find the error:

function getprop (obj) {
  return obj.nonexist;
}

for (var n = 0; n < 100; n++) {
  var a = new Object();
  getprop(a);
}

- 64bit --ion -n
- 32bit --ion -n
- 32bit --ion-eager -n
Assertion failure: unexpected type, at /home/h4writer/Build/ionmonkey/js/src/ion/LIR.h:568

Here an ins with type MIRType_Undefined is provided to define().
Comment 1 Hannes Verschore [:h4writer] 2012-01-10 05:28:52 PST
Created attachment 587278 [details] [diff] [review]
Undefined get property access

Return a UndefinedValue() when the type of a property access is undefined.
Comment 2 Hannes Verschore [:h4writer] 2012-01-10 05:58:24 PST
With this patch applied v8 runs using --ion-eager -n.
--ion -n still segment faults
Comment 3 David Anderson [:dvander] 2012-01-10 17:28:28 PST
Comment on attachment 587278 [details] [diff] [review]
Undefined get property access

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

::: js/src/ion/Lowering.cpp
@@ +855,1 @@
>      }

Something is fishy here -- we should have either inserted an MConstant(Undefined) in the MIR, or added a type barrier. The problem is we could be eliminating a call to an effectful GETPROP.

I'd do this instead. When we create an MGetPropertyCache node, we should set its type to None if it's a Null or Undefined (and subsequently not perform any loads in the generated code). pushTypeBarrier will automatically insert an MConstant for you. You'll need to modify the code around |if (ins->type() == MIRType_Value)| to make sure it ignores MIRType_None.
Comment 4 Hannes Verschore [:h4writer] 2012-01-11 12:29:24 PST
Created attachment 587779 [details] [diff] [review]
Undefined get property access

Do you mean like this?
While testing I still noticed a bug in handling of bailouts?:

on ./js --ion-eager -n
(it is possible to create the same bug in --ion, but with more code.)

function test(o) {
    return o.value
}

var w = {value: null}
print(test(w)); // 1
var w = {value: 0}
print(test(w)); // 2

Expected result:
null
undefined

Result:
null
2.1219957287e-314


So the function "test" gets compiled.
The bugs only surfaces when pushTypeBarrier (in jsop_getprop) is called with certain arguments 
1) test get's compiled as observed=JSVAL_TYPE_UNKOWN and actual=JSVAL_TYPE_UNKNOWN // not important
2) test get's compiled as observed=JSVAL_TYPE_NULL and actual=JSVAL_TYPE_NULL

So the second time the function is compiled for MIRType_NULL and we run it with a non-null value (undefined, integer, ...).
The typeBarrier bails because another type is provided. But I assume the state isn't restored properly and therefor that strange number appears instead of undefined.
Though I didn't succeeded in finding what is going wrong precisely.
Comment 5 Hannes Verschore [:h4writer] 2012-01-12 10:39:19 PST
Comment on attachment 587779 [details] [diff] [review]
Undefined get property access

After further investigation I think this is a bug is caused by bug #716743 (x86 in this case)
GetPropertyCache adds the type "undefined" to the observed types. This triggers an invalidatation of the script (requested by TI). The registers are clobbered by the VM call. So we bail to the interpreter and get that strange result (2.1219957287e-314). When I disable the invalidation, the result is fine.
Comment 6 Hannes Verschore [:h4writer] 2012-01-12 13:23:39 PST
Owh I ment bug #715111
Comment 7 David Anderson [:dvander] 2012-01-13 12:02:05 PST
Comment on attachment 587779 [details] [diff] [review]
Undefined get property access

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

::: js/src/ion/IonBuilder.cpp
@@ +3129,5 @@
>      TypeOracle::Unary unary = oracle->unaryOp(script, pc);
>      if (unary.ival == MIRType_Object) {
>          MGetPropertyCache *load = MGetPropertyCache::New(obj, atom, script, pc);
> +        if (!barrier) {
> +            if (unary.rval == MIRType_Undefined || unary.rval == MIRType_Null)

A short comment above here would be good :)

::: js/src/ion/Lowering.cpp
@@ +847,5 @@
>          if (!defineBox(lir, ins))
>              return false;
>          return assignSafepoint(lir, ins);
> +    } else if (ins->type() == MIRType_None) {
> +        return true; 

We still have to generate the LGetPropertyCacheV because it could have side effects. For example:

> var o = { y: 1 };
> Object.defineProperty(o, "x", { get: function () { this.y++; return null; } });

In this case, o.x always returns null but we have to execute the getter each time. The trick is to generate LGetPropertyCacheT and after generating each shape guard, where we'd normally emit a load, we just don't have to emit anything.
Comment 8 Hannes Verschore [:h4writer] 2012-01-15 12:35:42 PST
Created attachment 588765 [details] [diff] [review]
Undefined get property access

Third time lucky :P.

Previously I was trying to create an increased performance patch.
Now this is only a correctness patch.

I noticed to implement the improvement patch correctly I had to create an extra lir (to set there are no defines) and afterward adjust the OOL to ignore the result.
And actually this will probably not or have neglectable performance increase (because it only happens on undefined and null, something that doesn't happen often ).
And I think it doesn't outweight the increased complexity (atm).
So that's the reasoning after the change of scope. 

If you think it would indeed increase performance, I could still do the performance patch.
Comment 9 David Anderson [:dvander] 2012-01-17 14:26:37 PST
Comment on attachment 588765 [details] [diff] [review]
Undefined get property access

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

Thanks for the test cases! re: not baking in the constants, seems totally reasonable.

::: js/src/ion/IonBuilder.cpp
@@ +3102,5 @@
>      if (unary.ival == MIRType_Object) {
>          MGetPropertyCache *load = MGetPropertyCache::New(obj, atom, script, pc);
> +        if (!barrier) {
> +            // Use the default type (Value) when the output type is undefined or null.
> +            // Because specializing to those types isn't possible.

I would write a short explanation of why it's not possible.
Comment 10 Hannes Verschore [:h4writer] 2012-01-23 13:45:21 PST
Created attachment 590855 [details] [diff] [review]
Add testcase

Fix isn't needed anymore. Looks like it is already fixed in the tree. 
This patch only contains the tests now.
Comment 11 David Anderson [:dvander] 2012-01-30 18:15:03 PST
This still fails with a slightly different test case:

function getprop (obj) {
  return obj.nonexist;
}

for (var n = 0; n < 100; n++) {
  var a = (n % 2) ? ((n % 3) ? new Object() : new Object()) : new Object();
  getprop(a);
}

So I think the original patch is still relevant.
Comment 12 David Anderson [:dvander] 2012-01-30 18:24:52 PST
http://hg.mozilla.org/projects/ionmonkey/rev/a6cdb71835b5

Note You need to log in before you can comment on or make changes to this bug.