Author: Yao Qi Date: 2026-08-27T11:35:13+01:00 New Revision: d5df28d36f7564365266caa767ed221bcd5b60b4
URL: https://github.com/llvm/llvm-project/commit/d5df28d36f7564365266caa767ed221bcd5b60b4 DIFF: https://github.com/llvm/llvm-project/commit/d5df28d36f7564365266caa767ed221bcd5b60b4.diff LOG: [lldb] Replace the VERIFY_MEMORY_READS define with a setting (#218928) The cache verification block reads every range twice, once through the cache and once straight from the process, and compares them. It sat behind a commented-out `#define`, so nothing compiled it, and it had stopped compiling: `m_memory_cache.Read(this, addr, buf, size, error)` passes five arguments to a four-argument function. Since I am going to change L1/L2 cache, I'd like to enable the cache verification. Replace the `#define` with `target.process.verify-memory-reads`, next to `disable-memory-cache`, and check both entry points that go through the cache: `ReadMemory`, and `ReadMemoryRanges` one range at a time. A mismatch names which of the three checks failed, logs both results on the `process` channel because the assert message cannot carry them, and then asserts. Reporting is an assert, so the getter, the helper and the two calls sit behind `#ifndef NDEBUG`, and a release build carries none of it. The setting defaults to false, so nothing changes unless it is turned on. Run the API suite with it on through ``` LIT_OPTS='--param dotest-args=--setting=target.process.verify-memory-reads=true' \ ninja check-lldb-api ``` Two Darwin tests fail under that flag and are not worked around here. `TestObjCMethodsNSError.py` and `TestExpeditedStackMemory.py` count packets, and the second read changes the count. `TestGdbClientModuleLoad.py` also aborts, because its mock server answers the same address two ways; that is a defect in the mock, fixed separately. Added: Modified: lldb/cmake/modules/AddLLDB.cmake lldb/include/lldb/Target/Process.h lldb/source/Target/Process.cpp lldb/source/Target/TargetProperties.td lldb/unittests/Target/MemoryTest.cpp Removed: ################################################################################ diff --git a/lldb/cmake/modules/AddLLDB.cmake b/lldb/cmake/modules/AddLLDB.cmake index b6ad8d46380b3..621b31f46be79 100644 --- a/lldb/cmake/modules/AddLLDB.cmake +++ b/lldb/cmake/modules/AddLLDB.cmake @@ -25,6 +25,11 @@ function(lldb_tablegen) list(APPEND LTG_UNPARSED_ARGUMENTS -DLLDB_SANITIZED) endif() + string(TOUPPER "${CMAKE_BUILD_TYPE}" LTG_BUILD_TYPE) + if (NOT LLVM_ENABLE_ASSERTIONS AND NOT LTG_BUILD_TYPE STREQUAL "DEBUG") + list(APPEND LTG_UNPARSED_ARGUMENTS -DNDEBUG) + endif() + tablegen(LLDB ${LTG_UNPARSED_ARGUMENTS}) if(LTG_TARGET) diff --git a/lldb/include/lldb/Target/Process.h b/lldb/include/lldb/Target/Process.h index c260a4204d2f7..8b10a16df1bd3 100644 --- a/lldb/include/lldb/Target/Process.h +++ b/lldb/include/lldb/Target/Process.h @@ -87,6 +87,9 @@ class ProcessProperties : public Properties { ~ProcessProperties() override; bool GetDisableMemoryCache() const; +#ifndef NDEBUG + bool GetVerifyMemoryReads() const; +#endif uint64_t GetMemoryCacheLineSize() const; Args GetExtraStartupCommands() const; void SetExtraStartupCommands(const Args &args); @@ -3734,6 +3737,13 @@ void PruneThreadPlans(); private: Status DestroyImpl(bool force_kill); +#ifndef NDEBUG + /// Re-read \a size bytes at \a addr and assert they match the cache. + void VerifyMemoryRead(lldb::addr_t addr, const void *cache_buf, + size_t cache_bytes_read, size_t size, + const Status &cache_error); +#endif + /// This is the part of the event handling that for a process event. It /// decides what to do with the event and returns true if the event needs to /// be propagated to the user, and false otherwise. If the event is not diff --git a/lldb/source/Target/Process.cpp b/lldb/source/Target/Process.cpp index fca16b955021b..fdc56e1c310eb 100644 --- a/lldb/source/Target/Process.cpp +++ b/lldb/source/Target/Process.cpp @@ -204,6 +204,14 @@ bool ProcessProperties::GetDisableMemoryCache() const { idx, g_process_properties[idx].default_uint_value != 0); } +#ifndef NDEBUG +bool ProcessProperties::GetVerifyMemoryReads() const { + const uint32_t idx = ePropertyVerifyMemoryReads; + return GetPropertyAtIndexAs<bool>( + idx, g_process_properties[idx].default_uint_value != 0); +} +#endif + uint64_t ProcessProperties::GetMemoryCacheLineSize() const { const uint32_t idx = ePropertyMemCacheLineSize; return GetPropertyAtIndexAs<uint64_t>( @@ -2031,9 +2039,40 @@ Status Process::DisableSoftwareBreakpoint(BreakpointSite *bp_site) { return error; } -// Uncomment to verify memory caching works after making changes to caching -// code -//#define VERIFY_MEMORY_READS +#ifndef NDEBUG +void Process::VerifyMemoryRead(addr_t addr, const void *cache_buf, + size_t cache_bytes_read, size_t size, + const Status &cache_error) { + // A failed cache read stopped early, so only the bytes it did return and + // the contents can be compared. + const bool truncated = cache_error.Fail(); + + std::vector<uint8_t> verify_buf(size, 0); + Status verify_error; + const size_t verify_bytes_read = ReadMemoryFromInferior( + addr, verify_buf.data(), verify_buf.size(), verify_error); + const size_t comparable = std::min(cache_bytes_read, verify_bytes_read); + + const char *mismatch = nullptr; + if (!truncated && cache_bytes_read != verify_bytes_read) + mismatch = "byte count"; + else if (memcmp(cache_buf, verify_buf.data(), comparable) != 0) + mismatch = "contents"; + else if (!truncated && cache_error.Success() != verify_error.Success()) + mismatch = "status"; + if (!mismatch) + return; + + // Log before the assert, which cannot carry the two results. + LLDB_LOG(GetLog(LLDBLog::Process), + "memory cache verification failed on {0}: read of {1} bytes at " + "{2:x} returned {3} bytes ({4}) from the cache and {5} bytes ({6}) " + "from the process", + mismatch, size, addr, cache_bytes_read, cache_error, + verify_bytes_read, verify_error); + assert(false && "memory cache returned something the process did not"); +} +#endif size_t Process::ReadMemory(const ProcessAddress &process_addr, void *buf, size_t size, Status &error) { @@ -2042,43 +2081,15 @@ size_t Process::ReadMemory(const ProcessAddress &process_addr, void *buf, addr = abi_sp->FixAnyAddress(addr); error.Clear(); - if (!GetDisableMemoryCache()) { -#if defined(VERIFY_MEMORY_READS) - // Memory caching is enabled, with debug verification - - if (buf && size) { - // Uncomment the line below to make sure memory caching is working. - // I ran this through the test suite and got no assertions, so I am - // pretty confident this is working well. If any changes are made to - // memory caching, uncomment the line below and test your changes! - - // Verify all memory reads by using the cache first, then redundantly - // reading the same memory from the inferior and comparing to make sure - // everything is exactly the same. - std::string verify_buf(size, '\0'); - assert(verify_buf.size() == size); - const size_t cache_bytes_read = - m_memory_cache.Read(this, addr, buf, size, error); - Status verify_error; - const size_t verify_bytes_read = - ReadMemoryFromInferior(addr, const_cast<char *>(verify_buf.data()), - verify_buf.size(), verify_error); - assert(cache_bytes_read == verify_bytes_read); - assert(memcmp(buf, verify_buf.data(), verify_buf.size()) == 0); - assert(verify_error.Success() == error.Success()); - return cache_bytes_read; - } - return 0; -#else // !defined(VERIFY_MEMORY_READS) - // Memory caching is enabled, without debug verification - - return m_memory_cache.Read(addr, buf, size, error); -#endif // defined (VERIFY_MEMORY_READS) - } else { - // Memory caching is disabled - + if (GetDisableMemoryCache()) return ReadMemoryFromInferior(addr, buf, size, error); - } + + const size_t bytes_read = m_memory_cache.Read(addr, buf, size, error); +#ifndef NDEBUG + if (buf && size && GetVerifyMemoryReads()) + VerifyMemoryRead(addr, buf, bytes_read, size, error); +#endif + return bytes_read; } llvm::SmallVector<llvm::MutableArrayRef<uint8_t>> @@ -2089,9 +2100,23 @@ Process::ReadMemoryRanges(llvm::ArrayRef<Range<lldb::addr_t, size_t>> ranges, for (const Range<lldb::addr_t, size_t> &range : ranges) fixed_ranges.emplace_back(FixAnyAddress(range.GetRangeBase()), range.GetByteSize()); - if (!GetDisableMemoryCache()) - return m_memory_cache.ReadRanges(fixed_ranges, buffer); - return DoReadMemoryRanges(fixed_ranges, buffer); + if (GetDisableMemoryCache()) + return DoReadMemoryRanges(fixed_ranges, buffer); + + llvm::SmallVector<llvm::MutableArrayRef<uint8_t>> results = + m_memory_cache.ReadRanges(fixed_ranges, buffer); +#ifndef NDEBUG + if (GetVerifyMemoryReads()) { + for (auto [range, result] : llvm::zip(fixed_ranges, results)) { + if (!result.empty()) { + Status error; + VerifyMemoryRead(range.GetRangeBase(), result.data(), result.size(), + range.GetByteSize(), error); + } + } + } +#endif + return results; } llvm::SmallVector<llvm::MutableArrayRef<uint8_t>> diff --git a/lldb/source/Target/TargetProperties.td b/lldb/source/Target/TargetProperties.td index 0e5a41e8eca1b..7b296da45b26e 100644 --- a/lldb/source/Target/TargetProperties.td +++ b/lldb/source/Target/TargetProperties.td @@ -248,6 +248,11 @@ let Definition = "process", Path = "target.process" in { def DisableMemCache: Property<"disable-memory-cache", "Boolean">, DefaultFalse, Desc<"Disable reading and caching of memory in fixed-size units.">; +#ifndef NDEBUG + def VerifyMemoryReads: Property<"verify-memory-reads", "Boolean">, + DefaultFalse, + Desc<"Read memory that goes through the memory cache a second time directly from the process and report any diff erence. Debug builds only.">; +#endif def ExtraStartCommand: Property<"extra-startup-command", "Array">, ElementType<"String">, Desc<"A list containing extra commands understood by the particular process plugin used. For instance, to turn on debugserver logging set this to 'QSetLogging:bitmask=LOG_DEFAULT;'">; diff --git a/lldb/unittests/Target/MemoryTest.cpp b/lldb/unittests/Target/MemoryTest.cpp index 5679d863f62d4..73f17ca4ce122 100644 --- a/lldb/unittests/Target/MemoryTest.cpp +++ b/lldb/unittests/Target/MemoryTest.cpp @@ -520,6 +520,7 @@ class DummyReaderProcess : public Process { buffer[addr - vm_addr] = static_cast<uint8_t>(addr); // LSB of addr. return size; } + MemoryCache &GetMemoryCache() { return m_memory_cache; } // Boilerplate, nothing interesting below. DummyReaderProcess(lldb::TargetSP target_sp, lldb::ListenerSP listener_sp) : Process(target_sp, listener_sp) {} @@ -845,6 +846,62 @@ class StringReaderProcess : public Process { llvm::StringRef GetPluginName() override { return "Dummy"; } }; +#ifndef NDEBUG +TEST_F(MemoryDeathTest, TestVerifyMemoryReads) { + GTEST_FLAG_SET(death_test_style, "threadsafe"); + + 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); + ListenerSP listener_sp(Listener::MakeListener("dummy")); + auto process_sp = + std::make_shared<DummyReaderProcess>(target_sp, listener_sp); + + // Off by default, and set on this process, so there is nothing to restore. + ASSERT_FALSE(process_sp->GetVerifyMemoryReads()); + Status set_error = process_sp->SetPropertyValue( + nullptr, eVarSetOperationAssign, "verify-memory-reads", "true"); + ASSERT_TRUE(set_error.Success()) << set_error.AsCString(); + ASSERT_TRUE(process_sp->GetVerifyMemoryReads()); + + // A cache that agrees with the process passes, and still returns the bytes. + Status error; + std::vector<uint8_t> buf(16, 0); + EXPECT_EQ(process_sp->ReadMemory(0x1000, buf.data(), buf.size(), error), + buf.size()); + for (size_t i = 0; i < buf.size(); ++i) + ASSERT_EQ(buf[i], static_cast<uint8_t>(0x1000 + i)) << "byte " << i; + + // The same holds for the ranges API. + llvm::SmallVector<Range<addr_t, size_t>> ranges = {{0x1000, 16}, + {0x3000, 16}}; + llvm::SmallVector<uint8_t, 0> ranges_buf(32, 0); + for (auto [range, memory] : + llvm::zip(ranges, process_sp->ReadMemoryRanges(ranges, ranges_buf))) { + ASSERT_EQ(memory.size(), 16u); + for (auto [i, byte] : llvm::enumerate(memory)) + ASSERT_EQ(byte, static_cast<uint8_t>(range.GetRangeBase() + i)); + } + + // DummyReaderProcess returns the low byte of each address, so a run of + // zeroes cannot be what it would read. + process_sp->GetMemoryCache().Clear(); + process_sp->GetMemoryCache().AddL1CacheData( + 0x2000, std::make_shared<DataBufferHeap>(16, 0)); + std::vector<uint8_t> bad(16, 0); + ASSERT_DEATH( + { process_sp->ReadMemory(0x2000, bad.data(), bad.size(), error); }, + "memory cache returned something the process did not"); + Range<addr_t, size_t> bad_range(0x2000, 16); + ASSERT_DEATH( + { process_sp->ReadMemoryRanges(bad_range, bad); }, + "memory cache returned something the process did not"); +} +#endif // NDEBUG + TEST_F(MemoryTest, TestReadCStringsFromMemory) { ArchSpec arch("x86_64-apple-macosx-"); Platform::SetHostPlatform(PlatformRemoteMacOSX::CreateInstance(true, &arch)); _______________________________________________ lldb-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
