Closed
Bug 1440394
Opened 4 years ago
Closed 4 years ago
Require use of masm methods for object shape/group/class accesses in JIT code
Categories
(Core :: JavaScript Engine: JIT, enhancement, P1)
Core
JavaScript Engine: JIT
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)
53.24 KB,
patch
|
tcampbell
:
review+
|
Details | Diff | Splinter Review |
18.47 KB,
patch
|
tcampbell
:
review+
|
Details | Diff | Splinter Review |
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.
Updated•4 years ago
|
status-firefox60:
--- → affected
Priority: -- → P1
Assignee | ||
Comment 1•4 years ago
|
||
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)
Assignee | ||
Comment 2•4 years ago
|
||
(attached the wrong version)
Attachment #8953531 -
Attachment is obsolete: true
Attachment #8953531 -
Flags: review?(tcampbell)
Attachment #8953534 -
Flags: review?(tcampbell)
Assignee | ||
Comment 3•4 years ago
|
||
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 4•4 years ago
|
||
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 5•4 years ago
|
||
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+
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
Assignee | ||
Comment 7•4 years ago
|
||
(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!
Pushed by jandemooij@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/91faa780430b followup - Fix build bustage. r=red
Assignee | ||
Comment 9•4 years ago
|
||
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•4 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/12797fd8b6d6 https://hg.mozilla.org/mozilla-central/rev/e74bc3cf52f0 https://hg.mozilla.org/mozilla-central/rev/91faa780430b
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in
before you can comment on or make changes to this bug.
Description
•