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().

Return a UndefinedValue() when the type of a property access is undefined.

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.

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 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.

Owh I ment bug #715111

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.

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 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.

Fix isn't needed anymore. Looks like it is already fixed in the tree. This patch only contains the tests now.

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.