Reland "[lldb] Do not bump memory modificator ID when "internal" debugger memory is updated (#129092)"
This reverts commit daa4061d61216456baa83ab404e096200e327fb4.
Original PR https://github.com/llvm/llvm-project/pull/129092.
I have restricted the test to X86 Windows because it turns out the only
reason that `expr x.get()` would change m_memory_id is that on x86 we
have to write the return address to the stack in ABIWindows_X86_64::PrepareTrivialCall:
```
// Save return address onto the stack
if (!process_sp->WritePointerToMemory(sp, return_addr, error))
return false;
```
This is not required on AArch64 so m_memory_id was not changed:
```
(lldb) expr x.get()
(int) $0 = 0
(lldb) process status -d
Process 15316 stopped
* thread #1, stop reason = Exception 0x80000003 encountered at address 0x7ff764a31034
frame #0: 0x00007ff764a31038 TestProcessModificationIdOnExpr.cpp.tmp`main at TestProcessModificationIdOnExpr.cpp:35
32 __builtin_debugtrap();
33 __builtin_debugtrap();
34 return 0;
-> 35 }
36
37 // CHECK-LABEL: process status -d
38 // CHECK: m_stop_id: 2
ProcessModID:
m_stop_id: 3
m_last_natural_stop_id: 0
m_resume_id: 0
m_memory_id: 0
```
Really we should find a better way to force a memory write here, but
I can't think of one right now.
diff --git a/lldb/include/lldb/Target/Memory.h b/lldb/include/lldb/Target/Memory.h
index 2d1489a..864ef6c 100644
--- a/lldb/include/lldb/Target/Memory.h
+++ b/lldb/include/lldb/Target/Memory.h
@@ -125,6 +125,8 @@
bool DeallocateMemory(lldb::addr_t ptr);
+ bool IsInCache(lldb::addr_t addr) const;
+
protected:
typedef std::shared_ptr<AllocatedBlock> AllocatedBlockSP;
@@ -133,7 +135,7 @@
// Classes that inherit from MemoryCache can see and modify these
Process &m_process;
- std::recursive_mutex m_mutex;
+ mutable std::recursive_mutex m_mutex;
typedef std::multimap<uint32_t, AllocatedBlockSP> PermissionsToBlockMap;
PermissionsToBlockMap m_memory_map;
diff --git a/lldb/include/lldb/Target/Process.h b/lldb/include/lldb/Target/Process.h
index f5ee01d..536a69f 100644
--- a/lldb/include/lldb/Target/Process.h
+++ b/lldb/include/lldb/Target/Process.h
@@ -111,6 +111,7 @@
void SetOSPluginReportsAllThreads(bool does_report);
bool GetSteppingRunsAllThreads() const;
FollowForkMode GetFollowForkMode() const;
+ bool TrackMemoryCacheChanges() const;
protected:
Process *m_process; // Can be nullptr for global ProcessProperties
@@ -312,6 +313,18 @@
return lldb::EventSP();
}
+ void Dump(Stream &stream) const {
+ stream.Format("ProcessModID:\n"
+ " m_stop_id: {0}\n m_last_natural_stop_id: {1}\n"
+ " m_resume_id: {2}\n m_memory_id: {3}\n"
+ " m_last_user_expression_resume: {4}\n"
+ " m_running_user_expression: {5}\n"
+ " m_running_utility_function: {6}\n",
+ m_stop_id, m_last_natural_stop_id, m_resume_id, m_memory_id,
+ m_last_user_expression_resume, m_running_user_expression,
+ m_running_utility_function);
+ }
+
private:
uint32_t m_stop_id = 0;
uint32_t m_last_natural_stop_id = 0;
diff --git a/lldb/source/Commands/CommandObjectProcess.cpp b/lldb/source/Commands/CommandObjectProcess.cpp
index ed80c85..d0f5eaf 100644
--- a/lldb/source/Commands/CommandObjectProcess.cpp
+++ b/lldb/source/Commands/CommandObjectProcess.cpp
@@ -1388,6 +1388,9 @@
case 'v':
m_verbose = true;
break;
+ case 'd':
+ m_dump = true;
+ break;
default:
llvm_unreachable("Unimplemented option");
}
@@ -1397,6 +1400,7 @@
void OptionParsingStarting(ExecutionContext *execution_context) override {
m_verbose = false;
+ m_dump = false;
}
llvm::ArrayRef<OptionDefinition> GetDefinitions() override {
@@ -1405,6 +1409,7 @@
// Instance variables to hold the values for command options.
bool m_verbose = false;
+ bool m_dump = false;
};
protected:
@@ -1459,6 +1464,14 @@
crash_info_sp->GetDescription(strm);
}
}
+
+ if (m_options.m_dump) {
+ StateType state = process->GetState();
+ if (state == eStateStopped) {
+ ProcessModID process_mod_id = process->GetModID();
+ process_mod_id.Dump(result.GetOutputStream());
+ }
+ }
}
private:
diff --git a/lldb/source/Commands/Options.td b/lldb/source/Commands/Options.td
index 346ca4ae..eb2e511 100644
--- a/lldb/source/Commands/Options.td
+++ b/lldb/source/Commands/Options.td
@@ -788,6 +788,8 @@
let Command = "process status" in {
def process_status_verbose : Option<"verbose", "v">, Group<1>,
Desc<"Show verbose process status including extended crash information.">;
+ def process_status_dump : Option<"dump-modification-id", "d">, Group<1>,
+ Desc<"Dump the state of the ProcessModID of the stopped process.">;
}
let Command = "process save_core" in {
diff --git a/lldb/source/Target/Memory.cpp b/lldb/source/Target/Memory.cpp
index 5cdd84f..a23a507 100644
--- a/lldb/source/Target/Memory.cpp
+++ b/lldb/source/Target/Memory.cpp
@@ -433,3 +433,11 @@
(uint64_t)addr, success);
return success;
}
+
+bool AllocatedMemoryCache::IsInCache(lldb::addr_t addr) const {
+ std::lock_guard<std::recursive_mutex> guard(m_mutex);
+
+ return llvm::any_of(m_memory_map, [addr](const auto &block) {
+ return block.second->Contains(addr);
+ });
+}
diff --git a/lldb/source/Target/Process.cpp b/lldb/source/Target/Process.cpp
index 2557924..ce64b44 100644
--- a/lldb/source/Target/Process.cpp
+++ b/lldb/source/Target/Process.cpp
@@ -370,6 +370,12 @@
g_process_properties[idx].default_uint_value));
}
+bool ProcessProperties::TrackMemoryCacheChanges() const {
+ const uint32_t idx = ePropertyTrackMemoryCacheChanges;
+ return GetPropertyAtIndexAs<bool>(
+ idx, g_process_properties[idx].default_uint_value != 0);
+}
+
ProcessSP Process::FindPlugin(lldb::TargetSP target_sp,
llvm::StringRef plugin_name,
ListenerSP listener_sp,
@@ -2285,6 +2291,7 @@
return bytes_written;
}
+#define USE_ALLOCATE_MEMORY_CACHE 1
size_t Process::WriteMemory(addr_t addr, const void *buf, size_t size,
Status &error) {
if (ABISP abi_sp = GetABI())
@@ -2297,7 +2304,12 @@
if (buf == nullptr || size == 0)
return 0;
+#if defined(USE_ALLOCATE_MEMORY_CACHE)
+ if (TrackMemoryCacheChanges() || !m_allocated_memory_cache.IsInCache(addr))
+ m_mod_id.BumpMemoryID();
+#else
m_mod_id.BumpMemoryID();
+#endif
// We need to write any data that would go where any current software traps
// (enabled software breakpoints) any software traps (breakpoints) that we
@@ -2426,7 +2438,6 @@
return error;
}
-#define USE_ALLOCATE_MEMORY_CACHE 1
addr_t Process::AllocateMemory(size_t size, uint32_t permissions,
Status &error) {
if (GetPrivateState() != eStateStopped) {
diff --git a/lldb/source/Target/TargetProperties.td b/lldb/source/Target/TargetProperties.td
index 3940ac0..14fa33c 100644
--- a/lldb/source/Target/TargetProperties.td
+++ b/lldb/source/Target/TargetProperties.td
@@ -299,6 +299,9 @@
DefaultEnumValue<"eFollowParent">,
EnumValues<"OptionEnumValues(g_follow_fork_mode_values)">,
Desc<"Debugger's behavior upon fork or vfork.">;
+ def TrackMemoryCacheChanges: Property<"track-memory-cache-changes", "Boolean">,
+ DefaultTrue,
+ Desc<"If true, memory cache modifications (which happen often during expressions evaluation) will bump process state ID (and invalidate all synthetic children). Disabling this option helps to avoid synthetic children reevaluation when pretty printers heavily use expressions. The downside of disabled setting is that convenience variables won't reevaluate synthetic children automatically.">;
}
let Definition = "platform" in {
diff --git a/lldb/test/API/commands/settings/TestSettings.py b/lldb/test/API/commands/settings/TestSettings.py
index 4eb8684..4f4fa0e 100644
--- a/lldb/test/API/commands/settings/TestSettings.py
+++ b/lldb/test/API/commands/settings/TestSettings.py
@@ -905,6 +905,7 @@
"target.use-hex-immediates",
"target.process.disable-memory-cache",
"target.process.extra-startup-command",
+ "target.process.track-memory-cache-changes",
"target.process.thread.trace-thread",
"target.process.thread.step-avoid-regexp",
],
diff --git a/lldb/test/Shell/Expr/TestExprWithSideEffect.cpp b/lldb/test/Shell/Expr/TestExprWithSideEffect.cpp
new file mode 100644
index 0000000..4fc88b6
--- /dev/null
+++ b/lldb/test/Shell/Expr/TestExprWithSideEffect.cpp
@@ -0,0 +1,55 @@
+// Tests evaluating expressions with side effects.
+// Applied side effect should be visible to the debugger.
+
+// RUN: %build %s -o %t
+// RUN: %lldb %t \
+// RUN: -o "settings set target.process.track-memory-cache-changes false" \
+// RUN: -o "run" \
+// RUN: -o "frame variable x" \
+// RUN: -o "expr x.inc()" \
+// RUN: -o "frame variable x" \
+// RUN: -o "continue" \
+// RUN: -o "frame variable x" \
+// RUN: -o "expr x.i = 10" \
+// RUN: -o "frame variable x" \
+// RUN: -o "continue" \
+// RUN: -o "frame variable x" \
+// RUN: -o "exit" | FileCheck %s -dump-input=fail
+
+class X {
+ int i = 0;
+
+public:
+ void inc() { ++i; }
+};
+
+int main() {
+ X x;
+ x.inc();
+
+ __builtin_debugtrap();
+ __builtin_debugtrap();
+ __builtin_debugtrap();
+ return 0;
+}
+
+// CHECK-LABEL: frame variable x
+// CHECK: (X) x = (i = 1)
+
+// CHECK-LABEL: expr x.inc()
+// CHECK-LABEL: frame variable x
+// CHECK: (X) x = (i = 2)
+
+// CHECK-LABEL: continue
+// CHECK-LABEL: frame variable x
+// CHECK: (X) x = (i = 2)
+
+// CHECK-LABEL: expr x.i = 10
+// CHECK: (int) $0 = 10
+
+// CHECK-LABEL: frame variable x
+// CHECK: (X) x = (i = 10)
+
+// CHECK-LABEL: continue
+// CHECK-LABEL: frame variable x
+// CHECK: (X) x = (i = 10)
diff --git a/lldb/test/Shell/Expr/TestExprWithSideEffectOnConvenienceVar.cpp b/lldb/test/Shell/Expr/TestExprWithSideEffectOnConvenienceVar.cpp
new file mode 100644
index 0000000..b41a81f
--- /dev/null
+++ b/lldb/test/Shell/Expr/TestExprWithSideEffectOnConvenienceVar.cpp
@@ -0,0 +1,52 @@
+// Tests evaluating expressions with side effects on convenience variable.
+// Applied side effect should be visible to the debugger.
+
+// UNSUPPORTED: system-windows
+
+// RUN: %build %s -o %t
+// RUN: %lldb %t \
+// RUN: -o "settings set target.process.track-memory-cache-changes false" \
+// RUN: -o "run" \
+// RUN: -o "expr int \$y = 11" \
+// RUN: -o "expr \$y" \
+// RUN: -o "expr \$y = 100" \
+// RUN: -o "expr \$y" \
+// RUN: -o "continue" \
+// RUN: -o "expr \$y" \
+// RUN: -o "expr X \$mine = {100, 200}" \
+// RUN: -o "expr \$mine.a = 300" \
+// RUN: -o "expr \$mine" \
+// RUN: -o "exit" | FileCheck %s -dump-input=fail
+
+struct X {
+ int a;
+ int b;
+};
+
+int main() {
+ X x;
+
+ __builtin_debugtrap();
+ __builtin_debugtrap();
+ return 0;
+}
+
+// CHECK-LABEL: expr int $y = 11
+// CHECK-LABEL: expr $y
+// CHECK: (int) $y = 11
+
+// CHECK-LABEL: expr $y = 100
+// CHECK: (int) $0 = 100
+
+// CHECK-LABEL: expr $y
+// CHECK: (int) $y = 100
+
+// CHECK-LABEL: continue
+// CHECK-LABEL: expr $y
+// CHECK: (int) $y = 100
+
+// CHECK-LABEL: expr X $mine = {100, 200}
+// CHECK-LABEL: expr $mine.a = 300
+// CHECK: (int) $1 = 300
+// CHECK-LABEL: expr $mine
+// CHECK: (X) $mine = (a = 300, b = 200)
diff --git a/lldb/test/Shell/Expr/TestExprWithSideEffectOnConvenienceVarWindows.cpp b/lldb/test/Shell/Expr/TestExprWithSideEffectOnConvenienceVarWindows.cpp
new file mode 100644
index 0000000..8ac4b96
--- /dev/null
+++ b/lldb/test/Shell/Expr/TestExprWithSideEffectOnConvenienceVarWindows.cpp
@@ -0,0 +1,51 @@
+// Same as TestExprWithSideEffectOnConvenienceVar.cpp but without $ escaping
+
+// REQUIRES: target-windows
+
+// RUN: %build %s -o %t
+// RUN: %lldb %t \
+// RUN: -o "settings set target.process.track-memory-cache-changes false" \
+// RUN: -o "run" \
+// RUN: -o "expr int $y = 11" \
+// RUN: -o "expr $y" \
+// RUN: -o "expr $y = 100" \
+// RUN: -o "expr $y" \
+// RUN: -o "continue" \
+// RUN: -o "expr $y" \
+// RUN: -o "expr X $mine = {100, 200}" \
+// RUN: -o "expr $mine.a = 300" \
+// RUN: -o "expr $mine" \
+// RUN: -o "exit" | FileCheck %s -dump-input=fail
+
+struct X {
+ int a;
+ int b;
+};
+
+int main() {
+ X x;
+
+ __builtin_debugtrap();
+ __builtin_debugtrap();
+ return 0;
+}
+
+// CHECK-LABEL: expr int $y = 11
+// CHECK-LABEL: expr $y
+// CHECK: (int) $y = 11
+
+// CHECK-LABEL: expr $y = 100
+// CHECK: (int) $0 = 100
+
+// CHECK-LABEL: expr $y
+// CHECK: (int) $y = 100
+
+// CHECK-LABEL: continue
+// CHECK-LABEL: expr $y
+// CHECK: (int) $y = 100
+
+// CHECK-LABEL: expr X $mine = {100, 200}
+// CHECK-LABEL: expr $mine.a = 300
+// CHECK: (int) $1 = 300
+// CHECK-LABEL: expr $mine
+// CHECK: (X) $mine = (a = 300, b = 200)
diff --git a/lldb/test/Shell/Expr/TestProcessModificationIdOnExpr.cpp b/lldb/test/Shell/Expr/TestProcessModificationIdOnExpr.cpp
new file mode 100644
index 0000000..fe6c2fd
--- /dev/null
+++ b/lldb/test/Shell/Expr/TestProcessModificationIdOnExpr.cpp
@@ -0,0 +1,69 @@
+// Tests that ProcessModID.m_memory_id is not bumped when evaluating expressions without side effects.
+
+// REQUIRES: target-windows && target-x86
+// Due to different implementations exact numbers (m_stop_id) are different on different OSs. So we lock this test to specific platform (Windows). It is limited to x86 because on x86, running get()
+// requires that we write the return address to the stack, this does not happen on AArch64.
+
+// RUN: %build %s -o %t
+// RUN: %lldb %t \
+// RUN: -o "settings set target.process.track-memory-cache-changes false" \
+// RUN: -o "run" \
+// RUN: -o "process status -d" \
+// RUN: -o "expr x.i != 42" \
+// RUN: -o "process status -d" \
+// RUN: -o "expr x.get()" \
+// RUN: -o "process status -d" \
+// RUN: -o "expr x.i = 10" \
+// RUN: -o "process status -d" \
+// RUN: -o "continue" \
+// RUN: -o "process status -d" \
+// RUN: -o "exit" | FileCheck %s -dump-input=fail
+
+class X {
+ int i = 0;
+
+public:
+ int get() { return i; }
+};
+
+int main() {
+ X x;
+ x.get();
+
+ __builtin_debugtrap();
+ __builtin_debugtrap();
+ return 0;
+}
+
+// CHECK-LABEL: process status -d
+// CHECK: m_stop_id: 2
+// CHECK: m_memory_id: 0
+
+// CHECK-LABEL: expr x.i != 42
+// IDs are not changed when executing simple expressions
+
+// CHECK-LABEL: process status -d
+// CHECK: m_stop_id: 2
+// CHECK: m_memory_id: 0
+
+// CHECK-LABEL: expr x.get()
+// Expression causes ID to be bumped because LLDB has to execute function and in doing
+// so must write the return address to the stack.
+
+// CHECK-LABEL: process status -d
+// CHECK: m_stop_id: 3
+// CHECK: m_memory_id: 1
+
+// CHECK-LABEL: expr x.i = 10
+// Expression causes MemoryID to be bumped because LLDB writes to non-cache memory
+
+// CHECK-LABEL: process status -d
+// CHECK: m_stop_id: 3
+// CHECK: m_memory_id: 2
+
+// CHECK-LABEL: continue
+// Continue causes StopID to be bumped because process is resumed
+
+// CHECK-LABEL: process status -d
+// CHECK: m_stop_id: 4
+// CHECK: m_memory_id: 2
