Stream: git-wasmtime

Topic: wasmtime / PR #2289 Cranelift: refactoring of unwind info

Wasmtime GitHub notifications bot (Oct 12 2020 at 23:46):

yurydelendik opened PR #2289 from common-unwind to main:

This PR:

• Creates common unwind info data structures (at cranelift::isa::unwind::input)
• Extracts common logic for SystemV and FastCall into "cranelift/codegen/src/isa/x86/unwind.rs"
• Moves emit logic from cranelift/codegen/src/isa/x86/unwind/winx64.rs to cranelift/codegen/src/isa/unwind/winx64.rs
• Moves emit logic from cranelift/codegen/src/isa/x86/unwind/systemv.rs to cranelift/codegen/src/isa/unwind/systemv.rs
• Fixes test at x86/unwind/systemv.rs

This is a preparation PR to expose unwind info for different backends, e.g. new backend for Aarch64. Also, will be useful to move emitting logic out of the codegen crate and keep only common data structures.

See also #2266

Wasmtime GitHub notifications bot (Oct 12 2020 at 23:46):

yurydelendik requested peterhuene for a review on PR #2289.

Wasmtime GitHub notifications bot (Oct 13 2020 at 00:23):

yurydelendik updated PR #2289 from common-unwind to main:

This PR:

• Creates common unwind info data structures (at cranelift::isa::unwind::input)
• Extracts common logic for SystemV and FastCall into "cranelift/codegen/src/isa/x86/unwind.rs"
• Moves emit logic from cranelift/codegen/src/isa/x86/unwind/winx64.rs to cranelift/codegen/src/isa/unwind/winx64.rs
• Moves emit logic from cranelift/codegen/src/isa/x86/unwind/systemv.rs to cranelift/codegen/src/isa/unwind/systemv.rs
• Fixes test at x86/unwind/systemv.rs

This is a preparation PR to expose unwind info for different backends, e.g. new backend for Aarch64. Also, will be useful to move emitting logic out of the codegen crate and keep only common data structures.

See also #2266

Wasmtime GitHub notifications bot (Oct 13 2020 at 16:28):

yurydelendik updated PR #2289 from common-unwind to main:

This PR:

• Creates common unwind info data structures (at cranelift::isa::unwind::input)
• Extracts common logic for SystemV and FastCall into "cranelift/codegen/src/isa/x86/unwind.rs"
• Moves emit logic from cranelift/codegen/src/isa/x86/unwind/winx64.rs to cranelift/codegen/src/isa/unwind/winx64.rs
• Moves emit logic from cranelift/codegen/src/isa/x86/unwind/systemv.rs to cranelift/codegen/src/isa/unwind/systemv.rs
• Fixes test at x86/unwind/systemv.rs

This is a preparation PR to expose unwind info for different backends, e.g. new backend for Aarch64. Also, will be useful to move emitting logic out of the codegen crate and keep only common data structures.

See also #2266

Wasmtime GitHub notifications bot (Oct 13 2020 at 20:43):

peterhuene submitted PR Review.

Wasmtime GitHub notifications bot (Oct 13 2020 at 20:43):

peterhuene submitted PR Review.

Wasmtime GitHub notifications bot (Oct 13 2020 at 20:43):

peterhuene created PR Review Comment:

For a more general-purpose name, perhaps SaveRegister?

Wasmtime GitHub notifications bot (Oct 13 2020 at 20:43):

peterhuene created PR Review Comment:

Likewise, perhaps RestoreRegister?

Wasmtime GitHub notifications bot (Oct 13 2020 at 20:43):

peterhuene created PR Review Comment:

Typo in old code:

            cfa_offset: word_size as i32, // CFA offset starts at 8 to account for the return address on stack

Wasmtime GitHub notifications bot (Oct 13 2020 at 20:43):

peterhuene created PR Review Comment:

Xmm is architecture-specific, so perhaps we should combine this with SaveRegister and let the specific ABI emit the needed unwind code / CFI depending on integer vs. floating point register? It looks like the unwind codes for Windows ARM64 ABI will need SP-relative offsets too, even for the integer registers (unlike with x64 where only the floating-point XMM register saves need the SP-relative offset), so combining this with an Option<u32> for the stack offset might make sense to do.

Wasmtime GitHub notifications bot (Oct 13 2020 at 20:43):

peterhuene created PR Review Comment:

I wonder if we should call this SetFramePointer, which will correspond to the CFA register in DWARF and the set_fp unwind code for Windows ARM64? Windows x64 doesn't need the concept of a "frame pointer" currently as Cranelift doesn't support alloca and this is why it is only used for SystemV at the moment.

Wasmtime GitHub notifications bot (Oct 13 2020 at 20:43):

peterhuene created PR Review Comment:

nit: perhaps call this function_size to correspond with prologue_size?

Wasmtime GitHub notifications bot (Oct 13 2020 at 20:44):

peterhuene submitted PR Review.

Wasmtime GitHub notifications bot (Oct 13 2020 at 20:44):

peterhuene created PR Review Comment:

Note that Windows ARM64 unwind information will have a "set_fp" unwind code.

Wasmtime GitHub notifications bot (Oct 13 2020 at 20:49):

peterhuene submitted PR Review.

Wasmtime GitHub notifications bot (Oct 13 2020 at 23:33):

yurydelendik updated PR #2289 from common-unwind to main:

This PR:

• Creates common unwind info data structures (at cranelift::isa::unwind::input)
• Extracts common logic for SystemV and FastCall into "cranelift/codegen/src/isa/x86/unwind.rs"
• Moves emit logic from cranelift/codegen/src/isa/x86/unwind/winx64.rs to cranelift/codegen/src/isa/unwind/winx64.rs
• Moves emit logic from cranelift/codegen/src/isa/x86/unwind/systemv.rs to cranelift/codegen/src/isa/unwind/systemv.rs
• Fixes test at x86/unwind/systemv.rs

This is a preparation PR to expose unwind info for different backends, e.g. new backend for Aarch64. Also, will be useful to move emitting logic out of the codegen crate and keep only common data structures.

See also #2266

Wasmtime GitHub notifications bot (Oct 14 2020 at 00:41):

yurydelendik updated PR #2289 from common-unwind to main:

This PR:

• Creates common unwind info data structures (at cranelift::isa::unwind::input)
• Extracts common logic for SystemV and FastCall into "cranelift/codegen/src/isa/x86/unwind.rs"
• Moves emit logic from cranelift/codegen/src/isa/x86/unwind/winx64.rs to cranelift/codegen/src/isa/unwind/winx64.rs
• Moves emit logic from cranelift/codegen/src/isa/x86/unwind/systemv.rs to cranelift/codegen/src/isa/unwind/systemv.rs
• Fixes test at x86/unwind/systemv.rs

This is a preparation PR to expose unwind info for different backends, e.g. new backend for Aarch64. Also, will be useful to move emitting logic out of the codegen crate and keep only common data structures.

See also #2266

Wasmtime GitHub notifications bot (Oct 14 2020 at 00:44):

yurydelendik updated PR #2289 from common-unwind to main:

This PR:

• Creates common unwind info data structures (at cranelift::isa::unwind::input)
• Extracts common logic for SystemV and FastCall into "cranelift/codegen/src/isa/x86/unwind.rs"
• Moves emit logic from cranelift/codegen/src/isa/x86/unwind/winx64.rs to cranelift/codegen/src/isa/unwind/winx64.rs
• Moves emit logic from cranelift/codegen/src/isa/x86/unwind/systemv.rs to cranelift/codegen/src/isa/unwind/systemv.rs
• Fixes test at x86/unwind/systemv.rs

This is a preparation PR to expose unwind info for different backends, e.g. new backend for Aarch64. Also, will be useful to move emitting logic out of the codegen crate and keep only common data structures.

See also #2266

Wasmtime GitHub notifications bot (Oct 14 2020 at 02:41):

yurydelendik updated PR #2289 from common-unwind to main:

This PR:

• Creates common unwind info data structures (at cranelift::isa::unwind::input)
• Extracts common logic for SystemV and FastCall into "cranelift/codegen/src/isa/x86/unwind.rs"
• Moves emit logic from cranelift/codegen/src/isa/x86/unwind/winx64.rs to cranelift/codegen/src/isa/unwind/winx64.rs
• Moves emit logic from cranelift/codegen/src/isa/x86/unwind/systemv.rs to cranelift/codegen/src/isa/unwind/systemv.rs
• Fixes test at x86/unwind/systemv.rs

This is a preparation PR to expose unwind info for different backends, e.g. new backend for Aarch64. Also, will be useful to move emitting logic out of the codegen crate and keep only common data structures.

See also #2266

Wasmtime GitHub notifications bot (Oct 14 2020 at 13:50):

yurydelendik updated PR #2289 from common-unwind to main:

This PR:

• Creates common unwind info data structures (at cranelift::isa::unwind::input)
• Extracts common logic for SystemV and FastCall into "cranelift/codegen/src/isa/x86/unwind.rs"
• Moves emit logic from cranelift/codegen/src/isa/x86/unwind/winx64.rs to cranelift/codegen/src/isa/unwind/winx64.rs
• Moves emit logic from cranelift/codegen/src/isa/x86/unwind/systemv.rs to cranelift/codegen/src/isa/unwind/systemv.rs
• Fixes test at x86/unwind/systemv.rs

This is a preparation PR to expose unwind info for different backends, e.g. new backend for Aarch64. Also, will be useful to move emitting logic out of the codegen crate and keep only common data structures.

See also #2266

Wasmtime GitHub notifications bot (Oct 14 2020 at 13:54):

yurydelendik submitted PR Review.

Wasmtime GitHub notifications bot (Oct 14 2020 at 13:54):

yurydelendik created PR Review Comment:

Windows x64 emit logic wants to differentiate these two. I'll rename it to SaveXmmRegister, I'll defer this refactoring to different PR -- it is getting more complicated than simple refactoring.

Wasmtime GitHub notifications bot (Oct 14 2020 at 14:10):

yurydelendik updated PR #2289 from common-unwind to main:

This PR:

• Creates common unwind info data structures (at cranelift::isa::unwind::input)
• Extracts common logic for SystemV and FastCall into "cranelift/codegen/src/isa/x86/unwind.rs"
• Moves emit logic from cranelift/codegen/src/isa/x86/unwind/winx64.rs to cranelift/codegen/src/isa/unwind/winx64.rs
• Moves emit logic from cranelift/codegen/src/isa/x86/unwind/systemv.rs to cranelift/codegen/src/isa/unwind/systemv.rs
• Fixes test at x86/unwind/systemv.rs

This is a preparation PR to expose unwind info for different backends, e.g. new backend for Aarch64. Also, will be useful to move emitting logic out of the codegen crate and keep only common data structures.

See also #2266

Wasmtime GitHub notifications bot (Oct 14 2020 at 15:12):

yurydelendik updated PR #2289 from common-unwind to main:

This PR:

• Creates common unwind info data structures (at cranelift::isa::unwind::input)
• Extracts common logic for SystemV and FastCall into "cranelift/codegen/src/isa/x86/unwind.rs"
• Moves emit logic from cranelift/codegen/src/isa/x86/unwind/winx64.rs to cranelift/codegen/src/isa/unwind/winx64.rs
• Moves emit logic from cranelift/codegen/src/isa/x86/unwind/systemv.rs to cranelift/codegen/src/isa/unwind/systemv.rs
• Fixes test at x86/unwind/systemv.rs

This is a preparation PR to expose unwind info for different backends, e.g. new backend for Aarch64. Also, will be useful to move emitting logic out of the codegen crate and keep only common data structures.

See also #2266

Wasmtime GitHub notifications bot (Oct 14 2020 at 15:13):

yurydelendik updated PR #2289 from common-unwind to main:

This PR:

• Creates common unwind info data structures (at cranelift::isa::unwind::input)
• Extracts common logic for SystemV and FastCall into "cranelift/codegen/src/isa/x86/unwind.rs"
• Moves emit logic from cranelift/codegen/src/isa/x86/unwind/winx64.rs to cranelift/codegen/src/isa/unwind/winx64.rs
• Moves emit logic from cranelift/codegen/src/isa/x86/unwind/systemv.rs to cranelift/codegen/src/isa/unwind/systemv.rs
• Fixes test at x86/unwind/systemv.rs

This is a preparation PR to expose unwind info for different backends, e.g. new backend for Aarch64. Also, will be useful to move emitting logic out of the codegen crate and keep only common data structures.

See also #2266

Wasmtime GitHub notifications bot (Oct 14 2020 at 16:59):

yurydelendik requested peterhuene for a review on PR #2289.

Wasmtime GitHub notifications bot (Oct 15 2020 at 04:51):

peterhuene submitted PR Review.

Wasmtime GitHub notifications bot (Oct 15 2020 at 13:34):

yurydelendik merged PR #2289.

Last updated: Oct 23 2024 at 20:03 UTC