search

rust-lang / backtrace-rs

Backtraces in Rust
https://docs.rs/backtrace
Other
530 stars 245 forks source link

Use /proc/self/maps when available instead of std::env::current_exe #488

Closed pnkfelix closed 1 year ago

pnkfelix commented 1 year ago

rust-lang/rust#101913 illustrates a scenario on Linux where backtraces fail because of their reliance on std::env::current_exe() always yielding the path to the Rust binary that is driving execution.

This PR changes that strategy, by first trying to parse /proc/self/maps and extract from it the path to the Rust binary that was mmap'ed in as part of linking/loading.

It addresses the problem for my local Linux host, as illustrated by the included regression test. I have not yet tested it on other platforms, but I am hoping I got the logic right (in terms of just falling back gracefully to std::env::current_exe() in cases where we either cannot find a /proc/self/maps, or we cannot find the instruction pointer of interest anywhere in the mapping reported by /proc/self/maps).

pnkfelix commented 1 year ago

hmm, cargo test --no-default-features failed, something with how I'm using the std library. Will investigate

pnkfelix commented 1 year ago

This seems reasonable-enough to me, thanks for implementing this! One thing that might be worth checking is the binary size impact of adding this, since I'd expect that BufReader is not trivial in its code size.

Hmm. I honestly did not know there was a significant cost associated with BufReader itself. My sole motivation for using BufReader was just to get easy line-by-line parsing.

So here's some data:

On master, after doing cargo build --release:

% ls -sk target/release/libbacktrace.rlib
992 target/release/libbacktrace.rlib

On this PR's branch, after doing cargo build --release:

% ls -sk target/release/libbacktrace.rlib
1052 target/release/libbacktrace.rlib

I am inferring adding 60kb to a 992kb rlib is more than you want to pay for this..

If the code size of this is somewhat substantial there's probably a number of ways that this can cut down on code size, for example there's a fair number of allocations which I think could be removed and/or changed to iterators. Not necessarily trivially so I don't think it's worth doing unless motivated though.

I cannot tell from your feedback whether you are suggesting that I leave in the BufReader usage while removing allocations elsewhere, or to remove the BufReader in addition to making other changes.

In any case I'll poke at it and see if I can figure out a way to cut down on the code size increase without too much pain.

(/me does some searching on crates.io.) Let me look into using https://crates.io/crates/bstr; at least, I assume that would be an acceptable dependency to pull in here. (hmm, or I guess I'll need to pull in a local copy of its code, since the whole point is that this needs to work while depending on Rust's libstd alone...)

alexcrichton commented 1 year ago

Thanks for measuring! 60k indeed seems pretty small and probably largely inconsequential given the size of this crate already. In that case while I think it's possible to somewhat easily cut down on code size it's not a priority, so it's fine to leave as-is, sorry if that wasn't clear!

It looks like there's some minor nits on CI but feel free to bump the msrv in the CI configuration file and re-push, there's no need to strictly maintain compatibilty with that.

pnkfelix commented 1 year ago

Thanks for measuring! 60k indeed seems pretty small and probably largely inconsequential given the size of this crate already. In that case while I think it's possible to somewhat easily cut down on code size it's not a priority, so it's fine to leave as-is, sorry if that wasn't clear!

It looks like there's some minor nits on CI but feel free to bump the msrv in the CI configuration file and re-push, there's no need to strictly maintain compatibilty with that.

Well, I've already jumped through some other hoops, let me take a shot at seeing if I can satisfy the existing MSRV

pnkfelix commented 1 year ago

Thanks for measuring! 60k indeed seems pretty small and probably largely inconsequential given the size of this crate already. In that case while I think it's possible to somewhat easily cut down on code size it's not a priority, so it's fine to leave as-is, sorry if that wasn't clear!

So ... in https://github.com/rust-lang/backtrace-rs/pull/488/commits/cc0cdbb9fd7ef325f419420df86a302cb8ed6c59 I did sidestep the use of BufRead by instead doing a (very naive) Read-based implementation: namely, read the whole file into one String, and then split that into lines.

Doing this cut out 8K from the rlib size.

I'm sort of sad about doing this, but then again, even my previous implementation was allocating a Vec of String whose size was proportional to the full contents of /proc/self/maps, so I think its at most a 2x runtime-memory overhead over what I was previously doing.

Anyway, @alexcrichton , let me know if you'd prefer I put a line-by-line parsing version back. (I suspect if anyone invests time in doing that, they should also get rid of the Vec and just have the code directly extract the desired entry as it traverses the lines of /proc/self/maps.)

alexcrichton commented 1 year ago

Seems reasonable to me, thanks again for this @pnkfelix!