Return llvm::Error and llvm::Expected from some DWARF parsing functions

[zturner]

Summary

The goal here is to improve our error handling and error recovery while parsing DWARF, while at the same time getting us closer to being able to merge LLDB's DWARF parser with LLVM's. To this end, I've udpated several of the low-level parsing functions in LLDB to return llvm::Error and llvm::Expected.

For now, this only updates LLDB parsing functions and not LLVM. In some ways, this actually gets us *farther* from parity with the two interfaces, because prior to this patch, at least the parsing interfaces were the same (i.e. they all just returned bools, and now with this patch they're diverging). But, I chose to do this for two primary reasons.

1. LLDB has error logging code engrained deep within some of its parsing functions. We don't want to lose this logging information, but obviously LLVM has no logging mechanism at all. So if we're to merge the interfaces, we have to find a way to still allow LLDB to properly report parsing errors while not having the reporting code be inside of LLVM.

2. LLDB (and indeed, LLVM) overload the meaning of the false return value from all of these extraction functions to mean both "We reached the null entry at the end of a list of items, therefore everything was successful" as well as "something bad and unrecoverable happened during parsing". So you would have a lot code that would do something like:

while (foo.extract(...)) {
  ...
}

But when the loop stops, why did it stop? Did it stop because it finished parsing, or because there was an error? Because of this, in some cases we don't always know whether it is ok to proceed, or how to proceed, but we were doing it anyway.

In this patch, I solve the second problem by introducing an enumeration called DWARFEnumState which has two values MoreItems and Complete. Both of these indicate success, but the latter indicates that we reached the null entry. Then, I return this value instead of bool, and convey parsing failure separately.

To solve the first problem (and convey parsing failure) these functions now return either llvm::Error or llvm::Expected<DWARFEnumState>. Having this extra bit of information allows us to properly convey all 3 of us "error, bail out", "success, call this function again", and "success, don't call this function again".

In subsequent patches I plan to extend this pattern to the rest of the parsing interfaces, which will ultimately get all of the log statements and error reporting out of the low level parsing code and into the high level parsing code (e.g. SymbolFileDWARF, DWARFASTParserClang, etc).

Eventually, these same changes will have to be backported to LLVM's DWARF parser, but I feel like diverging in the short term is the quickest way to converge in the long term.

[aprantl]

lldb/source/Plugins/SymbolFile/DWARF/DWARFAbbreviationDeclaration.h

50

half-finished comment?

[zturner]

Fix half-written comment and add comments to other extraction function declarations.

[clayborg]

As long as there is not a large performance regress when parsing large DWARF files this looks good to me. Abbreviation declarations are small, so I don't expect one.

lldb/source/Plugins/SymbolFile/DWARF/DWARFAbbreviationDeclaration.cpp

37–42

Seems like this would be malformed and should be an error. Something about NULL tag when valid tag was expected

lldb/source/Plugins/SymbolFile/DWARF/DWARFAbbreviationDeclaration.h

 //===-- DWARFAbbreviationDeclaration.h --------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef liblldb_DWARFAbbreviationDeclaration_h_
 #define liblldb_DWARFAbbreviationDeclaration_h_
 
 #include "DWARFAttribute.h"
+#include "DWARFDefines.h"
 #include "SymbolFileDWARF.h"
+#include "llvm/Support/Error.h"
 
 class DWARFAbbreviationDeclaration {
 public:
   enum { InvalidCode = 0 };
   DWARFAbbreviationDeclaration();
 
   // For hand crafting an abbreviation declaration
   DWARFAbbreviationDeclaration(dw_tag_t tag, uint8_t has_children);
public:
   void GetAttrAndFormValueByIndex(uint32_t idx, dw_attr_t &attr,
                                   DWARFFormValue &form_value) const {
     m_attributes[idx].get(attr, form_value.FormRef(), form_value.ValueRef());
   }
   dw_form_t GetFormByIndexUnchecked(uint32_t idx) const {
     return m_attributes[idx].get_form();
   }
   uint32_t FindAttributeIndex(dw_attr_t attr) const;
-  bool Extract(const lldb_private::DWARFDataExtractor &data,
-               lldb::offset_t *offset_ptr);
-  bool Extract(const lldb_private::DWARFDataExtractor &data,
-               lldb::offset_t *offset_ptr, dw_uleb128_t code);
+
+  /// Extract one a
+  llvm::Expected<lldb_private::DWARFEnumState>
+  extract(const lldb_private::DWARFDataExtractor &data,
   bool IsValid();
   void Dump(lldb_private::Stream *s) const;
   bool operator==(const DWARFAbbreviationDeclaration &rhs) const;
   const DWARFAttribute::collection &Attributes() const { return m_attributes; }
 
 protected:
   dw_uleb128_t m_code;
   dw_tag_t m_tag;
   uint8_t m_has_children;
   DWARFAttribute::collection m_attributes;
 };
 
 #endif // liblldb_DWARFAbbreviationDeclaration_h_

lldb/source/Plugins/SymbolFile/DWARF/DWARFAbbreviationDeclaration.cpp

 //===-- DWARFAbbreviationDeclaration.cpp ------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "DWARFAbbreviationDeclaration.h"
 
 #include "lldb/Core/dwarf.h"
 #include "lldb/Utility/Stream.h"
 
+#include "llvm/Object/Error.h"
+
 #include "DWARFFormValue.h"
 
 using namespace lldb_private;
 
 DWARFAbbreviationDeclaration::DWARFAbbreviationDeclaration()
     : m_code(InvalidCode), m_tag(0), m_has_children(0), m_attributes() {}
 
 DWARFAbbreviationDeclaration::DWARFAbbreviationDeclaration(dw_tag_t tag,
                                                            uint8_t has_children)
     : m_code(InvalidCode), m_tag(tag), m_has_children(has_children),
       m_attributes() {}
 
-bool DWARFAbbreviationDeclaration::Extract(const DWARFDataExtractor &data,
-                                           lldb::offset_t *offset_ptr) {
-  return Extract(data, offset_ptr, data.GetULEB128(offset_ptr));
-}
+llvm::Expected<DWARFEnumState>
+DWARFAbbreviationDeclaration::extract(const DWARFDataExtractor &data,
+  m_code = data.GetULEB128(offset_ptr);
+  if (m_code == 0)
+    return DWARFEnumState::Complete;
 
-bool DWARFAbbreviationDeclaration::Extract(const DWARFDataExtractor &data,
-                                           lldb::offset_t *offset_ptr,
-                                           dw_uleb128_t code) {
-  m_code = code;
   m_attributes.clear();
-  if (m_code) {
-    m_tag = data.GetULEB128(offset_ptr);
-    m_has_children = data.GetU8(offset_ptr);
+  if (m_tag == DW_TAG_null) {
+    // FIXME: According to the DWARF spec this may actually be malformed.
+    // Should this return an error instead?
+    return DWARFEnumState::Complete;
+  }
+
 
-    while (data.ValidOffset(*offset_ptr)) {
-      dw_attr_t attr = data.GetULEB128(offset_ptr);
-      dw_form_t form = data.GetULEB128(offset_ptr);
-      DWARFFormValue::ValueType val;
+
+    // This is the last attribute for this abbrev decl, but there may still be
+    // more abbrev decls, so return MoreItems to indicate to the caller that
+    // they should call this function again.
+    if (!attr && !form)
+      return DWARFEnumState::MoreItems;
+
+    if (!attr || !form)
+      return llvm::make_error<llvm::object::GenericBinaryError>(
+          "malformed abbreviation declaration attribute");
+
 
-      if (form == DW_FORM_implicit_const)
-        val.value.sval = data.GetULEB128(offset_ptr);
 
-      if (attr && form)
-        m_attributes.push_back(DWARFAttribute(attr, form, val));
-      else
-        break;
-    }
-
-    return m_tag != 0;
-  } else {
-    m_tag = 0;
-    m_has_children = 0;
   }
 
-  return false;
+  return llvm::make_error<llvm::object::GenericBinaryError>(
+      "abbreviation declaration attribute list not terminated with a null "
+      "entry");
 }
 
 void DWARFAbbreviationDeclaration::Dump(Stream *s) const {
   s->Printf("Debug Abbreviation Declaration: code = 0x%4.4x, tag = %s, "
             "has_children = %s\n",
             m_code, DW_TAG_value_to_name(m_tag),
             DW_CHILDREN_value_to_name(m_has_children));
 

lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugAbbrev.h

DWARFAbbreviationDeclarationSet()
       : m_offset(DW_INVALID_OFFSET), m_idx_offset(0), m_decls() {}
 
   DWARFAbbreviationDeclarationSet(dw_offset_t offset, uint32_t idx_offset)
       : m_offset(offset), m_idx_offset(idx_offset), m_decls() {}
 
   void Clear();
   dw_offset_t GetOffset() const { return m_offset; }
   void Dump(lldb_private::Stream *s) const;
-  bool Extract(const lldb_private::DWARFDataExtractor &data,
+  llvm::Error extract(const lldb_private::DWARFDataExtractor &data,
                       lldb::offset_t *offset_ptr);
   // void Encode(BinaryStreamBuf& debug_abbrev_buf) const;
   dw_uleb128_t
   AppendAbbrevDeclSequential(const DWARFAbbreviationDeclaration &abbrevDecl);
   void GetUnsupportedForms(std::set<dw_form_t> &invalid_forms) const;
 
   const DWARFAbbreviationDeclaration *
   GetAbbreviationDeclaration(dw_uleb128_t abbrCode) const;
 
typedef DWARFAbbreviationDeclarationCollMap::const_iterator
     DWARFAbbreviationDeclarationCollMapConstIter;
 
 class DWARFDebugAbbrev {
 public:
   DWARFDebugAbbrev();
   const DWARFAbbreviationDeclarationSet *
   GetAbbreviationDeclarationSet(dw_offset_t cu_abbr_offset) const;
   void Dump(lldb_private::Stream *s) const;
-  void Parse(const lldb_private::DWARFDataExtractor &data);
+  llvm::Error parse(const lldb_private::DWARFDataExtractor &data);
   void GetUnsupportedForms(std::set<dw_form_t> &invalid_forms) const;
 
 protected:
   DWARFAbbreviationDeclarationCollMap m_abbrevCollMap;
   mutable DWARFAbbreviationDeclarationCollMapConstIter m_prev_abbr_offset_pos;
 };
 
 #endif // SymbolFileDWARF_DWARFDebugAbbrev_h_

lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugAbbrev.cpp

 void DWARFAbbreviationDeclarationSet::Clear() {
   m_idx_offset = 0;
   m_decls.clear();
 }
 
 //----------------------------------------------------------------------
 // DWARFAbbreviationDeclarationSet::Extract()
 //----------------------------------------------------------------------
-bool DWARFAbbreviationDeclarationSet::Extract(const DWARFDataExtractor &data,
-                                              lldb::offset_t *offset_ptr) {
+llvm::Error
+DWARFAbbreviationDeclarationSet::extract(const DWARFDataExtractor &data,
   const lldb::offset_t begin_offset = *offset_ptr;
   m_offset = begin_offset;
   Clear();
   DWARFAbbreviationDeclaration abbrevDeclaration;
   dw_uleb128_t prev_abbr_code = 0;
-  while (abbrevDeclaration.Extract(data, offset_ptr)) {
+  DWARFEnumState state = DWARFEnumState::MoreItems;
+  while (state == DWARFEnumState::MoreItems) {
+    llvm::Expected<DWARFEnumState> es =
+        abbrevDeclaration.extract(data, offset_ptr);
+    if (!es)
+      return es.takeError();
+
+    state = *es;
     m_decls.push_back(abbrevDeclaration);
     if (m_idx_offset == 0)
       m_idx_offset = abbrevDeclaration.Code();
     else {
       if (prev_abbr_code + 1 != abbrevDeclaration.Code())
         m_idx_offset =
             UINT32_MAX; // Out of order indexes, we can't do O(1) lookups...
     }
     prev_abbr_code = abbrevDeclaration.Code();
   }
-  return begin_offset != *offset_ptr;
+  return llvm::ErrorSuccess();
 }
 
 //----------------------------------------------------------------------
 // DWARFAbbreviationDeclarationSet::Dump()
 //----------------------------------------------------------------------
 void DWARFAbbreviationDeclarationSet::Dump(Stream *s) const {
   std::for_each(
       m_decls.begin(), m_decls.end(),

 // DWARFDebugAbbrev constructor
 //----------------------------------------------------------------------
 DWARFDebugAbbrev::DWARFDebugAbbrev()
     : m_abbrevCollMap(), m_prev_abbr_offset_pos(m_abbrevCollMap.end()) {}
 
 //----------------------------------------------------------------------
 // DWARFDebugAbbrev::Parse()
 //----------------------------------------------------------------------
-void DWARFDebugAbbrev::Parse(const DWARFDataExtractor &data) {
+llvm::Error DWARFDebugAbbrev::parse(const DWARFDataExtractor &data) {
   lldb::offset_t offset = 0;
 
   while (data.ValidOffset(offset)) {
     uint32_t initial_cu_offset = offset;
     DWARFAbbreviationDeclarationSet abbrevDeclSet;
 
-    if (abbrevDeclSet.Extract(data, &offset))
-      m_abbrevCollMap[initial_cu_offset] = abbrevDeclSet;
-    else
-      break;
+    llvm::Error error = abbrevDeclSet.extract(data, &offset);
+    if (error)
+      return error;
+
   }
   m_prev_abbr_offset_pos = m_abbrevCollMap.end();
+  return llvm::ErrorSuccess();
 }
 
 //----------------------------------------------------------------------
 // DWARFDebugAbbrev::Dump()
 //----------------------------------------------------------------------
 void DWARFDebugAbbrev::Dump(Stream *s) const {
   if (m_abbrevCollMap.empty()) {
     s->PutCString("< EMPTY >\n");

lldb/source/Plugins/SymbolFile/DWARF/DWARFDefines.h

 //===-- DWARFDefines.h ------------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef SymbolFileDWARF_DWARFDefines_h_
 #define SymbolFileDWARF_DWARFDefines_h_
 
 #include "lldb/Core/dwarf.h"
 #include <stdint.h>
 
 namespace lldb_private {
 
+enum class DWARFEnumState { MoreItems, Complete };
+
 typedef uint32_t DRC_class; // Holds DRC_* class bitfields
 
 enum DW_TAG_Category {
   TagCategoryVariable,
   TagCategoryType,
   TagCategoryProgram,
   kNumTagCategories
 };

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp

 }
 
 const DWARFDataExtractor &SymbolFileDWARF::get_gnu_debugaltlink() {
   return GetCachedSectionData(eSectionTypeDWARFGNUDebugAltLink,
                               m_data_gnu_debugaltlink);
 }
 
 DWARFDebugAbbrev *SymbolFileDWARF::DebugAbbrev() {
-  if (m_abbr == NULL) {
-    const DWARFDataExtractor &debug_abbrev_data = get_debug_abbrev_data();
-    if (debug_abbrev_data.GetByteSize() > 0) {
-      m_abbr.reset(new DWARFDebugAbbrev());
-      if (m_abbr)
-        m_abbr->Parse(debug_abbrev_data);
-    }
+    return m_abbr.get();
+
+  const DWARFDataExtractor &debug_abbrev_data = get_debug_abbrev_data();
+  if (debug_abbrev_data.GetByteSize() == 0)
+    return nullptr;
+
+  auto abbr = llvm::make_unique<DWARFDebugAbbrev>();
+  llvm::Error error = abbr->parse(debug_abbrev_data);
+  if (error) {
+    Log *log = LogChannelDWARF::GetLogIfAll(DWARF_LOG_DEBUG_INFO);
+    LLDB_LOG_ERROR(log, std::move(error),
+                   "Unable to read .debug_abbrev section: {0}");
+    return nullptr;
   }
+
+  m_abbr = std::move(abbr);
   return m_abbr.get();
 }
 
 const DWARFDebugAbbrev *SymbolFileDWARF::DebugAbbrev() const {
   return m_abbr.get();
 }
 
 DWARFDebugInfo *SymbolFileDWARF::DebugInfo() {