-
Notifications
You must be signed in to change notification settings - Fork 113
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
DWARF processing fixes #224
Conversation
421813c
to
150b98c
Compare
@snehasish @shenhanc78 Please take a look. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, would it be possible to add a test for this?
I'm happy to do that but currently no GCC tests run in CI. Is it possible to enable them? |
Sure, can you send a separate PR to do so? |
@snehasish @shenhanc78 I see this comment in #213 (comment):
How can I run those tests and repro the failures? |
Looks like there is just one failure in the symbolize test. I created draft PR #225 which adds make test to the gcov build. Here is the log: https://github.com/google/autofdo/actions/runs/10359876746/job/28677175564?pr=225 |
All of these tests are from third_party directories, e.g., symbolize test is from |
That's a good point, I don't have any experience on the gcov side but taking a quick look at the source, it doesn't look like we have any tests that cover gcov profile generation. Maybe we can skip the symbolize test on the CI when built for gcov, enable the CI for gcov and then add a test which covers this PR too? |
I can't repro the symbolize test failure. Do you know how to disable it in CI when built for gcov? |
@snehasish I opened #226. I disabled third_party tests similar to how it's done for build-llvm. I have a test for this PR, will update it once #226 is merged. |
150b98c
to
9e325de
Compare
@snehasish I added a test for this fix. PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm with a couple of minor comments.
GetSection(sections, ".debug_rnglists", &debug_rnglists_data, &debug_rnglists_size); | ||
|
||
if (debug_ranges_data == NULL && debug_rnglists_data == NULL) { | ||
LOG(WARNING) << "File '" << binary_name_ << "' does not have .debug_ranges nor .debug_rnglists sections."; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if we can proceed if both of them are not available for newer versions of dwarf. Should this be a fatal error instead (based on the dwarf version)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's possible that the binary has no address ranges in its dwarf info. If we attempt to use address ranges later, we will assert at that time (in dwarf3ranges.cc).
addr2line_test.cc
Outdated
|
||
using ::devtools_crosstool_autofdo::Addr2line; | ||
|
||
TEST(Addr2lineTest, ComdatFuncHasNoDwp) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we change the name of the test? I don't think this has anything to do with Comdats :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, fixed.
Add support for binaries with both .debug_ranges and .debug_rnglists sections .debug_rnglists was added in DWARF5 to replace .debug_ranges. This changes handles cases when the binary has compilation units with DWARF5 info and compilation units with pre-version-5 DWARF info. Fix processing of DW_AT_ranges attributes to allow DW_FORM_data4 and DW_FORM_data8 This affects DWARF pre-version-4 and was broken by google@dd1395a . This patch also adds a test with a binary from google#154 . Fixes google#154
9e325de
to
7086a84
Compare
Add support for binaries with both .debug_ranges and .debug_rnglists sections .debug_rnglists was added in DWARF5 to replace .debug_ranges. This changes handles cases when the binary has compilation units with DWARF5 info and compilation units with pre-version-5 DWARF info.
Fix processing of DW_AT_ranges attributes to allow DW_FORM_data4 and DW_FORM_data8 This affects DWARF pre-version-4 and was broken by dd1395a .
Fixes #154