1444473 - Spectre mitigations for C++ calls from ICs.

Status: RESOLVED FIXED

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

Description

[Nicolas B. Pierron [:nbp]]

This is a follow-up from Bug 1438886, to support ICs calls too, by adding fences after returning from the call with a potentially speculated value.

(see Bug 1438886 comment 13)

Comment 1

[Nicolas B. Pierron [:nbp]]

Attachment: Spectre: Add Jit inline caches mitigation for values returned from C++.

Comment 2

[Jan de Mooij [:jandem]]

Megamorphic IC stubs are very perf-sensitive. Can you measure the perf overhead on some micro-benchmarks?

Comment 3

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #2)
> Megamorphic IC stubs are very perf-sensitive. Can you measure the perf
> overhead on some micro-benchmarks?

Actually I don't know if the megamorphic ICs need this because slot loads do obj->slots[shape->slot()], so the slot number has to come from the shape. Unfortunately there might be confusion with fixed slots vs dynamic slots but because numFixedSlots is stored in the same word I don't know if this is a real risk in practice. Many instructions would have to be executed speculatively to return to IC code, then to the caller, then use the speculative value.

I'm just worried that LFENCE will make megamorphic lookups > 2x slower.

Comment 4

[Nicolas B. Pierron [:nbp]]

Bug 835306 micro benchmark:

Before:
 ext | num |  Caf  |  Car  |  Cam  |  CAf  |  CAr  |  CAm  |  caf  |  car  |  cam  |  cAf  |  cAr  |  cAm  |
 10  |  1  | 5.392 | 5.220 | 5.756 | 5.135 | 5.144 | 5.370 | 5.002 | 5.056 | 5.302 | 4.899 | 4.940 | 5.520 |
 10  |  2  | 4.946 | 5.028 | 5.612 | 5.177 | 5.883 | 5.978 | 38.54 | 38.43 | 38.42 | 38.15 | 38.65 | 39.02 |
 10  |  4  | 5.205 | 6.175 | 7.228 | 5.429 | 5.648 | 6.788 | 42.20 | 41.75 | 42.43 | 42.05 | 43.06 | 42.86 |
 10  |  6  | 80.38 | 80.74 | 74.36 | 75.20 | 74.94 | 74.40 | 75.67 | 75.48 | 73.20 | 81.65 | 78.60 | 72.77 |
 10  |  8  | 258.0 | 258.3 | 233.0 | 343.1 | 333.8 | 291.6 | 258.6 | 256.3 | 216.5 | 335.0 | 333.1 | 303.0 |
 10  | 12  | 274.7 | 262.1 | 213.6 | 336.7 | 338.8 | 286.3 | 265.6 | 259.3 | 222.3 | 332.1 | 323.3 | 307.1 |
 10  | 16  | 269.3 | 261.1 | 213.1 | 316.5 | 337.6 | 290.7 | 277.0 | 255.2 | 218.6 | 338.5 | 330.4 | 275.6 |
 10  | 24  | 258.8 | 258.7 | 223.1 | 293.1 | 309.7 | 248.6 | 255.8 | 259.1 | 213.3 | 308.2 | 304.9 | 248.4 |
 10  | 32  | 255.5 | 252.4 | 211.1 | 298.3 | 300.4 | 255.4 | 256.5 | 255.2 | 217.2 | 303.5 | 299.2 | 246.3 |

After:
 ext | num |  Caf  |  Car  |  Cam  |  CAf  |  CAr  |  CAm  |  caf  |  car  |  cam  |  cAf  |  cAr  |  cAm  |
 10  |  1  | 5.498 | 5.107 | 5.338 | 5.095 | 5.131 | 7.208 | 5.322 | 5.086 | 5.333 | 5.002 | 5.041 | 5.273 |
 10  |  2  | 5.065 | 4.992 | 5.482 | 5.035 | 5.596 | 7.905 | 38.28 | 38.44 | 37.44 | 37.61 | 37.54 | 37.57 |
 10  |  4  | 4.975 | 6.126 | 8.029 | 6.338 | 5.622 | 6.775 | 42.68 | 43.00 | 43.62 | 42.55 | 42.62 | 43.41 |
 10  |  6  |*83.97 | 81.58 | 75.21 | 75.60 | 74.25 | 73.48 | 75.36 | 76.26 | 72.23 | 81.68 | 78.33 | 70.98 |
 10  |  8  |*304.5 |*295.3 |*244.6 |*377.1 |*364.2 |*327.2 |*303.0 |*299.3 |*248.6 |*381.5 |*366.4 |*470.0 |
 10  | 12  |*304.2 |*299.6 |*245.2 |*383.5 |*363.4 |*319.7 |*300.9 |*298.6 |*249.4 |*371.0 |*362.5 |*330.1 |
 10  | 16  |*300.9 |*296.8 |*250.8 |*356.2 |*371.9 |*308.4 |*305.0 |*297.3 |*249.9 |*365.7 |*363.3 |*304.2 |
 10  | 24  |*305.1 |*293.8 |*244.7 |*349.3 |*348.1 |*279.8 |*313.1 |*295.9 |*247.8 |*351.5 |*353.1 |*279.9 |
 10  | 32  |*307.3 |*306.3 |*249.6 |*347.9 |*343.6 |*278.1 |*306.4 |*320.1 |*249.3 |*341.5 |*348.9 |*277.1 |

Comment 5

[Jan de Mooij [:jandem]]

Comment on attachment 8960545 [details] [diff] [review]
Spectre: Add Jit inline caches mitigation for values returned from C++.

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

That's a smaller regression than I thought it would be but still >= 10%

We can keep an eye on AWFY and Talos, if there's a significant regression we can reconsider.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/814eccdff53e
Spectre: Add Jit inline caches mitigation for values returned from C++. r=jandem

Comment 7

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/814eccdff53e

Comment 8

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8960545 [details] [diff] [review]
Spectre: Add Jit inline caches mitigation for values returned from C++.

Approval Request Comment
[Feature/Bug causing the regression]: N/A
[User impact if declined]: We want to uplift Spectre fixes in 61 to 60.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None. (depends on Bug 1431829)
[Is the change risky?]: No.
[Why is the change risky/not risky?]: Add similar speculation barriers as Bug 1431829.
[String changes made/needed]: None.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8960545 [details] [diff] [review]
Spectre: Add Jit inline caches mitigation for values returned from C++.

Spectre mitigation needed for 60. Approved for 60.0b11.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a3dff4cabfb7