[PATCH] D71875: [DWARF] Return Error from DWARFDebugArangeSet::extract().

[David Blaikie via Phabricator via llvm-commits]

dblaikie accepted this revision.
dblaikie added inline comments.
This revision is now accepted and ready to land.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugArangeSet.cpp:85
+
+  while (data.isValidOffset(*offset_ptr)) {
+    arangeDescriptor.Address = data.getUnsigned(offset_ptr, HeaderData.AddrSize);
----------------
In a separate patch, might be good to add error handling to stop when the Header length is reached, rather than when the section end is reached.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugArangeSet.cpp:91
+    // for the length.
+    if (!arangeDescriptor.Address && !arangeDescriptor.Length)
+      return ErrorSuccess();
----------------
I'd probably write these ays "x == 0" rather than "!x"


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugArangeSet.cpp:91-92
+    // for the length.
+    if (!arangeDescriptor.Address && !arangeDescriptor.Length)
+      return ErrorSuccess();
+    ArangeDescriptors.push_back(arangeDescriptor);
----------------
dblaikie wrote:
> I'd probably write these ays "x == 0" rather than "!x"
Potentially another error case could be added here (in another patch) in the case where the list is terminated earlier than the Length specified in the header.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugArangeSet.cpp:96
-
-    return !ArangeDescriptors.empty();
   }
----------------
Looks like the old code fails if the range list is empty - and maybe the new code deosn't fail in that case? Worth adding a test case? (it actually seems reasonable to me to support it - you could imagine a CU with only type information in it & so no covered addresses, and having an arange entry describing that so the consumer didn't have to go discover it through parsing the CU, etc)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71875