This is an archive of the discontinued LLVM Phabricator instance.

[DWARF] Partial verification for .apple_names accelerator table in llvm-dwarfdump output.
ClosedPublic

Authored by sgravani on Jun 13 2017, 3:19 PM.

Details

Summary

This patch adds code which verifies that each bucket in the .apple_names
accelerator table is either empty or has a valid hash index.

Diff Detail

Event Timeline

sgravani created this revision.Jun 13 2017, 3:19 PM
probinson edited edge metadata.Jun 13 2017, 3:39 PM

Please subscribe 'llvm-commits' when you create a review, so the whole community can see the proposed patch.
(If you add llvm-commits later, we recommend that you paste the original description into a comment, because otherwise the mailing list never sees the original description.)

I understand that DWARFAcceleratorTable already exists and you are building on that, but I have to wonder how much (if any) of this can be leveraged for the actual DWARF v5 standard's .debug_names section. (I haven't looked at any of this stuff yet; the accelerator table is pretty far down my list of v5 features to look at.) If the answer is "not much" then maybe it ought to be renamed DWARFAppleNamesTable instead.

aprantl edited edge metadata.Jun 13 2017, 4:06 PM

Please subscribe 'llvm-commits' when you create a review, so the whole community can see the proposed patch.
(If you add llvm-commits later, we recommend that you paste the original description into a comment, because otherwise the mailing list never sees the original description.)

I understand that DWARFAcceleratorTable already exists and you are building on that, but I have to wonder how much (if any) of this can be leveraged for the actual DWARF v5 standard's .debug_names section. (I haven't looked at any of this stuff yet; the accelerator table is pretty far down my list of v5 features to look at.) If the answer is "not much" then maybe it ought to be renamed DWARFAppleNamesTable instead.

The DWARF v5 accelerator table is a direct evolution of the Apple accelerator table, so it is quite possible that we can share some of the implementation. Form a cursory glance it looks like at least the accessors for the hash table (bucket count, ...) are similar enough that they can share a common interface. We probably can't make that call until someone actually sits down and comes up with a design for how to implement the DWARF 5 accelerator tables, though. How about we revisit this question then? Hopefully this won't be too far out into the future.

test/tools/llvm-dwarfdump/X86/apple_names_verify_buckets.s
7

Could you check for the actual error message and move the # CHECK: line right next to the injected error?

I understand that DWARFAcceleratorTable already exists and you are building on that, but I have to wonder how much (if any) of this can be leveraged for the actual DWARF v5 standard's .debug_names section. (I haven't looked at any of this stuff yet; the accelerator table is pretty far down my list of v5 features to look at.) If the answer is "not much" then maybe it ought to be renamed DWARFAppleNamesTable instead.

The DWARF v5 accelerator table is a direct evolution of the Apple accelerator table, so it is quite possible that we can share some of the implementation. Form a cursory glance it looks like at least the accessors for the hash table (bucket count, ...) are similar enough that they can share a common interface. We probably can't make that call until someone actually sits down and comes up with a design for how to implement the DWARF 5 accelerator tables, though. How about we revisit this question then? Hopefully this won't be too far out into the future.

Thanks for the info, if there's a reasonable chance to make it reasonably common then I'm happy to leave the naming as is for now.

Adding llvm-commits.

Summary:
This patch adds code which verifies that each bucket in the .apple_names
accelerator table is either empty or has a valid hash index.

I understand that DWARFAcceleratorTable already exists and you are building on that, but I have to wonder how much (if any) of this can be leveraged for the actual DWARF v5 standard's .debug_names section. (I haven't looked at any of this stuff yet; the accelerator table is pretty far down my list of v5 features to look at.) If the answer is "not much" then maybe it ought to be renamed DWARFAppleNamesTable instead.

The DWARF v5 accelerator table is a direct evolution of the Apple accelerator table, so it is quite possible that we can share some of the implementation. Form a cursory glance it looks like at least the accessors for the hash table (bucket count, ...) are similar enough that they can share a common interface. We probably can't make that call until someone actually sits down and comes up with a design for how to implement the DWARF 5 accelerator tables, though. How about we revisit this question then? Hopefully this won't be too far out into the future.

Thanks for the info, if there's a reasonable chance to make it reasonably common then I'm happy to leave the naming as is for now.

And it is totally possible that

include/llvm/DebugInfo/DWARF/DWARFVerifier.h
103

side-note: We generally use \return over @return. You might want to update this in the entire file in a separate commit.

lib/DebugInfo/DWARF/DWARFVerifier.cpp
291

General question: Should error messages be printed to errs() (or rather a separate error stream passed to DWARFVerifier)?
This is a pre-existing problem with the other checks, too, so it doesn't need to be fixed in this patch.

A couple of minor comments inline. I'm okay with this, assuming Adrian is.

test/tools/llvm-dwarfdump/X86/apple_names_verify_buckets.s
2

Is the "env" necessary? I don't recall seeing other tests that do that.

11

s/the there/there/

sgravani updated this revision to Diff 102454.Jun 13 2017, 4:57 PM

Removed the env from test case.
Fixed the description in test case.
Added the error message next to the line in the .s file that triggers the error in verify.

sgravani marked 2 inline comments as done.Jun 13 2017, 4:59 PM
test/tools/llvm-dwarfdump/X86/apple_names_verify_buckets.s
2

It wasn't, good catch.

11

Not sure I addressed your comment, I fixed the text a bit.

sgravani marked an inline comment as done.Jun 13 2017, 5:01 PM
sgravani added inline comments.
include/llvm/DebugInfo/DWARF/DWARFVerifier.h
103

Ok, I'll do that.

aprantl accepted this revision.Jun 13 2017, 5:10 PM

This is good to go now. Thanks!

This revision is now accepted and ready to land.Jun 13 2017, 5:10 PM
probinson added inline comments.Jun 13 2017, 5:18 PM
test/tools/llvm-dwarfdump/X86/apple_names_verify_buckets.s
11

New comment looks fine thanks!

This revision was automatically updated to reflect the committed changes.
dblaikie added inline comments.Jun 15 2017, 2:09 PM
llvm/trunk/include/llvm/DebugInfo/DWARF/DWARFVerifier.h
81 ↗(On Diff #102459)

Maybe use a non-static data member initializer here (& move the other members over to using that syntax too (in a separate pre-commit, doesn't need review))

llvm/trunk/lib/DebugInfo/DWARF/DWARFVerifier.cpp
290–293 ↗(On Diff #102459)

Is this error produced when the section is not present? Is that the right behavior/does this need an error in that case?

310 ↗(On Diff #102459)

Probably use a boolean flag if that's the only thing that matters? ("HasErrors"?)

llvm/trunk/test/tools/llvm-dwarfdump/X86/apple_names_verify_buckets.s
1–10 ↗(On Diff #102459)

Provide some detail about what this test case was built from (source code, command line, etc)?

5–6 ↗(On Diff #102459)

This seems pretty vague/non-specific for a test case. The error message should be tested.

Also, add a test case with an object with no apple names, to demonstrate what happens there. (be it an error, or silence, depending on the resolution of my other comment).

sgravani marked 4 inline comments as done.Jun 16 2017, 3:06 PM
sgravani added inline comments.
llvm/trunk/lib/DebugInfo/DWARF/DWARFVerifier.cpp
290–293 ↗(On Diff #102459)

In such a case, we should not have any output message.
Thanks for noticing! fixed it in new commit.

310 ↗(On Diff #102459)

I prefer to stay consistent with the naming in the other handle[section] functions in the class.