[lldb] Improve error reporting in DWARFExpression::GetLocation_DW_OP_addr

Instead of simply raising an error flag, use an llvm::Expected to
propagate a meaningful error to the caller, who can report it.

rdar://139705570

Author: JDevlieghere

lldb/include/lldb/Expression/DWARFExpression.h

@@ -16,6 +16,7 @@
 #include "lldb/Utility/Status.h"
 #include "lldb/lldb-private.h"
 #include "llvm/DebugInfo/DWARF/DWARFLocationExpression.h"
+#include "llvm/Support/Error.h"
 #include <functional>
 
 namespace lldb_private {
@@ -61,15 +62,11 @@ class DWARFExpression {
   ///     The dwarf unit this expression belongs to. Only required to resolve
   ///     DW_OP{addrx, GNU_addr_index}.
   ///
-  /// \param[out] error
-  ///     If the location stream contains unknown DW_OP opcodes or the
-  ///     data is missing, \a error will be set to \b true.
-  ///
   /// \return
   ///     The address specified by the operation, if the operation exists, or
-  ///     LLDB_INVALID_ADDRESS otherwise.
-  lldb::addr_t GetLocation_DW_OP_addr(const plugin::dwarf::DWARFUnit *dwarf_cu,
-                                      bool &error) const;
+  ///     an llvm::Error otherwise.
+  llvm::Expected<lldb::addr_t>
+  GetLocation_DW_OP_addr(const plugin::dwarf::DWARFUnit *dwarf_cu) const;
 
   bool Update_DW_OP_addr(const plugin::dwarf::DWARFUnit *dwarf_cu,
                          lldb::addr_t file_addr);

lldb/source/Expression/DWARFExpression.cpp

@@ -343,30 +343,32 @@ static lldb::offset_t GetOpcodeDataSize(const DataExtractor &data,
   }
 }
 
-lldb::addr_t DWARFExpression::GetLocation_DW_OP_addr(const DWARFUnit *dwarf_cu,
-                                                     bool &error) const {
-  error = false;
+llvm::Expected<lldb::addr_t>
+DWARFExpression::GetLocation_DW_OP_addr(const DWARFUnit *dwarf_cu) const {
   lldb::offset_t offset = 0;
   while (m_data.ValidOffset(offset)) {
     const uint8_t op = m_data.GetU8(&offset);
 
     if (op == DW_OP_addr)
       return m_data.GetAddress(&offset);
+
     if (op == DW_OP_GNU_addr_index || op == DW_OP_addrx) {
-      uint64_t index = m_data.GetULEB128(&offset);
+      const uint64_t index = m_data.GetULEB128(&offset);
       if (dwarf_cu)
         return dwarf_cu->ReadAddressFromDebugAddrSection(index);
-      error = true;
-      break;
+      return llvm::createStringError("cannot evaluate %s without a DWARF unit",
+                                     DW_OP_value_to_name(op));
     }
+
     const lldb::offset_t op_arg_size =
         GetOpcodeDataSize(m_data, offset, op, dwarf_cu);
-    if (op_arg_size == LLDB_INVALID_OFFSET) {
-      error = true;
-      break;
-    }
+    if (op_arg_size == LLDB_INVALID_OFFSET)
+      return llvm::createStringError("cannot get opcode data size for %s",
+                                     DW_OP_value_to_name(op));
+
     offset += op_arg_size;
   }
+
   return LLDB_INVALID_ADDRESS;
 }
 

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

@@ -14,6 +14,7 @@
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/FileUtilities.h"
 #include "llvm/Support/Format.h"
+#include "llvm/Support/FormatAdapters.h"
 #include "llvm/Support/Threading.h"
 
 #include "lldb/Core/Module.h"
@@ -1705,7 +1706,7 @@ SymbolFileDWARF *SymbolFileDWARF::GetDIERefSymbolFile(const DIERef &die_ref) {
       // We have a SymbolFileDWARFDebugMap, so let it find the right file
     if (SymbolFileDWARFDebugMap *debug_map = GetDebugMapSymfile())
       return debug_map->GetSymbolFileByOSOIndex(*file_index);
-    
+
     // Handle the .dwp file case correctly
     if (*file_index == DIERef::k_file_index_mask)
       return GetDwpSymbolFile().get(); // DWP case
@@ -3539,17 +3540,20 @@ VariableSP SymbolFileDWARF::ParseVariableDIE(const SymbolContext &sc,
     // Check if the location has a DW_OP_addr with any address value...
     lldb::addr_t location_DW_OP_addr = LLDB_INVALID_ADDRESS;
     if (!location_is_const_value_data) {
-      bool op_error = false;
-      const DWARFExpression* location = location_list.GetAlwaysValidExpr();
-      if (location)
-        location_DW_OP_addr =
-            location->GetLocation_DW_OP_addr(location_form.GetUnit(), op_error);
-      if (op_error) {
-        StreamString strm;
-        location->DumpLocation(&strm, eDescriptionLevelFull, nullptr);
-        GetObjectFile()->GetModule()->ReportError(
-            "{0:x16}: {1} ({2}) has an invalid location: {3}", die.GetOffset(),
-            DW_TAG_value_to_name(die.Tag()), die.Tag(), strm.GetData());
+      if (const DWARFExpression *location =
+              location_list.GetAlwaysValidExpr()) {
+        if (auto maybe_location_DW_OP_addr =
+                location->GetLocation_DW_OP_addr(location_form.GetUnit())) {
+          location_DW_OP_addr = *maybe_location_DW_OP_addr;
+        } else {
+          StreamString strm;
+          location->DumpLocation(&strm, eDescriptionLevelFull, nullptr);
+          GetObjectFile()->GetModule()->ReportError(
+              "{0:x16}: {1} ({2}) has an invalid location: {3}: {4}",
+              die.GetOffset(), DW_TAG_value_to_name(die.Tag()), die.Tag(),
+              llvm::fmt_consume(maybe_location_DW_OP_addr.takeError()),
+              strm.GetData());
+        }
       }
       if (location_DW_OP_addr != LLDB_INVALID_ADDRESS)
         is_static_lifetime = true;