This is an archive of the discontinued LLVM Phabricator instance.

[DebugInfo] Add error-handling to DWARFAbbreviationDeclarationSet
ClosedPublic

Authored by bulbazord on May 24 2023, 10:45 AM.

Details

Summary

This commit aims to improve error handling in the
DWARFAbbreviationDeclarationSet class. Specifically, we change the return type
of DWARFAbbreviationDeclarationSet::extract to an llvm::Error. In doing
so, we propagate the error from DWARFAbbreviationDeclaration::extract
another layer upward.

I have built on the previous unittest for DWARFDebugAbbrev that I
wrote a few days prior.
Namely, I am verifying that the following should give an error:

  • An invalid tag following a non-null code
  • An invalid attribute with a valid form
  • A valid attribute with an invalid form
  • An incorrectly terminated DWARFAbbreviationDeclaration

Additionally, I uncovered some invalid DWARF in an unrelated dsymutil
test. Namely the last Abbreviation Decl was missing a code.
This test has been updated accordingly. However, this commit does
not fix the underlying issue: llvm-dwarfdump does not correctly
verify the debug abbreviation section to catch these kinds of
mistakes. I have updated DWARFVerifier to not dereference a
pointer without first checking it and left a FIXME for future
contributors.

Diff Detail

Event Timeline

bulbazord created this revision.May 24 2023, 10:45 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 24 2023, 10:45 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
bulbazord requested review of this revision.May 24 2023, 10:45 AM
Herald added a project: Restricted Project. · View Herald Transcript
fdeazeve added inline comments.May 24 2023, 12:10 PM
llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp
304

I am a bit confused by this: isn't NumErrors always 0 here?

312

Same questions about NumErrors here

fdeazeve added inline comments.May 24 2023, 12:12 PM
llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp
304

Oh nvm, I see that the previous code was already doing the same

bulbazord added inline comments.May 24 2023, 12:31 PM
llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp
304

I can move the declaration down and return 0 directly here. That might make it less confusing.

aprantl accepted this revision.May 24 2023, 3:49 PM

LGTM if the other reviewers are happy!

This revision is now accepted and ready to land.May 24 2023, 3:49 PM
jhenderson added inline comments.May 25 2023, 3:18 AM
llvm/include/llvm/DebugInfo/DWARF/DWARFDebugAbbrev.h
37

llvm:: seems unnecessary?

llvm/lib/DebugInfo/DWARF/DWARFDebugAbbrev.cpp
28

Ditto.

36

I know this isn't something you changed in this patch, but it might be worth removing the unnecessary qualifier here too, whilst you're in the immediate area.

53

I feel like it's stated somewhere, although I don't know where, but the preferred way of returning a success state is to do return Error::success();

127

Is there a particular reason you're using auto rather than Error here and in the similar code below? It doesn't seem to improve readability to me.

129

Unnecessary llvm:: qualifier?

169

Ditto.

llvm/unittests/DebugInfo/DWARF/DWARFDebugAbbrevTest.cpp
56–57

This is the canonical way of testing errors. There's an equivalent invocation for testing Expected and also failure states too.

88–89

See above.

130–134

Use EXPECT_THAT_ERROR with FailedWithMessage to check the error has a specific message. EXPECT version should be preferred, since there's no further code in this test.

159–164

As above.

189–194

As above.

215–220

As above.

bulbazord added inline comments.May 25 2023, 10:58 AM
llvm/lib/DebugInfo/DWARF/DWARFDebugAbbrev.cpp
53

Ah yes, I took some time to read the LLVM Programmer's Manual and they suggest doing the same: https://llvm.org/docs/ProgrammersManual.html#recoverable-errors

127

In general, I use auto when I feel I can convey the type information through other means (the expression or name of variable), but I hold my opinion on the use of auto very weakly. I'll switch it over to use Error.

llvm/unittests/DebugInfo/DWARF/DWARFDebugAbbrevTest.cpp
56–57

Thanks for pointing that out! I'm still trying to understand LLVM's best practices with respect to testing.

bulbazord updated this revision to Diff 525765.May 25 2023, 1:06 PM

Address feedback from @jhenderson

fdeazeve added inline comments.May 26 2023, 4:53 AM
llvm/unittests/DebugInfo/DWARF/DWARFDebugAbbrevTest.cpp
24

This is likely irrelevant for the patch itself, but I missed the boat on the original review: aren't we missing the compile unit terminator for this in most tests?

We have this structure:

First code
  DW_TAG_compile_unit
    Has Children = true
    Name, strp
    0, 0   // terminates list of attributes

    DW_TAG_subprogram   // children of compile unit
      Has children = false
      Name, strp
      0, 0   // terminates list of attributes

This is all the function writes. Sometimes, the callees write an extra zero, which is terminating the DW_TAG_compile_unit children list. (Maybe we should write that one here instead? The extra entries we write manually write in each test would simply become siblings of the TAG_compile_unit, which is fine)

But even with that extra zero, we're missing an extra zero terminating the abbreviation table itself (when multiple abbreviation tables are stitched together, they are separated by this terminator, see page 207 line 15 of the D5 spec)

Anyhow, I am pointing all of this out because it's curious that the DWARFAbbreviationDeclarationSet::extract method seems to happily ignore the absence of this?

fdeazeve accepted this revision.May 26 2023, 5:01 AM

LGTM! Thanks for driving this

llvm/unittests/DebugInfo/DWARF/DWARFDebugAbbrevTest.cpp
24

Ahhhhhh nvm, I am making the same mistake I made months ago when I first looked into this:

Sometimes, the callees write an extra zero, which is terminating the DW_TAG_compile_unit children list

This is wrong. The extra zero terminating the children lists are part of the debug info section, not the debug abbrev section. The zero you write is indeed terminating the abbrev table itself.

bulbazord marked 17 inline comments as done.May 28 2023, 3:58 PM
jhenderson accepted this revision.May 30 2023, 12:34 AM

LGMT. There are a couple of pre-merge tests that failed that have something to do with DWARF. It's unlikely to be related to this patch, but it might be worth double-checking before landing this.

llvm/lib/DebugInfo/DWARF/DWARFDebugAbbrev.cpp
127

It's worth noting that LLVM has policies on auto usage documented in the coding standards document. I think it largely boils down to "use auto for iterators and where the type can be inferred from the right-hand-side of the expression", although there are some nuances to it beyond that.

bulbazord marked an inline comment as done.May 30 2023, 10:17 AM

LGMT. There are a couple of pre-merge tests that failed that have something to do with DWARF. It's unlikely to be related to this patch, but it might be worth double-checking before landing this.

Yeah, I saw that as well. I was going to see if I could reproduce the issue locally before landing. If not, I'll watch out for failures on the bot and go from there. Thank you for the review!

llvm/lib/DebugInfo/DWARF/DWARFDebugAbbrev.cpp
127

I'll take a look, thanks for pointing that out.

JDevlieghere accepted this revision.May 30 2023, 10:47 AM
bulbazord marked an inline comment as done.May 30 2023, 10:49 AM

Ok, the pre-submission checks failing are actually due to this change. Specifically, parsing the debug_abbrev section of the objects produced by these tests leads to an infinite loop. It goes as follows:

  • We fail to get an ULEB128 value for the code because the contents of the debug_abbrev are invalid. The DataExtractor thus returns 0, signaling to the DWARFAbbreviationDeclaration parsing code that were done with this DWARFAbbreviationDeclaration.
  • The DWARFAbbreviationDeclarationSet sees it's done and returns an empty Set to the DWARFDebugAbbrev parsing code. The offset is still at 0, so it's still technically a valid offset for the DataExtractor. So we continue on...

I believe DWARFDebugAbbrev::parse was relying on knowing when it's done by checking if the offset has moved. Otherwise we end up in an infinite loop. I'm going to add some additional safety checks, but it shouldn't be significant. I'll request another review after adding it.

I've identified the root cause and I aim to address it in a separate change before this change goes in: https://reviews.llvm.org/D151755

Afterwards I will revisit and adjust this change if needed. If this change needs no adjustments, I will land it shortly after that change.

bulbazord updated this revision to Diff 528995.Jun 6 2023, 12:37 PM

Updating to match changes from D151755

bulbazord requested review of this revision.Jun 6 2023, 12:38 PM

I didn't change anything except the tests, could I get a pair of eyes on the tests one last time? Thanks!

jhenderson accepted this revision.Jun 7 2023, 4:00 AM

Latest changes look fine, but there are a number of failures in the pre-merge check. Is this patch based on another pending commit? If so, please make sure to update the parent/child relationship that Phabricator knows about by selecting the Edit Related Revisions option in the UI and updating accordingly. You could also put in the child patch description "Depends on D......" (where D...... is the relevant patch ID).

This revision is now accepted and ready to land.Jun 7 2023, 4:00 AM

Latest changes look fine, but there are a number of failures in the pre-merge check. Is this patch based on another pending commit? If so, please make sure to update the parent/child relationship that Phabricator knows about by selecting the Edit Related Revisions option in the UI and updating accordingly. You could also put in the child patch description "Depends on D......" (where D...... is the relevant patch ID).

This patch is not based on another pending commit, it looks like these these are a result of this change. Normally I run the test suite before submitting a patch but I don't have a memory of doing that again here, so I'm assuming that I didn't. I'll dig into it, thanks for the help.

bulbazord updated this revision to Diff 529486.Jun 7 2023, 7:03 PM

Update tests with incorrect debug_abbrev data.
Modified one dsymutil test to generate an object instead of checking it in.

fdeazeve accepted this revision.Jun 8 2023, 3:32 AM

LGTM! Some of those invalid tests must have been painful to track, thanks for looking into them