IonMonkey: Add IonMonkey frame support to FrameRegsIter.

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: nbp, Assigned: nbp)

Tracking

Other Branch
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

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

Updated

5 years ago
Depends on: 744670
(Assignee)

Updated

5 years ago
Assignee: general → nicolas.b.pierron
Status: NEW → ASSIGNED
No longer depends on: 744670
(Assignee)

Updated

5 years ago
Depends on: 745057
(Assignee)

Comment 1

5 years ago
Created attachment 616803 [details] [diff] [review]
Add isConstructing and thisv support to ion frames.
Attachment #616803 - Flags: review?(dvander)
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:
Attachment #616803 - Flags: review?(dvander) → review+
(Assignee)

Comment 3

5 years ago
(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.
Else after return  non-sequitur over-indentation alert.

/be
(Assignee)

Comment 5

5 years ago
Created attachment 617520 [details] [diff] [review]
Add isConstructing and thisObject support to ion frames.

replace thisv & maybeRead by thisObject.
Attachment #616803 - Attachment is obsolete: true
Attachment #617520 - Flags: review?(dvander)
Is the right patch attached? I don't see thisObject. Anyway, carry my r+ over assuming the thisObject exists on the new patch :)
(Assignee)

Comment 7

5 years ago
https://hg.mozilla.org/projects/ionmonkey/rev/f39ac9c99046
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Attachment #617520 - Flags: review?(dvander) → review+
You need to log in before you can comment on or make changes to this bug.