[Lldb-commits] [lldb] [lldb] Do not bump memory modificator ID when "internal" debugger memory is updated (PR #129092)
Mikhail Zakharov via lldb-commits
lldb-commits at lists.llvm.org
Fri Mar 14 15:06:10 PDT 2025
https://github.com/real-mikhail updated https://github.com/llvm/llvm-project/pull/129092
>From 1d2da22bceb83cbda54567e5f819e55555eef4e6 Mon Sep 17 00:00:00 2001
From: Mikhail Zakharov <mikhail.zakharov at jetbrains.com>
Date: Thu, 27 Feb 2025 18:43:33 +0100
Subject: [PATCH 1/2] [lldb] Do not bump memory modificator ID when "internal"
debugger memory is updated
This change prevents invalidating and updating values in `ValueObject::UpdateValueIfNeeded` when only "internal" debugger memory is updated.
Writing to "internal" debugger memory happens when, for instance, user expressions are evaluated by pretty printers.
For some collections getting collection size is an expensive operation (it requires traversal of the collection) and broken value caching (being invalidated after executing expressions) make things worse.
At the same time evaluating user expression with side effects (visible to target, not only to debugger) will still bump memory ID because:
1. If expression is evaluated via interpreter: it will cause write to "non-internal" memory
2. If expression is JIT-compiled: then to call the function LLDB will write to "non-internal" stack memory
---
lldb/include/lldb/Target/Memory.h | 4 +-
lldb/source/Target/Memory.cpp | 8 ++
lldb/source/Target/Process.cpp | 7 +-
.../Shell/Expr/TestExprWithSideEffect.cpp | 82 +++++++++++++++++++
4 files changed, 99 insertions(+), 2 deletions(-)
create mode 100644 lldb/test/Shell/Expr/TestExprWithSideEffect.cpp
diff --git a/lldb/include/lldb/Target/Memory.h b/lldb/include/lldb/Target/Memory.h
index 2d1489a2c96ea..864ef6ca00802 100644
--- a/lldb/include/lldb/Target/Memory.h
+++ b/lldb/include/lldb/Target/Memory.h
@@ -125,6 +125,8 @@ class AllocatedMemoryCache {
bool DeallocateMemory(lldb::addr_t ptr);
+ bool IsInCache(lldb::addr_t addr) const;
+
protected:
typedef std::shared_ptr<AllocatedBlock> AllocatedBlockSP;
@@ -133,7 +135,7 @@ class AllocatedMemoryCache {
// 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/source/Target/Memory.cpp b/lldb/source/Target/Memory.cpp
index 5cdd84f6640f0..9bfb8c349aa2c 100644
--- a/lldb/source/Target/Memory.cpp
+++ b/lldb/source/Target/Memory.cpp
@@ -433,3 +433,11 @@ bool AllocatedMemoryCache::DeallocateMemory(lldb::addr_t addr) {
(uint64_t)addr, success);
return success;
}
+
+bool AllocatedMemoryCache::IsInCache(lldb::addr_t addr) const {
+ std::lock_guard 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 6db582096155f..1150fabf2d0ed 100644
--- a/lldb/source/Target/Process.cpp
+++ b/lldb/source/Target/Process.cpp
@@ -2267,6 +2267,7 @@ size_t Process::WriteMemoryPrivate(addr_t addr, const void *buf, size_t size,
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())
@@ -2279,7 +2280,12 @@ size_t Process::WriteMemory(addr_t addr, const void *buf, size_t size,
if (buf == nullptr || size == 0)
return 0;
+#if defined(USE_ALLOCATE_MEMORY_CACHE)
+ if (!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
@@ -2408,7 +2414,6 @@ Status Process::WriteObjectFile(std::vector<ObjectFile::LoadableData> entries) {
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/test/Shell/Expr/TestExprWithSideEffect.cpp b/lldb/test/Shell/Expr/TestExprWithSideEffect.cpp
new file mode 100644
index 0000000000000..d072fe3121031
--- /dev/null
+++ b/lldb/test/Shell/Expr/TestExprWithSideEffect.cpp
@@ -0,0 +1,82 @@
+// Tests evaluating expressions with side effects.
+// Applied side effect should be visible to the debugger.
+
+// RUN: %build %s -o %t
+// RUN: %lldb %t -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 "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 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();
+ __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)
+
+
+
+// CHECK-LABEL: expr int $y = 11
+// CHECK-LABEL: expr $y
+// CHECK: (int) $y = 11
+
+// CHECK-LABEL: expr $y = 100
+// CHECK: (int) $1 = 100
+
+// CHECK-LABEL: expr $y
+// CHECK: (int) $y = 100
+
+// CHECK-LABEL: continue
+// CHECK-LABEL: expr $y
+// CHECK: (int) $y = 100
>From 5ce65f156a732a399c5f8c45407e68668c9c8fd8 Mon Sep 17 00:00:00 2001
From: Mikhail Zakharov <mikhail.zakharov at jetbrains.com>
Date: Fri, 14 Mar 2025 18:38:52 +0100
Subject: [PATCH 2/2] [lldb] Reformat test code and add additional test for
modifying member of the struct allocated in the cache
---
.../Shell/Expr/TestExprWithSideEffect.cpp | 37 ++------------
...TestExprWithSideEffectOnConvenienceVar.cpp | 48 +++++++++++++++++++
2 files changed, 52 insertions(+), 33 deletions(-)
create mode 100644 lldb/test/Shell/Expr/TestExprWithSideEffectOnConvenienceVar.cpp
diff --git a/lldb/test/Shell/Expr/TestExprWithSideEffect.cpp b/lldb/test/Shell/Expr/TestExprWithSideEffect.cpp
index d072fe3121031..78cbff07004bb 100644
--- a/lldb/test/Shell/Expr/TestExprWithSideEffect.cpp
+++ b/lldb/test/Shell/Expr/TestExprWithSideEffect.cpp
@@ -12,33 +12,22 @@
// RUN: -o "frame variable x" \
// RUN: -o "continue" \
// RUN: -o "frame variable x" \
-// 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 exit | FileCheck %s -dump-input=fail
-class X
-{
+class X {
int i = 0;
+
public:
- void inc()
- {
- ++i;
- }
+ void inc() { ++i; }
};
-int main()
-{
+int main() {
X x;
x.inc();
__builtin_debugtrap();
__builtin_debugtrap();
__builtin_debugtrap();
- __builtin_debugtrap();
return 0;
}
@@ -53,8 +42,6 @@ int main()
// CHECK-LABEL: frame variable x
// CHECK: (X) x = (i = 2)
-
-
// CHECK-LABEL: expr x.i = 10
// CHECK: (int) $0 = 10
@@ -64,19 +51,3 @@ int main()
// CHECK-LABEL: continue
// CHECK-LABEL: frame variable x
// CHECK: (X) x = (i = 10)
-
-
-
-// CHECK-LABEL: expr int $y = 11
-// CHECK-LABEL: expr $y
-// CHECK: (int) $y = 11
-
-// CHECK-LABEL: expr $y = 100
-// CHECK: (int) $1 = 100
-
-// CHECK-LABEL: expr $y
-// CHECK: (int) $y = 100
-
-// CHECK-LABEL: continue
-// CHECK-LABEL: expr $y
-// CHECK: (int) $y = 100
diff --git a/lldb/test/Shell/Expr/TestExprWithSideEffectOnConvenienceVar.cpp b/lldb/test/Shell/Expr/TestExprWithSideEffectOnConvenienceVar.cpp
new file mode 100644
index 0000000000000..201bbafb22137
--- /dev/null
+++ b/lldb/test/Shell/Expr/TestExprWithSideEffectOnConvenienceVar.cpp
@@ -0,0 +1,48 @@
+// Tests evaluating expressions with side effects on convenience variable.
+// Applied side effect should be visible to the debugger.
+
+// RUN: %build %s -o %t
+// RUN: %lldb %t -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)
More information about the lldb-commits
mailing list