implement From<Frame> for BacktraceFrame #420
Conversation
There are situations where we can only capture raw `Frame` (example: capturing from a signal handler), but it could still be that we can process them later and are not fully nostd in the environment. With a conversion from Frame to BacktraceFrame, this is trivial. But without it, we can't really use the pretty printers as they are reliant on BacktraceFrame, not Frame. Fixes: rust-lang#419
|
Thanks! Can you add some tests for this as well? |
|
I can, but I noticed that there aren't any tests in-file. |
|
Mostly just something that exercises this code. For example creating a list from a call to |
|
test added |
src/capture.rs
Outdated
|
|
||
| let mut frames = 0; | ||
| unsafe { | ||
| crate::trace_unsynchronized(|frame| { |
There was a problem hiding this comment.
Could this use trace instead of trace_unsynchronized? Additionally can this use the BacktraceFrame after the closure to ensure that it's not accidentally holding on to any ephemeral pointeres?
There was a problem hiding this comment.
yeah, we can use trace. I ended up using the unsynchronized version because that is what I was using in my code, no problem.
By using the backtrace frame, maybe I can return a vector and do the asserts by zipping both iterators. Would that be enough for you ?
There was a problem hiding this comment.
Thanks for your comments. I also noticed that the test would fail in --release mode, and indeed skipping a frame is fragile as inlining can change it.
I used a better method that now succeeds with release and debug.
A new version is force-pushed.
There was a problem hiding this comment.
Whatever testing is fine by me, I mainly just want to confirm that if the BacktraceFrame leaks out of the closure then it's still safe to use.
There was a problem hiding this comment.
Looks good to me, thanks! (sorry our last two comments sort of collided)
I think the miri tests are failing but otherwise this looks good to go once tests are passing
There was a problem hiding this comment.
Ok, just saw that. I bet it is a similar issue with --release and --debug. I will try it locally and get back to you
There was a problem hiding this comment.
Ok. The issue with miri is different: every backtrace generated has completely different addresses!
Fortunately they resolve to the same thing, so I am testing with that
There was a problem hiding this comment.
my god, lol...
At least your code coverage is pretty good!!
The test that fails now is about features disabled. If I understand your code correctly, that's because resolving symbols need features to run. I can remove the last assert. It won't be doing that much in that case, but it is better than disabling the whole test in the absence of features. At least the conversion path is stressed.
Every backtrace that miri generates - conversion or no conversion, has totally different addresses. They all resolve to the same thing, though, so test with that.
`cargo test --no-default-features --features std` fails because frames are not resolved to symbols in that case. We'll just remove the assert, rather than disabling the whole test on that case. At least the code paths are stressed.
|
Looks good to me, thanks! |
There are situations where we can only capture raw
Frame(example:capturing from a signal handler), but it could still be that we can
process them later and are not fully nostd in the environment.
With a conversion from Frame to BacktraceFrame, this is trivial. But
without it, we can't really use the pretty printers as they are reliant
on BacktraceFrame, not Frame.
Fixes: #419