IonMonkey: Fail invalidation properly

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: cdleary, Assigned: cdleary)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

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.
Assignee: general → christopher.leary
Status: NEW → ASSIGNED
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.
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.
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?
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.
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.
Duplicate of this bug: 712845
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.
Attachment #586322 - Flags: review?(dvander)
Whoops, the "pushed by" should be "pushed after" in the generateVMWrapper comment.
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.
Attachment #586322 - Flags: review?(dvander) → review+
https://hg.mozilla.org/projects/ionmonkey/rev/a399e98a8d4a
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.