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

fix(debuginfo): Improve .o handling on MachO (#173) #173

Merged
merged 2 commits into from
Mar 17, 2020

Conversation

nnethercote
Copy link
Contributor

I am using symbolic_debuginfo in a symbolicator used for stack traces produced by Firefox.

On Mac, we don't build dSYMs by default because doing so is slow. So I want to emulate atos, which is able to read debuginfo from a binary by consulting the binary's symbol table and then reading debug info from the object files and archive files mentioned.

I have this working, but it requires a couple of small changes to symbolic_debuginfo, due to slight differences between .o files and executables/libraries.

Currently `raw_section` finds the given section by assuming that it's
within a segment named `__TEXT` or `__DWARF`, finding such a segment,
and then searching within that segment.

However, this fails for `.o` files, because they have a single nameless
segment. As
https://github.com/aidansteele/osx-abi-macho-file-format-reference says:

> For compactness, an intermediate object file contains only one
> segment. This segment has no name; it contains all the sections
> destined ultimately for different segments in the final object file.

This commit changes `raw_section` so it ignores segment names, and
simply searches through all segments.

As well as working with `.o` files, the resulting code is a little
shorter and simpler.
Because they occur in `.o` files.

Without the first change in this commit, debuginfo for entire `.o` files
can be missed. Without the second change, debuginfo for the first
function (which can start at 0x0) can be missed.
@nnethercote
Copy link
Contributor Author

This is my first PR for this project, so please let me know if I've overlooked anything w.r.t. contributing. I did run cargo test --all --all-features, and it passed.

Copy link
Member

@jan-auer jan-auer left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, @nnethercote! Once we get my question below addressed, I'm happy to merge.

This is my first PR for this project, so please let me know if I've overlooked anything w.r.t. contributing.

All good, thanks for asking!

debuginfo/src/dwarf.rs Show resolved Hide resolved
@nnethercote
Copy link
Contributor Author

@jan-auer: Any new thoughts here? The project I'm working on currently uses a fork of Symbolic that contains this PR, because it needs to work directly with .o files on Mac. It's not an ideal situation, and it would be great if it could be resolved. Thanks.

@nnethercote
Copy link
Contributor Author

My tool is now being used in production for Firefox. It's still relying on the version of symbolic-debuginfo in this PR, because the release version doesn't do the right thing for my use case. This is sub-optimal.

@jan-auer
Copy link
Member

Sorry for the late reply, I took a closer look and ran some tests now. Performance checks out. It seems that this was an outdated concern, and I can frankly no longer find the cases where this would've hit us.

For some reason I remember this also being a correctness concern, but this might be false memory.

Thanks for this patch. Will look into merging all PRs now and then releasing a new version to crates.io as soon as we get rid of all the git dependencies.

@jan-auer jan-auer changed the title Improve .o handling on Mac fix(debuginfo): Improve .o handling on MachO (#173) Mar 17, 2020
@jan-auer jan-auer merged commit e4b3939 into getsentry:master Mar 17, 2020
@nnethercote nnethercote deleted the improve-o-handling-on-Mac branch March 17, 2020 21:57
@nnethercote nnethercote restored the improve-o-handling-on-Mac branch March 18, 2020 04:01
let low_pc = match low_pc {
Some(low_pc) if low_pc != 0 => low_pc,
Some(low_pc) => low_pc,
Copy link
Member

Choose a reason for hiding this comment

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

@nnethercote I'm afraid we'll need to make this check conditional, as you suggested. See #173

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.

None yet

2 participants