Remove suppressGC hacks from wasm code

RESOLVED FIXED in Firefox 66

Status

()

enhancement
P2
normal
RESOLVED FIXED
10 months ago
6 months ago

People

(Reporter: lth, Assigned: jseward)

Tracking

(Blocks 1 bug)

Trunk
mozilla66
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 wontfix, firefox64 wontfix, firefox65 wontfix, firefox66 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Reporter

Description

10 months ago
Once we have stack maps and tracing we no longer need to suppress GC when wasm activations are on the stack.
Reporter

Updated

10 months ago
Blocks: 1488205
Reporter

Updated

10 months ago
No longer blocks: 1487329
Reporter

Comment 1

10 months ago
In addition to obvious uses in WasmStubs.cpp, also note weird cases like GCRuntime::temporaryAbortIfWasmGc(), we should be able to track them all down by looking for uses of JS::ContextOptions::wasmGc().
Reporter

Updated

8 months ago
Assignee: nobody → jseward
Status: NEW → ASSIGNED
Assignee

Comment 2

8 months ago
This removes GCRuntime::temporaryAbortIfWasmGc() and SuppressGC and
all the C++ fragments guarded by them, and also the 7 associated test files.
I also checked all uses of JS::ContextOptions::wasmGc() but didn't find
any further C++ frags guarded by it.
Attachment #9018330 - Flags: review?(lhansen)
Reporter

Comment 3

8 months ago
Comment on attachment 9018330 [details] [diff] [review]
bug1488162-rm-suppressGC-1.diff

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

This seems fine.  I assume the --wasm-gc flag and all the variables propagating that through the engine will be removed on the other bug.

::: js/src/wasm/WasmStubs.cpp
@@ -356,5 @@
>          masm.loadPtr(Address(masm.getStackPointer(), argBase + arg.offsetFromArgBase()), WasmTlsReg);
>      }
>  
>  #ifdef ENABLE_WASM_GC
>      WasmPush(masm, WasmTlsReg);

Any reason you can't remove this line, the corresponding pop, and then both of the ifdef blocks altogether?
Attachment #9018330 - Flags: review?(lhansen) → review+
Reporter

Updated

7 months ago
Assignee

Comment 4

7 months ago
Updated patch.
Attachment #9018330 - Attachment is obsolete: true
Assignee

Comment 5

7 months ago
Rebased to 449244:0ae2634de8e1.
Attachment #9026419 - Attachment is obsolete: true

Comment 6

6 months ago
Pushed by jseward@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9e2ee18925b
Remove suppressGC hacks from wasm code.  r=lth.

Comment 7

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d9e2ee18925b
Status: ASSIGNED → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
You need to log in before you can comment on or make changes to this bug.