2049308 - Harden bailout and recover instruction code

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, task, P1)

Description

[Jan de Mooij [:jandem]]

Bug 2044606 is an example of a bailout/recover-instruction bug where release assertions would have helped. Bailout and recover instruction code is cold - this means it's hard to test well but it also means we could upgrade many assertions to release assertions without affecting performance.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: (secure)

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/e3b6ef594890
https://hg.mozilla.org/integration/autoland/rev/133fc73680c4
Add some release assertions to the bailout code. r=mgaudet

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/318501fda5ec
https://hg.mozilla.org/integration/autoland/rev/84f4e7d72dce
Revert "Bug 2049308 - Add some release assertions to the bailout code. r=mgaudet" for causing bustages in InstrProfilingValue.c.

Comment 4

[Jan de Mooij [:jandem]]

Interesting, that looks like an LLVM PGO bug... I'll see if I can figure out which part is responsible.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/00f95d292337
https://hg.mozilla.org/integration/autoland/rev/3f82ed2b8963
Add some release assertions to the bailout code. r=mgaudet

Comment 6

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/3f82ed2b8963

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Is this worth uplifting to Beta for the benefit of ESR153?

Comment 8

[Jan de Mooij [:jandem]]

Attachment: (secure)

Original Revision: https://phabricator.services.mozilla.com/D309004

Comment 9

[Phabricator Automation]

firefox-beta Uplift Approval Request

• User impact if declined/Reason for urgency: This patch adds some release assertions to help prevent security bugs. Would be nice to have on ESR153.
• Code covered by automated testing?: yes
• Fix verified in Nightly?: yes
• Needs manual QE testing?: no
• Steps to reproduce for manual QE testing:
• Risk associated with taking this patch: low
• Explanation of risk level: Has been on Nightly for a while. Changes some assertions to release assertions.
• String changes made/needed?: N/A
• Is Android affected?: yes

Comment 10

[Jan de Mooij [:jandem]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #7)

Is this worth uplifting to Beta for the benefit of ESR153?

Yes; I've requested uplift.

Comment 11

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr153/rev/ce413a7cb2ae