-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Support using LLVM's libunwind as the unwinder implementation #59089
Conversation
(rust_highfive has picked a reviewer for you, use r? to override) |
|
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
.gitmodules
Outdated
@@ -47,3 +47,6 @@ | |||
[submodule "src/doc/embedded-book"] | |||
path = src/doc/embedded-book | |||
url = https://github.com/rust-embedded/book.git | |||
[submodule "src/libunwind/libunwind"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't this be the libunwind from the LLVM monorepo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Especially since we already have a llvm-project
submodule with a copy of libunwind
in tree, that would be better imo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine switching to llvm-project
, but that version is behind upstream and is missing some important changes to libunwind that removed the C++ standard library dependency. We need to roll it first.
Looks like the travis failures are coming from tidy errors in |
Thanks for this @petrhosek! I had no idea it was so easy to pull this in :) To get an idea, are y'all thinking of turning this on by default for Fuchsia? Additionally, do you know if there are other targets which may wish to turn this on by default? Turning this on by default seems like a tantalizing proposition at this point, but we probably can't take a dependency on a C++ compiler just yet and we'd also want to give this some time to bake. In terms of llvm-project and such, when you say upstream to you mean rust-lang's fork of the llvm-project repo? If that's all that's necessary to update I think we'll want to go ahead and pull the necessary patches into our fork and then use the I'm also somewhat surprised that there's no source level changes here, even for platforms like MSVC. This seems to have build logic for MSVC/Linux/macOS, but I was wondering if you'd tested it there as well? MSVC for example I thought we only used kernel32.dll things rather than a linked in unwinder. |
Yes, we plan to enable this for our Rust toolchain build. This is currently a blocker for deployment in our internal Linux environment since we don't have
To clarify, this requires only C++ compiler, not C++ standard library (once we bring in the libuwind changes).
Yes, that's what I had in mind. I'd be fine using rust-lang's fork if we can cherry-pick the necessary patches (assuming that's easier than rolling the entire repository forward): llvm/llvm-project@7fac517
I haven't tested the change on those platforms yet. It might be safer to restrict this only for Linux and Fuchsia. I'd expect macOS to be safe as well since libunwind is already being used there as the default unwinder. I'm less familiar with the status on Windows. |
AFAIK Dwarf unwinding doesn't work yet so it cannot be used for |
@petrhosek ok cool, that all sounds great! In that case how about:
How's that sound? (and just to confirm, LLVM's libunwind has the same API as the libgcc_s one we expect, so that's why there's no source-level changes necessary on Linux?) |
How do you do that? Do I need to send those changes as pull requests to your llvm-project fork or can you or someone else just cherry pick them directly?
SGTM
Yes, it's designed to be a drop-in replacement. We're using LLVM's libunwind in our C/C++ toolchain for both Linux and Fuchsia without any problems. |
Oh sorry yeah, a cherry-pick would be doing the cherry-pick locally and then sending a PR to https://github.com/rust-lang/llvm-project/tree/rustc/8.0-2019-01-16. I can merge that and then afterwards the submodule can be updated here |
☔ The latest upstream changes (presumably #59226) made this pull request unmergeable. Please resolve the merge conflicts. |
@petrhosek when rebasing this can you also update the llvm-project submodule to rust-lang/llvm-project@8f80418? I was testing out an experimental change in #58854 on a separate branch, and that PR merged, so I've merged my temporary branch into the master branch of rust-lang/llvm-project which should have both your changes and my changes |
Ah actually in parallel as well we have another submodule update which includes the changes needed for this PR as well as the ones I mentioned previously (JFYI) |
Ping @petrhosek, this PR requires rebase. LLVM submodule has been updated by another PR so it's no longer necessary. |
☔ The latest upstream changes (presumably #59464) made this pull request unmergeable. Please resolve the merge conflicts. |
Ping? Would it be possible to take a look again? |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Oops sorry didn't realize this was ready to go again! If you want to run Otherwise this lgtm, so @bors: delegate+ |
✌️ @petrhosek can now approve this pull request |
@bors r+ |
📌 Commit 6c0c9a00f9d4ee7c7c136bddf9b5a9f76b4acee5 has been approved by |
🔒 Merge conflict This pull request and the master branch diverged in a way that cannot be automatically merged. Please rebase on top of the latest master branch, and let the reviewer approve again. How do I rebase?Assuming
You may also read Git Rebasing to Resolve Conflicts by Drew Blessing for a short tutorial. Please avoid the "Resolve conflicts" button on GitHub. It uses Sometimes step 4 will complete without asking for resolution. This is usually due to difference between how Error message
|
This avoids the dependency on host libraries such as libgcc_s which may be undesirable in some deployment environments where these aren't available.
@bors r+ |
📌 Commit 86d1678 has been approved by |
@petrhosek You might want to use |
Support using LLVM's libunwind as the unwinder implementation This avoids the dependency on host libraries such as libgcc_s which may be undesirable in some deployment environments where these aren't available.
☀️ Test successful - checks-travis, status-appveyor |
Is there a reason you're not just invoking CMake? |
We could use I'm not sure if it's necessarily going to reduce the need for maintenance. libunwind's CMake build is also evolving, and most new flags are usually gated on options which would need to be reflected here as well. |
This avoids the dependency on host libraries such as libgcc_s which
may be undesirable in some deployment environments where these aren't
available.