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

feat: Support DWARF debug info in PE files #744

Merged
merged 9 commits into from
Jan 23, 2023

Conversation

casept
Copy link
Contributor

@casept casept commented Jan 16, 2023

I'm not quite sure how I should implement tests for this. All the other formats seem to use breakpad binaries as the test case, but breakpad_client doesn't build under MinGW. Is it relevant that the test cases match? Or can I just use some random project for this?

Also, is support for platforms with non-4K page size relevant? Goblin doesn't seem to expose page size, so this metadata would have to be kept up-to-date in symbolic.

Closes #352.

@codecov
Copy link

codecov bot commented Jan 17, 2023

Codecov Report

Merging #744 (2a51d5b) into master (bad5cb8) will increase coverage by 0.37%.
The diff coverage is 95.55%.

❗ Current head 2a51d5b differs from pull request most recent head a032761. Consider uploading reports for the commit a032761 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #744      +/-   ##
==========================================
+ Coverage   73.43%   73.80%   +0.37%     
==========================================
  Files          69       69              
  Lines       14875    14875              
==========================================
+ Hits        10923    10979      +56     
+ Misses       3952     3896      -56     

@casept
Copy link
Contributor Author

casept commented Jan 22, 2023

I've snapshotted a binary of the sqlite3 shell I compiled for the test cases now.

@casept casept marked this pull request as ready for review January 22, 2023 23:15
@casept casept requested a review from a team January 22, 2023 23:15
Copy link
Contributor

@loewenheim loewenheim left a comment

Choose a reason for hiding this comment

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

Looks good to me, thank you very much for this!

symbolic-debuginfo/src/pe.rs Outdated Show resolved Hide resolved
symbolic-debuginfo/src/pe.rs Show resolved Hide resolved
symbolic-debuginfo/src/pe.rs Outdated Show resolved Hide resolved
@@ -446,7 +446,6 @@ pub enum ObjectDebugSession<'d> {
Breakpad(BreakpadDebugSession<'d>),
Dwarf(DwarfDebugSession<'d>),
Pdb(PdbDebugSession<'d>),
Pe(PeDebugSession<'d>),
Copy link
Member

Choose a reason for hiding this comment

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

Just a NOTE: removing these types is considered a breaking change.

As getsentry/publish#1700 has been stuck and noone has tried to fix this yet, we can squeeze this breaking change into the next release still.

Please make sure to document the changes in the changelog though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated the changelog.

@Swatinem
Copy link
Member

Also, 3+M for the testcase is a bit hefty. All you would need is a main fn with a printf, just enough to get some kind of debug info out of it.

@casept
Copy link
Contributor Author

casept commented Jan 23, 2023

Also, 3+M for the testcase is a bit hefty. All you would need is a main fn with a printf, just enough to get some kind of debug info out of it.

The reason why I didn't do this is because the other test fixtures looked more like "real world" programs. The MacOS example is also over 1M in size.

Anyways, the test cases are replaced now.

@Swatinem
Copy link
Member

Clippy still complains about unneeded return statements. After that I would say its good to go :-)

@casept
Copy link
Contributor Author

casept commented Jan 23, 2023

Clippy still complains about unneeded return statements. After that I would say its good to go :-)

Fixed!

@Swatinem Swatinem enabled auto-merge (squash) January 23, 2023 13:32
@Swatinem Swatinem merged commit 741cc80 into getsentry:master Jan 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support DWARF in PE
4 participants