1029963 - RematerializedFrame should be able to recover slots using recover instructions

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Shu-yu Guo [:shu]]

Currently RematerializedFrames do not handle recover instructions.

While we can't recover the slots in PJS, we should be able to do so when rematerializing on the main thread.

Comment 1

[Nicolas B. Pierron [:nbp]]

What is the RematerializedFrames used for?  Can we guarantee that a bailout will occur when we request such frame, and would the bailout reuse the content of the rematerialized frame instead of bailing a second time?

If we intend to evaluate recover instruction while constructing rematerialized frames, we need to be aware that this implies that we have to bailout.  The reason being the some Recover instruction might allocate values which are uniquely identifiable, such as allocating objects.  If we are running the recover instructions twice, we will have 2 different objects.

Comment 2

[Shu-yu Guo [:shu]]

(In reply to Nicolas B. Pierron [:nbp] from comment #1)
> What is the RematerializedFrames used for?  Can we guarantee that a bailout
> will occur when we request such frame, and would the bailout reuse the
> content of the rematerialized frame instead of bailing a second time?
> 
> If we intend to evaluate recover instruction while constructing
> rematerialized frames, we need to be aware that this implies that we have to
> bailout.  The reason being the some Recover instruction might allocate
> values which are uniquely identifiable, such as allocating objects.  If we
> are running the recover instructions twice, we will have 2 different objects.

I think we discussed this on IRC.

Comment 4

[Shu-yu Guo [:shu]]

Attachment: Recover slots in RematerializedFrames.

This is not quite safe yet, so I've blocked this bug on removepjs.

Imagine all the ThreadSafeContexts being JSContexts instead, which will be the
case once PJS is all removed.

This also fixes the fuzz bug 1118878.

Comment 5

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8545504 [details] [diff] [review]
Recover slots in RematerializedFrames.

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

This looks good.

::: js/src/jit/JitFrameIterator.h
@@ -653,5 @@
>  
>          // Read the scope chain.
>          if (scopeChain) {
> -            MOZ_ASSERT(!fallback.canRecoverResults());
> -            JS::AutoSuppressGCAnalysis nogc; // If we cannot recover then we cannot GC.

As computeScopeChain is again checking for this flag, I am confident that this might be fine now.  Still, you might want to check the rooting analysis on the try-server, as I had to add this code because of false-positive.

::: js/src/jit/RematerializedFrame.cpp
@@ -38,5 @@
>      numActualArgs_(numActualArgs),
>      script_(iter.script())
>  {
>      CopyValueToRematerializedFrame op(slots_);
> -    MaybeReadFallback fallback(MagicValue(JS_OPTIMIZED_OUT));

This was the only use of a non-undefined fallback value.  We can open a follow-up bug to remove the enum from the MaybeReadFallback class.

::: js/src/vm/Stack.cpp
@@ +1519,5 @@
> +
> +        // Frames are often rematerialized with the cx inside a Debugger's
> +        // compartment. To recover slots and to create CallObjects, we need to
> +        // be in the activation's compartment.
> +        AutoCompartment ac(cx, compartment_);

Nice catch.

Comment 6

[Shu-yu Guo [:shu]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4f1b2f93ae48

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4f1b2f93ae48