Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cranelift: refactoring of unwind info #2289

Merged
merged 7 commits into from
Oct 15, 2020

Conversation

yurydelendik
Copy link
Contributor

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

@github-actions github-actions bot added the cranelift Issues related to the Cranelift code generator label Oct 13, 2020
Copy link
Member

@peterhuene peterhuene left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, the refactoring seems pretty straight-forward and most of my comments are simply about the naming of the ABI-agnostic names of the unwind operations.

I do have a concern about the performance of this change, primarily for Windows. The Windows x64 ABI only needs to look at the entry block to fully describe the unwind information because epilogues are not described for Windows x64. Epilogues on Windows x64 must have a specific form and Windows will detect an unwind inside of an epilogue and emulate the completion of the epilogue to unwind the function.

With these changes, the more complex scan needed to build DWARF CFI is used even on Windows. This includes sorting the blocks and then inspecting every instruction in the function. I would imagine this can be pretty expensive for modules with a lot of large functions. There's also now the additional allocations being done for building up this intermediary "ABI-agnostic" unwind information and then feeding it to the build function that transforms it to the "ABI-specific" unwind information.

Obviously these are merely concerns until some actual performance measurements are made, but it's something to think about.

Regarding Windows ARM64, I think this refactoring won't introduce any limitations for emitting unwind information 👍. Unlike Windows x64, the ARM64 unwind information does describe the epilogues, but in the majority case the epilogues are described in terms of the prologue (i.e. there would just be a set of unwind codes for the prologue and the epilogue unwind information would point at the same set of codes to indicate a "mirror" operation). However, this means the ARM64 unwind information will require the offset to the start of each epilogue within the function.

That said, I haven't looked at what the ARM backend emits for prologues and epilogues and whether or not it will emit noncontiguous functions (complicates the unwind information a little). If the ARM backend only ever emits "mirror" epilogues (i.e. an epilogue that has a 1:1 reverse instruction for each prologue instruction), then, as with the x64 unwind information, the Windows ARM64 unwind information would ever only need to look at the entry block.

@yurydelendik
Copy link
Contributor Author

I do have a concern about the performance of this change, primarily for Windows. The Windows x64 ABI only needs to look at the entry block to fully describe the unwind information because epilogues are not described for Windows x64.

I improved performance of isa::x86::create_unwind_info by scanning only prologue and epilogue blocks, and better epilogues_start check. That's something between windows x64 and systemv unwind info performance speeds. Assuming that a majority of function will not have long entry and epilogue block, that shall be fine.

Nature of our generated code (even in systemv) does not require to have epilogues -- it is just correct to have them for e.g. step-by-step debugging. If the performance will be the issue, their generation can be removed without effect on backtrace logic.

I expect that new backends will allow us to know exact ranges of prologue/epilogue instructions.

@yurydelendik yurydelendik force-pushed the common-unwind branch 2 times, most recently from fededba to c935c99 Compare October 14, 2020 15:12
Copy link
Member

@peterhuene peterhuene left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Storing the block with the epilogue starts definitely makes sense.

@yurydelendik yurydelendik merged commit 3c68845 into bytecodealliance:main Oct 15, 2020
cfallin pushed a commit that referenced this pull request Nov 30, 2020
* factor common code

* move fde/unwind emit to more abstract level

* code_len -> function_size

* speedup block scanning

* better function_size calciulation

* Rename UnwindCode enums
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cranelift Issues related to the Cranelift code generator
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants