[PATCH] D151755: [DebugInfo] Add error checking around data extraction in DWARFAbbreviationDeclaration::extract

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

There are five new places that the function you've changed can bail out with an error, but you've only got one test. I think it makes sense to test the other new places too?



================
Comment at: llvm/unittests/DebugInfo/DWARF/DWARFDebugAbbrevTest.cpp:47
+                                         uint32_t Data) {
+  std::vector<uint32_t> BadData((Size / 4) + 1, Data);
+  OS.write(reinterpret_cast<char *>(BadData.data()), Size);
----------------
Might be worth adding an `assert` that `Size` is a multiple of 4?


================
Comment at: llvm/unittests/DebugInfo/DWARF/DWARFDebugAbbrevTest.cpp:125-126
+  DWARFAbbreviationDeclarationSet AbbrevSet;
+  const bool DataWasExtracted = AbbrevSet.extract(Data, &Offset);
+  EXPECT_FALSE(DataWasExtracted);
+}
----------------
Nit: it seems unnecessary to have this over two lines?

Is it also worth checking that `Offset` is some specific value?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151755