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

Add -Zuse-sync-unwind #117744

Merged
merged 1 commit into from
Jan 9, 2024
Merged

Add -Zuse-sync-unwind #117744

merged 1 commit into from
Jan 9, 2024

Conversation

quininer
Copy link
Contributor

@quininer quininer commented Nov 9, 2023

Currently Rust uses async unwind by default, but async unwind will bring non-negligible size overhead. it would be nice to allow users to choose this.

In addition, async unwind currently prevents LLVM from generate compact unwind for MachO, if one wishes to generate compact unwind for MachO, then also needs this flag.

@rustbot
Copy link
Collaborator

rustbot commented Nov 9, 2023

r? @wesleywiser

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 9, 2023
@rust-log-analyzer

This comment has been minimized.

@bjorn3
Copy link
Member

bjorn3 commented Nov 9, 2023

In addition, async unwind currently prevents LLVM from generate compact unwind for MachO, if one wishes to generate compact unwind for MachO, then also needs this flag.

For compact unwind info there is also the issue that compact unwinding only supports 3 different personality functions, all of which are used by apple itself (__gxx_personality_v0, __gcc_personality_v0 and __objc_personality_v0): #102754 The fix for that may be to only use compact unwind info when using one of these personality functions, which would mean that rust doesn't get to use compact unwinding at all.

@quininer
Copy link
Contributor Author

quininer commented Nov 9, 2023

The fix for that may be to only use compact unwind info when using one of these personality functions, which would mean that rust doesn't get to use compact unwinding at all.

Yes, if want to compact unwind you also must use -femit-compact-unwind-non-canonical=true flag. and __gcc_personality_v0 is rarely used, in production we only have __gxx_personality_v0, __objc_personality_v0 and _rust_eh_personality.

@quininer
Copy link
Contributor Author

ping @wesleywiser

@quininer
Copy link
Contributor Author

r? @bjorn3 Can you help take a look?

@rustbot rustbot assigned bjorn3 and unassigned wesleywiser Nov 16, 2023
@@ -95,11 +95,12 @@ pub fn sanitize_attrs<'ll>(

/// Tell LLVM to emit or not emit the information necessary to unwind the stack for the function.
#[inline]
pub fn uwtable_attr(llcx: &llvm::Context) -> &Attribute {
pub fn uwtable_attr(llcx: &llvm::Context, use_sync_unwind: Option<bool>) -> &Attribute {
// NOTE: We should determine if we even need async unwind tables, as they
// take have more overhead and if we can use sync unwind tables we
// probably should.
Copy link
Member

Choose a reason for hiding this comment

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

@durin42 do you think unconditionally (or by default) using sync only unwind tables would work?

@quininer
Copy link
Contributor Author

ping? @bjorn3 @durin42

@Noratrieb
Copy link
Member

Can you link some more material on the advantages and disadvantages of sync unwind vs async unwind? What are the advantages of async unwind?

@quininer
Copy link
Contributor Author

@Nilstrieb I'm not an expert on async unwind but I found some links.

https://maskray.me/blog/2020-11-08-stack-unwinding
https://groups.google.com/g/llvm-dev/c/KH4QAKPBQ5A
https://gcc.gnu.org/legacy-ml/gcc-help/2009-10/msg00195.html

I understand that async unwind is mainly used to support unwinding in function prologue/epilogue. It may be used by tools such as profiling tools (but usually frame pointer should be used). this is achieved by add more cfi to eh_frame, so there is an impact on binary size.

@apiraino
Copy link
Contributor

apiraino commented Dec 28, 2023

Do the proposed changes here need an MCP?

(mostly asking to someone from compiler team)

edit: PR was discussed by T-compiler meeting (on Zulip) and it was decided it doesnt need one

@quininer quininer force-pushed the add-z-sync-uw branch 2 times, most recently from 24d5856 to ff2d246 Compare December 31, 2023 07:04
@rustbot rustbot added O-unix Operating system: Unix-like T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Dec 31, 2023
@rustbot
Copy link
Collaborator

rustbot commented Dec 31, 2023

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

These commits modify compiler targets.
(See the Target Tier Policy.)

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

This flag specifies whether LLVM generates async unwind or sync unwind.
@quininer
Copy link
Contributor Author

quininer commented Jan 8, 2024

what do I need to do to move this PR forward?

@bjorn3
Copy link
Member

bjorn3 commented Jan 8, 2024

@bors r+

@bors
Copy link
Contributor

bors commented Jan 8, 2024

📌 Commit 12784c3 has been approved by bjorn3

It is now in the queue for this repository.

@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 Jan 8, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 8, 2024
Add -Zuse-sync-unwind

Currently Rust uses async unwind by default, but async unwind will bring non-negligible size overhead. it would be nice to allow users to choose this.

In addition, async unwind currently prevents LLVM from generate compact unwind for MachO, if one wishes to generate compact unwind for MachO, then also needs this flag.
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 9, 2024
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#117744 (Add -Zuse-sync-unwind)
 - rust-lang#118649 (Make inductive cycles in coherence ambiguous always)
 - rust-lang#118979 (Use `assert_unsafe_precondition` for `char::from_u32_unchecked`)
 - rust-lang#119562 (Rename `pointer` field on `Pin`)
 - rust-lang#119619 (mir-opt and custom target fixes)
 - rust-lang#119632 (Fix broken build for ESP IDF due to rust-lang#119026)
 - rust-lang#119712 (Adding alignment to the cases to test for specific error messages.)
 - rust-lang#119734 (Miri subtree update)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 9, 2024
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#117744 (Add -Zuse-sync-unwind)
 - rust-lang#118649 (Make inductive cycles in coherence ambiguous always)
 - rust-lang#118979 (Use `assert_unsafe_precondition` for `char::from_u32_unchecked`)
 - rust-lang#119619 (mir-opt and custom target fixes)
 - rust-lang#119632 (Fix broken build for ESP IDF due to rust-lang#119026)
 - rust-lang#119712 (Adding alignment to the cases to test for specific error messages.)
 - rust-lang#119734 (Miri subtree update)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 9e4843e into rust-lang:master Jan 9, 2024
11 checks passed
@rustbot rustbot added this to the 1.77.0 milestone Jan 9, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jan 9, 2024
Rollup merge of rust-lang#117744 - quininer:add-z-sync-uw, r=bjorn3

Add -Zuse-sync-unwind

Currently Rust uses async unwind by default, but async unwind will bring non-negligible size overhead. it would be nice to allow users to choose this.

In addition, async unwind currently prevents LLVM from generate compact unwind for MachO, if one wishes to generate compact unwind for MachO, then also needs this flag.
@quininer quininer deleted the add-z-sync-uw branch January 9, 2024 08:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-unix Operating system: Unix-like S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants