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

std: Add a backtrace module #64154

Merged
merged 1 commit into from
Sep 11, 2019
Merged

Conversation

alexcrichton
Copy link
Member

This commit adds a backtrace module to the standard library, as
designed in RFC 2504. The Backtrace type is intentionally very
conservative, effectively only allowing capturing it and printing it.

Additionally this commit also adds a backtrace method to the Error
trait which defaults to returning None, as specified in RFC 2504.
More information about the design here can be found in RFC 2504 and in
the tracking issue.

Implementation-wise this is all based on the backtrace crate and very
closely mirrors the backtrace::Backtrace type on crates.io. Otherwise
it's pretty standard in how it handles everything internally.

cc #53487

@rust-highfive
Copy link
Collaborator

r? @cramertj

(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 Sep 4, 2019
@alexcrichton
Copy link
Member Author

r? @sfackler

@alexcrichton
Copy link
Member Author

Logistically I want #64152 to land before this, and then I want to leverage that to ensure that the standard library only has one piece of code for printing backtraces instead of both here and for panics.

(in case anyone's wondering why it's duplicated here)

@withoutboats
Copy link
Contributor

Thanks for doing this @alexcrichton!

@cbeck88
Copy link

cbeck88 commented Sep 5, 2019

This is nice functionality, but because it is added directly to std::error::Error, it blocks #64017
which would have succeeded in moving Error to the alloc crate, or at least, that seems to be why it was closed?

Can we consider doing this differently so that these PR's are still compatible? It would really help rust portability if std::error::Error could be the one recommendable way to represent errors. Ideally it can eventually be in core, and moving to alloc is big progress. If it is forever going to be an std-only thing then it will make it hard to be used in embedded, and then libraries that want to be embedded-friendly won't be able to use std::error::Error.

I think one good way might be to make a Backtrace trait, and make Error::backtrace() return Option<&dyn Backtrace>. There's some overhead for the virtual dispatch, but your backtrace object contains dynamic allocations anyways, so it's probably negligible. This kind of overhead is similar to fn source(&self) -> Option<&dyn Error + 'static> anyways. It looks that Backtrace interface is extremely simple.

There are a few other ways I thought about, you probably can think of more than I can.
Interested what you think.

src/libstd/backtrace.rs Outdated Show resolved Hide resolved
@sfackler
Copy link
Member

sfackler commented Sep 5, 2019 via email

@withoutboats
Copy link
Contributor

@cbeck88 There are solutions to someday making the Error trait available to no_std applications. Two possible solutions to the problem of backtraces, which don't require changing this API at all, are discussed toward the end of the closed PR.

@alexcrichton
Copy link
Member Author

I've updated with the various comments here. @cbeck88 I don't have anything to add over what @withoutboats already mentioned.

@sfackler
Copy link
Member

sfackler commented Sep 5, 2019

@bors r+

@bors
Copy link
Contributor

bors commented Sep 5, 2019

📌 Commit abe20c5a2fc489c1f2b5d41dbe43421c87e3078e has been approved by sfackler

@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 Sep 5, 2019
@alexcrichton
Copy link
Member Author

@bors: r-

just gonna be sure to sequence this behind #64152, but I'll carry over the r+ after that rebase. Thanks for reviewing @sfackler!

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 5, 2019
@cbeck88
Copy link

cbeck88 commented Sep 5, 2019

@withoutboats @alexcrichton thanks, makes sense

@bors
Copy link
Contributor

bors commented Sep 8, 2019

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

This commit adds a `backtrace` module to the standard library, as
designed in [RFC 2504]. The `Backtrace` type is intentionally very
conservative, effectively only allowing capturing it and printing it.

Additionally this commit also adds a `backtrace` method to the `Error`
trait which defaults to returning `None`, as specified in [RFC 2504].
More information about the design here can be found in [RFC 2504] and in
the [tracking issue].

Implementation-wise this is all based on the `backtrace` crate and very
closely mirrors the `backtrace::Backtrace` type on crates.io. Otherwise
it's pretty standard in how it handles everything internally.

[RFC 2504]: https://github.com/rust-lang/rfcs/blob/master/text/2504-fix-error.md
[tracking issue]: rust-lang#53487

cc rust-lang#53487
@alexcrichton
Copy link
Member Author

@bors: r=sfackler

@bors
Copy link
Contributor

bors commented Sep 9, 2019

📌 Commit abe20c5a2fc489c1f2b5d41dbe43421c87e3078e has been approved by sfackler

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 9, 2019
@alexcrichton
Copy link
Member Author

@bors: r=sfackler

@bors
Copy link
Contributor

bors commented Sep 10, 2019

📌 Commit 34662c6 has been approved by sfackler

@bors
Copy link
Contributor

bors commented Sep 11, 2019

⌛ Testing commit 34662c6 with merge bba531caa5a6e8291c3f37dd3c44700f436e6cd5...

@bors
Copy link
Contributor

bors commented Sep 11, 2019

💥 Test timed out

@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 Sep 11, 2019
@alexcrichton
Copy link
Member Author

@bors: retry

Looks like a normal timeout, but let's see if it's affected by this..

@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 Sep 11, 2019
@bors
Copy link
Contributor

bors commented Sep 11, 2019

⌛ Testing commit 34662c6 with merge fe6d05a...

bors added a commit that referenced this pull request Sep 11, 2019
std: Add a `backtrace` module

This commit adds a `backtrace` module to the standard library, as
designed in [RFC 2504]. The `Backtrace` type is intentionally very
conservative, effectively only allowing capturing it and printing it.

Additionally this commit also adds a `backtrace` method to the `Error`
trait which defaults to returning `None`, as specified in [RFC 2504].
More information about the design here can be found in [RFC 2504] and in
the [tracking issue].

Implementation-wise this is all based on the `backtrace` crate and very
closely mirrors the `backtrace::Backtrace` type on crates.io. Otherwise
it's pretty standard in how it handles everything internally.

[RFC 2504]: https://github.com/rust-lang/rfcs/blob/master/text/2504-fix-error.md
[tracking issue]: #53487

cc #53487
@bors
Copy link
Contributor

bors commented Sep 11, 2019

☀️ Test successful - checks-azure
Approved by: sfackler
Pushing fe6d05a to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 11, 2019
@bors bors merged commit 34662c6 into rust-lang:master Sep 11, 2019
@rust-highfive
Copy link
Collaborator

📣 Toolstate changed by #64154!

Tested on commit fe6d05a.
Direct link to PR: #64154

💔 rls on linux: test-pass → test-fail (cc @Xanewok, @rust-lang/infra).

rust-highfive added a commit to rust-lang-nursery/rust-toolstate that referenced this pull request Sep 11, 2019
Tested on commit rust-lang/rust@fe6d05a.
Direct link to PR: <rust-lang/rust#64154>

💔 rls on linux: test-pass → test-fail (cc @Xanewok, @rust-lang/infra).
@alexcrichton alexcrichton deleted the std-backtrace branch September 11, 2019 19:13
@RalfJung
Copy link
Member

RalfJung commented Sep 12, 2019

This PR broke xargo, or at least the Xargo configuration used by Miri:

error[E0432]: unresolved import `crate::sys_common::backtrace`
  --> /home/travis/build/RalfJung/miri-test-libstd/rust-src-patched/src/libstd/backtrace.rs:98:24
   |
98 | use crate::sys_common::backtrace::{output_filename, lock};
   |                        ^^^^^^^^^ could not find `backtrace` in `sys_common`

@alexcrichton is it possible that libstd no longer compiles with the default features?

EDIT: Ah, this was already reported at #64410.

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.

9 participants