1354275 - Ion: Inlining native calls need resume points if they throw

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Ted Campbell [:tcampbell]]

As explained in the comment at the top of |IonBuilder::visitThrow|, a throw must be preceded by a resume point or we risk getting stale data. This concern also exists when making VM calls that can throw. Specifically, inlining native methods that may throw can be missing a resume point.

The other effect is when a bailout occurs during the exception, the resume point has a different pc than the call chain where the fault occurred. This complicates unwinding scopes and debugging.

Comment 1

[Ted Campbell [:tcampbell]]

Testcase:

> // --ion-eager --ion-offthread-compile=off
> function f(t) {
>     for (var i = 0; i < 2; i++) {
>         try {
>             var x = 1;
>             Array(0);
>             x = 2;
>             Array(t);
>         } catch (e) {
>             assertEq(x, 2);
>         }
>     }
> }
> 
> f(-1);

Comment 2

[Ted Campbell [:tcampbell]]

As discussed with :jandem, the problem seems to be missing resumeAfter calls for MIR nodes that may throw, such as MNewArrayDynamicLength.

Comment 3

[Ted Campbell [:tcampbell]]

Attachment: Bug 1354275 - Fix handling of Array() throwing in Ion

Review commit: https://reviewboard.mozilla.org/r/128482/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/128482/

Comment 4

[Ted Campbell [:tcampbell]]

A basic manual review looks like MNewArrayDynamicLength is the only obviously incorrect case. Submitting a patch to get this fixed.

Marking as leave-open because I'd like to put together an experiment on if there are other cases we can through without having a correct resume point.

Comment 5

[Jan de Mooij [:jandem]]

Comment on attachment 8856529 [details]
Bug 1354275 - Fix handling of Array() throwing in Ion

https://reviewboard.mozilla.org/r/128482/#review131990

Sorry for the delay.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9590ce12459c
Fix handling of Array() throwing in Ion r=jandem

Comment 7

[Ted Campbell [:tcampbell]]

Comment on attachment 8856529 [details]
Bug 1354275 - Fix handling of Array() throwing in Ion

Approval Request Comment
[Feature/Bug causing the regression]:
Introduced by Bug 1100513.

[User impact if declined]:
Rare cases cause javascript to generate incorrect value. See Comment 1.

[Is this code covered by automated tests?]:
Test case included in patch.

[Has the fix been verified in Nightly?]:
Waiting for autoland merge. Try builds fix the included automated jit-test.

[Needs manual test from QE? If yes, steps to reproduce]:
No.

[List of other uplifts needed for the feature/fix]:
None.

[Is the change risky?]:
Low

[Why is the change risky/not risky?]:
This patch adds a missing ResumePoint to a specific MIR node in IonMonkey. ResumePoints are used for MIR nodes with side-effects (most of them), but can be avoided as a performance improvement. In this cases the ResumePoint was left out when the operation does have side-effect concerns.

[String changes made/needed]:
No.

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9590ce12459c

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8856529 [details]
Bug 1354275 - Fix handling of Array() throwing in Ion

54 was merged to Beta today.

Comment 10

[Julien Cristau [:jcristau]]

Comment on attachment 8856529 [details]
Bug 1354275 - Fix handling of Array() throwing in Ion

ion fix for beta54, should be in b3

Comment 11

[Ted Campbell [:tcampbell]]

Cherry-pick from central -> beta applies cleanly on my end. Can someone apply to real repository? I'm unclear on the steps for uplifts.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/4bde1b05f7bf

Comment 13

[Andrei Vaida [:avaida]]

(In reply to Ted Campbell [:tcampbell] from comment #7)
> [Is this code covered by automated tests?]:
> Test case included in patch.
> [Needs manual test from QE? If yes, steps to reproduce]:
> No.

Setting qe-verify- based on Ted's assessment on manual testing needs and the fact that this fix has automated coverage.

Comment 14

[Ted Campbell [:tcampbell]]

Follow-up work moved to Bug 1365758.

The specific case discussed in this bug is fixed, so closing bug.

Comment 15

[Firefox Bug Husbandry Bot]

Removing leave-open keyword from resolved bugs, per :sylvestre.