1112632 - Scalar Replacement & Eliminate*DeadResumePointOperands can cause invalid bailouts.

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Nicolas B. Pierron [:nbp | PTO until 11/11]]

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.

Comment 1

[Nicolas B. Pierron [:nbp | PTO until 11/11]]

[Tracking Requested - why for this release]:
Tracking Fx36, as this issue might be cause crashes / interruption of the JS code.

Comment 2

[Nicolas B. Pierron [:nbp | PTO until 11/11]]

Attachment: Flag object/array allocations as implictly used as we are removing uses.

Comment 3

[Sylvestre Ledru [:Sylvestre]]

We don't want crashes, tracking.

Comment 4

[Jan de Mooij [:jandem]]

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.

Comment 5

[Nicolas B. Pierron [:nbp | PTO until 11/11]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f4eba33e0d61

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f4eba33e0d61

Comment 7

[Sylvestre Ledru [:Sylvestre]]

Nicolas, can we have an uplift request for aurora? thanks

Comment 8

[Nicolas B. Pierron [:nbp | PTO until 11/11]]

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

Comment 9

[Nicolas B. Pierron [:nbp | PTO until 11/11]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5200b09176a2