https://github.com/qiyao updated https://github.com/llvm/llvm-project/pull/213451
>From 3217ad4f857b5ab004e48559831fccda65f0c745 Mon Sep 17 00:00:00 2001 From: Yao Qi <[email protected]> Date: Fri, 31 Jul 2026 16:37:36 +0100 Subject: [PATCH 1/2] [lldb] Clear stale error in Target::ReadMemory on file-cache fallback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Load the checked-in core `linux-aarch64-pac.core` from `lldb/test/API/functionalities/postmortem/elf-core/` with its binary and ask for a `char16_t *` summary at the start of `.text`: ``` (lldb) settings set target.max-string-summary-length 8 (lldb) expression -l c++ -- (char16_t *)0x400140 (char16_t *) $0 = 0x0000000000400140 unable to read data (lldb) memory read -s1 -c16 0x400140 0x00400140: 3f 23 03 d5 ff 83 00 d1 fd 7b 01 a9 fd 43 00 91 ?#.......{...C.. ``` `memory read` prints the very bytes the summary just claimed it could not read. Both go through `Target::ReadMemory()`, which reuses a single `Status &error` across the process read and the file-cache fallback at the end of the function, and `Target::ReadMemoryFromFileCache()` only ever sets that `Status`, it never clears it on success. So when the process read fails outright and the fallback then satisfies the whole request, `ReadMemory()` returns the correct bytes with the failed process read's message still in `error`. `memory read` compares the returned count against the requested length and is fine, but `Target::ReadStringFromMemory()`, which `StringPrinter` uses for UTF-16 and UTF-32, checks `error.Success()` and gives up. `SBTarget::ReadMemory()` hands the same stale `Status` to any scripted client. Core files reach this routinely. `ProcessMachCore` and `ProcessElfCore` both report `IsAlive()`, so the process read is attempted and fails for a read-only page that was not dumped into the core, and the fallback then serves that page out of the binary on disk. In this core the `PT_LOAD` covering `.text` has `p_filesz == 0`. Clear `error` after the fallback, but only for a full read, that is `bytes_read == dst_len`. `ObjectFile::ReadSectionData()` clamps a request that overruns the section, so clearing unconditionally would report success for a truncated read and leave the tail of `dst` uninitialized. That is why the session lowers `target.max-string-summary-length`: it keeps the request inside the 148-byte `.text`, whereas a larger request really is a short read and still fails. Other exits that return a partial count without setting an error are pre-existing. With the fix: ``` (lldb) expression -l c++ -- (char16_t *)0x400140 (char16_t *) $0 = 0x0000000000400140 u"⌿픃菿턀篽꤁䏽" ``` --- lldb/source/Target/Target.cpp | 8 +- .../postmortem/elf-core/TestLinuxCore.py | 7 ++ lldb/unittests/Target/CMakeLists.txt | 1 + lldb/unittests/Target/MemoryTest.cpp | 104 ++++++++++++++++++ 4 files changed, 119 insertions(+), 1 deletion(-) diff --git a/lldb/source/Target/Target.cpp b/lldb/source/Target/Target.cpp index 148d7e0b30dbb..21aeb3ba1f446 100644 --- a/lldb/source/Target/Target.cpp +++ b/lldb/source/Target/Target.cpp @@ -2209,7 +2209,13 @@ size_t Target::ReadMemory(const Address &addr, void *dst, size_t dst_len, if (!file_cache_read_buffer && resolved_addr.IsSectionOffset()) { // If we didn't already try and read from the object file cache, then try // it after failing to read from the process. - return ReadMemoryFromFileCache(resolved_addr, dst, dst_len, error); + // ReadMemoryFromFileCache() only ever sets "error", so clear it to keep a + // failed process read from poisoning a successful read here. Only a full + // read counts: ReadSectionData() clamps, leaving the tail of "dst" unset. + bytes_read = ReadMemoryFromFileCache(resolved_addr, dst, dst_len, error); + if (bytes_read == dst_len) + error.Clear(); + return bytes_read; } return 0; } diff --git a/lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py b/lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py index f211ac4454209..2dfba0cd71da1 100644 --- a/lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py +++ b/lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py @@ -1238,6 +1238,13 @@ def test_read_only_cstring(self): cstr = var.GetSummary() self.assertEqual(cstr, '"_start"') + # Reading through the target falls back to the application binary too, + # and must not report the failed process read. + error = lldb.SBError() + addr = target.ResolveLoadAddress(var.GetValueAsUnsigned()) + self.assertEqual(target.ReadMemory(addr, 7, error), b"_start\0") + self.assertSuccess(error) + @skipIfLLVMTargetMissing("X86") @skipIfWindows def test_linux_no_exe(self): diff --git a/lldb/unittests/Target/CMakeLists.txt b/lldb/unittests/Target/CMakeLists.txt index bf08a8f015ba0..6f88a43f7ee7c 100644 --- a/lldb/unittests/Target/CMakeLists.txt +++ b/lldb/unittests/Target/CMakeLists.txt @@ -23,6 +23,7 @@ add_lldb_unittest(TargetTests lldbHost lldbPluginObjectFileBreakpad lldbPluginObjectFileELF + lldbPluginObjectFileMachO lldbPluginPlatformLinux lldbPluginPlatformMacOSX lldbPluginPlatformAndroid diff --git a/lldb/unittests/Target/MemoryTest.cpp b/lldb/unittests/Target/MemoryTest.cpp index 9d04376b4fd5b..f1d804b90c317 100644 --- a/lldb/unittests/Target/MemoryTest.cpp +++ b/lldb/unittests/Target/MemoryTest.cpp @@ -7,10 +7,15 @@ //===----------------------------------------------------------------------===// #include "lldb/Target/Memory.h" +#include "Plugins/ObjectFile/Mach-O/ObjectFileMachO.h" #include "Plugins/Platform/MacOSX/PlatformMacOSX.h" #include "Plugins/Platform/MacOSX/PlatformRemoteMacOSX.h" +#include "TestingSupport/SubsystemRAII.h" +#include "TestingSupport/TestUtilities.h" #include "lldb/Core/Debugger.h" +#include "lldb/Core/Module.h" #include "lldb/Core/PluginManager.h" +#include "lldb/Core/Section.h" #include "lldb/Host/FileSystem.h" #include "lldb/Host/HostInfo.h" #include "lldb/Target/ABI.h" @@ -18,6 +23,7 @@ #include "lldb/Target/Target.h" #include "lldb/Utility/ArchSpec.h" #include "lldb/Utility/DataBufferHeap.h" +#include "llvm/Testing/Support/Error.h" #include "gtest/gtest.h" #include <cstdint> @@ -702,3 +708,101 @@ TEST_F(MemoryTest, TestReadMemoryRangesClearMetadata) { ASSERT_EQ(read_results[0].size(), 1ull); ASSERT_EQ(read_results[0][0], 0xf0); // The ABI masks with 0xf0. } + +// The live process read fails outright, so Target::ReadMemory must fall all the +// way through to the file-cache fallback at the end of the function, which +// serves the bytes out of the module's (__DATA,__data) section. A full read +// there must report success, and a short read must not. +TEST_F(MemoryTest, TestReadMemoryClearsStaleError) { + SubsystemRAII<ObjectFileMachO> subsystems; + + ArchSpec arch("x86_64-apple-macosx-"); + Platform::SetHostPlatform(PlatformRemoteMacOSX::CreateInstance(true, &arch)); + + DebuggerSP debugger_sp = Debugger::CreateInstance(); + ASSERT_TRUE(debugger_sp); + + TargetSP target_sp = CreateTarget(debugger_sp, arch); + ASSERT_TRUE(target_sp); + + ProcessSP process_sp = CreateProcess(target_sp); + ASSERT_TRUE(process_sp); + + // The process can't produce a single byte, so the read must fail. + static_cast<DummyProcess *>(process_sp.get())->SetMaxReadSize(0); + + auto expected_file = TestFile::fromYaml(R"( +--- !mach-o +FileHeader: + magic: 0xFEEDFACF + cputype: 0x1000007 + cpusubtype: 0x3 + filetype: 0x2 + ncmds: 1 + sizeofcmds: 152 + flags: 0x200085 + reserved: 0x0 +LoadCommands: + - cmd: LC_SEGMENT_64 + cmdsize: 152 + segname: __DATA + vmaddr: 0x100001000 + vmsize: 0xC + fileoff: 0x1000 + filesize: 0xC + maxprot: 3 + initprot: 3 + nsects: 1 + flags: 0 + Sections: + - sectname: __data + segname: __DATA + addr: 0x100001000 + size: 12 + offset: 0x1000 + align: 0 + reloff: 0x0 + nreloc: 0 + flags: 0x0 + reserved1: 0x0 + reserved2: 0x0 + reserved3: 0x0 + content: 68656C6C6F20776F726C6400 +... +)"); + // "expected_file" owns the buffer the Module reads through, so it has to + // outlive every ReadMemory() call below. + ASSERT_THAT_EXPECTED(expected_file, llvm::Succeeded()); + + ModuleSP module_sp = std::make_shared<Module>(expected_file->moduleSpec()); + target_sp->GetImages().Append(module_sp, /*notify=*/false); + + SectionList *sections = module_sp->GetSectionList(); + ASSERT_TRUE(sections); + SectionSP section_sp = sections->FindSectionByName(ConstString("__data")); + ASSERT_TRUE(section_sp); + target_sp->SetSectionLoadAddress(section_sp, section_sp->GetFileAddress()); + + // force_live_memory = true skips the read-only file-cache fast path near the + // top of ReadMemory, and the section is writable so that path would reject + // it anyway. The fallback at the end is the only thing that can serve this. + Address addr; + ASSERT_TRUE( + target_sp->ResolveLoadAddress(section_sp->GetFileAddress(), addr)); + char buf[5] = {}; + Status error; + size_t bytes_read = target_sp->ReadMemory(addr, buf, sizeof(buf), error, + /*force_live_memory=*/true); + ASSERT_EQ(bytes_read, sizeof(buf)); + EXPECT_TRUE(error.Success()) << error.AsCString(); + EXPECT_EQ(llvm::StringRef(buf, sizeof(buf)), "hello"); + + // A short read must not be reported as success: ReadSectionData() clamps to + // the 12-byte section, so the tail of "big" would be left uninitialized. + char big[20] = {}; + Status short_error; + EXPECT_EQ(target_sp->ReadMemory(addr, big, sizeof(big), short_error, + /*force_live_memory=*/true), + 12u); + EXPECT_TRUE(short_error.Fail()); +} >From a4b96a83237ceb4aeb2eec1fbb7d221bd7bff517 Mon Sep 17 00:00:00 2001 From: Yao Qi <[email protected]> Date: Tue, 4 Aug 2026 12:01:01 +0100 Subject: [PATCH 2/2] Clear error before ReadMemoryFromFileCache and set error after if needed so the error is clear. --- lldb/source/Target/Target.cpp | 11 ++++++----- lldb/unittests/Target/MemoryTest.cpp | 3 +-- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/lldb/source/Target/Target.cpp b/lldb/source/Target/Target.cpp index 21aeb3ba1f446..3b14fd034410d 100644 --- a/lldb/source/Target/Target.cpp +++ b/lldb/source/Target/Target.cpp @@ -2209,12 +2209,13 @@ size_t Target::ReadMemory(const Address &addr, void *dst, size_t dst_len, if (!file_cache_read_buffer && resolved_addr.IsSectionOffset()) { // If we didn't already try and read from the object file cache, then try // it after failing to read from the process. - // ReadMemoryFromFileCache() only ever sets "error", so clear it to keep a - // failed process read from poisoning a successful read here. Only a full - // read counts: ReadSectionData() clamps, leaving the tail of "dst" unset. + error.Clear(); bytes_read = ReadMemoryFromFileCache(resolved_addr, dst, dst_len, error); - if (bytes_read == dst_len) - error.Clear(); + // Match the process-read path: a short read is an error. + if (bytes_read > 0 && bytes_read != dst_len && error.Success()) + error = Status::FromErrorStringWithFormatv( + "only {0} of {1} bytes were read from the object file cache", + bytes_read, dst_len); return bytes_read; } return 0; diff --git a/lldb/unittests/Target/MemoryTest.cpp b/lldb/unittests/Target/MemoryTest.cpp index f1d804b90c317..0a56cabaa3c24 100644 --- a/lldb/unittests/Target/MemoryTest.cpp +++ b/lldb/unittests/Target/MemoryTest.cpp @@ -797,8 +797,7 @@ TEST_F(MemoryTest, TestReadMemoryClearsStaleError) { EXPECT_TRUE(error.Success()) << error.AsCString(); EXPECT_EQ(llvm::StringRef(buf, sizeof(buf)), "hello"); - // A short read must not be reported as success: ReadSectionData() clamps to - // the 12-byte section, so the tail of "big" would be left uninitialized. + // A short read must be reported as an error. char big[20] = {}; Status short_error; EXPECT_EQ(target_sp->ReadMemory(addr, big, sizeof(big), short_error, _______________________________________________ lldb-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
