Closed Bug 1440394 Opened 2 years ago Closed 2 years ago

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

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: jandem, Assigned: jandem)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

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.
Priority: -- → P1
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.
Attachment #8953531 - Flags: review?(tcampbell)
(attached the wrong version)
Attachment #8953531 - Attachment is obsolete: true
Attachment #8953531 - Flags: review?(tcampbell)
Attachment #8953534 - Flags: review?(tcampbell)
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.
Attachment #8953539 - Flags: review?(tcampbell)
Comment on attachment 8953539 [details] [diff] [review]
Part 2 - Abstract Class access

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

Makes sense to me.
Attachment #8953539 - Flags: review?(tcampbell) → review+
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.
Attachment #8953534 - Flags: review?(tcampbell) → review+
Blocks: 1441182
Pushed by jandemooij@gmail.com:
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
(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!
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.
Blocks: 1442561
You need to log in before you can comment on or make changes to this bug.