Closed Bug 1485162 Opened 5 years ago Closed 5 years ago

remove confusing StackDecrementForCall() overloads


(Core :: JavaScript: WebAssembly, enhancement)

Not set



Tracking Status
firefox63 --- fixed


(Reporter: luke, Assigned: luke)



(2 files, 1 obsolete file)

There are 3 overloads for StackDecrementForCall(): one shared with all Ion that makes sense, and 2 that do special stuff and superficially look the same as the shared one.  I'm always confusing these so I think it's better to have all of WasmStubs.cpp just use the one common one.  This requires being a bit explicit about what has been pushed for the alreadyPushed argument, but I think that's actually good.
Attached patch kill-overloadsSplinter Review
Assignee: nobody → luke
Attachment #9002933 - Flags: review?(bbouvier)
Attached patch rm-dead-fieldSplinter Review
Attachment #9002934 - Flags: review?(bbouvier)
Comment on attachment 9002933 [details] [diff] [review]

Review of attachment 9002933 [details] [diff] [review]:

Ha nice, I've been confused by these overloads in the past.

::: js/src/wasm/WasmStubs.cpp
@@ +983,5 @@
>      AssertExpectedSP(masm);
>      GenerateFunctionPrologue(masm, funcTypeId, Nothing(), offsets);
> +    unsigned framePushed = StackDecrementForCall(WasmStackAlignment,

In every single other case, it was obvious that masm.framePushed() is 0 because of a visible previous call to setFramePushed(0). Here, it's more subtle; there's an assertion at the end of GenerateFunctionPrologue, but this knowledge is hidden within the function's body. Could we re-assert here instead? or comment?
Attachment #9002933 - Flags: review?(bbouvier) → review+
Comment on attachment 9002934 [details] [diff] [review]

Review of attachment 9002934 [details] [diff] [review]:

Sweet, thanks.
Attachment #9002934 - Flags: review?(bbouvier) → review+
(In reply to Benjamin Bouvier [:bbouvier] from comment #3)
> Could we re-assert here instead? or comment?

Good point!  It is a rather subtle detail so I improved all the calls so that they are either (1) using masm.framePushed(), (2) using sizeof(Frame) with a comment "// pushed by prologue".  (Can't assert b/c the constant is usually computed before the Generate*Prologue(), so masm.framePushed() is 0 before and sizeof(Frame)+framePushed after.)
Pushed by
remove dead CodeGeneratorShared field (r=bbouvier)
Baldr: remove confusing StackDecrementForCall() overloads (r=bbouvier)
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Flags: needinfo?(afuerhoff00)
Attachment #9052091 - Attachment is obsolete: true
Attachment #9052091 - Flags: checkin+
You need to log in before you can comment on or make changes to this bug.