Closed
Bug 744253
Opened 12 years ago
Closed 12 years ago
IonMonkey: Add IonMonkey frame support to FrameRegsIter.
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: nbp, Assigned: nbp)
References
Details
Attachments
(1 file, 1 obsolete file)
19.01 KB,
patch
|
dvander
:
review+
|
Details | Diff | Splinter Review |
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•12 years ago
|
Assignee | ||
Comment 1•12 years ago
|
||
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•12 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.
Comment 4•12 years ago
|
||
Else after return non-sequitur over-indentation alert. /be
Assignee | ||
Comment 5•12 years ago
|
||
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•12 years ago
|
||
https://hg.mozilla.org/projects/ionmonkey/rev/f39ac9c99046
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Updated•12 years ago
|
Attachment #617520 -
Flags: review?(dvander) → review+
You need to log in
before you can comment on or make changes to this bug.
Description
•