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

Allow using -C force-unwind-tables=no when panic=unwind #83482

Merged
merged 1 commit into from Apr 11, 2021
Merged

Allow using -C force-unwind-tables=no when panic=unwind #83482

merged 1 commit into from Apr 11, 2021

Conversation

ghost
Copy link

@ghost ghost commented Mar 25, 2021

It seems LLVM still generates proper unwind tables even there is no uwtable attribute, unless I looked at the wrong place 🤔:
https://github.com/llvm/llvm-project/blob/c21016715f0ee4a36affdf7150ac135ca98b0eae/llvm/include/llvm/IR/Function.h#L666

Therefore, I assume it's safe to omit uwtable even when panic=unwind, and this PR removes the restriction that disallows using -C force-unwind-tables=no when panic=unwind.

@rust-highfive
Copy link
Collaborator

r? @davidtwco

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 25, 2021
@nagisa
Copy link
Member

nagisa commented Mar 25, 2021

So, what the linked LLVM code says is that unwinding tables will be generated if a function has a personality function.

A function will only have such a function associated if it has any in scope cleanups, or other unwind-interrupting mechanisms. A plain function that does not have any cleanups would still break unwinding here, i believe.

Basically this needs a test that verifies a stack along the lines of can work:

#0 panics
#1 no_cleanups_1
#2 has_cleanups_or_catches_panics
#3 no_cleanups_2

and similar. The calls mustn't be inlined nor tail calls.

@nikic
Copy link
Contributor

nikic commented Mar 25, 2021

@nagisa The code also checks for !doesNotThrow(), which is true for all functions which aren't nounwind, regardless of whether they have a personality function.

@nagisa
Copy link
Member

nagisa commented Mar 25, 2021

Ah, seems good to me then. Definitely can avoid generation of a fair amount of unnecessary data by default, then.

@ghost
Copy link
Author

ghost commented Mar 26, 2021

I think some more tests could still be helpful... @rustbot label -S-waiting-on-review S-waiting-on-author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 26, 2021
@davidtwco
Copy link
Member

r? @nagisa

@rust-highfive rust-highfive assigned nagisa and unassigned davidtwco Mar 26, 2021
@ghost
Copy link
Author

ghost commented Mar 26, 2021

@rustbot label S-waiting-on-review -S-waiting-on-author

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 26, 2021
@nagisa
Copy link
Member

nagisa commented Mar 28, 2021

r=me once the comments about tests are addressed and the commits squashed.

@nagisa
Copy link
Member

nagisa commented Apr 4, 2021

@bors r+ Thanks!

@bors
Copy link
Contributor

bors commented Apr 4, 2021

📌 Commit d1c591b has been approved by nagisa

@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 Apr 4, 2021
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Apr 4, 2021
Allow using `-C force-unwind-tables=no` when `panic=unwind`

It seems LLVM still generates proper unwind tables even there is no `uwtable` attribute, unless I looked at the wrong place 🤔:
https://github.com/llvm/llvm-project/blob/c21016715f0ee4a36affdf7150ac135ca98b0eae/llvm/include/llvm/IR/Function.h#L666

Therefore, I *assume* it's safe to omit `uwtable` even when `panic=unwind`, and this PR removes the restriction that disallows using `-C force-unwind-tables=no` when `panic=unwind`.
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Apr 4, 2021
Allow using `-C force-unwind-tables=no` when `panic=unwind`

It seems LLVM still generates proper unwind tables even there is no `uwtable` attribute, unless I looked at the wrong place 🤔:
https://github.com/llvm/llvm-project/blob/c21016715f0ee4a36affdf7150ac135ca98b0eae/llvm/include/llvm/IR/Function.h#L666

Therefore, I *assume* it's safe to omit `uwtable` even when `panic=unwind`, and this PR removes the restriction that disallows using `-C force-unwind-tables=no` when `panic=unwind`.
@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 Apr 10, 2021
@bors
Copy link
Contributor

bors commented Apr 10, 2021

⌛ Testing commit 40f51bb5ecae520228b79bdd69e33dbf64468c4a with merge 4f5e9c3324a8ffce36bb8d13139b1c74e9f51c14...

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Apr 10, 2021

💔 Test failed - checks-actions

@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 Apr 10, 2021
@ghost
Copy link
Author

ghost commented Apr 11, 2021

The failure was caused by dso_local between define and void (#83592). I rebased and changed // CHECK-LABEL: define void @foo to // CHECK-LABEL: define{{.*}}void @foo. Now codegen tests pass for me locally with --target wasm32-unknown-emscripten.

But the UI test failed...
   = note: error: undefined symbol: __gxx_personality_v0 (referenced by top-level compiled C/C++ code)
           warning: Link with `-s LLD_REPORT_UNDEFINED` to get more information on undefined symbols
           warning: To disable errors for undefined symbols use `-s ERROR_ON_UNDEFINED_SYMBOLS=0`
           warning: ___gxx_personality_v0 may need to be added to EXPORTED_FUNCTIONS if it arrives from a system library
           Error: Aborting compilation due to previous errors
           emcc: error: '/usr/bin/node /usr/share/emscripten/src/compiler.js /tmp/tmpsj7faogo.txt' failed (1)
           

error: aborting due to previous error

EDIT: Never mind. src/test/ui/builtin-clone-unwind.rs failed for me too. Maybe it's my local Emscripten issue...

@nagisa
Copy link
Member

nagisa commented Apr 11, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Apr 11, 2021

📌 Commit 2fd4dd2 has been approved by nagisa

@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 Apr 11, 2021
@bors
Copy link
Contributor

bors commented Apr 11, 2021

⌛ Testing commit 2fd4dd2 with merge a866124...

@bors
Copy link
Contributor

bors commented Apr 11, 2021

☀️ Test successful - checks-actions
Approved by: nagisa
Pushing a866124 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 11, 2021
@bors bors merged commit a866124 into rust-lang:master Apr 11, 2021
@rustbot rustbot added this to the 1.53.0 milestone Apr 11, 2021
@ghost ghost deleted the uwtable branch April 12, 2021 06:40
@ghost
Copy link
Author

ghost commented Jun 3, 2021

The documentation seems still indicating that panic=unwind might require force-unwind-tables to not be set to "no":

  • n, no, or off: Unwind tables are not forced to be generated. If unwind
    tables are required by the target or -C panic=unwind, an error will be
    emitted.

Created #85951 to update it.

JohnTitor added a commit to JohnTitor/rust that referenced this pull request Jun 8, 2021
…eklabnik

Update the documentation of `-C force-unwind-tables` for rust-lang#83482

`panic=unwind` does not require `force-unwind-tables` to be "yes" anymore.
I forgot to update this in rust-lang#83482.
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Jun 8, 2021
…eklabnik

Update the documentation of `-C force-unwind-tables` for rust-lang#83482

`panic=unwind` does not require `force-unwind-tables` to be "yes" anymore.
I forgot to update this in rust-lang#83482.
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 8, 2021
Rollup of 11 pull requests

Successful merges:

 - rust-lang#85906 (Use `Iterator::find` instead of open-coding it)
 - rust-lang#85951 (Update the documentation of `-C force-unwind-tables` for rust-lang#83482)
 - rust-lang#85985 (Clarify documentation of slice sorting methods)
 - rust-lang#85989 (Remove rustfmt tests from top-level .gitattributes)
 - rust-lang#86074 (Default panic message should print Box<dyn Any>)
 - rust-lang#86078 (Type page font weight)
 - rust-lang#86090 (:arrow_up: rust-analyzer)
 - rust-lang#86095 (Search description codeblock)
 - rust-lang#86096 (Comment out unused error codes and add description for E0316)
 - rust-lang#86101 (Correct type signature in doc for Bound::as_mut)
 - rust-lang#86103 (Remove lifetime hack)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants