Closed Bug 1485162 Opened Last year Closed Last year

remove confusing StackDecrementForCall() overloads

Categories

(Core :: Javascript: WebAssembly, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: luke, Assigned: luke)

Details

Attachments

(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]
kill-overloads

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]
rm-dead-field

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 lwagner@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/249688402db2
remove dead CodeGeneratorShared field (r=bbouvier)
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ff39f587a0d
Baldr: remove confusing StackDecrementForCall() overloads (r=bbouvier)
https://hg.mozilla.org/mozilla-central/rev/249688402db2
https://hg.mozilla.org/mozilla-central/rev/5ff39f587a0d
Status: NEW → RESOLVED
Closed: Last year
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.