Last Comment Bug 715357 - IonMonkey: Fail invalidation properly
: IonMonkey: Fail invalidation properly
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86_64 Linux
-- normal (vote)
: ---
Assigned To: Chris Leary [:cdleary] (not checking bugmail)
: Jason Orendorff [:jorendorff]
: 712845 (view as bug list)
Depends on: IonOSI
  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:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

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

Description User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image Chris Leary [:cdleary] (not checking bugmail) 2012-01-06 16:48:51 PST

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