755979 - IonMonkey: Use actual arguments for bailouts.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicolas B. Pierron [:nbp]]

Fixing the enterJIT to accept underflow of arguments and overflow of arguments implies that we have to adapt the bailouts to recover the actual number of arguments given to the functions.

Most of the logic of bailouts is duplicated in IonFrameIterator to handle StackIter properly. So, before implementing any modification to the Bailouts, it would be better to continue the replacement of FrameRecovery by IonFrameIterator.

Comment 1

[Nicolas B. Pierron [:nbp]]

Attachment: Clean-up: Replace FrameRecovery by IonBailoutIterator

Comment 2

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

Comment on attachment 624615 [details] [diff] [review]
Clean-up: Replace FrameRecovery by IonBailoutIterator

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

Nice!

::: js/src/ion/Bailouts.cpp
@@ +57,5 @@
>  
> +// These constructor are exactly the same except for the type of the iterator
> +// which is given to the SnapshotIterator constructor. Doing so avoid the
> +// creation of virtual functions for the IonIterator but may introduce some
> +// weirdness as IonInlineIterator is using an IonFrameIterator reference.

What sort of weirdness?

Comment 3

[Nicolas B. Pierron [:nbp]]

(In reply to David Anderson [:dvander] from comment #2)
> ::: js/src/ion/Bailouts.cpp
> @@ +57,5 @@
> >  
> > +// These constructor are exactly the same except for the type of the iterator
> > +// which is given to the SnapshotIterator constructor. Doing so avoid the
> > +// creation of virtual functions for the IonIterator but may introduce some
> > +// weirdness as IonInlineIterator is using an IonFrameIterator reference.
> 
> What sort of weirdness?

If a function decide to rely on ionScript() or to use OSIIndex(), due to the lack of virtual, these functions will use the IonFrameIterator reference contained in the InlineFrameIterator and thus are not able to recover correctly the data stored in IonBailoutIterator.

Currently, I am confident in the fact that we don't have such weirdness because our only use case of the IonFrameIterator within InlineFrameIterator is to read the frame content, or to clone it to find the parent scripted frame.  Both use cases are fine and should not cause any issue since the only potential issue is to read the bailed out frame.

Comment 4

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

Okay, might be good to add that in the comment so the scare words have some context ;)

Comment 5

[Nicolas B. Pierron [:nbp]]

Attachment: Clean-up: Remove pushBailoutFrame for global script.

Global script are necessary interpreted or the first frame to be jitted (Entry frame).  Thus, the initial frame is the only one which can be a global script.

Since we re-use the entry StackFrame for the bailout of the Entry IonFrame, we can garantee the else-branch of isInitialFrame is necessary a function.

Comment 6

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

Comment on attachment 625001 [details] [diff] [review]
Clean-up: Remove pushBailoutFrame for global script.

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

Nice. Looks like there was an erroneous REPORT_ERROR in the old code, to boot.

Comment 7

[Nicolas B. Pierron [:nbp]]

Comment on attachment 624615 [details] [diff] [review]
Clean-up: Replace FrameRecovery by IonBailoutIterator

https://hg.mozilla.org/projects/ionmonkey/rev/10d44ebdaa04

Comment 8

[Nicolas B. Pierron [:nbp]]

Comment on attachment 625001 [details] [diff] [review]
Clean-up: Remove pushBailoutFrame for global script.

https://hg.mozilla.org/projects/ionmonkey/rev/647a5c8f7dcc

Comment 9

[Paul Wright]

Resolved Fixed?

Comment 10

[Nicolas B. Pierron [:nbp]]

Attachment: Fix: Restore overflow and underflow of arguments after bailouts.

Part 3: Add pushBailoutArgs to reserve the space for the actual arguments and to
copy them from the Ion snapshot/stack to the reserved space.  Update pushBailoutFrame accordingly.

others:
- Move isInitialFrame to the IonFrameIterator.
- Add forEachCanonicalActualArgs to the SnapshotIterator to avoid building an InlineFrameIterator starting at the beginning of the function.
- Only create Args & Frame guards when they are used to create a new StackFrame.

Comment 11

[Luke Wagner [:luke]]

Comment on attachment 625827 [details] [diff] [review]
Fix: Restore overflow and underflow of arguments after bailouts.

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

Generally looks good.

::: js/src/ion/IonFrameIterator-inl.h
@@ +49,5 @@
>  template <class Op>
>  inline bool
> +SnapshotIterator::forEachCanonicalActualArgs(Op op, Value *argv,
> +                                             Value *scopeChain, Value *thisv,
> +                                             unsigned start, unsigned formalEnd, unsigned iterEnd)

It seems like this function does a lot more than just read args.  Perhaps SnapshotIterator::readFrame?

::: js/src/vm/Stack.cpp
@@ +864,5 @@
> +    CopyTo dst(iag->array());
> +    Value *src = it.actualArgs();
> +    if (!s.forEachCanonicalActualArgs(dst, src, NULL, &iag->thisv(), 0, fun->nargs, argc))
> +        return false;
> +    return true;

return s.readFrame :)

@@ +871,2 @@
>  StackFrame *
> +ContextStack::pushBailoutFrame(JSContext *cx, ion::IonBailoutIterator &it,

const ion::IonBailoutIterator &, if possible or, if 'it' is actually mutated, can it be passed by pointer?  In general we try to avoid mutable reference parameters.

Second, when a function doesn't report errors, the idiom is to make 'cx' the last parameter.

@@ +888,1 @@
>      bfg->regs_.prepareToRun(*fp, script);

This function body is almost structurally equivalent except for (1) MaybeReportError and (2) how fun/callee are extracted.  Could you add a pushInvokeFrame overload that accepts these three as parameters and have the original pushInvokeFrame and pushBailoutFrame call it?

Comment 12

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

Comment on attachment 625827 [details] [diff] [review]
Fix: Restore overflow and underflow of arguments after bailouts.

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

::: js/src/ion/Bailouts.cpp
@@ +227,5 @@
>          return BAILOUT_RETURN_FATAL_ERROR;
>      activation->setBailout(br);
>  
>      StackFrame *fp;
> +    if (it.isInitialFrame()) {

Since we're here, I think "isEntryFrame" is a slightly better name.

Comment 13

[Nicolas B. Pierron [:nbp]]

(In reply to Luke Wagner [:luke] from comment #11)
> @@ +871,2 @@
> >  StackFrame *
> > +ContextStack::pushBailoutFrame(JSContext *cx, ion::IonBailoutIterator &it,
> 
> Second, when a function doesn't report errors, the idiom is to make 'cx' the
> last parameter.

The function should not report an error but it can still fail to allocate the stack space for pushing the frame, and return a NULL point in such cases.

Comment 14

[Nicolas B. Pierron [:nbp]]

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