744253 - IonMonkey: Add IonMonkey frame support to FrameRegsIter.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicolas B. Pierron [:nbp]]

The following test case (jit-test/tests/jaeger/propertyOptimize-4.js with --ion -n --ion-eager) does not call the setter on 'y', added when '0 + a' is executed, because TypeObject::clearNewScript(JSContext *cx) does not know how to modify the 'this' of Ion frames.

function Foo(a) {
    var x = 0 + a;
    this.y = 1;
}

var updated = false;
var o = {
  valueOf: function() {
      print('defineProperty');
      Object.defineProperty(
          Object.prototype,
          'y',
          { set: function() {
                updated = true;
            }
          }
      );
      print('defineProperty end');
      return 0;
  }
};

var z = new Foo(o);
assertEq(updated, true);

Comment 1

[Nicolas B. Pierron [:nbp]]

Attachment: Add isConstructing and thisv support to ion frames.

Comment 2

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 616803 [details] [diff] [review]
Add isConstructing and thisv support to ion frames.

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

r=me with the thisv thing addressed

::: js/src/ion/IonFrameIterator.h
@@ +112,5 @@
>          return type_ == IonFrame_Entry;
>      }
>      bool isFunctionFrame() const;
>  
> +    bool isConstructing(IonActivation *activation) const;

It's a little gross to have to know the IonActivation to call this function. Ideally we'd require knowing it as part of constructing an IonFrameIterator, and we could cache it locally.

::: js/src/ion/IonFrames.cpp
@@ +762,5 @@
> +
> +        return (JSOp)*inlinedParent.pc() == JSOP_NEW;
> +    } else {
> +        JS_ASSERT(parent.done());
> +        return activation->entryfp()->isConstructing();

Global scripts cannot be constructing.

@@ +772,5 @@
> +{
> +    // JS_ASSERT(isConstructing(...));
> +    SnapshotIterator s(si_);
> +    s.skip(); // scopeChain
> +    return s.read(); // this

We have to be a little careful here. In strict-mode, |this| can be a non-gcthing, which means it won't be saved in safepoint regs.

If there are callers of thisv() that need a value for correctness, *and* they need to know about strict-mode non-object |this|, we'll need a special fix. Otherwise, it's good enough to just detect this case and return UndefinedValue() for now.

::: js/src/vm/Stack.h
@@ +1891,5 @@
>      bool isEvalFrame() const;
>      bool isNonEvalFunctionFrame() const;
>      bool isConstructing() const;
>  
> +    // TODO: Add && !isIon() in JS_ASSERT of fp() and sp().

No :TODO:

Comment 3

[Nicolas B. Pierron [:nbp]]

(In reply to David Anderson [:dvander] from comment #2)
> ::: js/src/ion/IonFrames.cpp
> @@ +762,5 @@
> > +
> > +        return (JSOp)*inlinedParent.pc() == JSOP_NEW;
> > +    } else {
> > +        JS_ASSERT(parent.done());
> > +        return activation->entryfp()->isConstructing();
> 
> Global scripts cannot be constructing.

You can have multiple activations, and the first activation is not necessary starting at the global script.

Comment 4

[Brendan Eich [:brendan]]

Else after return  non-sequitur over-indentation alert.

/be

Comment 5

[Nicolas B. Pierron [:nbp]]

Attachment: Add isConstructing and thisObject support to ion frames.

replace thisv & maybeRead by thisObject.

Comment 6

[David Anderson [:dvander] - inactive, e-mail if emergency]

Is the right patch attached? I don't see thisObject. Anyway, carry my r+ over assuming the thisObject exists on the new patch :)

Comment 7

[Nicolas B. Pierron [:nbp]]

https://hg.mozilla.org/projects/ionmonkey/rev/f39ac9c99046