Skip to content

Unwinding support for inline assembly - #88439

Merged
bors merged 14 commits into
rust-lang:masterfrom
cynecx:unwind_asm
Dec 4, 2021
Merged

Unwinding support for inline assembly#88439
bors merged 14 commits into
rust-lang:masterfrom
cynecx:unwind_asm

Conversation

@cynecx

@cynecxcynecx commented Aug 28, 2021•
edited
Loading

Copy link
Copy Markdown
Contributor

@rust-highfiverust-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
bjorn3
bjorn3 reviewed Aug 28, 2021
View reviewed changes
Comment threadcompiler/rustc_llvm/llvm-wrapper/RustWrapper.cpp Outdated
camelid
camelid reviewed Aug 28, 2021
View reviewed changes
Comment threadcompiler/rustc_ast_pretty/src/pprust/state.rs Outdated
@rust-log-analyzer

This comment has been minimized.

Sign in to view
bjorn3
bjorn3 reviewed Aug 29, 2021
View reviewed changes
Comment threadcompiler/rustc_codegen_cranelift/src/base.rs Outdated
bjorn3
bjorn3 reviewed Aug 29, 2021
View reviewed changes
Comment threadcompiler/rustc_codegen_llvm/src/asm.rs Outdated
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@cynecxcynecx mentioned this pull request Aug 29, 2021
Open
@Amanieu

Amanieu commented Aug 30, 2021

Copy link
Copy Markdown
Member

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
Amanieu reviewed Aug 30, 2021
View reviewed changes
Comment threadcompiler/rustc_codegen_llvm/src/asm.rs Outdated
@Amanieu

Amanieu commented Aug 30, 2021

Copy link
Copy Markdown
Member

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

@cynecx

cynecx commented Aug 30, 2021•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

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

@Amanieu

Amanieu commented Aug 30, 2021

Copy link
Copy Markdown
Member

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

@cynecx

cynecx commented Aug 30, 2021•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

@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

Amanieu commented Aug 31, 2021

Copy link
Copy Markdown
Member

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

@AmanieuAmanieu left a comment

Copy link
Copy Markdown
Member

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?

Comment threadcompiler/rustc_codegen_cranelift/src/base.rs Outdated
@cynecx

cynecx commented Sep 4, 2021•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor
  • 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

bors commented Sep 9, 2021

Copy link
Copy Markdown
Collaborator

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

@Amanieu

Amanieu commented Sep 18, 2021

Copy link
Copy Markdown
Member

@cynecx Are you still working on this PR?

@cynecx

cynecx commented Sep 19, 2021

Copy link
Copy Markdown
ContributorAuthor

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

@cynecx
cynecxforce-pushed the unwind_asm branch 2 times, most recently from 68a3b41 to 8553004CompareSeptember 19, 2021 20:40
@Amanieu

Amanieu commented Sep 22, 2021

Copy link
Copy Markdown
Member

@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

@borsbors 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

bors commented Dec 2, 2021

Copy link
Copy Markdown
Collaborator

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

Amanieuand others added 14 commits December 3, 2021 23:51
@cynecx

cynecx commented Dec 3, 2021

Copy link
Copy Markdown
ContributorAuthor

@Amanieu rebased and test fixed.

@Amanieu

Amanieu commented Dec 3, 2021

Copy link
Copy Markdown
Member

@bors r+

@bors

bors commented Dec 3, 2021

Copy link
Copy Markdown
Collaborator

📌 Commit 3dbb621 has been approved by Amanieu

@borsbors 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

bors commented Dec 4, 2021

Copy link
Copy Markdown
Collaborator

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

@bors

bors commented Dec 4, 2021

Copy link
Copy Markdown
Collaborator

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

@borsbors added the merged-by-bors This PR was explicitly merged by bors. label Dec 4, 2021
@bors
bors merged commit 887999d into rust-lang:masterDec 4, 2021
@rustbotrustbot added this to the 1.59.0 milestone Dec 4, 2021
@rust-timer

rust-timer commented Dec 4, 2021

Copy link
Copy Markdown
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 freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@AmanieuAmanieuAmanieu left review comments
@bjorn3bjorn3bjorn3 left review comments
@camelidcamelidcamelid left review comments

Assignees

@oli-obkoli-obk

Labels

merged-by-borsThis PR was explicitly merged by bors.S-waiting-on-borsStatus: Waiting on bors to run and complete tests. Bors will change the label on completion.T-compilerRelevant 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.

11 participants

@cynecx@rust-log-analyzer@Amanieu@bors@rust-timer@bjorn3@camelid@oli-obk@apiraino@rust-highfive@rustbot