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

Make backtraces work on Redox, copying Unix implementation #43635

Merged
merged 3 commits into from
Aug 15, 2017

Conversation

ids1024
Copy link
Contributor

@ids1024 ids1024 commented Aug 4, 2017

The backtrace/ directory here is the same as the Unix one, except for adding an implementation of get_executable_filename.

@rust-highfive
Copy link
Collaborator

r? @brson

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


#[cfg(any(target_os = "macos", target_os = "ios",
target_os = "emscripten"))]
#[path = "dladdr.rs"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Redox is never macos, so no need for dladdr.rs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True; I thought it might be best not to modify those files too much (I just changed get_executable_filename in mod.rs) so they can be easily synced with the Unix version, but otherwise several things could be removed.

The duplicated code is somewhat annoying; I don't think it's avoidable though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess (multiple) entire files that are useless is probably a waste, so I've pushed a commit addressing this.

@shepmaster shepmaster added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 4, 2017
@arielb1
Copy link
Contributor

arielb1 commented Aug 8, 2017

ping @brson - be our rubber stamp

@jackpot51
Copy link
Contributor

@brson @arielb1 @shepmaster Any updates on this?

@alexcrichton
Copy link
Member

Thanks @ids1024! Out of curiosity, where'd the changes to config.sub come from? We ideally like to stick pretty close to upstream, but if it's pretty easy to re-apply the changes to upstream that shouldn't be too bad.

@alexcrichton alexcrichton assigned alexcrichton and unassigned brson Aug 15, 2017
@ids1024
Copy link
Contributor Author

ids1024 commented Aug 15, 2017

I could change the config.sub modification to just the single line required for Redox, but I though it was best to just update it from GNU: http://git.savannah.gnu.org/gitweb/?p=config.git;a=blob;f=config.sub

Future releases of libbacktrace will probably have that fix.

@alexcrichton
Copy link
Member

Ok I don't really know what config.sub is anyway but the commit message is at least clear enough to me of how to reproduce it!

In that case let's...

@bors: r+

@bors
Copy link
Contributor

bors commented Aug 15, 2017

📌 Commit 9d67d5a has been approved by alexcrichton

@ids1024
Copy link
Contributor Author

ids1024 commented Aug 15, 2017

config.sub is used by autotools to get the canonical name of the system from the target triplet (trivial for most systems, but a few have several variations of their triplets that it needs to handle). It needs to be updated when new systems are added.

config.guess is similar; it guesses a target triplet based on uname output. That is only relevant for native compilation though.

TLDR: It's one of the weird and ugly things in autotools that seem awful, but does help with portability to various strange systems.

This update seems to add fuchsia as well, so if they decide to implement Rust backtraces they won't run into that (small) issue.

@bors
Copy link
Contributor

bors commented Aug 15, 2017

⌛ Testing commit 9d67d5a with merge f25c228...

bors added a commit that referenced this pull request Aug 15, 2017
Make backtraces work on Redox, copying Unix implementation

The `backtrace/` directory here is the same as the Unix one, except for adding an implementation of `get_executable_filename`.
@bors
Copy link
Contributor

bors commented Aug 15, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing f25c228 to master...

@bors bors merged commit 9d67d5a into rust-lang:master Aug 15, 2017
@ids1024 ids1024 deleted the backtrace-redox branch October 5, 2017 14:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants