[DebugInfo][NFCI] Refactor DWARFAbbreviationDeclaration::extract

[bulbazord]

Summary

The motivation behind this refactor is to be able to use
DWARFAbbreviationDeclaration from LLDB. LLDB has its own implementation
of DWARFAbbreviationDeclaration that is very similar to LLVM's but it
has different semantics around error handling.

This patch modifies llvm::DWARFAbbreviationDeclaration::extract to
return an llvm::Expected<ExtractState> to differentiate between "I am
done extracting" and "An error has occured", something which the current
return type (bool) does not accurately capture.

[rastogishubham]

Just a small comment, but otherwise LGTM

[jhenderson]

llvm/lib/DebugInfo/DWARF/DWARFAbbreviationDeclaration.cpp

50

(ignore this suggestion if this isn't going to end up as a user-facing error)

There are guidelines on error and warning formatting (see https://llvm.org/docs/CodingStandards.html#error-and-warning-messages), so we should conform to those in new code, unless there's a strong reason not to.

77

Same as above.

128–129

Same as above.

[bulbazord]

llvm/lib/DebugInfo/DWARF/DWARFAbbreviationDeclaration.cpp

50

Thanks for pointing that out. They will be user-facing from LLDB (and possibly from LLVM tools in the future), so I'll update this.

[bulbazord]

Update error message formatting to conform to LLVM's error messaging guidelines

llvm/include/llvm/DebugInfo/DWARF/DWARFAbbreviationDeclaration.h

 namespace llvm {
 
 class DataExtractor;
 class DWARFUnit;
 class raw_ostream;
 
 class DWARFAbbreviationDeclaration {
 public:
+  enum class ExtractState { Complete, MoreItems };
   struct AttributeSpec {
     AttributeSpec(dwarf::Attribute A, dwarf::Form F, int64_t Value)
         : Attr(A), Form(F), Value(Value) {
       assert(isImplicitConst());
     }
     AttributeSpec(dwarf::Attribute A, dwarf::Form F,
                   std::optional<uint8_t> ByteSize)
         : Attr(A), Form(F) {
public:
   /// \param AttrIndex an index of DWARF attribute.
   /// \param Offset offset of the attribute.
   /// \param U the DWARFUnit the contains the DIE.
   /// \returns Optional DWARF form value if the attribute was extracted.
   std::optional<DWARFFormValue>
   getAttributeValueFromOffset(uint32_t AttrIndex, uint64_t Offset,
                               const DWARFUnit &U) const;
 
-  bool extract(DataExtractor Data, uint64_t* OffsetPtr);
+  llvm::Expected<ExtractState> extract(DataExtractor Data, uint64_t *OffsetPtr);
   void dump(raw_ostream &OS) const;
 
   // Return an optional byte size of all attribute data in this abbreviation
   // if a constant byte size can be calculated given a DWARFUnit. This allows
   // DWARF parsing to be faster as many DWARF DIEs have a fixed byte size.
   std::optional<size_t> getFixedAttributesByteSize(const DWARFUnit &U) const;
 
 private:

llvm/lib/DebugInfo/DWARF/DWARFAbbreviationDeclaration.cpp

void DWARFAbbreviationDeclaration::clear() {
   AttributeSpecs.clear();
   FixedAttributeSize.reset();
 }
 
 DWARFAbbreviationDeclaration::DWARFAbbreviationDeclaration() {
   clear();
 }
 
-bool
-DWARFAbbreviationDeclaration::extract(DataExtractor Data,
-                                      uint64_t* OffsetPtr) {
+llvm::Expected<DWARFAbbreviationDeclaration::ExtractState>
+DWARFAbbreviationDeclaration::extract(DataExtractor Data, uint64_t *OffsetPtr) {
   clear();
   const uint64_t Offset = *OffsetPtr;
   Code = Data.getULEB128(OffsetPtr);
-  if (Code == 0) {
-    return false;
-  }
+  if (Code == 0)
+    return ExtractState::Complete;
+
   CodeByteSize = *OffsetPtr - Offset;
   Tag = static_cast<llvm::dwarf::Tag>(Data.getULEB128(OffsetPtr));
   if (Tag == DW_TAG_null) {
     clear();
-    return false;
+    return make_error<llvm::object::GenericBinaryError>(
+        "Abbreviation declaration requires a non-null Tag!");
   }
   uint8_t ChildrenByte = Data.getU8(OffsetPtr);
   HasChildren = (ChildrenByte == DW_CHILDREN_yes);
   // Assign a value to our optional FixedAttributeSize member variable. If
   // this member variable still has a value after the while loop below, then
   // all attribute data in this abbreviation declaration has a fixed byte size.
   FixedAttributeSize = FixedSizeInfo();
 
   // Read all of the abbreviation attributes and forms.
-  while (true) {
+  while (Data.isValidOffset(*OffsetPtr)) {
     auto A = static_cast<Attribute>(Data.getULEB128(OffsetPtr));
     auto F = static_cast<Form>(Data.getULEB128(OffsetPtr));
-    if (A && F) {
-      bool IsImplicitConst = (F == DW_FORM_implicit_const);
-      if (IsImplicitConst) {
-        int64_t V = Data.getSLEB128(OffsetPtr);
-        AttributeSpecs.push_back(AttributeSpec(A, F, V));
-        continue;
-      }
-      std::optional<uint8_t> ByteSize;
-      // If this abbrevation still has a fixed byte size, then update the
-      // FixedAttributeSize as needed.
-      switch (F) {
-      case DW_FORM_addr:
-        if (FixedAttributeSize)
-          ++FixedAttributeSize->NumAddrs;
-        break;
+
+    // We successfully reached the end of this abbreviation declaration
+    // since both attribute and form are zero. There may be more abbreviation
+    // declarations afterwards.
+    if (!A && !F)
+      return ExtractState::MoreItems;
+
+    if (!A || !F) {
+      // Attribute and form pairs must either both be non-zero, in which case
+      // they are added to the abbreviation declaration, or both be zero to
+      // terminate the abbrevation declaration. In this case only one was
+      // zero which is an error.
+      clear();
+      return make_error<llvm::object::GenericBinaryError>(
+          "Malformed abbreviation declaration attribute!");
+    }
+
 
-      case DW_FORM_ref_addr:
-        if (FixedAttributeSize)
-          ++FixedAttributeSize->NumRefAddrs;
-        break;
 
-      case DW_FORM_strp:
-      case DW_FORM_GNU_ref_alt:
-      case DW_FORM_GNU_strp_alt:
-      case DW_FORM_line_strp:
-      case DW_FORM_sec_offset:
-      case DW_FORM_strp_sup:
-        if (FixedAttributeSize)
-          ++FixedAttributeSize->NumDwarfOffsets;
-        break;
 
-      default:
-        // The form has a byte size that doesn't depend on Params.
-        // If it's a fixed size, keep track of it.
-        if ((ByteSize = dwarf::getFixedFormByteSize(F, dwarf::FormParams()))) {
-          if (FixedAttributeSize)
-            FixedAttributeSize->NumBytes += *ByteSize;
-          break;
-        }
-        // Indicate we no longer have a fixed byte size for this
-        // abbreviation by clearing the FixedAttributeSize optional value
-        // so it doesn't have a value.
-        FixedAttributeSize.reset();
-        break;
-      }
-      // Record this attribute and its fixed size if it has one.
-      AttributeSpecs.push_back(AttributeSpec(A, F, ByteSize));
-    } else if (A == 0 && F == 0) {
-      // We successfully reached the end of this abbreviation declaration
-      // since both attribute and form are zero.
-      break;
-    } else {
-      // Attribute and form pairs must either both be non-zero, in which case
-      // they are added to the abbreviation declaration, or both be zero to
-      // terminate the abbrevation declaration. In this case only one was
-      // zero which is an error.
-      clear();
-      return false;
-    }
   }
-  return true;
+  return make_error<llvm::object::GenericBinaryError>(
+      "Abbreviation declaration attribute list was not terminated with a null "
+      "entry!");
 }
 
 void DWARFAbbreviationDeclaration::dump(raw_ostream &OS) const {
   OS << '[' << getCode() << "] ";
   OS << formatv("{0}", getTag());
   OS << "\tDW_CHILDREN_" << (hasChildren() ? "yes" : "no") << '\n';
   for (const AttributeSpec &Spec : AttributeSpecs) {
     OS << formatv("\t{0}\t{1}", Spec.Attr, Spec.Form);

llvm/lib/DebugInfo/DWARF/DWARFDebugAbbrev.cpp

 
 bool DWARFAbbreviationDeclarationSet::extract(DataExtractor Data,
                                               uint64_t *OffsetPtr) {
   clear();
   const uint64_t BeginOffset = *OffsetPtr;
   Offset = BeginOffset;
   DWARFAbbreviationDeclaration AbbrDecl;
   uint32_t PrevAbbrCode = 0;
-  while (AbbrDecl.extract(Data, OffsetPtr)) {
+  while (true) {
+    llvm::Expected<DWARFAbbreviationDeclaration::ExtractState> ES =
+        AbbrDecl.extract(Data, OffsetPtr);
+    if (!ES) {
+      // FIXME: We should propagate the error upwards.
+      llvm::consumeError(ES.takeError());
+      break;
+    }
+
+    if (*ES == DWARFAbbreviationDeclaration::ExtractState::Complete)
+      break;
+
     if (FirstAbbrCode == 0) {
       FirstAbbrCode = AbbrDecl.getCode();
-    } else {
-      if (PrevAbbrCode + 1 != AbbrDecl.getCode()) {
-        // Codes are not consecutive, can't do O(1) lookups.
-        FirstAbbrCode = UINT32_MAX;
-      }
-    }
+    } else if (PrevAbbrCode + 1 != AbbrDecl.getCode()) {
     PrevAbbrCode = AbbrDecl.getCode();
     Decls.push_back(std::move(AbbrDecl));
   }
   return BeginOffset != *OffsetPtr;
 }
 
 void DWARFAbbreviationDeclarationSet::dump(raw_ostream &OS) const {
   for (const auto &Decl : Decls)