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

Unwinding support for inline assembly #88439

Merged
merged 14 commits into from
Dec 4, 2021
Merged

Unwinding support for inline assembly #88439

merged 14 commits into from
Dec 4, 2021

Conversation

cynecx
Copy link
Contributor

@cynecx cynecx commented Aug 28, 2021 •
edited
Loading

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 28, 2021
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@cynecx cynecx mentioned this pull request Aug 29, 2021
Open
@Amanieu
Copy link
Member

Amanieu commented Aug 30, 2021

I actually started working on a PR myself before you posted this one, but it's still very much a WIP: master...Amanieu:asm_unwind

More changes to MIR are needed to support this since MIR needs to be aware of unwinding. In particular call_return_effect needs to be adapted to also work with TerminatorKind::InlineAsm. This is used to mark the return value of a call as initialized only if a call returns normally but not when it unwinds. The same logic applies for output operands of InlineAsm.

@Amanieu
Copy link
Member

Amanieu commented Aug 30, 2021

I updated my branch at master...Amanieu:asm_unwind, it should now handle all of the MIR parts that deal with unwinding.

@cynecx
Copy link
Contributor Author

cynecx commented Aug 30, 2021 •
edited
Loading

@Amanieu That's great! Do you mind if I rebase this PR onto your branch? (at the end just adding codegen)

@Amanieu
Copy link
Member

Amanieu commented Aug 30, 2021

Sure! Just keep in mind I haven't actually tested any of it apart from checking it compiles.

@cynecx
Copy link
Contributor Author

cynecx commented Aug 30, 2021 •
edited
Loading

@Amanieu No worries. I am going to add some tests anyway and so then we shall see :) (But that has to wait until tomorrow or the day after tomorrow. But codgen-wise, it should be correct, as I've seen here: cynecx/libfringe#1 (comment). EDIT: however there might have been some issues after all because I haven't properly dealt with MIR xD).

@Amanieu
Copy link
Member

Amanieu commented Aug 31, 2021

I updated my branch again, there were a few more things I missed in MIR.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
Amanieu
Amanieu reviewed Sep 4, 2021
View reviewed changes
Copy link
Member

@Amanieu Amanieu left a comment

Choose a reason for hiding this comment

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

Can you also:

  • update the documentation in the unstable book to add may_unwind.
  • double-check the interaction between #[naked] and may_unwind: does this allow you to unwind from a naked function?

compiler/rustc_codegen_cranelift/src/base.rs Outdated Show resolved Hide resolved
@cynecx
Copy link
Contributor Author

cynecx commented Sep 4, 2021 •
edited
Loading

  • update the documentation in the unstable book to add may_unwind.

Will do.

  • double-check the interaction between #[naked] and may_unwind: does this allow you to unwind from a naked function?

As far as I have seen from several llvm-ir/assembly dumps (irl testcase: [1]), it should work just fine.

[1] https://github.com/cynecx/libfringe/blob/asm/src/arch/x86_64.rs (your unwinding branch for libfringe basically)

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented Sep 9, 2021

☔ The latest upstream changes (presumably #80522) made this pull request unmergeable. Please resolve the merge conflicts.

@Amanieu
Copy link
Member

Amanieu commented Sep 18, 2021

@cynecx Are you still working on this PR?

@cynecx
Copy link
Contributor Author

cynecx commented Sep 19, 2021

@Amanieu Yes I am. Sorry about the delay, I was caught up in some other things... Trying a rebase rn.

@cynecx cynecx force-pushed the unwind_asm branch 2 times, most recently from 68a3b41 to 8553004 Compare September 19, 2021 20:40
@Amanieu
Copy link
Member

Amanieu commented Sep 22, 2021

@cynecx are there any other changes that you plan on making (this PR is still marked as a draft)?

Otherwise this LGTM. I'd like someone more familiar with MIR to review the MIR-level changes here.

r? @oli-obk

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Dec 1, 2021
@bors
Copy link
Contributor

bors commented Dec 2, 2021

☔ The latest upstream changes (presumably #91318) made this pull request unmergeable. Please resolve the merge conflicts.

Amanieu and others added 14 commits December 3, 2021 23:51
@Amanieu @cynecx
Add initial AST and MIR support for unwinding from inline assembly
940b2ea
@cynecx
Adjust llvm wrapper for unwinding support for inlineasm
491dd1f
@cynecx
LLVM codgen support for unwinding inline assembly
91021de
@cynecx
cg_cranelift: check may_unwind flag instead of cleanup
6cbf44f
@cynecx
Update unstable-book with recent asm changes (may_unwind)
e33a3f9
@cynecx
rustc_codegen_gcc: error on unwinding inline asm
059d3b3
@cynecx
add unwind_asm feature gate for may_unwind option
686ace3
@cynecx
add tests for asm's options(may_unwind)
8e9ccdf
@cynecx
add clobber_abi("C") to may_unwind inline-asm tests
fb35cee
@cynecx
rustc_codegen_gcc: proper check for may_unwind
021a8d8
@cynecx
fix inline asm test because of missing attribute
7f870be
@cynecx
fix inline asm test by not hardcoding symbol names
233dede
@cynecx
fix feature-gate test for may_unwind
afdafcb
@cynecx
limit may_unwind codegen test to x86_64
3dbb621
@cynecx
Copy link
Contributor Author

cynecx commented Dec 3, 2021

@Amanieu rebased and test fixed.

@Amanieu
Copy link
Member

Amanieu commented Dec 3, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Dec 3, 2021

📌 Commit 3dbb621 has been approved by Amanieu

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 3, 2021
@bors
Copy link
Contributor

bors commented Dec 4, 2021

⌛ Testing commit 3dbb621 with merge 887999d...

@bors
Copy link
Contributor

bors commented Dec 4, 2021

☀️ Test successful - checks-actions
Approved by: Amanieu
Pushing 887999d to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 4, 2021
@bors bors merged commit 887999d into rust-lang:master Dec 4, 2021
@rustbot rustbot added this to the 1.59.0 milestone Dec 4, 2021
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (887999d): comparison url.

Summary: This benchmark run did not return any relevant changes.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

@cynecx cynecx deleted the unwind_asm branch December 4, 2021 13:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Amanieu Amanieu Amanieu left review comments

@bjorn3 bjorn3 bjorn3 left review comments

@camelid camelid camelid left review comments

Assignees

@oli-obk oli-obk

Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.59.0
Development

Successfully merging this pull request may close these issues.