IonMonkey: Assertion failure: unexpected type, at ../ion/LIR.h:568

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: h4writer, Assigned: h4writer)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

5 years ago
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().
(Assignee)

Comment 1

5 years ago
Created attachment 587278 [details] [diff] [review]
Undefined get property access

Return a UndefinedValue() when the type of a property access is undefined.
Attachment #587278 - Flags: review?(dvander)
(Assignee)

Comment 2

5 years ago
With this patch applied v8 runs using --ion-eager -n.
--ion -n still segment faults
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.
Attachment #587278 - Flags: review?(dvander)
Blocks: 677337
(Assignee)

Comment 4

5 years ago
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.
Attachment #587278 - Attachment is obsolete: true
(Assignee)

Comment 5

5 years ago
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.
Attachment #587779 - Flags: review?(dvander)
(Assignee)

Updated

5 years ago
Depends on: 716743
(Assignee)

Comment 6

5 years ago
Owh I ment bug #715111
Depends on: 715111
No longer depends on: 716743
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.
Attachment #587779 - Flags: review?(dvander)
(Assignee)

Comment 8

5 years ago
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.
Assignee: general → hv1989
Attachment #587779 - Attachment is obsolete: true
Attachment #588765 - Flags: review?(dvander)
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.
Attachment #588765 - Flags: review?(dvander) → review+
(Assignee)

Comment 10

5 years ago
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.
Attachment #588765 - Attachment is obsolete: true
Attachment #590855 - Flags: review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
Whiteboard: checkin on ionmonkey branch
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.
http://hg.mozilla.org/projects/ionmonkey/rev/a6cdb71835b5
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Keywords: checkin-needed
Whiteboard: checkin on ionmonkey branch
You need to log in before you can comment on or make changes to this bug.