1440394 - Require use of masm methods for object shape/group/class accesses in JIT code

Status: RESOLVED FIXED

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

Description

[Jan de Mooij [:jandem]]

Similar to what we did for strings, we should expose JSObject::offsetOfGroup() et al only to MacroAssembler (MacroAssembler will be a friend class).

That will force all JIT code to use masm methods for loading/storing ObjectGroup/Shape/Class pointers. Then there will hopefully be a small number of masm methods we will have to make Spectre-safe.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Part 1 - Abstract shape/group access

This does what's described in comment 0 for groups and shapes. Having (almost) all object shape/group stuff abstracted behind MacroAssembler methods makes it much easier to audit this and we can mitigate these methods one by one.

Shapes are fully "abstracted". For groups we have loadObjGroupUnsafe that "leaks" the group - this is used in CodeGenerator::visitObjectGroupDispatch and is hard to avoid, but hopefully the "Unsafe" in the name sounds scary enough.

LGuardShape/LGuardGroup/LGuardClass now have the same implementation on all platforms (I removed some unnecessary temps on ARM/MIPS). We could move them to LIR.h/CodeGenerator.cpp instead of platform-dependent files, but I didn't do that here to not make this patch more complicated.

Also, some methods use "Obj" and others use "Object", I tried to match similar methods but at some point we should decide which one we want to use.

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Part 1 - Abstract shape/group access

(attached the wrong version)

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Part 2 - Abstract Class access

Similar to the previous patch.

I renamed loadObjClass to loadObjClassUnsafe. I hope we can remove this at some point - I kept it for some CodeGenerator methods like IsTypedArray that I want to rewrite to a branchless version (with cmov) so they're automatically Spectre-safe. I'll do that in another bug though.

Comment 4

[Ted Campbell [:tcampbell]]

Comment on attachment 8953539 [details] [diff] [review]
Part 2 - Abstract Class access

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

Makes sense to me.

Comment 5

[Ted Campbell [:tcampbell]]

Comment on attachment 8953534 [details] [diff] [review]
Part 1 - Abstract shape/group access

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

Decoupling all the things!

::: js/src/jit/BaselineCacheIRCompiler.cpp
@@ +297,5 @@
>      if (!addFailurePath(&failure))
>          return false;
>  
>      Address testAddr(stubAddress(reader.stubOffset()));
> +    masm.branchTestObjClass(Assembler::NotEqual, obj, scratch, testAddr, failure->label());

It's a little painful that some of these are |in, out, scratch| and others are |in, scratch, out|. Probably a mix throughout the engine so probably not worth worrying about.

::: js/src/jit/arm/Lowering-arm.cpp
@@ -484,5 @@
>  LIRGeneratorARM::visitGuardShape(MGuardShape* ins)
>  {
>      MOZ_ASSERT(ins->object()->type() == MIRType::Object);
>  
> -    LDefinition tempObj = temp(LDefinition::OBJECT);

Nice simplification.

::: js/src/vm/Shape.cpp
@@ +1800,4 @@
>  
>      if (listpPointsIntoShape) {
>          // listp points to the parent field of the next shape.
>          Shape* next = reinterpret_cast<Shape*>(uintptr_t(listp) - offsetof(Shape, parent));

Can we add a fromParentFieldPointer too? These casts are so gross to have in the middle of code.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/12797fd8b6d6
part 1 - Require use of MacroAssembler methods for shape/group accesses. r=tcampbell
https://hg.mozilla.org/integration/mozilla-inbound/rev/e74bc3cf52f0
part 2 - Require use of MacroAssembler methods for Class accesses. r=tcampbell

Comment 7

[Jan de Mooij [:jandem]]

(In reply to Ted Campbell [:tcampbell] (PTO until Mar 5. Checking mail) from comment #5)
> It's a little painful that some of these are |in, out, scratch| and others
> are |in, scratch, out|. Probably a mix throughout the engine so probably not
> worth worrying about.

Yeah maybe at some point we should go through them...

> Can we add a fromParentFieldPointer too? These casts are so gross to have in
> the middle of code.

Done!

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/91faa780430b
followup - Fix build bustage. r=red

Comment 9

[Jan de Mooij [:jandem]]

So this trivial compile error I fixed locally but qref'd it as part of the wrong patch. Unfortunately I did push that other patch to Try with these patches, so it was all green there, but not when I pushed to inbound without the unrelated patch. Oh well.

Comment 10

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/12797fd8b6d6
https://hg.mozilla.org/mozilla-central/rev/e74bc3cf52f0
https://hg.mozilla.org/mozilla-central/rev/91faa780430b