Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Better backtrace cleaning on panic #40201

Closed
Yamakaky opened this issue Mar 2, 2017 · 7 comments
Closed

Better backtrace cleaning on panic #40201

Yamakaky opened this issue Mar 2, 2017 · 7 comments

Comments

@Yamakaky
Copy link
Contributor

Yamakaky commented Mar 2, 2017

Followup of #38165.

The current behaviour is to maintain a blacklist of frames to remove from the backtrace. This poses problems in portability and accuracy. For example, the bottom of the backtrace is not cleaned when the panic is in a test or an other thread.

I propose that just before calling the user main/test/closure, the calling code would capture a backtrace. Then, on panic, the frames present is the backtrace pre and post panic would be removed. That way, no need to have a big whitelist. Some small post-processing may be needed to remove some additional frames.

A few questions:

  • Is __rust_maybe_catch_panic used on all the platforms? If so, it could be used as a basis for this technique, since it's used for tests, the main thread and the secondary threads.
@Yamakaky
Copy link
Contributor Author

Yamakaky commented Mar 2, 2017

r? @alexcrichton

@alexcrichton
Copy link
Member

I don't think we should too aggressively try to clean backtraces because we run the risk of hiding actually useful information.

I'd be against capturing a backtrace at the beginning of tests/main/etc as that's somewhat surprising and we should otherwise be able to infer the bottom of the stack. I'd be fine just adding more heuristics.

@Yamakaky
Copy link
Contributor Author

Yamakaky commented Mar 2, 2017

What do you mean by surprising?

@alexcrichton
Copy link
Member

It's a pretty weighty piece of code to unconditionally run, it's also quite slow. We should avoid running it by default.

@Yamakaky
Copy link
Contributor Author

Yamakaky commented Mar 2, 2017

Not wrong

@petrochenkov
Copy link
Contributor

@Yamakaky
I thought you were going to detect main and friends by their symbol names, not by literally producing a backtrace.

@Yamakaky
Copy link
Contributor Author

Yamakaky commented Mar 4, 2017

No, that's what I meant.

Yamakaky added a commit to Yamakaky/rust that referenced this issue Mar 4, 2017
Correctly handles panics in threads and tests. First, the frames after
`__rust_maybe_catch_panic` are discarded, then it uses a blacklist that
does some more fine-tuning. Since frames after the call to
`__rust_maybe_catch_panic` seem to be platform-independant,
`BAD_PREFIXES_BOTTOM` could probably be cleaned a bit.

Fixes rust-lang#40201
@bors bors closed this as completed in ca8b754 May 10, 2017
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants