1442561 - Spectre mitigations for object shape/group/class guards

Status: RESOLVED FIXED

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

Description

[Jan de Mooij [:jandem]]

After type barriers (bug 1437483) this is the main thing to fix next. Bug 1440394 already added MacroAssembler abstractions for this.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Part 1 - Add pref

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Part 2 - Mitigate shape/group/class guards

This has mitigations for most shape/group/class guards in JIT code.

Most of this is pretty straight-forward: adding a scratch register and Specre mitigations to the MacroAssembler methods. The spectreRegToZero argument is a register that will be zeroed on speculative paths.

I changed UnboxedPlainObject::convertToNative to return its argument (as NativeObject*) instead of bool. This simplifies LConvertUnboxedObjectToNative a bit - explaining why is a bit complicated, but it has to do with the defineReuseInput we have when Spectre mitigations are enabled.

The shell benchmarks seem unaffected, but Octane is noisy so let's see what AWFY says.

Ted, flagging you for review too, to spread the Spectre mitigations knowledge a bit. If anything is unclear I'm happy to explain.

Comment 3

[Luke Wagner [:luke]]

Comment on attachment 8956132 [details] [diff] [review]
Part 2 - Mitigate shape/group/class guards

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

Nice work, and glad to see there isn't any significant regression.

::: js/src/jit/Lowering.cpp
@@ +3503,5 @@
> +
> +    if (JitOptions.spectreObjectMitigationsMisc) {
> +        auto* lir = new(alloc()) LConvertUnboxedObjectToNative(useRegisterAtStart(ins->object()),
> +                                                               temp());
> +        defineReuseInput(lir, ins, 0);

Ah, I went with a similar technique in bug 1432345.

Comment 4

[Ted Campbell [:tcampbell]]

Comment on attachment 8956132 [details] [diff] [review]
Part 2 - Mitigate shape/group/class guards

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

Looks good. Was more approachable than I was expecting!

::: js/src/jit/BaselineIC.cpp
@@ +3428,5 @@
>      masm.branchTestObject(Assembler::NotEqual, R1, &failure);
>  
> +    // Ensure the callee's class matches the one in this stub. Note that we use
> +    // spectreRegToZero := ICStubReg here: the callee register isn't used after
> +    // this, but ICStubReg is used below to make the call.

// Ensure the callee's class matches the one in this stub.
// We use |Address(ICStubReg, ICCall_ClassHook::offsetOfNative())| below instead of extracting the hook from callee. As a result the callee register is no longer used and we must use spectreRegToZero := ICStubReg instead.

::: js/src/jit/CacheIRCompiler.h
@@ +378,5 @@
>          currentOpRegs_.clear();
>          currentInstruction_++;
>      }
>  
> +    bool isDeadAfterInstruction(OperandId opId) const {

Nice trick.

::: js/src/vm/UnboxedObject.cpp
@@ +542,1 @@
>  UnboxedPlainObject::convertToNative(JSContext* cx, JSObject* obj)

Can you add a comment explaining the return value change?

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2bf39f14c6f8
part 1 - Add browser pref for misc Spectre object type mitigations. r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/240114d8acd3
part 2 - Add Spectre mitigations for most shape/group/class guards in JIT code. r=luke,tcampbell

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef4436b9240e
part 3 - Flip the pref. r=me

Comment 7

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/2bf39f14c6f8
https://hg.mozilla.org/mozilla-central/rev/240114d8acd3

Comment 8

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/ef4436b9240e