[PATCH] D151353: [DebugInfo] Add error-handling to DWARFAbbreviationDeclarationSet

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFDebugAbbrev.h:37
   void dump(raw_ostream &OS) const;
-  bool extract(DataExtractor Data, uint64_t *OffsetPtr);
+  llvm::Error extract(DataExtractor Data, uint64_t *OffsetPtr);
 
----------------
`llvm::` seems unnecessary?


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugAbbrev.cpp:28
 
-bool DWARFAbbreviationDeclarationSet::extract(DataExtractor Data,
-                                              uint64_t *OffsetPtr) {
+llvm::Error DWARFAbbreviationDeclarationSet::extract(DataExtractor Data,
+                                                     uint64_t *OffsetPtr) {
----------------
Ditto.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugAbbrev.cpp:36
   while (true) {
     llvm::Expected<DWARFAbbreviationDeclaration::ExtractState> ES =
         AbbrDecl.extract(Data, OffsetPtr);
----------------
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.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugAbbrev.cpp:53
   }
-  return BeginOffset != *OffsetPtr;
+  return llvm::ErrorSuccess();
 }
----------------
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();`


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugAbbrev.cpp:127
     DWARFAbbreviationDeclarationSet AbbrDecls;
-    if (!AbbrDecls.extract(*Data, &Offset))
+    if (auto Err = AbbrDecls.extract(*Data, &Offset)) {
+      // FIXME: We should propagate the error upwards.
----------------
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.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugAbbrev.cpp:129
+      // FIXME: We should propagate the error upwards.
+      llvm::consumeError(std::move(Err));
       break;
----------------
Unnecessary `llvm::` qualifier?


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugAbbrev.cpp:169
+      // FIXME: We should propagate the error upwards.
+      llvm::consumeError(std::move(Err));
       return nullptr;
----------------
Ditto.


================
Comment at: llvm/unittests/DebugInfo/DWARF/DWARFDebugAbbrevTest.cpp:56-57
   DWARFAbbreviationDeclarationSet AbbrevSet;
-  const bool DataWasExtracted = AbbrevSet.extract(Data, &Offset);
-  EXPECT_TRUE(DataWasExtracted);
+  llvm::Error Err = AbbrevSet.extract(Data, &Offset);
+  ASSERT_FALSE(bool(Err));
   // The Abbreviation Declarations are in order and contiguous, so we want to
----------------
This is the canonical way of testing errors. There's an equivalent invocation for testing Expected and also failure states too.


================
Comment at: llvm/unittests/DebugInfo/DWARF/DWARFDebugAbbrevTest.cpp:88-89
   DWARFAbbreviationDeclarationSet AbbrevSet;
-  const bool DataWasExtracted = AbbrevSet.extract(Data, &Offset);
-  EXPECT_TRUE(DataWasExtracted);
+  llvm::Error Err = AbbrevSet.extract(Data, &Offset);
+  ASSERT_FALSE(bool(Err));
   // The declarations are out of order, ensure that FirstAbbrCode is UINT32_MAX.
----------------
See above.


================
Comment at: llvm/unittests/DebugInfo/DWARF/DWARFDebugAbbrevTest.cpp:130-134
+  llvm::Error Err = AbbrevSet.extract(Data, &Offset);
+  // Verify that we got an error and it is the one we expected.
+  ASSERT_TRUE(bool(Err));
+  EXPECT_EQ("abbreviation declaration requires a non-null tag",
+            llvm::toString(std::move(Err)));
----------------
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.


================
Comment at: llvm/unittests/DebugInfo/DWARF/DWARFDebugAbbrevTest.cpp:159-164
+  llvm::Error Err = AbbrevSet.extract(Data, &Offset);
+  // Verify that we got an error and it is the one we expected.
+  ASSERT_TRUE(bool(Err));
+  EXPECT_EQ("malformed abbreviation declaration attribute. Either the "
+            "attribute or the form is zero while the other is not",
+            llvm::toString(std::move(Err)));
----------------
As above.


================
Comment at: llvm/unittests/DebugInfo/DWARF/DWARFDebugAbbrevTest.cpp:189-194
+  llvm::Error Err = AbbrevSet.extract(Data, &Offset);
+  // Verify that we got an error and it is the one we expected.
+  ASSERT_TRUE(bool(Err));
+  EXPECT_EQ("malformed abbreviation declaration attribute. Either the "
+            "attribute or the form is zero while the other is not",
+            llvm::toString(std::move(Err)));
----------------
As above.


================
Comment at: llvm/unittests/DebugInfo/DWARF/DWARFDebugAbbrevTest.cpp:215-220
+  llvm::Error Err = AbbrevSet.extract(Data, &Offset);
+  // Verify that we got an error and it is the one we expected.
+  ASSERT_TRUE(bool(Err));
+  EXPECT_EQ("abbreviation declaration attribute list was not terminated with a "
+            "null entry",
+            llvm::toString(std::move(Err)));
----------------
As above.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151353/new/

https://reviews.llvm.org/D151353