Closed Bug 1112632 Opened 6 years ago Closed 6 years ago

Scalar Replacement & Eliminate*DeadResumePointOperands can cause invalid bailouts.

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla37
Tracking Status
firefox35 --- unaffected
firefox36 + fixed
firefox37 --- fixed

People

(Reporter: nbp, Assigned: nbp)

References

Details

Attachments

(1 file)

Scalar Replacement can flag instructions as being recovered on bailout, and replace uses of the object / array by a constant / slot-value instead.  Combined with EliminateTriviallyDeadResumePointOperands, this can shorten the live-span of recovered instructions.

In case of bailout, this means that where we expected an object we now have a MagicValue flowing on the stack, which might be responsible for crashes / interrupted executions of JavaScript.

This bug appeared, during the test of Bug 1073033 & Bug 991720 & Bug 1110939, in the test suite while running compacting GC with ion/recover-array.js (arrayWithGCInit) test case.

This likely needs to be backported back to the addition of Scalar Replacement, such that we do not risk crashing because of Magic values flowing into baseline.
[Tracking Requested - why for this release]:
Tracking Fx36, as this issue might be cause crashes / interruption of the JS code.
We don't want crashes, tracking.
Comment on attachment 8537901 [details] [diff] [review]
Flag object/array allocations as implictly used as we are removing uses.

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

::: js/src/jit/ScalarReplacement.cpp
@@ +251,5 @@
>    : alloc_(alloc),
>      obj_(obj),
>      startBlock_(obj->block())
>  {
> +    // Annoate the instruction such that we do not replace it by a

Nite: Annotate (typo)

@@ +254,5 @@
>  {
> +    // Annoate the instruction such that we do not replace it by a
> +    // Magic(JS_OPTIMIZED_OUT) in case of removed uses.
> +    if (!obj_->isImplicitlyUsed())
> +        obj_->setImplicitlyUsed();

Nit: instead of the if you can also do obj_->setImplicitlyUsedUnchecked().

@@ +702,5 @@
>      arr_(arr),
>      startBlock_(arr->block()),
>      state_(nullptr)
>  {
> +    // Annoate the instruction such that we do not replace it by a

Same here.
Attachment #8537901 - Flags: review?(jdemooij) → review+
https://hg.mozilla.org/mozilla-central/rev/f4eba33e0d61
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Nicolas, can we have an uplift request for aurora? thanks
Flags: needinfo?(nicolas.b.pierron)
Comment on attachment 8537901 [details] [diff] [review]
Flag object/array allocations as implictly used as we are removing uses.

Approval Request Comment
[Feature/regressing bug #]: Bug 1069307
[User impact if declined]:
This issue might be cause crashes / interruption of the JS code.

[Describe test coverage new/current, TBPL]:
2 weeks of mozilla-inbound & central.

[Risks and why]:
Low risk, as this patch add flags which are used to prevent optimizations.

[String/UUID change made/needed]: N/A
Flags: needinfo?(nicolas.b.pierron)
Attachment #8537901 - Flags: approval-mozilla-aurora?
Attachment #8537901 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.