Last Comment Bug 744253 - IonMonkey: Add IonMonkey frame support to FrameRegsIter.
: IonMonkey: Add IonMonkey frame support to FrameRegsIter.
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: All All
: -- normal (vote)
: ---
Assigned To: Nicolas B. Pierron [:nbp]
:
Mentors:
Depends on: 745057
Blocks: IonEager
  Show dependency treegraph
 
Reported: 2012-04-10 16:32 PDT by Nicolas B. Pierron [:nbp]
Modified: 2012-05-07 14:35 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Add isConstructing and thisv support to ion frames. (14.39 KB, patch)
2012-04-19 16:30 PDT, Nicolas B. Pierron [:nbp]
dvander: review+
Details | Diff | Review
Add isConstructing and thisObject support to ion frames. (19.01 KB, patch)
2012-04-23 10:17 PDT, Nicolas B. Pierron [:nbp]
dvander: review+
Details | Diff | Review

Description Nicolas B. Pierron [:nbp] 2012-04-10 16:32:38 PDT
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] 2012-04-19 16:30:12 PDT
Created attachment 616803 [details] [diff] [review]
Add isConstructing and thisv support to ion frames.
Comment 2 David Anderson [:dvander] 2012-04-19 17:30:55 PDT
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] 2012-04-19 19:47:45 PDT
(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] 2012-04-19 19:53:04 PDT
Else after return  non-sequitur over-indentation alert.

/be
Comment 5 Nicolas B. Pierron [:nbp] 2012-04-23 10:17:46 PDT
Created attachment 617520 [details] [diff] [review]
Add isConstructing and thisObject support to ion frames.

replace thisv & maybeRead by thisObject.
Comment 6 David Anderson [:dvander] 2012-04-23 21:56:50 PDT
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] 2012-05-01 18:04:19 PDT
https://hg.mozilla.org/projects/ionmonkey/rev/f39ac9c99046

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