Closed Bug 1447578 Opened 2 years ago Closed 2 years ago

Remove MacroAssembler GC rooting/tracing code

Categories

(Core :: JavaScript Engine: JIT, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: jandem, Assigned: jandem)

References

Details

Attachments

(5 files, 1 obsolete file)

We don't have any places where we can trigger GC while masm is live (except for wasm but it doesn't embed GC pointers).

I have patches to add {StackMacroAssembler, WasmMacroAssembler, IonHeapMacroAssembler} derived classes. Then StackMacroAssembler can have an AutoCheckCannotGC to use the static analysis and runtime checks to ensure no GC happens. This is green on Try.
These were used for the old Ion ICs, but nowadays we don't have any callers that pass them.
Attachment #8960891 - Flags: review?(luke)
Attachment #8960892 - Flags: review?(luke)
Attachment #8960892 - Flags: review?(jcoppeard)
Attachment #8960894 - Flags: review?(jcoppeard)
Comment on attachment 8960892 [details] [diff] [review]
Part 2 - Add derived classes, add AutoCheckCannotGC

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

::: js/src/jit/MacroAssembler.h
@@ +2698,5 @@
>      Vector<JSObject*, 0, SystemAllocPolicy> pendingObjectReadBarriers_;
>      Vector<ObjectGroup*, 0, SystemAllocPolicy> pendingObjectGroupReadBarriers_;
>  };
>  
> +// StackMacroAssembler checks no GC will happen while its on the stack.

s/its/it's/ - Fixed locally
This makes the header file a lot more readable.
Attachment #8960897 - Flags: review?(luke)
Comment on attachment 8960892 [details] [diff] [review]
Part 2 - Add derived classes, add AutoCheckCannotGC

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

Overall this looks really good, but I have a couple of concerns.

::: js/src/jit/MacroAssembler.h
@@ +2712,5 @@
> +      : MacroAssembler(cx)
> +    {}
> +};
> +
> +// WasmMacroAssembler does not contain GC pointers, so it doesn't need the no-GC

This may be true now but bug 1444925 will change this situation.  Do we have a plan for dealing with that?  Can we just make these StackMacroAssemblers?

If not, is there some way we can check that no GC pointers are used in this kind of MacroAssembler?

@@ +2730,5 @@
> +  public:
> +    IonHeapMacroAssembler()
> +      : MacroAssembler()
> +    {
> +        MOZ_ASSERT(CurrentThreadIsIonCompiling());

Can we assert that we are on a helper thread here too?
Comment on attachment 8960894 [details] [diff] [review]
Part 3 - Remove masm tracing code

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

It's great to see all this basically-unused code disappear.
Attachment #8960894 - Flags: review?(jcoppeard) → review+
(In reply to Jon Coppeard (:jonco) from comment #6)
> This may be true now but bug 1444925 will change this situation.  Do we have
> a plan for dealing with that? 

I asked Luke about this last night and we won't embed GC pointers in wasm code; they could be stored in wasm's TlsData.

> Can we just make these StackMacroAssemblers?

It's what I had at first, but there are some places where we can GC in wasm code with a MacroAssembler on the stack..

> If not, is there some way we can check that no GC pointers are used in this
> kind of MacroAssembler?

We already have this one: https://searchfox.org/mozilla-central/rev/877c99c523a054419ec964d4dfb3f0cadac9d497/js/src/jit/shared/Assembler-shared.h#252-253

We could also MOZ_ASSERT(dataRelocations_.empty()) in the destructor maybe. I can try that.

> Can we assert that we are on a helper thread here too?

With --no-threads etc, CodeGenerator will be used on the main thread (but this is in an AutoEnterAnalysis scope which suppresses GC):

https://searchfox.org/mozilla-central/rev/877c99c523a054419ec964d4dfb3f0cadac9d497/js/src/jit/Ion.cpp#2286-2287
Comment on attachment 8960892 [details] [diff] [review]
Part 2 - Add derived classes, add AutoCheckCannotGC

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

(In reply to Jan de Mooij [:jandem] from comment #8)
Thanks for the answers.  This sounds fine then.
Attachment #8960892 - Flags: review?(jcoppeard) → review+
This makes ~WasmMacroAssembler assertNoGCThings() as an extra sanity check.
Attachment #8960915 - Flags: review?(jcoppeard)
Fixes a compile error in the ARM64 code...
Attachment #8960915 - Attachment is obsolete: true
Attachment #8960915 - Flags: review?(jcoppeard)
Attachment #8960917 - Flags: review?(jcoppeard)
Comment on attachment 8960917 [details] [diff] [review]
Part 5 - Assert no GC things in ~WasmMacroAssembler

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

Nice, thanks for adding this.
Attachment #8960917 - Flags: review?(jcoppeard) → review+
Comment on attachment 8960891 [details] [diff] [review]
Part 1 - Remove unused parameters from MacroAssembler constructor

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

Nice to see that go.
Attachment #8960891 - Flags: review?(luke) → review+
Comment on attachment 8960892 [details] [diff] [review]
Part 2 - Add derived classes, add AutoCheckCannotGC

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

::: js/src/jit/MacroAssembler.h
@@ +2712,5 @@
> +      : MacroAssembler(cx)
> +    {}
> +};
> +
> +// WasmMacroAssembler does not contain GC pointers, so it doesn't need the no-GC

Just like we currently assert !IsCompilingWasm() in the ctor of AbsoluteAddress(), we could similarly assert in core methods that embed GC pointers?  Wasm may be operating on GC things at runtime, but it won't be baking any pointers into machine code (b/c cross-thread shareability and serialization...).
Attachment #8960892 - Flags: review?(luke) → review+
Comment on attachment 8960897 [details] [diff] [review]
Part 4 - Move MacroAssembler constructors out-of-line

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

Much nicer.
Attachment #8960897 - Flags: review?(luke) → review+
(In reply to Luke Wagner [:luke] from comment #14)
> Just like we currently assert !IsCompilingWasm() in the ctor of
> AbsoluteAddress(), we could similarly assert in core methods that embed GC
> pointers?  Wasm may be operating on GC things at runtime, but it won't be
> baking any pointers into machine code (b/c cross-thread shareability and
> serialization...).

We already assert !IsCompilingWasm in ImmGCPtr etc. Part 5 adds asserts to the WasmMacroAssembler destructor to check there's no relocation data for GC pointers. All these things together should make it pretty difficult to accidentally embed a GC pointer in wasm code.
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/769fed48d30f
part 1 - Remove unused parameters from MacroAssembler constructor. r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d56f55622c6
part 2 - Refactor MacroAssembler, add AutoCheckCannotGC for stack-allocated assemblers. r=jonco,luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/af5c036e68f4
part 3 - Remove MacroAssembler rooting/tracing code. r=jonco
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff52c7cdce5e
part 4 - Move MacroAssembler constructors out-of-line. r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/e2ddf4dbfa33
part 5 - Assert WasmMacroAssembler does not have GC relocation data. r=jonco
You need to log in before you can comment on or make changes to this bug.