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)); +} _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
