Skip to content

Conversation

Yamakaky
Copy link
Contributor

@Yamakaky Yamakaky commented May 7, 2017

Following #40264. It only cleans the bottom of the trace (after the main). It handles correctly the normal main, tests, benchmarks and threads.

I kept skipped_before since it will be used later for the cleaning of the top.

@rust-highfive
Copy link
Contributor

r? @brson

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

@Yamakaky
Copy link
Contributor Author

Yamakaky commented May 7, 2017

r? @alexcrichton

@rust-highfive rust-highfive assigned alexcrichton and unassigned brson May 7, 2017
@carols10cents carols10cents added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 8, 2017
Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

Looks good! Could a test be added for this as well?

@@ -122,7 +158,7 @@ pub fn log_enabled() -> Option<PrintFormat> {
}

let val = match env::var_os("RUST_BACKTRACE") {
Some(x) => if &x == "0" {
Some(x) => if &x == "0" || &x == "" {
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps we could hold off on changing the interpretation of RUST_BACKTRACE for now? I'm wary to add more and more cases for positive/negative intepretation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK

let _ = resolve_symname(*frame, |symname| {
if let Some(mangled_symbol_name) = symname {
// Use grep to find the concerned functions
if mangled_symbol_name.contains("__rust_begin_short_backtrace") {
Copy link
Member

Choose a reason for hiding this comment

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

This should have a deterministic symbol name, right? Would it be possible to use == here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Ah yes right, that's a good point.

@alexcrichton
Copy link
Member

Ok looks good to me! Want to squash and I'll r+?

@Yamakaky
Copy link
Contributor Author

Yamakaky commented May 9, 2017

Yeay !

When `RUST_BACKTRACE=1`, remove all frames after
`__rust_maybe_catch_panic`. Tested on `main`, threads, tests and
benches. Cleaning of the top of the stacktrace is let to a future PR.

Fixes rust-lang#40201

See rust-lang#41815
@Yamakaky Yamakaky force-pushed the improve-backtrace-bottom branch from 7b41ca0 to ca8b754 Compare May 9, 2017 17:52
@Yamakaky
Copy link
Contributor Author

Yamakaky commented May 9, 2017

(not really proud of the commit's title ^^)

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Collaborator

bors commented May 9, 2017

📌 Commit ca8b754 has been approved by alexcrichton

@alexcrichton alexcrichton added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label May 9, 2017
frewsxcv added a commit to frewsxcv/rust that referenced this pull request May 10, 2017
…r=alexcrichton

Improve cleaning of the bottom of the backtrace

Following rust-lang#40264. It only cleans the bottom of the trace (after the main). It handles correctly the normal main, tests, benchmarks and threads.

I kept `skipped_before` since it will be used later for the cleaning of the top.
@bors
Copy link
Collaborator

bors commented May 10, 2017

⌛ Testing commit ca8b754 with merge 25a1617...

bors added a commit that referenced this pull request May 10, 2017
Improve cleaning of the bottom of the backtrace

Following #40264. It only cleans the bottom of the trace (after the main). It handles correctly the normal main, tests, benchmarks and threads.

I kept `skipped_before` since it will be used later for the cleaning of the top.
@bors
Copy link
Collaborator

bors commented May 10, 2017

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

@bors bors merged commit ca8b754 into rust-lang:master May 10, 2017
@Yamakaky Yamakaky deleted the improve-backtrace-bottom branch May 10, 2017 16:29
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. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants