Add libbacktrace support for Apple platforms #43422

JohnColanduoni · 2017-07-23T08:21:22Z

This adds support for Mach-O files (used for binaries on macOS and iOS) to libbacktrace, and enables libbacktrace on those platforms. Unlike the current dladdr based method used on these platforms, it provides filenames and line numbers (fixing #24346 for Apple targets).

These changes to libbacktrace are on their way upstream, but have been stalled for a number of months so I've put together a PR that adds them directly to Rust's fork.

libbacktrace's internal detection of the executable path that was relied on for other platforms does not work for macOS, so I've replaced the stub backtrace::gnu::get_executable_filename with a working version based on _NSGetExecutablePath (the same method used to implement std::env::current_exe). I've avoided calling that function to prevent having to play with the type of the returned filename, since the existing version calls for a null-terminated Vec<c_char> instead of a Vec<u8> with no null termination.

rust-highfive · 2017-07-23T08:21:34Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nikomatsakis (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

nikomatsakis · 2017-07-23T13:47:14Z

This is great! Not having filenames / line-numbers on mac os X is so sad. This PR looks good to me, but I think it's missing a few things. In particular, there is no testing! Poking around in the codebase, it appears that our existing RUST_BACKTRACE machinery is tested in the test src/test/run-pass/backtrace-debuginfo.rs, which seems to be disabled for OS X and iOS.

Truth is though that I am not an expert on this stuff in general. cc @michaelwoerister and @alexcrichton, both of whom seem like likely candidates to give good suggestions for other tests and things to re-enable.

nikomatsakis

Well, this seems to be passing all the existing tests, so I am happy with it, but I'd like to get the advice of @alexcrichton or someone else on how to handle the patch to libbacktrace.

nikomatsakis · 2017-07-23T18:42:41Z

src/test/run-pass/backtrace-debuginfo.rs

-        if cfg!(any(target_os = "macos",
-                    target_os = "ios",
-                    target_os = "android",
+        if cfg!(any(target_os = "android",


nikomatsakis · 2017-07-23T18:44:06Z

src/libbacktrace/macho.c

@@ -0,0 +1,1414 @@
+/* macho.c -- Get debug data from an Mach-O file for backtraces.


Can you maybe insert a link to the pending patch in libbacktrace itself?

Clearly we're not in a good position to review this independently, but it'd be nice to be able to place this in the context of that project.

nikomatsakis · 2017-07-23T18:44:45Z

r? @alexcrichton

alexcrichton · 2017-07-24T14:39:24Z

@JohnColanduoni are the upstream changes located at ianlancetaylor/libbacktrace#2? If that's the case, do you know if that's "close to landing"?

alexcrichton · 2017-07-24T14:40:27Z

src/libstd/sys/unix/backtrace/mod.rs

    pub fn get_executable_filename() -> io::Result<(Vec<c_char>, fs::File)> {
        Err(io::Error::new(io::ErrorKind::Other, "Not implemented"))
    }
+
+    #[cfg(any(target_os = "macos", target_os = "ios"))]
+    pub fn get_executable_filename() -> io::Result<(Vec<c_char>, fs::File)> {


This looks almost equivalent to std::env::current_exe(), could you elaborate a bit on why that couldn't be used here?

@alexcrichton the PR description talks about that:

I've avoided calling that function to prevent having to play with the type of the returned filename, since the existing version calls for a null-terminated Vec<c_char> instead of a Vec with no null termination.

Ah yes indeed! @JohnColanduoni would you be willing to change this to use std::env::current_exe? I believe the return value can then be converted to a Vec of the appropriate type to nul terminate (or just create a CString with) and such.

@alexcrichton Sorry I missed your message, I've made the requested changes.

JohnColanduoni · 2017-07-25T02:41:26Z

@alexcrichton I made the changes Ian requested about two months ago but I don't think he's had time to look at them.

alexcrichton · 2017-07-26T22:59:52Z

@JohnColanduoni ok that's fine, just wanted to make sure everything was linked up correctly! I think it's ok to land this ahead of it landing upstream if it's taking awhile.

Out of curiosity, how confident are you in this implementation? Historically we've had a number of segfaults due to libbacktrace bugs, which in turn could unfortunately translate to security vulnerabilities for Rust programs if we're not careful! Do know know if this has been thoroughly tested and such?

JohnColanduoni · 2017-08-01T21:07:01Z

@alexcrichton I'm fairly confident, I coded it under the assumption that the input may be malformed and the Mach-O header and symbol table formats are pretty simple. The more sophisticated DWARF data is handled by the same code that handles the ELF/PE versions.

alexcrichton · 2017-08-09T14:51:57Z

@rust-lang/libs any thoughts on this? I'm a little worried about the maintenance burden we're bringing on ourselves as downstream consumers of libbacktrace for this, but this is a highly desired feature to have filename/line number working on OSX when possible.

sfackler · 2017-08-09T15:50:11Z

We should land it IMO.

alexcrichton · 2017-08-10T16:50:50Z

@bors: r+

Ok Thanks @JohnColanduoni! Let's see how this goes.

bors · 2017-08-10T16:50:51Z

📌 Commit 5991ab7 has been approved by alexcrichton

bors · 2017-08-10T18:44:21Z

⌛ Testing commit 5991ab7 with merge d5ca0e6da9ce5f933a1c8fb0d0f9d985cad0a01a...

bors · 2017-08-10T19:48:11Z

💔 Test failed - status-travis

kennytm · 2017-08-11T00:01:50Z

Failed to build libbacktrace for iOS at stage2-libstd. Legit.

[00:56:47] --- stderr
[00:56:47] Invalid configuration `aarch64-apple-ios': system `ios' not recognized
[00:56:47] configure: error: /bin/sh /Users/travis/build/rust-lang/rust/src/libstd/../libbacktrace/config.sub aarch64-apple-ios failed

Probably just change this.

diff --git a/src/libbacktrace/config.sub b/src/libbacktrace/config.sub
index da6d1b6826..f6cac20b58 100644
--- a/src/libbacktrace/config.sub
+++ b/src/libbacktrace/config.sub
@@ -1392,7 +1392,7 @@ case $os in
 	      | -mingw32* | -mingw64* | -linux-gnu* | -linux-android* \
 	      | -linux-newlib* | -linux-musl* | -linux-uclibc* \
 	      | -uxpv* | -beos* | -mpeix* | -udk* | -moxiebox* \
-	      | -interix* | -uwin* | -mks* | -rhapsody* | -darwin* | -opened* \
+	      | -interix* | -uwin* | -mks* | -rhapsody* | -darwin* | -ios* | -opened* \
 	      | -openstep* | -oskit* | -conix* | -pw32* | -nonstopux* \
 	      | -storm-chaos* | -tops10* | -tenex* | -tops20* | -its* \
 	      | -os2* | -vos* | -palmos* | -uclinux* | -nucleus* \

bors · 2017-08-15T22:32:29Z

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

gilescope · 2017-08-16T16:35:10Z

Would be a great early Christmas present to get this working!

ids1024 · 2017-08-16T22:28:06Z

Changing config.sub (as @kennytm suggests) should be unnecessary once the conflicts with master are resolved; in #43635 (which this is conflicting with) I updated the config.sub from the latest upstream version, which seems to add ios.

aidanhs · 2017-08-23T23:50:38Z

@JohnColanduoni just a friendly ping to keep this on your radar! I can see there are some merge conflicts.

aidanhs · 2017-08-31T14:29:32Z

I'm going to close this to keep the PR queue clean, but we would love to get this in so please feel free to make a new PR, or ping me once resolving the conflicts and I'll reopen!

@JohnColanduoni

Add libbacktrace support for Apple platforms (resubmitted) Resubmitting #43422 rebased on the current master (cc @JohnColanduoni). I have added an additional commit to fallback to `dladdr`-based `resolve_symbol` if `libbacktrace` returns `None`, otherwise the stack trace will be full of `<unknown>` when you forget to pass the `-g` flag (actually it seems — at least on macOS — the `dladdr` symbol is more accurate than the `libbacktrace` one).

Add libbacktrace support for Apple platforms

df79cd9

rust-highfive assigned nikomatsakis Jul 23, 2017

Enable backtrace tests for macOS and iOS

6fca8aa

nikomatsakis approved these changes Jul 23, 2017

View reviewed changes

rust-highfive assigned alexcrichton and unassigned nikomatsakis Jul 23, 2017

Add upstream pull request link for libbacktrace

6dd9ea7

carols10cents added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 24, 2017

alexcrichton reviewed Jul 24, 2017

View reviewed changes

carols10cents 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 Jul 31, 2017

kennytm mentioned this pull request Aug 9, 2017

RFC: Implicit caller location (third try to the unwrap/expect line info problem) rust-lang/rfcs#2091

Merged

Use env::current_exe for libbacktrace on macOS

5991ab7

alexcrichton added the relnotes Marks issues that should be documented in the release notes of the next release. label Aug 10, 2017

aidanhs closed this Aug 31, 2017

kennytm mentioned this pull request Sep 1, 2017

Add libbacktrace support for Apple platforms (resubmitted) #44251

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add libbacktrace support for Apple platforms #43422

Add libbacktrace support for Apple platforms #43422

JohnColanduoni commented Jul 23, 2017 •

edited

Loading

rust-highfive commented Jul 23, 2017

nikomatsakis commented Jul 23, 2017

nikomatsakis left a comment

nikomatsakis Jul 23, 2017

nikomatsakis Jul 23, 2017

nikomatsakis commented Jul 23, 2017

alexcrichton commented Jul 24, 2017

alexcrichton Jul 24, 2017

codyps Jul 25, 2017

alexcrichton Jul 26, 2017

JohnColanduoni Aug 10, 2017

JohnColanduoni commented Jul 25, 2017

alexcrichton commented Jul 26, 2017

JohnColanduoni commented Aug 1, 2017

alexcrichton commented Aug 9, 2017

sfackler commented Aug 9, 2017

alexcrichton commented Aug 10, 2017

bors commented Aug 10, 2017

bors commented Aug 10, 2017

bors commented Aug 10, 2017

kennytm commented Aug 11, 2017 •

edited

Loading

bors commented Aug 15, 2017

gilescope commented Aug 16, 2017

ids1024 commented Aug 16, 2017

aidanhs commented Aug 23, 2017

aidanhs commented Aug 31, 2017

		@@ -0,0 +1,1414 @@
		/* macho.c -- Get debug data from an Mach-O file for backtraces.

Add libbacktrace support for Apple platforms #43422

Add libbacktrace support for Apple platforms #43422

Conversation

JohnColanduoni commented Jul 23, 2017 • edited Loading

rust-highfive commented Jul 23, 2017

nikomatsakis commented Jul 23, 2017

nikomatsakis left a comment

Choose a reason for hiding this comment

nikomatsakis Jul 23, 2017

Choose a reason for hiding this comment

nikomatsakis Jul 23, 2017

Choose a reason for hiding this comment

nikomatsakis commented Jul 23, 2017

alexcrichton commented Jul 24, 2017

alexcrichton Jul 24, 2017

Choose a reason for hiding this comment

codyps Jul 25, 2017

Choose a reason for hiding this comment

alexcrichton Jul 26, 2017

Choose a reason for hiding this comment

JohnColanduoni Aug 10, 2017

Choose a reason for hiding this comment

JohnColanduoni commented Jul 25, 2017

alexcrichton commented Jul 26, 2017

JohnColanduoni commented Aug 1, 2017

alexcrichton commented Aug 9, 2017

sfackler commented Aug 9, 2017

alexcrichton commented Aug 10, 2017

bors commented Aug 10, 2017

bors commented Aug 10, 2017

bors commented Aug 10, 2017

kennytm commented Aug 11, 2017 • edited Loading

bors commented Aug 15, 2017

gilescope commented Aug 16, 2017

ids1024 commented Aug 16, 2017

aidanhs commented Aug 23, 2017

aidanhs commented Aug 31, 2017

JohnColanduoni commented Jul 23, 2017 •

edited

Loading

kennytm commented Aug 11, 2017 •

edited

Loading