We've decided to prevent speculative stores in addition to loads, to be on the safe side, to match what some other engines are doing, and to be more consistent.

This is actually a lot simpler because we can remove BoundsCheckKind now. I also added some comments. On Kraken I don't see much of a regression from this, definitely much smaller than for the loads. I think a lot of code does both a load + store and in that case we will be able to fold the MSpectreMaskIndex so there's no overhead.

(sed also added a missing \n at the end of some files...)

Comment on attachment 8959103 [details] [diff] [review] Part 1 - MBoundsCheck Review of attachment 8959103 [details] [diff] [review]: ----------------------------------------------------------------- Nice! ::: js/src/jit/IonBuilder.cpp @@ +13312,5 @@ > check->setNotMovable(); > > + if (JitOptions.spectreIndexMasking) { > + // Use a separate MIR instruction for the index masking. Doing this as > + // part of MBoundsCheck is unsound because bounds checks can be nit: s/is/would be/

Pushed by jandemooij@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/485c44f946e9 part 1 - Also add Spectre mitigations for MBoundsCheck added for stores. r=luke https://hg.mozilla.org/integration/mozilla-inbound/rev/0a403eafe620 part 2 - Rename MacroAssembler boundsCheck32ForLoad to spectreBoundsCheck32. r=nbp

https://hg.mozilla.org/mozilla-central/rev/485c44f946e9 https://hg.mozilla.org/mozilla-central/rev/0a403eafe620

This is a bit of an edge case: well-written code shouldn't write to out-of-bounds typed array indexes (it's a no-op), so I also simplified the code a bit.

Removing RegisterOrInt32Constant simplifies the next patch.

Comment on attachment 8961761 [details] [diff] [review] Part 4 - Remove RegisterOrInt32Constant Review of attachment 8961761 [details] [diff] [review]: ----------------------------------------------------------------- I could ask for seeing a tiny micro benchmark regression, but my understanding is that we are going to ship with this regression anyway … so let's just land it and see what AWFY reports.

This patch refactors spectreBoundsCheck32 so that its scratch register is optional. On (32-bit) x86, the scratch register helps emit better code - without a scratch register we do a push/pop and some jumps. On other platforms the scratch register makes no difference. This simplifies the next patch, because in IC code we don't have scratch registers available on x86 unfortunately.

The StoreElementHole code is pretty complicated unfortunately :/ In Ion code this requests a spectreTemp register only on x86 because it helps spectreBoundsCheck32. In Baseline IC code we don't have registers available on x86 so we pass InvalidReg as maybeScratch.

Comment on attachment 8961762 [details] [diff] [review] Part 5 - Refactor spectreBoundsCheck32 Review of attachment 8961762 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/MacroAssembler.h @@ +1349,5 @@ > DEFINED_ON(arm, arm64, mips_shared, x86_shared); > > // Performs a bounds check and zeroes the index register if out-of-bounds > // (to mitigate Spectre). > + inline void spectreBoundsCheck32(Register index, const Operand& length, Register maybeScratch, nit: Operand like templates should not be allowed in public the interface of the MacroAssembler as they obscure with what arguments they are being used. Make it private.

Comment on attachment 8961763 [details] [diff] [review] Part 6 - Use spectreBoundsCheck32 for stores Review of attachment 8961763 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/BaselineCacheIRCompiler.cpp @@ +1687,5 @@ > // Reload obj->elements as callTypeUpdateIC used the scratch register. > masm.loadPtr(Address(obj, NativeObject::offsetOfElements()), scratch); > > // Increment initLength and length. > + masm.add32(Imm32(1), elementsInitLength); note: I found my-self confused while reading this code, because we define the Address with the base register "scratch", but we do change the value of scratch in the mean time.

Pushed by jandemooij@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/7aa5b7d5198c part 3 - Use Spectre-safe bounds check for LStoreTypedArrayElementHole. r=nbp https://hg.mozilla.org/integration/mozilla-inbound/rev/70f44f69accd part 4 - Remove RegisterOrInt32Constant. r=nbp https://hg.mozilla.org/integration/mozilla-inbound/rev/0ba5e089f9a0 part 5 - Refactor spectreBoundsCheck32 to work without a scratch register. r=nbp https://hg.mozilla.org/integration/mozilla-inbound/rev/03f1b458b986 part 6 - Use spectreBoundsCheck32 for more stores in JIT code. r=nbp

Pushed by jandemooij@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/a0d9c1cc3c97 followup - Add some blank lines to appease masm style checker. r=red

https://hg.mozilla.org/mozilla-central/rev/7aa5b7d5198c https://hg.mozilla.org/mozilla-central/rev/70f44f69accd https://hg.mozilla.org/mozilla-central/rev/0ba5e089f9a0 https://hg.mozilla.org/mozilla-central/rev/03f1b458b986 https://hg.mozilla.org/mozilla-central/rev/a0d9c1cc3c97

Comment on attachment 8959103 [details] [diff] [review] Part 1 - MBoundsCheck (This applies to all patches in this bug.) --- 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. [Is the change risky?]: Low risk. [Why is the change risky/not risky?]: Has been on Nightly for a few weeks now. [String changes made/needed]: None.

Comment on attachment 8959103 [details] [diff] [review] Part 1 - MBoundsCheck Spectre mitigation needed for 60. Approved for 60.0b11.

https://hg.mozilla.org/releases/mozilla-beta/rev/39a6945f06ed8899c0146e91418ee8e322c3a0aa https://hg.mozilla.org/releases/mozilla-beta/rev/02a7736cbf001656b7aa15480fd92520632f5872 https://hg.mozilla.org/releases/mozilla-beta/rev/93bd90643c5b5bb0e90eee0dd3f71645c638d656 https://hg.mozilla.org/releases/mozilla-beta/rev/707fd3a2a6886566d9f7a56017c4983c869b5f2c https://hg.mozilla.org/releases/mozilla-beta/rev/d757d31f4b9d88462f24e57f4823717b986d3572 https://hg.mozilla.org/releases/mozilla-beta/rev/ce00b156aeb78fb7ddafd71d03c9709278265880 https://hg.mozilla.org/releases/mozilla-beta/rev/ebe5227dc318fc234d0abb45d868b1e34c6779c2