[lldb] Support reading DW_OP_piece from file address (#94026)

We received a bug report where someone was trying to print a global
variable without a process. This would succeed in a debug build but fail
in a on optimized build. We traced the issue back to the location being
described by a DW_OP_addr + DW_OP_piece.

The issue is that the DWARF expression evaluator only support reading
pieces from a load address. There's no reason it cannot do the same for
a file address, and indeed, that solves the problem.

I unsuccessfully tried to craft a test case to illustrate the original
example, using a global struct and trying to trick the compiler into
breaking it apart with SROA. Instead I wrote a unit test that uses a
mock target to read memory from.

rdar://127435923

3 files changed, 97 insertions, 29 deletions

diff --git a/lldb/include/lldb/Target/Target.h b/lldb/include/lldb/Target/Target.h
index 7ad9f33..5d5ae1b 100644
--- a/lldb/include/lldb/Target/Target.h
+++ b/lldb/include/lldb/Target/Target.h
@@ -1077,9 +1077,11 @@ public:
   // section, then read from the file cache
   // 2 - if there is a process, then read from memory
   // 3 - if there is no process, then read from the file cache
-  size_t ReadMemory(const Address &addr, void *dst, size_t dst_len,
-                    Status &error, bool force_live_memory = false,
-                    lldb::addr_t *load_addr_ptr = nullptr);
+  //
+  // The method is virtual for mocking in the unit tests.
+  virtual size_t ReadMemory(const Address &addr, void *dst, size_t dst_len,
+                            Status &error, bool force_live_memory = false,
+                            lldb::addr_t *load_addr_ptr = nullptr);
 
   size_t ReadCStringFromMemory(const Address &addr, std::string &out_str,
                                Status &error, bool force_live_memory = false);
@@ -1615,7 +1617,7 @@ public:
 
   TargetStats &GetStatistics() { return m_stats; }
 
-private:
+protected:
   /// Construct with optional file and arch.
   ///
   /// This member is private. Clients must use
diff --git a/lldb/source/Expression/DWARFExpression.cpp b/lldb/source/Expression/DWARFExpression.cpp
index c061fd1..7473bb8 100644
--- a/lldb/source/Expression/DWARFExpression.cpp
+++ b/lldb/source/Expression/DWARFExpression.cpp
@@ -2123,23 +2123,29 @@ bool DWARFExpression::Evaluate(
 
           const Value::ValueType curr_piece_source_value_type =
               curr_piece_source_value.GetValueType();
+          Scalar &scalar = curr_piece_source_value.GetScalar();
+          const lldb::addr_t addr = scalar.ULongLong(LLDB_INVALID_ADDRESS);
           switch (curr_piece_source_value_type) {
           case Value::ValueType::Invalid:
             return false;
           case Value::ValueType::LoadAddress:
-            if (process) {
+          case Value::ValueType::FileAddress: {
+            if (target) {
               if (curr_piece.ResizeData(piece_byte_size) == piece_byte_size) {
-                lldb::addr_t load_addr =
-                    curr_piece_source_value.GetScalar().ULongLong(
-                        LLDB_INVALID_ADDRESS);
-                if (process->ReadMemory(
-                        load_addr, curr_piece.GetBuffer().GetBytes(),
-                        piece_byte_size, error) != piece_byte_size) {
-                  if (error_ptr)
+                if (target->ReadMemory(addr, curr_piece.GetBuffer().GetBytes(),
+                                       piece_byte_size, error,
+                                       /*force_live_memory=*/false) !=
+                    piece_byte_size) {
+                  if (error_ptr) {
+                    const char *addr_type = (curr_piece_source_value_type ==
+                                             Value::ValueType::LoadAddress)
+                                                ? "load"
+                                                : "file";
                     error_ptr->SetErrorStringWithFormat(
                         "failed to read memory DW_OP_piece(%" PRIu64
-                        ") from 0x%" PRIx64,
-                        piece_byte_size, load_addr);
+                        ") from %s address 0x%" PRIx64,
+                        piece_byte_size, addr_type, addr);
+                  }
                   return false;
                 }
               } else {
@@ -2151,28 +2157,19 @@ bool DWARFExpression::Evaluate(
                 return false;
               }
             }
-            break;
-
-          case Value::ValueType::FileAddress:
-          case Value::ValueType::HostAddress:
-            if (error_ptr) {
-              lldb::addr_t addr = curr_piece_source_value.GetScalar().ULongLong(
-                  LLDB_INVALID_ADDRESS);
+          } break;
+          case Value::ValueType::HostAddress: {
+            if (error_ptr)
               error_ptr->SetErrorStringWithFormat(
                   "failed to read memory DW_OP_piece(%" PRIu64
-                  ") from %s address 0x%" PRIx64,
-                  piece_byte_size, curr_piece_source_value.GetValueType() ==
-                                           Value::ValueType::FileAddress
-                                       ? "file"
-                                       : "host",
-                  addr);
-            }
+                  ") from host address 0x%" PRIx64,
+                  piece_byte_size, addr);
             return false;
+          } break;
 
           case Value::ValueType::Scalar: {
             uint32_t bit_size = piece_byte_size * 8;
             uint32_t bit_offset = 0;
-            Scalar &scalar = curr_piece_source_value.GetScalar();
             if (!scalar.ExtractBitfield(
                     bit_size, bit_offset)) {
               if (error_ptr)
diff --git a/lldb/unittests/Expression/DWARFExpressionTest.cpp b/lldb/unittests/Expression/DWARFExpressionTest.cpp
index 8d77d6b..602bd19 100644
--- a/lldb/unittests/Expression/DWARFExpressionTest.cpp
+++ b/lldb/unittests/Expression/DWARFExpressionTest.cpp
@@ -95,6 +95,34 @@ public:
   }
 };
 
+// NB: This class doesn't use the override keyword to avoid
+// -Winconsistent-missing-override warnings from the compiler. The
+// inconsistency comes from the overriding definitions in the MOCK_*** macros.
+class MockTarget : public Target {
+public:
+  MockTarget(Debugger &debugger, const ArchSpec &target_arch,
+             const lldb::PlatformSP &platform_sp)
+      : Target(debugger, target_arch, platform_sp, true) {}
+
+  MOCK_METHOD2(ReadMemory,
+               llvm::Expected<std::vector<uint8_t>>(lldb::addr_t addr,
+                                                    size_t size));
+
+  size_t ReadMemory(const Address &addr, void *dst, size_t dst_len,
+                    Status &error, bool force_live_memory = false,
+                    lldb::addr_t *load_addr_ptr = nullptr) /*override*/ {
+    auto expected_memory = this->ReadMemory(addr.GetOffset(), dst_len);
+    if (!expected_memory) {
+      llvm::consumeError(expected_memory.takeError());
+      return 0;
+    }
+    const size_t bytes_read = expected_memory->size();
+    assert(bytes_read <= dst_len);
+    std::memcpy(dst, expected_memory->data(), bytes_read);
+    return bytes_read;
+  }
+};
+
 TEST(DWARFExpression, DW_OP_pick) {
   EXPECT_THAT_EXPECTED(Evaluate({DW_OP_lit1, DW_OP_lit0, DW_OP_pick, 0}),
                        llvm::HasValue(0));
@@ -768,3 +796,44 @@ Sections:
 
   testExpressionVendorExtensions(dwo_module_sp, *dwo_dwarf_unit);
 }
+
+TEST_F(DWARFExpressionMockProcessTest, DW_OP_piece_file_addr) {
+  using ::testing::ByMove;
+  using ::testing::ElementsAre;
+  using ::testing::Return;
+
+  // Set up a mock process.
+  ArchSpec arch("i386-pc-linux");
+  Platform::SetHostPlatform(
+      platform_linux::PlatformLinux::CreateInstance(true, &arch));
+  lldb::DebuggerSP debugger_sp = Debugger::CreateInstance();
+  ASSERT_TRUE(debugger_sp);
+  lldb::PlatformSP platform_sp;
+  auto target_sp =
+      std::make_shared<MockTarget>(*debugger_sp, arch, platform_sp);
+  ASSERT_TRUE(target_sp);
+  ASSERT_TRUE(target_sp->GetArchitecture().IsValid());
+
+  EXPECT_CALL(*target_sp, ReadMemory(0x40, 1))
+      .WillOnce(Return(ByMove(std::vector<uint8_t>{0x11})));
+  EXPECT_CALL(*target_sp, ReadMemory(0x50, 1))
+      .WillOnce(Return(ByMove(std::vector<uint8_t>{0x22})));
+
+  ExecutionContext exe_ctx(static_cast<lldb::TargetSP>(target_sp), false);
+
+  uint8_t expr[] = {DW_OP_addr, 0x40, 0x0, 0x0, 0x0, DW_OP_piece, 1,
+                    DW_OP_addr, 0x50, 0x0, 0x0, 0x0, DW_OP_piece, 1};
+  DataExtractor extractor(expr, sizeof(expr), lldb::eByteOrderLittle,
+                          /*addr_size*/ 4);
+  Value result;
+  Status status;
+  ASSERT_TRUE(DWARFExpression::Evaluate(
+      &exe_ctx, /*reg_ctx*/ nullptr, /*module_sp*/ {}, extractor,
+      /*unit*/ nullptr, lldb::eRegisterKindLLDB,
+      /*initial_value_ptr*/ nullptr,
+      /*object_address_ptr*/ nullptr, result, &status))
+      << status.ToError();
+
+  ASSERT_EQ(result.GetValueType(), Value::ValueType::HostAddress);
+  ASSERT_THAT(result.GetBuffer().GetData(), ElementsAre(0x11, 0x22));
+}