[Lldb-commits] [lldb] d5df28d - [lldb] Replace the VERIFY_MEMORY_READS define with a setting (#218928)
via lldb-commits
lldb-commits at lists.llvm.org
Thu Aug 27 03:35:18 PDT 2026
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));
More information about the lldb-commits
mailing list