Join GitHub today
GitHub is home to over 31 million developers working together to host and review code, manage projects, and build software together.
Sign upAdd line numbers to MSVC backtrace #28242
Conversation
rust-highfive
assigned
aturon
Sep 4, 2015
This comment has been minimized.
This comment has been minimized.
|
r? @aturon (rust_highfive has picked a reviewer for you, use r? to override) |
alexcrichton
reviewed
Sep 4, 2015
| @@ -15,7 +15,8 @@ pub fn callback<F>(f: F) where F: FnOnce((&'static str, u32)) { | |||
| f((file!(), line!())) | |||
| } | |||
|
|
|||
| #[inline(always)] | |||
| #[cfg_attr(not(target_env = "msvc"), inline(always))] | |||
| #[cfg_attr(target_env = "msvc", inline(never))] | |||
This comment has been minimized.
This comment has been minimized.
alexcrichton
reviewed
Sep 4, 2015
| let basename = file.split(&['/', '\\'][..]).last().unwrap(); | ||
| println!("{}:{}", basename, line); | ||
| } | ||
| } |
This comment has been minimized.
This comment has been minimized.
alexcrichton
Sep 4, 2015
Member
Instead of duplicating the function, could this do something like:
// explanatory comment
let skip = if cfg!(target_env = "msvc") {1} else {0};
for &(file, line) in filelines.iter().rev().skip(skip) {
// ...
}
alexcrichton
reviewed
Sep 4, 2015
| fn inner_inlined(counter: &mut i32, main_pos: Pos, outer_pos: Pos) { | ||
| check!(counter; main_pos, outer_pos); | ||
| check!(counter; main_pos, outer_pos); | ||
|
|
||
| #[inline(always)] | ||
| #[cfg_attr(not(target_env = "msvc"), inline(always))] | ||
| #[cfg_attr(target_env = "msvc", inline(never))] |
This comment has been minimized.
This comment has been minimized.
alexcrichton
Sep 4, 2015
Member
Like the above one, could you add explanatory comments here for the #[cfg]? It's fine for all of them to point to just one comment
alexcrichton
reviewed
Sep 4, 2015
| unsafe { CStr::from_ptr(line.Filename).to_bytes() }, | ||
| line.LineNumber as libc::c_int, | ||
| false | ||
| ) |
This comment has been minimized.
This comment has been minimized.
alexcrichton
Sep 4, 2015
Member
Could you format this like:
output_fileline(w,
unsafe { CStr::from_ptr(line.Filename).to_bytes() },
line.LineNumber as libc::c_int,
false)
This comment has been minimized.
This comment has been minimized.
|
Awesome, thanks @Diggsey! I had implemented this in backtrace-rs but apparently forgot to port it over! |
alexcrichton
assigned
alexcrichton
and unassigned
aturon
Sep 4, 2015
Diggsey
force-pushed the
Diggsey:msvc-backtrace
branch
from
f0279e7
to
9a83842
Sep 4, 2015
This comment has been minimized.
This comment has been minimized.
|
All done! |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
bors
added a commit
that referenced
this pull request
Sep 5, 2015
This comment has been minimized.
This comment has been minimized.
bors
merged commit 9a83842
into
rust-lang:master
Sep 5, 2015
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Diggsey commentedSep 4, 2015
Currently LLVM does not generate the debug info required to get complete backtraces even when functions are inlined, so that part of the
run-pass/backtrace-debuginfo.rstest is disabled when targetting MSVC. At worst this results in missing stack frames where functions have been inlined.