Closed Bug 1029963 Opened 5 years ago Closed 5 years ago

RematerializedFrame should be able to recover slots using recover instructions

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: shu, Unassigned)

References

Details

Attachments

(1 file)

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.
Depends on: 1029910
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.
(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.
Duplicate of this bug: 1118878
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.
Attachment #8545504 - Flags: review?(nicolas.b.pierron)
Depends on: removepjs
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.
Attachment #8545504 - Flags: review?(nicolas.b.pierron) → review+
https://hg.mozilla.org/mozilla-central/rev/4f1b2f93ae48
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
You need to log in before you can comment on or make changes to this bug.