Last Comment Bug 715357 - IonMonkey: Fail invalidation properly
: IonMonkey: Fail invalidation properly
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: ---
Assigned To: Chris Leary [:cdleary] (not checking bugmail)
:
Mentors:
: 712845 (view as bug list)
Depends on: IonOSI
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-04 14:59 PST by Chris Leary [:cdleary] (not checking bugmail)
Modified: 2012-01-06 16:48 PST (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Fail invalidation appropriately. (13.76 KB, patch)
2012-01-05 19:15 PST, Chris Leary [:cdleary] (not checking bugmail)
dvander: review+
Details | Diff | Review

Description Chris Leary [:cdleary] (not checking bugmail) 2012-01-04 14:59:03 PST
See bug 686927 comment 16.

If we fail during invalidation, we should flag the activation as being "bad" and, before returning to the ion code in that activation, we must check for that condition.

If the activation is bad, it must still be in a walkable state, so that invalidation records / ion scripts can be released appropriately. After doing this walk, the activation should be popped and an error condition should be returned from the ion activation invocation.
Comment 1 Chris Leary [:cdleary] (not checking bugmail) 2012-01-04 15:38:12 PST
This check has to be inserted at the end of every chunk of C++ that is called (first in the call graph) from Ion code, and that every Ion-code-to-C++ call site (i.e. code that runs in the exit frame epilogue) has to check for this error condition before returning success.

Bailouts could get an epilogue like:

return activation->failedInvalidation() ? BAILOUT_RETURN_FATAL_ERROR : BAILOUT_RETURN_OK;

For VM calls I think we'll need to embed an activation member check into the VM wrapper (a la generateVMWrapper). We can burn in the thread data pointer because it gets merged into JSRuntime in two weeks or so, so checking for failure should just be a load and a subsequent (dependent) load.
Comment 2 Chris Leary [:cdleary] (not checking bugmail) 2012-01-04 16:03:57 PST
dvander points out we have a nicer way of checking for this.

In bug 713997 we added a way of checking whether the caller frame has been invalidated in the VM wrapper by comparing the return address before and after -- if it has changed, we know that the caller has been invalidated. In order to preserve the original return value across the call, we duplicate the exit frame's return address field on the stack.

In a similar fashion, the act of failing invalidation can set the return value of the most recent frame in the activation to NULL (instead of the invalidator, as it would typically do for an invalidated frame). The comparison mentioned in the prior paragraph will fail the equality test, and we can refine to a test for NULL to see if we should return (to the invalidator) or handle an exception. This doesn't put any additional instructions on the hot path.
Comment 3 Chris Leary [:cdleary] (not checking bugmail) 2012-01-04 16:10:51 PST
In the bailout path epilogue we can check whether the stack word immediately underneath the exit frame is NULL, I suppose. Not sure if that exceeds grossness thresholds -- maybe we can extend the exit frame structure to include two return value words?
Comment 4 David Anderson [:dvander] 2012-01-04 17:03:56 PST
I think either one is fine. If we extend the exit frame structure then we just have to change it everywhere - linkExitFrame could push the final word and then we wouldn't have to worry about it.
Comment 5 David Anderson [:dvander] 2012-01-04 17:05:50 PST
Note that the exit frame created for Bailouts has to deal with invalidation as well (it's not generated in a vm wrapper), not the retn problem but it has to check to see if the activation is no longer usable.
Comment 6 Chris Leary [:cdleary] (not checking bugmail) 2012-01-05 16:15:53 PST
*** Bug 712845 has been marked as a duplicate of this bug. ***
Comment 7 Chris Leary [:cdleary] (not checking bugmail) 2012-01-05 19:15:09 PST
Created attachment 586322 [details] [diff] [review]
Fail invalidation appropriately.

Pretty simple: failing invalidation sets the exit frame's return value from each activation to NULL. The bailout tests for this in C++ and reuses the existing fatal error path (avoiding the NULL return value entirely), while the VM wrapper adds a dependent test for a NULL return value on its caller-invalidation path.
Comment 8 Chris Leary [:cdleary] (not checking bugmail) 2012-01-05 19:16:08 PST
Whoops, the "pushed by" should be "pushed after" in the generateVMWrapper comment.
Comment 9 David Anderson [:dvander] 2012-01-06 12:54:44 PST
Comment on attachment 586322 [details] [diff] [review]
Fail invalidation appropriately.

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

::: js/src/ion/Bailouts.cpp
@@ +312,5 @@
>      uint32 retval = ConvertFrames(cx, activation, in);
>  
>      EnsureExitFrame(in.fp());
>  
> +    if (retval != BAILOUT_RETURN_FATAL_ERROR && !activation->failedInvalidation())

Why is this check needed? Is it possible to have a failed invalidation and still be in ::Bailout? (It doesn't seem like it, at first glance)

::: js/src/ion/Ion.cpp
@@ +927,5 @@
>  
> +    js_ReportOutOfMemory(cx);
> +
> +    IonActivation *activation = cx->threadData()->ionActivation;
> +    while (activation) {

Is there a reason you can't use IonActivationIterator here?

::: js/src/ion/IonFrames.cpp
@@ +253,5 @@
>  ion::HandleException(ResumeFromException *rfe)
>  {
>      JSContext *cx = GetIonContext()->cx;
>  
> +    IonSpew(IonSpew_Invalidate, "handling exception");

This doesnt seem like the right spew channel but I don't think it really matters.

::: js/src/ion/Lowering.cpp
@@ +454,5 @@
>  bool
>  LIRGenerator::visitMod(MMod *ins)
>  {
>      MDefinition *lhs = ins->lhs();
> +    //MDefinition *rhs = ins->rhs();

Is lhs used here either? Looks like you can remove both lines.
Comment 10 Chris Leary [:cdleary] (not checking bugmail) 2012-01-06 16:48:51 PST
https://hg.mozilla.org/projects/ionmonkey/rev/a399e98a8d4a

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