https://github.com/JDevlieghere updated
https://github.com/llvm/llvm-project/pull/94026
>From 72844ebd5cf8f74f6db5d1c52d1f557ca942dbee Mon Sep 17 00:00:00 2001
From: Jonas Devlieghere <jo...@devlieghere.com>
Date: Fri, 31 May 2024 12:21:28 -0700
Subject: [PATCH 1/4] [lldb] Support reading DW_OP_piece from file address
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
---
lldb/include/lldb/Target/Target.h | 8 +--
lldb/source/Expression/DWARFExpression.cpp | 57 +++++++++++-------
.../Expression/DWARFExpressionTest.cpp | 60 +++++++++++++++++++
3 files changed, 100 insertions(+), 25 deletions(-)
diff --git a/lldb/include/lldb/Target/Target.h
b/lldb/include/lldb/Target/Target.h
index 7ad9f33586054..792a4caa76e2d 100644
--- a/lldb/include/lldb/Target/Target.h
+++ b/lldb/include/lldb/Target/Target.h
@@ -1077,9 +1077,9 @@ class Target : public
std::enable_shared_from_this<Target>,
// 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);
+ 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 +1615,7 @@ class Target : public
std::enable_shared_from_this<Target>,
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 c061fd1140fff..326be0d683804 100644
--- a/lldb/source/Expression/DWARFExpression.cpp
+++ b/lldb/source/Expression/DWARFExpression.cpp
@@ -2123,23 +2123,22 @@ 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) {
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 (process->ReadMemory(addr,
curr_piece.GetBuffer().GetBytes(),
+ piece_byte_size,
+ error) != piece_byte_size) {
if (error_ptr)
error_ptr->SetErrorStringWithFormat(
"failed to read memory DW_OP_piece(%" PRIu64
- ") from 0x%" PRIx64,
- piece_byte_size, load_addr);
+ ") from load address 0x%" PRIx64,
+ piece_byte_size, addr);
return false;
}
} else {
@@ -2153,26 +2152,42 @@ bool DWARFExpression::Evaluate(
}
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);
+ case Value::ValueType::FileAddress: {
+ if (target) {
+ if (curr_piece.ResizeData(piece_byte_size) == piece_byte_size) {
+ if (target->ReadMemory(addr, curr_piece.GetBuffer().GetBytes(),
+ piece_byte_size, error,
+ /*force_live_memory=*/false) !=
+ piece_byte_size) {
+ if (error_ptr)
+ error_ptr->SetErrorStringWithFormat(
+ "failed to read memory DW_OP_piece(%" PRIu64
+ ") from file address 0x%" PRIx64,
+ piece_byte_size, addr);
+ return false;
+ }
+ } else {
+ if (error_ptr)
+ error_ptr->SetErrorStringWithFormat(
+ "failed to resize the piece memory buffer for "
+ "DW_OP_piece(%" PRIu64 ")",
+ piece_byte_size);
+ return false;
+ }
+ }
+ } 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 8d77d6b2585f1..7b20c603889b2 100644
--- a/lldb/unittests/Expression/DWARFExpressionTest.cpp
+++ b/lldb/unittests/Expression/DWARFExpressionTest.cpp
@@ -768,3 +768,63 @@ TEST(DWARFExpression, ExtensionsDWO) {
testExpressionVendorExtensions(dwo_module_sp, *dwo_dwarf_unit);
}
+
+TEST_F(DWARFExpressionMockProcessTest, DW_OP_piece_file_addr) {
+ struct MockTarget : Target {
+ MockTarget(Debugger &debugger, const ArchSpec &target_arch,
+ const lldb::PlatformSP &platform_sp)
+ : Target(debugger, target_arch, platform_sp, true) {}
+
+ 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 {
+ // We expected to be called in a very specific way.
+ assert(dst_len == 1);
+ assert(addr.GetOffset() == 0x40 || addr.GetOffset() == 0x50);
+
+ if (addr.GetOffset() == 0x40)
+ ((uint8_t *)dst)[0] = 0x11;
+
+ if (addr.GetOffset() == 0x50)
+ ((uint8_t *)dst)[0] = 0x22;
+
+ return 1;
+ }
+ };
+
+ // 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;
+ lldb::TargetSP target_sp =
+ std::make_shared<MockTarget>(*debugger_sp, arch, platform_sp);
+ ASSERT_TRUE(target_sp);
+ ASSERT_TRUE(target_sp->GetArchitecture().IsValid());
+
+ ExecutionContext exe_ctx(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);
+
+ DataBufferHeap &buf = result.GetBuffer();
+ ASSERT_EQ(buf.GetByteSize(), 2U);
+
+ const uint8_t *bytes = buf.GetBytes();
+ EXPECT_EQ(bytes[0], 0x11);
+ EXPECT_EQ(bytes[1], 0x22);
+}
>From 1c2af3852f6cccf984d52faa359a5a443f3b788c Mon Sep 17 00:00:00 2001
From: Jonas Devlieghere <jo...@devlieghere.com>
Date: Mon, 3 Jun 2024 10:56:12 -0700
Subject: [PATCH 2/4] Use gMock
---
.../Expression/DWARFExpressionTest.cpp | 62 ++++++++++---------
1 file changed, 32 insertions(+), 30 deletions(-)
diff --git a/lldb/unittests/Expression/DWARFExpressionTest.cpp
b/lldb/unittests/Expression/DWARFExpressionTest.cpp
index 7b20c603889b2..c9f1138078700 100644
--- a/lldb/unittests/Expression/DWARFExpressionTest.cpp
+++ b/lldb/unittests/Expression/DWARFExpressionTest.cpp
@@ -95,6 +95,16 @@ class DWARFExpressionMockProcessTest : public
::testing::Test {
}
};
+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_METHOD6(ReadMemory, size_t(const Address &, void *, size_t, Status &,
+ bool, lldb::addr_t *));
+};
+
TEST(DWARFExpression, DW_OP_pick) {
EXPECT_THAT_EXPECTED(Evaluate({DW_OP_lit1, DW_OP_lit0, DW_OP_pick, 0}),
llvm::HasValue(0));
@@ -770,27 +780,9 @@ TEST(DWARFExpression, ExtensionsDWO) {
}
TEST_F(DWARFExpressionMockProcessTest, DW_OP_piece_file_addr) {
- struct MockTarget : Target {
- MockTarget(Debugger &debugger, const ArchSpec &target_arch,
- const lldb::PlatformSP &platform_sp)
- : Target(debugger, target_arch, platform_sp, true) {}
-
- 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 {
- // We expected to be called in a very specific way.
- assert(dst_len == 1);
- assert(addr.GetOffset() == 0x40 || addr.GetOffset() == 0x50);
-
- if (addr.GetOffset() == 0x40)
- ((uint8_t *)dst)[0] = 0x11;
-
- if (addr.GetOffset() == 0x50)
- ((uint8_t *)dst)[0] = 0x22;
-
- return 1;
- }
- };
+ using ::testing::_;
+ using ::testing::ElementsAre;
+ using ::testing::Return;
// Set up a mock process.
ArchSpec arch("i386-pc-linux");
@@ -799,12 +791,28 @@ TEST_F(DWARFExpressionMockProcessTest,
DW_OP_piece_file_addr) {
lldb::DebuggerSP debugger_sp = Debugger::CreateInstance();
ASSERT_TRUE(debugger_sp);
lldb::PlatformSP platform_sp;
- lldb::TargetSP target_sp =
+ auto target_sp =
std::make_shared<MockTarget>(*debugger_sp, arch, platform_sp);
ASSERT_TRUE(target_sp);
ASSERT_TRUE(target_sp->GetArchitecture().IsValid());
- ExecutionContext exe_ctx(target_sp, false);
+ EXPECT_CALL(*target_sp, ReadMemory(Address(0x40), _, 1, _, _, _))
+ .WillOnce([&](const Address &addr, void *dst, size_t dst_len,
+ Status &error, bool force_live_memory = false,
+ lldb::addr_t *load_addr_ptr = nullptr) {
+ ((uint8_t *)dst)[0] = 0x11;
+ return 1;
+ });
+
+ EXPECT_CALL(*target_sp, ReadMemory(Address(0x50), _, 1, _, _, _))
+ .WillOnce([&](const Address &addr, void *dst, size_t dst_len,
+ Status &error, bool force_live_memory = false,
+ lldb::addr_t *load_addr_ptr = nullptr) {
+ ((uint8_t *)dst)[0] = 0x22;
+ return 1;
+ });
+
+ 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};
@@ -820,11 +828,5 @@ TEST_F(DWARFExpressionMockProcessTest,
DW_OP_piece_file_addr) {
<< status.ToError();
ASSERT_EQ(result.GetValueType(), Value::ValueType::HostAddress);
-
- DataBufferHeap &buf = result.GetBuffer();
- ASSERT_EQ(buf.GetByteSize(), 2U);
-
- const uint8_t *bytes = buf.GetBytes();
- EXPECT_EQ(bytes[0], 0x11);
- EXPECT_EQ(bytes[1], 0x22);
+ ASSERT_THAT(result.GetBuffer().GetData(), ElementsAre(0x11, 0x22));
}
>From 16bab2d22d4367c9c24417b5664289c4176df125 Mon Sep 17 00:00:00 2001
From: Jonas Devlieghere <jo...@devlieghere.com>
Date: Mon, 3 Jun 2024 13:12:04 -0700
Subject: [PATCH 3/4] Use two-level mocking
---
.../Expression/DWARFExpressionTest.cpp | 43 +++++++++++--------
1 file changed, 25 insertions(+), 18 deletions(-)
diff --git a/lldb/unittests/Expression/DWARFExpressionTest.cpp
b/lldb/unittests/Expression/DWARFExpressionTest.cpp
index c9f1138078700..602bd19ceecf8 100644
--- a/lldb/unittests/Expression/DWARFExpressionTest.cpp
+++ b/lldb/unittests/Expression/DWARFExpressionTest.cpp
@@ -95,14 +95,32 @@ class DWARFExpressionMockProcessTest : public
::testing::Test {
}
};
+// 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_METHOD6(ReadMemory, size_t(const Address &, void *, size_t, Status &,
- bool, lldb::addr_t *));
+ 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) {
@@ -780,7 +798,7 @@ TEST(DWARFExpression, ExtensionsDWO) {
}
TEST_F(DWARFExpressionMockProcessTest, DW_OP_piece_file_addr) {
- using ::testing::_;
+ using ::testing::ByMove;
using ::testing::ElementsAre;
using ::testing::Return;
@@ -796,21 +814,10 @@ TEST_F(DWARFExpressionMockProcessTest,
DW_OP_piece_file_addr) {
ASSERT_TRUE(target_sp);
ASSERT_TRUE(target_sp->GetArchitecture().IsValid());
- EXPECT_CALL(*target_sp, ReadMemory(Address(0x40), _, 1, _, _, _))
- .WillOnce([&](const Address &addr, void *dst, size_t dst_len,
- Status &error, bool force_live_memory = false,
- lldb::addr_t *load_addr_ptr = nullptr) {
- ((uint8_t *)dst)[0] = 0x11;
- return 1;
- });
-
- EXPECT_CALL(*target_sp, ReadMemory(Address(0x50), _, 1, _, _, _))
- .WillOnce([&](const Address &addr, void *dst, size_t dst_len,
- Status &error, bool force_live_memory = false,
- lldb::addr_t *load_addr_ptr = nullptr) {
- ((uint8_t *)dst)[0] = 0x22;
- return 1;
- });
+ 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);
>From 4c9db2dc98114d39df1d115c48aa6d73613fabaf Mon Sep 17 00:00:00 2001
From: Jonas Devlieghere <jo...@devlieghere.com>
Date: Mon, 3 Jun 2024 16:20:43 -0700
Subject: [PATCH 4/4] Unify load & file addresses
---
lldb/source/Expression/DWARFExpression.cpp | 34 +++++-----------------
1 file changed, 8 insertions(+), 26 deletions(-)
diff --git a/lldb/source/Expression/DWARFExpression.cpp
b/lldb/source/Expression/DWARFExpression.cpp
index 326be0d683804..7473bb8255d0a 100644
--- a/lldb/source/Expression/DWARFExpression.cpp
+++ b/lldb/source/Expression/DWARFExpression.cpp
@@ -2129,29 +2129,6 @@ bool DWARFExpression::Evaluate(
case Value::ValueType::Invalid:
return false;
case Value::ValueType::LoadAddress:
- if (process) {
- if (curr_piece.ResizeData(piece_byte_size) == piece_byte_size) {
- if (process->ReadMemory(addr,
curr_piece.GetBuffer().GetBytes(),
- piece_byte_size,
- error) != piece_byte_size) {
- if (error_ptr)
- error_ptr->SetErrorStringWithFormat(
- "failed to read memory DW_OP_piece(%" PRIu64
- ") from load address 0x%" PRIx64,
- piece_byte_size, addr);
- return false;
- }
- } else {
- if (error_ptr)
- error_ptr->SetErrorStringWithFormat(
- "failed to resize the piece memory buffer for "
- "DW_OP_piece(%" PRIu64 ")",
- piece_byte_size);
- return false;
- }
- }
- break;
-
case Value::ValueType::FileAddress: {
if (target) {
if (curr_piece.ResizeData(piece_byte_size) == piece_byte_size) {
@@ -2159,11 +2136,16 @@ bool DWARFExpression::Evaluate(
piece_byte_size, error,
/*force_live_memory=*/false) !=
piece_byte_size) {
- if (error_ptr)
+ 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 file address 0x%" PRIx64,
- piece_byte_size, addr);
+ ") from %s address 0x%" PRIx64,
+ piece_byte_size, addr_type, addr);
+ }
return false;
}
} else {
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
