[Lldb-commits] [lldb] 7dc84e2 - [lldb] Support reading DW_OP_piece from file address (#94026)
via lldb-commits
lldb-commits at lists.llvm.org
Tue Jun 4 10:25:04 PDT 2024
Author: Jonas Devlieghere
Date: 2024-06-04T10:24:59-07:00
New Revision: 7dc84e225e11e37925db6f4f08269f447d2f2347
URL: https://github.com/llvm/llvm-project/commit/7dc84e225e11e37925db6f4f08269f447d2f2347
DIFF: https://github.com/llvm/llvm-project/commit/7dc84e225e11e37925db6f4f08269f447d2f2347.diff
LOG: [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
Added:
Modified:
lldb/include/lldb/Target/Target.h
lldb/source/Expression/DWARFExpression.cpp
lldb/unittests/Expression/DWARFExpressionTest.cpp
Removed:
################################################################################
diff --git a/lldb/include/lldb/Target/Target.h b/lldb/include/lldb/Target/Target.h
index 7ad9f33586054..5d5ae1bfcd3bd 100644
--- a/lldb/include/lldb/Target/Target.h
+++ b/lldb/include/lldb/Target/Target.h
@@ -1077,9 +1077,11 @@ 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);
+ //
+ // 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 @@ 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..7473bb8255d0a 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 8d77d6b2585f1..602bd19ceecf8 100644
--- a/lldb/unittests/Expression/DWARFExpressionTest.cpp
+++ b/lldb/unittests/Expression/DWARFExpressionTest.cpp
@@ -95,6 +95,34 @@ 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_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 @@ TEST(DWARFExpression, ExtensionsDWO) {
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));
+}
More information about the lldb-commits
mailing list