1523558 - VMFunction::returnsData() tests for incorrect return-type

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[André Bargull [:anba]]

The returnType == Type_Pointer condition in this test can never be true, because returnType is required to be either Type_Void, Type_Bool, or Type_Object. I assume the correct test is to check for Type_Object in VMFunction::returnsData().

Comment 1

[André Bargull [:anba]]

Attachment: bug1523558.patch

I hope I get a carte blanche for any performance regressions which might happen here, just like happened for the initial issue at bug 1444856 comment 2.

Comment 2

[Nicolas B. Pierron [:nbp]]

Comment on attachment 9039751 [details] [diff] [review]
bug1523558.patch

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

(In reply to André Bargull [:anba] from comment #1)
> I hope I get a carte blanche for any performance regressions which might happen here, just like happened for the initial issue at bug 1444856 comment 2.

Yes. We should probably backport these changes too.

Comment 3

[André Bargull [:anba]]

Comment on attachment 9039751 [details] [diff] [review]
bug1523558.patch

Beta/Release Uplift Approval Request

Feature/Bug causing the regression

Bug 1438886

User impact if declined

Incomplete Spectre mitigation after execution of C++ calls, cf. bug 1438886.

Is this code covered by automated tests?

Yes

Has the fix been verified in Nightly?

No

Needs manual test from QE?

No

If yes, steps to reproduce

List of other uplifts needed

None

Risk to taking this patch

Low

Why is the change risky/not risky? (and alternatives if risky)

The change is non-risky, because it only ensures a speculation barrier (lfence on x86, csdb on ARM) is emitted. It may lead to a performance regression, though.

String changes made/needed

None.

ESR Uplift Approval Request

If this is not a sec:{high,crit} bug, please state case for ESR consideration

Incomplete Spectre mitigation after execution of C++ calls, cf. bug 1438886.

User impact if declined

See Beta approval.

Fix Landed on Version

66/67

Risk to taking this patch

Low

Why is the change risky/not risky? (and alternatives if risky)

See Beta approval.

String or UUID changes made by this patch

None.

Comment 4

[Cosmin Sabou [:CosminS]]

Andre, do you need this landed on inbound also?

Comment 5

[André Bargull [:anba]]

(In reply to Cosmin Sabou [:CosminS] from comment #4)

Andre, do you need this landed on inbound also?

Yes, first on inbound, please. (And after the Beta/ESR requests are approved also there.)

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/74d7d41da9e4
Fix test condition in VMFunction::returnsData(). r=nbp

Comment 7

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/74d7d41da9e4

Comment 8

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 9039751 [details] [diff] [review]
bug1523558.patch

Spectre mitigation, ok for uplift to beta 4.

Comment 9

[Sebastian Hengst [:aryx] (needinfo on intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/1a27e2f35d33

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9039751 [details] [diff] [review]
bug1523558.patch

Spectre mitigation improvement. Approved for 60.6esr.

Comment 11

[Sebastian Hengst [:aryx] (needinfo on intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-esr60/rev/f1b5873c848a