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.

Sign up for GitHub

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

Jump to bottom

machinst x64: New backend unwind #2266

Merged
merged 26 commits into from
Oct 23, 2020
Merged

machinst x64: New backend unwind #2266

merged 26 commits into from
Oct 23, 2020

Conversation

yurydelendik
Copy link
Contributor

@yurydelendik yurydelendik commented Oct 5, 2020 •
edited
Loading

Addresses unwind for experimental x64 item from #2079 . Code was just changed copy for the old x86 backend for SystemV. The preliminary code enables backtrace on SystemV call convension; enables "traps::*" tests there.

Investigate or not sure if possible:

  • fix unit test in new "systemv.rs"
  • detect "sp += " + "group" push
  • not to use UnwindInfoKind?
  • some externref / gc tests stopped working, due to stackmaps?
  • make unwind info available without feature?

@github-actions github-actions bot added cranelift Issues related to the Cranelift code generator cranelift:area:machinst Issues related to instruction selection and the new MachInst backend. cranelift:area:aarch64 Issues related to AArch64 backend. cranelift:area:x64 Issues related to x64 codegen labels Oct 5, 2020
@yurydelendik yurydelendik changed the title New backend unwind machinst x64: New backend unwind Oct 6, 2020
@yurydelendik
Copy link
Contributor Author

yurydelendik commented Oct 6, 2020 •
edited
Loading

I don't see how stackmaps in the CodeSink are populated (for machinst). Keeping tests in "tests/all/gc.rs" disabled.

bjorn3
bjorn3 reviewed Oct 9, 2020
View reviewed changes
cranelift/codegen/src/machinst/mod.rs Show resolved Hide resolved
cranelift/codegen/src/machinst/mod.rs Show resolved Hide resolved
cranelift/codegen/src/machinst/vcode.rs Outdated Show resolved Hide resolved
@yurydelendik yurydelendik mentioned this pull request Oct 12, 2020
Merged
@yurydelendik
Copy link
Contributor Author

not to use UnwindInfoKind?
make unwind info available without feature?

Cannot find solutions to address these two -- open for suggestions. Opening for review.

@yurydelendik yurydelendik marked this pull request as ready for review October 20, 2020 21:52
yurydelendik referenced this pull request in yurydelendik/wasmtime Oct 21, 2020
@yurydelendik
wip
9a05d0a
@peterhuene
Copy link
Member

I also have no strong opinion on removal of the unwind feature. I would probably favor keeping it at this point.

Regarding UnwindInfoKind, I would probably remove it if possible as I don't see much of a use case for generating unwind information in a different format than the function's (ABI-specific) calling convention.

@bjorn3
Copy link
Contributor

bjorn3 commented Oct 22, 2020

macOS has both DWARF unwinding and compact unwind info. Both of which can be used with the same SystemV calling convention.

@peterhuene
Copy link
Member

I'm 👍 with keeping it.

@yurydelendik
Copy link
Contributor Author

merged with #2307 changes: that allowed to replace "mov reg, imm(rsp)" todo -- carrying over the review approval

@yurydelendik yurydelendik merged commit de4af90 into bytecodealliance:main Oct 23, 2020
@yurydelendik yurydelendik mentioned this pull request Oct 23, 2020
Merged
2 tasks
cfallin pushed a commit that referenced this pull request Nov 30, 2020
@yurydelendik @cfallin
machinst x64: New backend unwind (#2266)
4d037a2 
Addresses unwind for experimental x64 backend. The preliminary code enables backtrace on SystemV call convension.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bjorn3 bjorn3 bjorn3 left review comments

@peterhuene peterhuene peterhuene approved these changes

Assignees
No one assigned
Labels
cranelift:area:aarch64 Issues related to AArch64 backend. cranelift:area:machinst Issues related to instruction selection and the new MachInst backend. cranelift:area:x64 Issues related to x64 codegen cranelift Issues related to the Cranelift code generator
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@yurydelendik @peterhuene @bjorn3