Skip to content
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

Handle compressed debug sections in ELF files #344

Merged
merged 1 commit into from
Jun 11, 2020

Conversation

benesch
Copy link
Contributor

@benesch benesch commented Jun 10, 2020

@alexcrichton this will need to wait for the unaligned feature to land in an actual release of the object crate, but otherwise it's ready to go from my perspective. Let me know what you think! Don't think there's a good way around the flate2 dependency (I guess we could depend on mini_oxide directly?). Do you think that'll be a showstopper for inclusion in libstd?


ELF files allow debug info sections to be compressed. The libbacktrace
backed supported these compressed sections, but the Gimli backend did
not. This commit adds that support to the Gimli backend.

In my tests these debug info sections do not obey the alignment
requirements that the object crate expects for the gABI compression
header (nor can I find a source documenting any alignment requirements),
so this commit additionally enables the "unaligned" feature in the
upcoming version of the object crate.

There is a bit of unsafe to ensure the lifetime of the decompressed
sections matches the lifetime of the mmap'd file. I don't think there is
a way around this unsafe code, unless we are willing to ditch Gimli's
EndianSlice for an (apparently slower) EndianReader backed by a
Cow<[u8]>.

Fix #342.

Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow very nice! That was a lot easier than I expected it to be :)

For alignment, I see that at least for gabi there's ch_addralign. Could that perhaps be used to properly align the Vec allocation? (we may need to more manually manage it). In any case though turning on the unaligned feature and having a little bit of unsafe here I believe is fine.

For the zlib dependency, I think for inclusion into libstd we'll probably want to avoid flate2 itself and instead use miniz-oxide directly (less dependencies). That being said I'd be happy to take that as a future action item for myself, no need to tackle that here.

src/symbolize/gimli/elf.rs Show resolved Hide resolved
src/symbolize/gimli/elf.rs Outdated Show resolved Hide resolved
@benesch benesch force-pushed the compressed-debuginfo branch from a2253e1 to 6308da3 Compare June 10, 2020 21:29
@benesch
Copy link
Contributor Author

benesch commented Jun 10, 2020

For alignment, I see that at least for gabi there's ch_addralign. Could that perhaps be used to properly align the Vec allocation? (we may need to more manually manage it). In any case though turning on the unaligned feature and having a little bit of unsafe here I believe is fine.

ch_addralign is actually a requirement on the alignment of the uncompressed data. Probably we should be handling that. I pushed up a commit to at least bail out if we see a ch_addralign value of anything besides 1. (I haven't seen anything like that in practice.) But if you think we should, we could try to get a vector of the alignment required by ch_addralign. It was just surprisingly annoying to do so.

The unaligned feature is about reading the compressed header out of the compressed section data, which for some reason does not appear to be aligned with any toolchain the way the ELF standard would seem to require. There's some more context in this upstream issue I filed: gimli-rs/object#235

I pushed up a potential fix for the memory unsafely. Need to think on whether it's sound a little bit harder, and also presumably fix up whatever breakages I introduced for Mach-O/COFF—the fix required touching API shared between the different platforms.

@benesch benesch force-pushed the compressed-debuginfo branch from 6308da3 to 87156e4 Compare June 10, 2020 21:35
@alexcrichton
Copy link
Member

Ah ok I see, so the unaligned is required because practically all toolchains have the compressed contents unaligned, but the standard expects that the header of the compressed contents is aligned, right? I was mixed up because I thought that the decompressed data was misaligned, but it sounds like the compressed data is misaligned.

If we're adding the unaligned feature to object (which I think we should do regardless to be compatible with any weird object files that crop up), then I think there's no need to respect alignment anywhere. It's fine to just discard all that information and run with what we have.

In that case I think it should be ok to remove the align of 1 check. That alignment is something we could respect with the alloc family of functions, but it's not worth dealing with it if we're using unaligned anyway.

@philipc
Copy link
Contributor

philipc commented Jun 10, 2020

That alignment is something we could respect with the alloc family of functions, but it's not worth dealing with it if we're using unaligned anyway.

Agree that we don't need to check ch_addralign, but it's because gimli doesn't care about the alignment, not due to the object's unaligned feature. DWARF sections shouldn't require alignment anyway.

@alexcrichton
Copy link
Member

d'oh, right!

I suppose that because this seems like a general ELF thing that maybe non-dwarf sections may need more alignment, but for our use cases definitely doesn't matter much!

@benesch benesch force-pushed the compressed-debuginfo branch from 87156e4 to e270b89 Compare June 11, 2020 04:53
ELF files allow debug info sections to be compressed. The libbacktrace
backed supported these compressed sections, but the Gimli backend did
not. This commit adds that support to the Gimli backend.

In my tests (with the BFD linker, lld, and gold) these debug info
sections do not obey the alignment requirements that the object crate
expects for the gABI compression header, so this commit additionally
enables the "unaligned" feature in the upcoming version of the object
crate.

There is a bit of unsafe to ensure the lifetime of the decompressed
sections matches the lifetime of the mmap'd file. I don't think there is
a way around this unsafe code, unless we are willing to ditch Gimli's
EndianSlice for an (apparently slower) EndianReader backed by a
Cow<[u8]>.

Fix rust-lang#342.
@benesch benesch force-pushed the compressed-debuginfo branch from e270b89 to 5af532b Compare June 11, 2020 04:58
@benesch
Copy link
Contributor Author

benesch commented Jun 11, 2020

👍 I've removed the check on ch_addralign. I also switched the decompression from flate2 to miniz_oxide directly. I think that's everything! Please double check the unsafe code, of course.

@alexcrichton
Copy link
Member

Looks great to me, thanks so much again for your help on this!

@alexcrichton alexcrichton merged commit 05319f4 into rust-lang:master Jun 11, 2020
@benesch
Copy link
Contributor Author

benesch commented Jun 11, 2020

Absolutely! Thanks very much for the quick review and merge!

It occurs to me that the master branch of backtrace now depends on the object crate from Git, rather than crates.io. This is certainly fine with me, but not sure if you meant to merge before getting an official release of object with the unaligned feature, so wanted to surface!

@alexcrichton
Copy link
Member

Ah yeah I'm not too worried about that.

@philipc would you be ok making a new publish of object in the near future?

@philipc
Copy link
Contributor

philipc commented Jun 11, 2020

Yep I can do that.

@philipc
Copy link
Contributor

philipc commented Jun 15, 2020

object 0.20.0 is published.

@alexcrichton
Copy link
Member

Thanks!

This support is now published with 0.3.49

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

gimli-symbolize fails with compressed debug info
3 participants