[lldb] Unify DW_OP_deref and DW_OP_deref_size implementations (#169587)

This commit unifies the code in the dwarf expression evaluator that
handles these two deref operations. Previously we had similar, but not
identical code for handling them.

The implementation was taken from the DW_OP_deref_size code path since
that handles the general case. We put that code into an
Evaluate_DW_OP_deref_size function and call it with the address size
when evaluating DW_OP_deref.

There were initially two test failures when I made the change. The
`DW_op_deref_no_ptr_fixing` unittest failed because we were not
correctly setting the address size when we created the DataExtractor.
The `DW_OP_deref test` failed because previously the expression
`DW_OP_lit4, DW_OP_deref` would evaluate to a LoadAddress, but the code
for deref_size was evaluating it to a scalar.

The difference was in how it handled a deref of a scalar value type. In
the deref code path we convert a scalar to a load address, but this was
not done in the deref_size code path.

```
  case Value::ValueType::Scalar:
      stack.back().SetValueType(Value::ValueType::LoadAddress);
```

I decided to modify the deref_size code to also convert the value to a
load address to keep the test passing.

There is no functional change intended here. The motivation is to reduce
the number of code paths that implement the deref operation.

Author: dmpots

lldb/source/Expression/DWARFExpression.cpp

@@ -861,84 +861,146 @@ ResolveLoadAddress(ExecutionContext *exe_ctx, lldb::ModuleSP &module_sp,
   return load_addr;
 }
 
-static llvm::Error Evaluate_DW_OP_deref(DWARFExpression::Stack &stack,
-                                        ExecutionContext *exe_ctx,
-                                        lldb::ModuleSP module_sp,
-                                        Process *process) {
+/// @brief Helper function to load sized data from a uint8_t buffer.
+///
+/// @param addr_bytes The buffer containing raw data.
+/// @param size_addr_bytes How large is the underlying raw data.
+/// @param byte_order What is the byte order of the underlying data.
+/// @param size How much of the underlying data we want to use.
+/// @return The underlying data converted into a Scalar.
+static Scalar DerefSizeExtractDataHelper(uint8_t *addr_bytes,
+                                         size_t size_addr_bytes,
+                                         ByteOrder byte_order, size_t size) {
+  DataExtractor addr_data(addr_bytes, size_addr_bytes, byte_order, size);
+
+  lldb::offset_t addr_data_offset = 0;
+  if (size <= 8)
+    return addr_data.GetMaxU64(&addr_data_offset, size);
+  return addr_data.GetAddress(&addr_data_offset);
+}
+
+static llvm::Error Evaluate_DW_OP_deref_size(DWARFExpression::Stack &stack,
+                                             ExecutionContext *exe_ctx,
+                                             lldb::ModuleSP module_sp,
+                                             Process *process, Target *target,
+                                             uint8_t size) {
   if (stack.empty())
-    return llvm::createStringError("expression stack empty for DW_OP_deref");
+    return llvm::createStringError(
+        "expression stack empty for DW_OP_deref_size");
 
-  const Value::ValueType value_type = stack.back().GetValueType();
+  if (size > 8)
+    return llvm::createStringError(
+        "Invalid address size for DW_OP_deref_size: %d\n", size);
+
+  Value::ValueType value_type = stack.back().GetValueType();
   switch (value_type) {
   case Value::ValueType::HostAddress: {
     void *src = (void *)stack.back().GetScalar().ULongLong();
     intptr_t ptr;
     ::memcpy(&ptr, src, sizeof(void *));
+    // I can't decide whether the size operand should apply to the bytes in
+    // their lldb-host endianness or the target endianness.. I doubt this'll
+    // ever come up but I'll opt for assuming big endian regardless.
+    switch (size) {
+    case 1:
+      ptr = ptr & 0xff;
+      break;
+    case 2:
+      ptr = ptr & 0xffff;
+      break;
+    case 3:
+      ptr = ptr & 0xffffff;
+      break;
+    case 4:
+      ptr = ptr & 0xffffffff;
+      break;
+    // The casts are added to work around the case where intptr_t is a 32-bit
+    // quantity. Presumably we won't hit the 5..7 cases if (void*) is 32-bits in
+    // this program.
+    case 5:
+      ptr = (intptr_t)ptr & 0xffffffffffULL;
+      break;
+    case 6:
+      ptr = (intptr_t)ptr & 0xffffffffffffULL;
+      break;
+    case 7:
+      ptr = (intptr_t)ptr & 0xffffffffffffffULL;
+      break;
+    default:
+      break;
+    }
     stack.back().GetScalar() = ptr;
     stack.back().ClearContext();
   } break;
   case Value::ValueType::FileAddress: {
     auto file_addr = stack.back().GetScalar().ULongLong(LLDB_INVALID_ADDRESS);
     Address so_addr;
-    auto maybe_load_addr = ResolveLoadAddress(exe_ctx, module_sp, "DW_OP_deref",
-                                              file_addr, so_addr);
+    auto maybe_load_addr = ResolveLoadAddress(
+        exe_ctx, module_sp, "DW_OP_deref_size", file_addr, so_addr,
+        /*check_sectionoffset=*/true);
+
     if (!maybe_load_addr)
       return maybe_load_addr.takeError();
-    stack.back().GetScalar() = *maybe_load_addr;
+
+    addr_t load_addr = *maybe_load_addr;
+
+    if (load_addr == LLDB_INVALID_ADDRESS && so_addr.IsSectionOffset()) {
+      uint8_t addr_bytes[8];
+      Status error;
+
+      if (!target || target->ReadMemory(so_addr, &addr_bytes, size, error,
+                                        /*force_live_memory=*/false) != size)
+        return llvm::createStringError(
+            "failed to dereference pointer for DW_OP_deref_size: "
+            "%s\n",
+            error.AsCString());
+
+      ObjectFile *objfile = module_sp->GetObjectFile();
+
+      stack.back().GetScalar() = DerefSizeExtractDataHelper(
+          addr_bytes, size, objfile->GetByteOrder(), size);
+      stack.back().ClearContext();
+      break;
+    }
+    stack.back().GetScalar() = load_addr;
     // Fall through to load address promotion code below.
   }
+
     [[fallthrough]];
   case Value::ValueType::Scalar:
     // Promote Scalar to LoadAddress and fall through.
     stack.back().SetValueType(Value::ValueType::LoadAddress);
     [[fallthrough]];
   case Value::ValueType::LoadAddress: {
     if (!exe_ctx)
-      return llvm::createStringError("NULL execution context for DW_OP_deref");
+      return llvm::createStringError(
+          "no execution context for DW_OP_deref_size");
     if (!process)
-      return llvm::createStringError("NULL process for DW_OP_deref");
+      return llvm::createStringError("no process for DW_OP_deref_size");
+
     lldb::addr_t pointer_addr =
         stack.back().GetScalar().ULongLong(LLDB_INVALID_ADDRESS);
+    uint8_t addr_bytes[sizeof(lldb::addr_t)];
     Status error;
-    lldb::addr_t pointer_value =
-        process->ReadPointerFromMemory(pointer_addr, error);
-    if (pointer_value == LLDB_INVALID_ADDRESS)
-      return llvm::joinErrors(
-          llvm::createStringError(
-              "Failed to dereference pointer from 0x%" PRIx64
-              " for DW_OP_deref",
-              pointer_addr),
-          error.takeError());
-    stack.back().GetScalar() = pointer_value;
+
+    if (process->ReadMemory(pointer_addr, &addr_bytes, size, error) != size)
+      return llvm::createStringError(
+          "failed to dereference pointer from 0x%" PRIx64
+          " for DW_OP_deref_size: %s\n",
+          pointer_addr, error.AsCString());
+
+    stack.back().GetScalar() = DerefSizeExtractDataHelper(
+        addr_bytes, sizeof(addr_bytes), process->GetByteOrder(), size);
     stack.back().ClearContext();
   } break;
+
   case Value::ValueType::Invalid:
-    return llvm::createStringError("invalid value type for DW_OP_deref");
+    return llvm::createStringError("invalid value for DW_OP_deref_size");
   }
 
   return llvm::Error::success();
 }
 
-/// Helper function to move common code used to load sized data from a uint8_t
-/// buffer.
-///
-/// \param addr_bytes uint8_t buffer containg raw data
-/// \param size_addr_bytes how large is the underlying raw data
-/// \param byte_order what is the byter order of the underlyig data
-/// \param size How much of the underlying data we want to use
-/// \return The underlying data converted into a Scalar
-static Scalar DerefSizeExtractDataHelper(uint8_t *addr_bytes,
-                                         size_t size_addr_bytes,
-                                         ByteOrder byte_order, size_t size) {
-  DataExtractor addr_data(addr_bytes, size_addr_bytes, byte_order, size);
-
-  lldb::offset_t addr_data_offset = 0;
-  if (size <= 8)
-    return addr_data.GetMaxU64(&addr_data_offset, size);
-  else
-    return addr_data.GetAddress(&addr_data_offset);
-}
-
 llvm::Expected<Value> DWARFExpression::Evaluate(
     ExecutionContext *exe_ctx, RegisterContext *reg_ctx,
     lldb::ModuleSP module_sp, const DataExtractor &opcodes,
@@ -1079,8 +1141,9 @@ llvm::Expected<Value> DWARFExpression::Evaluate(
     // retrieved from the dereferenced address is the size of an address on the
     // target machine.
     case DW_OP_deref: {
-      if (llvm::Error err =
-              Evaluate_DW_OP_deref(stack, exe_ctx, module_sp, process))
+      size_t size = opcodes.GetAddressByteSize();
+      if (llvm::Error err = Evaluate_DW_OP_deref_size(stack, exe_ctx, module_sp,
+                                                      process, target, size))
         return err;
     } break;
 
@@ -1097,131 +1160,10 @@ llvm::Expected<Value> DWARFExpression::Evaluate(
     // the size of an address on the target machine before being pushed on the
     // expression stack.
     case DW_OP_deref_size: {
-      if (stack.empty()) {
-        return llvm::createStringError(
-            "expression stack empty for DW_OP_deref_size");
-      }
-      uint8_t size = opcodes.GetU8(&offset);
-      if (size > 8) {
-        return llvm::createStringError(
-            "Invalid address size for DW_OP_deref_size: %d\n", size);
-      }
-      Value::ValueType value_type = stack.back().GetValueType();
-      switch (value_type) {
-      case Value::ValueType::HostAddress: {
-        void *src = (void *)stack.back().GetScalar().ULongLong();
-        intptr_t ptr;
-        ::memcpy(&ptr, src, sizeof(void *));
-        // I can't decide whether the size operand should apply to the bytes in
-        // their
-        // lldb-host endianness or the target endianness.. I doubt this'll ever
-        // come up but I'll opt for assuming big endian regardless.
-        switch (size) {
-        case 1:
-          ptr = ptr & 0xff;
-          break;
-        case 2:
-          ptr = ptr & 0xffff;
-          break;
-        case 3:
-          ptr = ptr & 0xffffff;
-          break;
-        case 4:
-          ptr = ptr & 0xffffffff;
-          break;
-        // the casts are added to work around the case where intptr_t is a 32
-        // bit quantity;
-        // presumably we won't hit the 5..7 cases if (void*) is 32-bits in this
-        // program.
-        case 5:
-          ptr = (intptr_t)ptr & 0xffffffffffULL;
-          break;
-        case 6:
-          ptr = (intptr_t)ptr & 0xffffffffffffULL;
-          break;
-        case 7:
-          ptr = (intptr_t)ptr & 0xffffffffffffffULL;
-          break;
-        default:
-          break;
-        }
-        stack.back().GetScalar() = ptr;
-        stack.back().ClearContext();
-      } break;
-      case Value::ValueType::FileAddress: {
-        auto file_addr =
-            stack.back().GetScalar().ULongLong(LLDB_INVALID_ADDRESS);
-        Address so_addr;
-        auto maybe_load_addr = ResolveLoadAddress(
-            exe_ctx, module_sp, "DW_OP_deref_size", file_addr, so_addr,
-            /*check_sectionoffset=*/true);
-
-        if (!maybe_load_addr)
-          return maybe_load_addr.takeError();
-
-        addr_t load_addr = *maybe_load_addr;
-
-        if (load_addr == LLDB_INVALID_ADDRESS && so_addr.IsSectionOffset()) {
-          uint8_t addr_bytes[8];
-          Status error;
-
-          if (target &&
-              target->ReadMemory(so_addr, &addr_bytes, size, error,
-                                 /*force_live_memory=*/false) == size) {
-            ObjectFile *objfile = module_sp->GetObjectFile();
-
-            stack.back().GetScalar() = DerefSizeExtractDataHelper(
-                addr_bytes, size, objfile->GetByteOrder(), size);
-            stack.back().ClearContext();
-            break;
-          } else {
-            return llvm::createStringError(
-                "Failed to dereference pointer for DW_OP_deref_size: "
-                "%s\n",
-                error.AsCString());
-          }
-        }
-        stack.back().GetScalar() = load_addr;
-        // Fall through to load address promotion code below.
-      }
-
-        [[fallthrough]];
-      case Value::ValueType::Scalar:
-      case Value::ValueType::LoadAddress:
-        if (exe_ctx) {
-          if (process) {
-            lldb::addr_t pointer_addr =
-                stack.back().GetScalar().ULongLong(LLDB_INVALID_ADDRESS);
-            uint8_t addr_bytes[sizeof(lldb::addr_t)];
-            Status error;
-            if (process->ReadMemory(pointer_addr, &addr_bytes, size, error) ==
-                size) {
-
-              stack.back().GetScalar() =
-                  DerefSizeExtractDataHelper(addr_bytes, sizeof(addr_bytes),
-                                             process->GetByteOrder(), size);
-              stack.back().ClearContext();
-            } else {
-              return llvm::createStringError(
-                  "Failed to dereference pointer from 0x%" PRIx64
-                  " for DW_OP_deref: %s\n",
-                  pointer_addr, error.AsCString());
-            }
-          } else {
-
-            return llvm::createStringError("NULL process for DW_OP_deref_size");
-          }
-        } else {
-          return llvm::createStringError(
-              "NULL execution context for DW_OP_deref_size");
-        }
-        break;
-
-      case Value::ValueType::Invalid:
-
-        return llvm::createStringError("invalid value for DW_OP_deref_size");
-      }
-
+      size_t size = opcodes.GetU8(&offset);
+      if (llvm::Error err = Evaluate_DW_OP_deref_size(stack, exe_ctx, module_sp,
+                                                      process, target, size))
+        return err;
     } break;
 
     // OPCODE: DW_OP_xderef_size

lldb/unittests/Expression/DWARFExpressionTest.cpp

@@ -237,8 +237,9 @@ static llvm::Expected<Value> Evaluate(llvm::ArrayRef<uint8_t> expr,
                                       DWARFExpression::Delegate *unit = nullptr,
                                       ExecutionContext *exe_ctx = nullptr,
                                       RegisterContext *reg_ctx = nullptr) {
-  DataExtractor extractor(expr.data(), expr.size(), lldb::eByteOrderLittle,
-                          /*addr_size*/ 4);
+  DataExtractor extractor(
+      expr.data(), expr.size(), lldb::eByteOrderLittle,
+      /*addr_size*/ exe_ctx ? exe_ctx->GetAddressByteSize() : 4);
 
   return DWARFExpression::Evaluate(exe_ctx, reg_ctx, module_sp, extractor, unit,
                                    lldb::eRegisterKindLLDB,