Closed Bug 1593736 Opened 5 months ago Closed 5 months ago

Add ABIArgType::Int32 and merge ABIArgType::(Pointer, General)

Categories

(Core :: Javascript: WebAssembly, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla72
Tracking Status
firefox72 --- fixed

People

(Reporter: rhunt, Assigned: rhunt)

Details

Attachments

(2 files)

See [1] and [2] for background. General implies Int32 and has correctness implications for passing an argument via the stack on 64bit systems, which makes it a footgun.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1591047#c10
[2] https://phabricator.services.mozilla.com/D51330#1560418

It appears a good amount of existing code assumes that ArgType_General == MIRType::Pointer [1] [2]. I think a more conservative course of action here is to change the definitions from bug 1591047 (which are wasm only) to use a new ArgType_Int32, and merge ArgType_Pointer, General.

[1] https://searchfox.org/mozilla-central/rev/d061ba55ac76f41129618d638f4ef674303ec103/js/src/jit/MacroAssembler-inl.h#116
[2] https://searchfox.org/mozilla-central/rev/d061ba55ac76f41129618d638f4ef674303ec103/js/src/jit/arm64/vixl/MozSimulator-vixl.cpp#459

Summary: Rename ABIArgType::General to ABIArgType::Int32 → Add ABIArgType::Int32 and merge ABIArgType::(Pointer, General)

After examining bug 1591047, I believe we should have added an Int32 type instead of changing the semantics of ArgType_General to be Int32. The reason is that the existing code assumes ArgType_General is pointer sized, and changing this is scary for all the existing uses. (e.g. simulator, MacroAssembler::appendSignatureType)

  • Adds ArgType_Int32
  • Changes ArgType_General -> ArgType_Int32, ArgType_Pointer -> ArgType_General for ABIFunctionTypes introduced in bug 1591047 (which are only used for Wasm instance calls).
  • ToMirType(ArgType_General) -> MIRType::Pointer (should only affect wasm)
  • ToMirType(ArgType_Int32) -> MIRType::Int32 (should only affect wasm)

Also shuffle constants and add a comment for clarity.

Depends on D52177

Pushed by rhunt@eqrion.net:
https://hg.mozilla.org/integration/autoland/rev/4d1dc9dc93be
Give ArgType_General pointer semantics in Wasm builtin code. r=lth
https://hg.mozilla.org/integration/autoland/rev/6f894c71dabe
Rename ArgType_Double to ArgType_Float64. r=lth
Attachment #9107205 - Attachment description: Bug 1593736 - Give ArgType_General pointer semantics in Wasm builtin code. r?lth → Bug 1593736 - Give ArgType_General pointer semantics in Wasm builtin code. r=lth
Attachment #9107206 - Attachment description: Bug 1593736 - Rename ArgType_Double to ArgType_Float64. r?lth → Bug 1593736 - Rename ArgType_Double to ArgType_Float64. r=lth
Pushed by rhunt@eqrion.net:
https://hg.mozilla.org/integration/autoland/rev/23218bed721c
Give ArgType_General pointer semantics in Wasm builtin code. r=lth
https://hg.mozilla.org/integration/autoland/rev/2475e9e1ea77
Rename ArgType_Double to ArgType_Float64. r=lth
Status: NEW → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla72
You need to log in before you can comment on or make changes to this bug.