-
Notifications
You must be signed in to change notification settings - Fork 341
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
Add API for capturing backtrace #1559
Conversation
Companion to rust-lang/miri#1559
The |
e892458
to
4814f9d
Compare
I have seen string formatting being very slow before, so this is likely another case of that. |
This is not a new problem, it already leads to isolation error (instead of an empty backtrace) right now. |
/// and `MiriFrame` should be declared as follows: | ||
/// | ||
/// ```rust | ||
/// struct MiriFrame { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be repr(C)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use mplace_field
to write to fields, which ends up using FieldsShape
to map the definition-order fields to their actual offsets in memory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh so we exploit that we actually have access to the user-defined type... sneaky.^^
README.md
Outdated
/// | ||
/// The fields must be declared in exactly the same order as they appear in `MiriFrame` above. | ||
/// This function can be called on any thread (not just the one which obtained `frame`) | ||
fn miri_resolve_frame(version: u8, frame: *mut ()) -> MiriFrame; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What we are effectively doing here is define something like a syscall interface. I have not seen version numbers in a syscall interface before, but I have seen flag fields -- so maybe it would make sense to instead make this flags: u64
? That seems to be a more common way of future-proofing an API.
Also, if we ever need to change the number of arguments, we should probably add a miri_resolve_frame_2
or so. Overall I think things will be least confusing if we can make this work and evolve like a normal C API, not exploiting the fact that Miri can do fancy things like determine how many arguments the caller actually passed (without varargs).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added a flags
field instead. Currently, I just throw an error if it's not 0
. We could return a Result
instead, but I don't know if there would ever be a need for code to support multiple Miri versions.
@RalfJung I've addressed your comments |
@RalfJung: I've addressed your latest round of comments |
@Aaron1011 yeah I know, I just didn't have time this week-end to look at your PR. |
But I was actually looking at it when you wrote your comment.^^ Thanks for the PR :) |
📌 Commit 2a2a93d has been approved by |
Add API for capturing backtrace This PR adds two new Miri-defined extern functions: `miri_get_backtrace` and `miri_resolve_frame`, which are documented in the README. Together, they allow obtaining a backtrace for the currently executing program. I've added a test showing how these APIs are used. I've also prepared a companion PR `backtrace-rs`, which will allow `backtrace::Backtrace::new()` to work automatically under Miri. Once these two PRs are merged, we will be able to print backtraces from the normal Rust panic hook (since libstd is now using backtrace-rs). A few notes: * Resolving the backtrace frames is *very* slow - you can actually see each line being printed out one at a time. Some local testing showed that this is not (primrary) caused by resolving a `Span` - it seems to be just Miri being slow. * For the first time, we now interact directly with a user-defined struct (instead of just executing the user-provided MIR that manipulates the struct). To allow for future changes, I've added a 'version' parameter (currently required to be 0). This should allow us to change the `MiriFrame` struct should the need ever arise. * I used the approach suggested by `@oli-obk` - a returned backtrace pointer consists of a base function allocation, with the 'offset' used to encode the `Span.lo`. This allows losslessly reconstructing the location information in `miri_resolve_frame`. * There are a few quirks on the `backtrace-rs` side: * `backtrace-rs` calls `getcwd()` by default to try to simplify the filename. This results in an isolation error by default, which could be annoying when printing a backtrace from libstd. * `backtrace-rs` tries to remove 'internal' frames (everything between the call to `Backtrace::new()` and the internal API call made by backtrace-rs) by comparing the returned frame pointer value to a Rust function pointer. This doesn't work due to the way we construct the frame pointers passed to the caller. We could attempt to support this kind of comparison, or just add a `#[cfg(miri)]` and ignore the frames ourselves.
@bors r+ retry |
📌 Commit 1670f8c has been approved by |
Add API for capturing backtrace This PR adds two new Miri-defined extern functions: `miri_get_backtrace` and `miri_resolve_frame`, which are documented in the README. Together, they allow obtaining a backtrace for the currently executing program. I've added a test showing how these APIs are used. I've also prepared a companion PR `backtrace-rs`, which will allow `backtrace::Backtrace::new()` to work automatically under Miri. Once these two PRs are merged, we will be able to print backtraces from the normal Rust panic hook (since libstd is now using backtrace-rs). A few notes: * Resolving the backtrace frames is *very* slow - you can actually see each line being printed out one at a time. Some local testing showed that this is not (primrary) caused by resolving a `Span` - it seems to be just Miri being slow. * For the first time, we now interact directly with a user-defined struct (instead of just executing the user-provided MIR that manipulates the struct). To allow for future changes, I've added a 'version' parameter (currently required to be 0). This should allow us to change the `MiriFrame` struct should the need ever arise. * I used the approach suggested by `@oli-obk` - a returned backtrace pointer consists of a base function allocation, with the 'offset' used to encode the `Span.lo`. This allows losslessly reconstructing the location information in `miri_resolve_frame`. * There are a few quirks on the `backtrace-rs` side: * `backtrace-rs` calls `getcwd()` by default to try to simplify the filename. This results in an isolation error by default, which could be annoying when printing a backtrace from libstd. * `backtrace-rs` tries to remove 'internal' frames (everything between the call to `Backtrace::new()` and the internal API call made by backtrace-rs) by comparing the returned frame pointer value to a Rust function pointer. This doesn't work due to the way we construct the frame pointers passed to the caller. We could attempt to support this kind of comparison, or just add a `#[cfg(miri)]` and ignore the frames ourselves.
This PR adds two new Miri-defined extern functions: `miri_get_backtrace` and `miri_resolve_frame`, which are documented in the README. Together, they allow obtaining a backtrace for the currently executing program. I've added a test showing how these APIs are used. I've also prepared a companion PR `backtrace-rs`, which will allow `backtrace::Backtrace::new()` to work automatically under Miri. Once these two PRs are merged, we will be able to print backtraces from the normal Rust panic hook (since libstd is now using backtrace-rs). A few notes: * Resolving the backtrace frames is *very* slow - you can actually see each line being printed out one at a time. Some local testing showed that this is not (primrary) caused by resolving a `Span` - it seems to be just Miri being slow. * For the first time, we now interact directly with a user-defined struct (instead of just executing the user-provided MIR that manipulates the struct). To allow for future changes, I've added a 'version' parameter (currently required to be 0). This should allow us to change the `MiriFrame` struct should the need ever arise. * I used the approach suggested by @oli-obk - a returned backtrace pointer consists of a base function allocation, with the 'offset' used to encode the `Span.lo`. This allows losslessly reconstructing the location information in `miri_resolve_frame`. * There are a few quirks on the `backtrace-rs` side: * `backtrace-rs` calls `getcwd()` by default to try to simplify the filename. This results in an isolation error by default, which could be annoying when printing a backtrace from libstd. * `backtrace-rs` tries to remove 'internal' frames (everything between the call to `Backtrace::new()` and the internal API call made by backtrace-rs) by comparing the returned frame pointer value to a Rust function pointer. This doesn't work due to the way we construct the frame pointers passed to the caller. We could attempt to support this kind of comparison, or just add a `#[cfg(miri)]` and ignore the frames ourselves.
Looks like I was right to be worried about test stability... we already have a change in the output:
Can you adjust normalization to remove more things? We might actually just want to cut off the function name after the first |
💔 Test failed - checks-travis |
This looks like it was caused by https://github.com/rust-lang/rust/pull/76176/files. That PR also caused a lot of changes in rustc UI tests, so I don't think this issue is specific to the way we're printing backtrace frames. However, I'm fine with normalizing more of the output. |
1670f8c
to
7fba3c2
Compare
@RalfJung: I've normalized out all turbofished generic arguments ( |
Companion to rust-lang/miri#1559
Thanks! |
📌 Commit 7fba3c2 has been approved by |
☀️ Test successful - checks-travis, status-appveyor |
This PR adds two new Miri-defined extern functions:
miri_get_backtrace
andmiri_resolve_frame
, which are documented inthe README. Together, they allow obtaining a backtrace for the currently
executing program.
I've added a test showing how these APIs are used. I've also prepared a
companion PR
backtrace-rs
, which will allowbacktrace::Backtrace::new()
to work automatically under Miri.Once these two PRs are merged, we will be able to print backtraces from
the normal Rust panic hook (since libstd is now using backtrace-rs).
A few notes:
each line being printed out one at a time. Some local testing showed
that this is not (primrary) caused by resolving a
Span
- it seemsto be just Miri being slow.
struct (instead of just executing the user-provided MIR that
manipulates the struct). To allow for future changes, I've added
a 'version' parameter (currently required to be 0). This should allow
us to change the
MiriFrame
struct should the need ever arise.pointer consists of a base function allocation, with the 'offset'
used to encode the
Span.lo
. This allows losslessly reconstructingthe location information in
miri_resolve_frame
.backtrace-rs
side:backtrace-rs
callsgetcwd()
by default to try to simplifythe filename. This results in an isolation error by default,
which could be annoying when printing a backtrace from libstd.
backtrace-rs
tries to remove 'internal' frames (everything betweenthe call to
Backtrace::new()
and the internal API call made bybacktrace-rs) by comparing the returned frame pointer value to
a Rust function pointer. This doesn't work due to the way we
construct the frame pointers passed to the caller. We could
attempt to support this kind of comparison, or just add a
#[cfg(miri)]
and ignore the frames ourselves.