Teach DWARFExpression about DWARF 4+ Location Descriptions

[aprantl]

Summary

DWARFExpression implements the DWARF2 expression model that left ambiguity on whether the result of an expression was a value or an address. This patch implements the DWARF location description model introduces in DWARF 4 and sets the result Value's kind accordingly, if the expression comes from a DWARF v4+ compile unit. The nomenclature is taken from DWARF 5, chapter 2.6 "Location Descriptions".

[dblaikie]

(idle question: Is this code remotely related to any code in LLVM's libDebugInfoDWARF? Does this patch take us closer to or further away from unifying them? If it takes us further away, any chance of designing it in a direction that points towards each other rather than away from?)

[jasonmolenda]

This fits with my reading of the DWARF5 doc; LGTM.

[labath]

Are you sure that this logic is correct in presence of DW_OP(_bit)_piece? If I follow this right, then the final value type will be determined by the last operand. That sounds like it could be right for regular dwarf expressions, but I'm not sure about those with pieces. The classification function will mark those as "memory", and that doesn't sound right...

lldb/unittests/Expression/DWARFExpressionTest.cpp

360–361

This comment doesn't seem right.

[JDevlieghere]

lldb/source/Expression/DWARFExpression.cpp

1000

That also makes it more explicit that this is updating the dwarf4_location_description_kind

1029

Could you return the kind and do LocationDescriptionKind dwarf4_location_description_kind = classifyLocationDescription? That way you can make it a static instead of a lambda in a function that's already a hundreds of lines long.

2655

lldb_assert?

[aprantl]

@dblaikie wrote:

(idle question: Is this code remotely related to any code in LLVM's libDebugInfoDWARF? Does this patch take us closer to or further away from unifying them? If it takes us further away, any chance of designing it in a direction that points towards each other rather than away from?)

There's definitely an opportunity to refactor the main loop of Evaluate() to use llvm::DWARFExpression::iterator. I would say The impact of this patch towards that goal is neutral. The iterator (and all of libDebugInfo) doesn't care about the semantics of DWARF expressions.

lldb/source/Expression/DWARFExpression.cpp

2634

@labath wrote:

Are you sure that this logic is correct in presence of DW_OP(_bit)_piece? If I follow this right, then the final value type will be determined by the last operand. That sounds like it could be right for regular dwarf expressions, but I'm not sure about those with pieces. The classification function will mark those as "memory", and that doesn't sound right...

DW_OP_pieces are handled in this block.

For your immediate question, this means that this patch is safe. However, it also highlights that this patch isn't fixing the same bug in a composite location description. I'll address this in my next revision of the patch.

[aprantl]

Updated the patch to compute the classification on the fly and take composition operators into account.

[JDevlieghere]

LGTM.

lldb/source/Expression/DWARFExpression.cpp

 //===-- DWARFExpression.cpp -----------------------------------------------===//
 //
 // 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 "lldb/Expression/DWARFExpression.h"
 
 #include <inttypes.h>
 
 #include <vector>
 
 #include "lldb/Core/Module.h"
 #include "lldb/Core/Value.h"
 #include "lldb/Core/dwarf.h"
bool DWARFExpression::Evaluate(ExecutionContext *exe_ctx,
   }
 
   // Not a location list, just a single expression.
   return DWARFExpression::Evaluate(exe_ctx, reg_ctx, module_sp, m_data,
                                    m_dwarf_cu, m_reg_kind, initial_value_ptr,
                                    object_address_ptr, result, error_ptr);
 }
 
+namespace {
+enum LocationDescriptionKind { Empty, Memory, Register, Implicit /*, Composite*/ };
+/// Adjust value's ValueType according to the kind of location description.
+void UpdateValueTypeFromLocationDescription(Log *log, const DWARFUnit *dwarf_cu,
+                                            LocationDescriptionKind kind,
+                                            Value *value = nullptr) {
+  // Note that this function is conflating DWARF expressions with
+  // DWARF location descriptions. Perhaps it would be better to define
+  // a wrapper for DWARFExpresssion::Eval() that deals with DWARF
+  // location descriptions (which consist of one or more DWARF
+  // expressions). But doing this would mean we'd also need factor the
+  // handling of DW_OP_(bit_)piece out of this function.
+  if (dwarf_cu && dwarf_cu->GetVersion() >= 4) {
+    const char *log_msg = "DWARF location description kind: %s";
+    switch (kind) {
+    case Empty:
+      LLDB_LOGF(log, log_msg, "Empty");
+      break;
+    case Memory:
+      LLDB_LOGF(log, log_msg, "Memory");
+      if (value->GetValueType() == Value::ValueType::Scalar)
+        value->SetValueType(Value::ValueType::LoadAddress);
+      break;
+    case Register:
+      LLDB_LOGF(log, log_msg, "Register");
+      value->SetValueType(Value::ValueType::Scalar);
+      break;
+    case Implicit:
+      LLDB_LOGF(log, log_msg, "Implicit");
+      if (value->GetValueType() == Value::ValueType::LoadAddress)
+        value->SetValueType(Value::ValueType::Scalar);
+      break;
+    }
+  }
+}
+}
+
 bool DWARFExpression::Evaluate(
     ExecutionContext *exe_ctx, RegisterContext *reg_ctx,
     lldb::ModuleSP module_sp, const DataExtractor &opcodes,
     const DWARFUnit *dwarf_cu, const lldb::RegisterKind reg_kind,
     const Value *initial_value_ptr, const Value *object_address_ptr,
     Value &result, Status *error_ptr) {
 
   if (opcodes.GetByteSize() == 0) {
bool DWARFExpression::Evaluate(
   // there.
   auto to_generic = [&](auto v) {
     bool is_signed = std::is_signed<decltype(v)>::value;
     return Scalar(llvm::APSInt(
         llvm::APInt(8 * opcodes.GetAddressByteSize(), v, is_signed),
         !is_signed));
   };
 
+  // The default kind is a memory location. This is updated by any
+  // operation that changes this, such as DW_OP_stack_value, and reset
+  // by composition operations like DW_OP_piece.
+  LocationDescriptionKind dwarf4_location_description_kind = Memory;
+
   while (opcodes.ValidOffset(offset)) {
     const lldb::offset_t op_offset = offset;
     const uint8_t op = opcodes.GetU8(&offset);
 
     if (log && log->GetVerbose()) {
       size_t count = stack.size();
       LLDB_LOGF(log, "Stack before operation has %" PRIu64 " values:",
                 (uint64_t)count);
       for (size_t i = 0; i < count; ++i) {
         StreamString new_value;
         new_value.Printf("[%" PRIu64 "]", (uint64_t)i);
         stack[i].Dump(&new_value);
case DW_OP_addr:
       // Convert the file address to a load address, so subsequent
       // DWARF operators can operate on it.
       if (frame)
         stack.back().ConvertToLoadAddress(module_sp.get(),
                                           frame->CalculateTarget().get());
       break;
 
     // The DW_OP_addr_sect_offset4 is used for any location expressions in
     // shared libraries that have a location like:
     //  DW_OP_addr(0x1000)
     // If this address resides in a shared library, then this virtual address
     // won't make sense when it is evaluated in the context of a running
     // process where shared libraries have been slid. To account for this, this
     // new address type where we can store the section pointer and a 4 byte
     // offset.
     //      case DW_OP_addr_sect_offset4:
     //          {
while (opcodes.ValidOffset(offset)) {
     case DW_OP_reg24:
     case DW_OP_reg25:
     case DW_OP_reg26:
     case DW_OP_reg27:
     case DW_OP_reg28:
     case DW_OP_reg29:
     case DW_OP_reg30:
     case DW_OP_reg31: {
+      dwarf4_location_description_kind = Register;
       reg_num = op - DW_OP_reg0;
 
       if (ReadRegisterValueAsScalar(reg_ctx, reg_kind, reg_num, error_ptr, tmp))
         stack.push_back(tmp);
       else
         return false;
     } break;
     // OPCODE: DW_OP_regx
     // OPERANDS:
     //      ULEB128 literal operand that encodes the register.
     // DESCRIPTION: Push the value in register on the top of the stack.
     case DW_OP_regx: {
+      dwarf4_location_description_kind = Register;
       reg_num = opcodes.GetULEB128(&offset);
       if (ReadRegisterValueAsScalar(reg_ctx, reg_kind, reg_num, error_ptr, tmp))
         stack.push_back(tmp);
       else
         return false;
     } break;
 
     // OPCODE: DW_OP_bregN

     // the entire register, the placement of the piece within that register is
     // defined by the ABI.
     //
     // Many compilers store a single variable in sets of registers, or store a
     // variable partially in memory and partially in registers. DW_OP_piece
     // provides a way of describing how large a part of a variable a particular
     // DWARF expression refers to.
     case DW_OP_piece: {
+      LocationDescriptionKind piece_locdesc = dwarf4_location_description_kind;
+      // Reset for the next piece.
+      dwarf4_location_description_kind = Memory;
+
       const uint64_t piece_byte_size = opcodes.GetULEB128(&offset);
 
       if (piece_byte_size > 0) {
         Value curr_piece;
 
         if (stack.empty()) {
+          UpdateValueTypeFromLocationDescription(
+              log, dwarf_cu, LocationDescriptionKind::Empty);
           // In a multi-piece expression, this means that the current piece is
           // not available. Fill with zeros for now by resizing the data and
           // appending it
           curr_piece.ResizeData(piece_byte_size);
           // Note that "0" is not a correct value for the unknown bits.
           // It would be better to also return a mask of valid bits together
           // with the expression result, so the debugger can print missing
           // members as "<optimized out>" or something.
           ::memset(curr_piece.GetBuffer().GetBytes(), 0, piece_byte_size);
           pieces.AppendDataToHostBuffer(curr_piece);
         } else {
           Status error;
           // Extract the current piece into "curr_piece"
           Value curr_piece_source_value(stack.back());
           stack.pop_back();
+          UpdateValueTypeFromLocationDescription(log, dwarf_cu, piece_locdesc,
+                                                 &curr_piece_source_value);
 
           const Value::ValueType curr_piece_source_value_type =
               curr_piece_source_value.GetValueType();
           switch (curr_piece_source_value_type) {
           case Value::ValueType::Invalid:
             return false;
           case Value::ValueType::LoadAddress:
             if (process) {
case DW_OP_piece: {
           }
         }
         op_piece_offset += piece_byte_size;
       }
     } break;
 
     case DW_OP_bit_piece: // 0x9d ULEB128 bit size, ULEB128 bit offset (DWARF3);
       if (stack.size() < 1) {
+        UpdateValueTypeFromLocationDescription(log, dwarf_cu,
+                                               LocationDescriptionKind::Empty);
+        // Reset for the next piece.
+        dwarf4_location_description_kind = Memory;
         if (error_ptr)
           error_ptr->SetErrorString(
               "Expression stack needs at least 1 item for DW_OP_bit_piece.");
         return false;
       } else {
+        UpdateValueTypeFromLocationDescription(
+            log, dwarf_cu, dwarf4_location_description_kind, &stack.back());
+        // Reset for the next piece.
+        dwarf4_location_description_kind = Memory;
         const uint64_t piece_bit_size = opcodes.GetULEB128(&offset);
         const uint64_t piece_bit_offset = opcodes.GetULEB128(&offset);
         switch (stack.back().GetValueType()) {
         case Value::ValueType::Invalid:
           return false;
         case Value::ValueType::Scalar: {
           if (!stack.back().GetScalar().ExtractBitfield(piece_bit_size,
                                                         piece_bit_offset)) {

     // OPCODE: DW_OP_implicit_value
     // OPERANDS: 2
     //      ULEB128  size of the value block in bytes
     //      uint8_t* block bytes encoding value in target's memory
     //      representation
     // DESCRIPTION: Value is immediately stored in block in the debug info with
     // the memory representation of the target.
     case DW_OP_implicit_value: {
+      dwarf4_location_description_kind = Implicit;
+
       const uint32_t len = opcodes.GetULEB128(&offset);
       const void *data = opcodes.GetData(&offset, len);
 
       if (!data) {
         LLDB_LOG(log, "Evaluate_DW_OP_implicit_value: could not be read data");
         LLDB_ERRORF(error_ptr, "Could not evaluate %s.",
                     DW_OP_value_to_name(op));
         return false;
       }
 
       Value result(data, len);
       stack.push_back(result);
       break;
     }
 
+    case DW_OP_implicit_pointer: {
+      dwarf4_location_description_kind = Implicit;
+      LLDB_ERRORF(error_ptr, "Could not evaluate %s.", DW_OP_value_to_name(op));
+      return false;
+    }
+
     // OPCODE: DW_OP_push_object_address
     // OPERANDS: none
     // DESCRIPTION: Pushes the address of the object currently being
     // evaluated as part of evaluation of a user presented expression. This
     // object may correspond to an independent variable described by its own
     // DIE or it may be a component of an array, structure, or class whose
     // address has been dynamically determined by an earlier step during user
     // expression evaluation.
case DW_OP_call4:
       return false;
 
     // OPCODE: DW_OP_stack_value
     // OPERANDS: None
     // DESCRIPTION: Specifies that the object does not exist in memory but
     // rather is a constant value.  The value from the top of the stack is the
     // value to be used.  This is the actual object value and not the location.
     case DW_OP_stack_value:
+      dwarf4_location_description_kind = Implicit;
       if (stack.empty()) {
         if (error_ptr)
           error_ptr->SetErrorString(
               "Expression stack needs at least 1 item for DW_OP_stack_value.");
         return false;
       }
       stack.back().SetValueType(Value::ValueType::Scalar);
       break;

     default:
       if (error_ptr)
         error_ptr->SetErrorStringWithFormatv(
             "Unhandled opcode {0} in DWARFExpression", LocationAtom(op));
       return false;
     }
   }
 
   if (stack.empty()) {
     // Nothing on the stack, check if we created a piece value from DW_OP_piece
     // or DW_OP_bit_piece opcodes
     if (pieces.GetBuffer().GetByteSize()) {
       result = pieces;
-    } else {
-      if (error_ptr)
-        error_ptr->SetErrorString("Stack empty after evaluation.");
-      return false;
-    }
-  } else {
-    if (log && log->GetVerbose()) {
-      size_t count = stack.size();
-      LLDB_LOGF(log, "Stack after operation has %" PRIu64 " values:",
-                (uint64_t)count);
-      for (size_t i = 0; i < count; ++i) {
-        StreamString new_value;
-        new_value.Printf("[%" PRIu64 "]", (uint64_t)i);
-        stack[i].Dump(&new_value);
-        LLDB_LOGF(log, "  %s", new_value.GetData());
-      }
-    }
-    result = stack.back();
-  }
+      return true;
+    }
+
+  UpdateValueTypeFromLocationDescription(
+      log, dwarf_cu, dwarf4_location_description_kind, &stack.back());
+
+    LLDB_LOGF(log,
+              "Stack after operation has %" PRIu64 " values:", (uint64_t)count);
   return true; // Return true on success
 }
 
 static DataExtractor ToDataExtractor(const llvm::DWARFLocationExpression &loc,
                                      ByteOrder byte_order, uint32_t addr_size) {
   auto buffer_sp =
       std::make_shared<DataBufferHeap>(loc.Expr.data(), loc.Expr.size());
   return DataExtractor(buffer_sp, byte_order, addr_size);

lldb/unittests/Expression/DWARFExpressionTest.cpp

 //===-- DWARFExpressionTest.cpp -------------------------------------------===//
 //
 // 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 "lldb/Expression/DWARFExpression.h"
 #include "Plugins/Platform/Linux/PlatformLinux.h"
 #include "Plugins/TypeSystem/Clang/TypeSystemClang.h"
 #include "TestingSupport/Symbol/YAMLModuleTester.h"
 #include "lldb/Core/Value.h"
 #include "lldb/Core/Debugger.h"
 #include "lldb/Core/dwarf.h"
 #include "lldb/Host/HostInfo.h"
 #include "lldb/Symbol/ObjectFile.h"
)";
   bool is_signed = true;
   bool not_signed = false;
 
   //
   // Positive tests.
   //
 
   // Leave as is.
-  EXPECT_THAT_EXPECTED(t.Eval({DW_OP_const4u, 0x11, 0x22, 0x33, 0x44, //
-                               DW_OP_convert, offs_uint32_t}),
-                       llvm::HasValue(GetScalar(64, 0x44332211, not_signed)));
+  EXPECT_THAT_EXPECTED(
+      t.Eval({DW_OP_const4u, 0x11, 0x22, 0x33, 0x44, //
+              DW_OP_convert, offs_uint32_t, DW_OP_stack_value}),
 
   // Zero-extend to 64 bits.
-  EXPECT_THAT_EXPECTED(t.Eval({DW_OP_const4u, 0x11, 0x22, 0x33, 0x44, //
-                               DW_OP_convert, offs_uint64_t}),
-                       llvm::HasValue(GetScalar(64, 0x44332211, not_signed)));
+  EXPECT_THAT_EXPECTED(
+      t.Eval({DW_OP_const4u, 0x11, 0x22, 0x33, 0x44, //
+              DW_OP_convert, offs_uint64_t, DW_OP_stack_value}),
 
   // Sign-extend to 64 bits.
   EXPECT_THAT_EXPECTED(
       t.Eval({DW_OP_const4s, 0xcc, 0xdd, 0xee, 0xff, //
-              DW_OP_convert, offs_sint64_t}),
+              DW_OP_convert, offs_sint64_t, DW_OP_stack_value}),
       llvm::HasValue(GetScalar(64, 0xffffffffffeeddcc, is_signed)));
 
   // Sign-extend, then truncate.
-  EXPECT_THAT_EXPECTED(t.Eval({DW_OP_const4s, 0xcc, 0xdd, 0xee, 0xff, //
-                               DW_OP_convert, offs_sint64_t,          //
-                               DW_OP_convert, offs_uint32_t}),
-                       llvm::HasValue(GetScalar(32, 0xffeeddcc, not_signed)));
+  EXPECT_THAT_EXPECTED(
+      t.Eval({DW_OP_const4s, 0xcc, 0xdd, 0xee, 0xff, //
+              DW_OP_convert, offs_uint32_t, DW_OP_stack_value}),
 
   // Truncate to default unspecified (pointer-sized) type.
   EXPECT_THAT_EXPECTED(t.Eval({DW_OP_const4s, 0xcc, 0xdd, 0xee, 0xff, //
                                DW_OP_convert, offs_sint64_t,          //
-                               DW_OP_convert, 0x00}),
+                               DW_OP_convert, 0x00, DW_OP_stack_value}),
                        llvm::HasValue(GetScalar(32, 0xffeeddcc, not_signed)));
 
   // Truncate to 8 bits.
-  EXPECT_THAT_EXPECTED(
-      t.Eval({DW_OP_const4s, 'A', 'B', 'C', 'D', DW_OP_convert, offs_uchar}),
+  EXPECT_THAT_EXPECTED(t.Eval({DW_OP_const4s, 'A', 'B', 'C', 'D', DW_OP_convert,
+                               offs_uchar, DW_OP_stack_value}),
                        llvm::HasValue(GetScalar(8, 'A', not_signed)));
 
   // Also truncate to 8 bits.
-  EXPECT_THAT_EXPECTED(
-      t.Eval({DW_OP_const4s, 'A', 'B', 'C', 'D', DW_OP_convert, offs_schar}),
+  EXPECT_THAT_EXPECTED(t.Eval({DW_OP_const4s, 'A', 'B', 'C', 'D', DW_OP_convert,
+                               offs_schar, DW_OP_stack_value}),
                        llvm::HasValue(GetScalar(8, 'A', is_signed)));
 
   //
   // Errors.
   //
 
   // No Module.
   EXPECT_THAT_ERROR(Evaluate({DW_OP_const1s, 'X', DW_OP_convert, 0x00}, nullptr,
                              t.GetDwarfUnit())
TEST_F(DWARFExpressionMockProcessTest, DW_OP_deref) {
   // Implicit location: *0x4.
   EXPECT_THAT_EXPECTED(Evaluate({DW_OP_lit4, DW_OP_deref, DW_OP_stack_value},
                                 {}, {}, &exe_ctx),
                        llvm::HasValue(GetScalar(32, 0x07060504, false)));
   // Memory location: *(*0x4).
   // Evaluate returns LLDB_INVALID_ADDRESS for all load addresses.
   EXPECT_THAT_EXPECTED(Evaluate({DW_OP_lit4, DW_OP_deref}, {}, {}, &exe_ctx),
                        llvm::HasValue(Scalar(LLDB_INVALID_ADDRESS)));
+  // Memory location: *0x4.
+  // Evaluate returns LLDB_INVALID_ADDRESS for all load addresses.
+  EXPECT_THAT_EXPECTED(Evaluate({DW_OP_lit4}, {}, {}, &exe_ctx),
+                       llvm::HasValue(Scalar(4)));
+  // Implicit location: *0x4.
+  // Evaluate returns LLDB_INVALID_ADDRESS for all load addresses.
+  EXPECT_THAT_EXPECTED(
+      Evaluate({DW_OP_lit4, DW_OP_deref, DW_OP_stack_value}, {}, {}, &exe_ctx),
+      llvm::HasValue(GetScalar(32, 0x07060504, false)));
 }