rust-lang / backtrace-rs

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

Upgrade addr2line and properly handle split DWARF on Linux #513

Closed khuey closed 1 year ago

khuey commented 1 year ago

This doesn't actually implement loading for split DWARF but it will make backtrace continue to compile with the existing functionality once addr2line cuts a new release.

Cc @philipc

philipc commented 1 year ago

addr2line 0.20.0 has been released.

khuey commented 1 year ago

Ok the latest csets I've pushed support packed split DWARF on Linux. They don't yet support unpacked split DWARF (that's harder because we need to load the DWOs and presumably stash them somewhere). And bumping addr2line causes the MSRV test to start failing.

philipc commented 1 year ago

Is it possible to add a CI test for this?

They don't yet support unpacked split DWARF (that's harder because we need to load the DWOs and presumably stash them somewhere).

We could hash by DWO id, but an alternative would be to change SplitDwarfLoad to include the ResUnit index, and add an API to query the number of ResUnits so that we can preallocate a Vec for lazy loading (similar to unpacked Mach-O).

khuey commented 1 year ago

There is a CI test for this.

philipc commented 1 year ago

There is a CI test for this.

Oh, so there is, but I must be missing something still. How was it passing before this PR?

khuey commented 1 year ago

What you're missing is that it's not passing before this PR. These tests are failing on the tip of this repo.

khuey commented 1 year ago

so that we can preallocate a Vec for lazy loading (similar to unpacked Mach-O).

What's the point of preallocating the Vec here? search_object_map still takes &mut self to modify it. It looks like Mach-O gives indexes to tell you which object file to use, so having something you can index is helpful?

DWARF just gives us file paths and DWO ids. I think we'd want a HashMap here, but that's not available in alloc. We could do a BTreeMap on the paths, or just linearly search a Vec. Regardless preallocating doesn't seem to do any good with DWARF, unless I'm missing something.

philipc commented 1 year ago

The difference from Mach-O that I overlooked is that addr2line is internally doing the indexing itself. I don't think we actually need to look up anything. addr2line isn't going to ask for the same DWO twice. All we need is somewhere to stash it before returning it to addr2line, so we could just append to a Vec.

khuey commented 1 year ago

Hmm, yes, addr2line's ResUnit will cache the gimli::Dwarf for the DWO, and backtrace's Context contains addr2line's Context, so that does sound right.

Perhaps instead of having mmap_aux and mmap_dwp in Stash we should just have an append-only Vec<Mmap> like we do for Stash::buffers.

khuey commented 1 year ago

And that adds support for unpacked split DWARF, passing all the relevant tests.

khuey commented 1 year ago

Does @philipc review changes for this repo too?

philipc commented 1 year ago

I don't approve PRs in this repo. LGTM though.

khuey commented 1 year ago

It would be nice to get this merged and a new release cut. Split DWARF inlining was turned off by default in 1.68.

JohnTitor commented 1 year ago

r? @rust-lang/crate-maintainers could someone review this? I think this is kinda notable change but I'm not familiar enough to review exhaustively.

khuey commented 1 year ago

Realistically I don't know that you're going to find a reviewer with more subject matter expertise than me and @philipc here.

khuey commented 1 year ago

Can we get a new release with this?

workingjubilee commented 1 year ago

It would be nice if we passed the test suite on the commit we cut our release from. I will make sure a release is cut once https://github.com/rust-lang/backtrace-rs/issues/531 is resolved.

workingjubilee commented 1 year ago

backtrace 0.3.68 is out, let us know if there are any bugs to be squashed!