VMFunction::returnsData() tests for incorrect return-type
Categories
(Core :: JavaScript Engine: JIT, defect)
Tracking
()
People
(Reporter: anba, Assigned: anba)
References
Details
Attachments
(1 file)
1.09 KB,
patch
|
nbp
:
review+
lizzard
:
approval-mozilla-beta+
RyanVM
:
approval-mozilla-esr60+
|
Details | Diff | Splinter Review |
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()
.
Assignee | ||
Comment 1•6 years ago
|
||
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•6 years ago
|
||
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 3•6 years ago
|
||
Comment on attachment 9039751 [details] [diff] [review]
bug1523558.patch
Beta/Release Uplift Approval Request
Feature/Bug causing the regression
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•6 years ago
|
||
Andre, do you need this landed on inbound also?
Assignee | ||
Comment 5•6 years ago
|
||
(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.)
Pushed by opoprus@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/74d7d41da9e4
Fix test condition in VMFunction::returnsData(). r=nbp
Comment 7•6 years ago
|
||
bugherder |
Updated•6 years ago
|
Comment 8•6 years ago
|
||
![]() |
||
Comment 9•6 years ago
|
||
bugherder uplift |
Updated•6 years ago
|
Comment 10•6 years ago
|
||
![]() |
||
Comment 11•6 years ago
|
||
bugherder uplift |
Description
•