Skip to content

Commit 7d01b85

Browse files
DavidSpickettDavidSpickett
committed
Reland "[lldb] Do not bump memory modificator ID when "internal" debugger memory is updated (#129092)"
This reverts commit daa4061. Original PR llvm/llvm-project#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.
1 parent f6ac527 commit 7d01b85

12 files changed

+282
-2
lines changed

‎lldb/include/lldb/Target/Memory.h

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -125,6 +125,8 @@ class AllocatedMemoryCache {
125125

126126
bool DeallocateMemory(lldb::addr_t ptr);
127127

128+
bool IsInCache(lldb::addr_t addr) const;
129+
128130
protected:
129131
typedef std::shared_ptr<AllocatedBlock> AllocatedBlockSP;
130132

@@ -133,7 +135,7 @@ class AllocatedMemoryCache {
133135

134136
// Classes that inherit from MemoryCache can see and modify these
135137
Process &m_process;
136-
std::recursive_mutex m_mutex;
138+
mutable std::recursive_mutex m_mutex;
137139
typedef std::multimap<uint32_t, AllocatedBlockSP> PermissionsToBlockMap;
138140
PermissionsToBlockMap m_memory_map;
139141

‎lldb/include/lldb/Target/Process.h

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -111,6 +111,7 @@ class ProcessProperties : public Properties {
111111
void SetOSPluginReportsAllThreads(bool does_report);
112112
bool GetSteppingRunsAllThreads() const;
113113
FollowForkMode GetFollowForkMode() const;
114+
bool TrackMemoryCacheChanges() const;
114115

115116
protected:
116117
Process *m_process; // Can be nullptr for global ProcessProperties
@@ -312,6 +313,18 @@ class ProcessModID {
312313
return lldb::EventSP();
313314
}
314315

316+
void Dump(Stream &stream) const {
317+
stream.Format("ProcessModID:\n"
318+
" m_stop_id: {0}\n m_last_natural_stop_id: {1}\n"
319+
" m_resume_id: {2}\n m_memory_id: {3}\n"
320+
" m_last_user_expression_resume: {4}\n"
321+
" m_running_user_expression: {5}\n"
322+
" m_running_utility_function: {6}\n",
323+
m_stop_id, m_last_natural_stop_id, m_resume_id, m_memory_id,
324+
m_last_user_expression_resume, m_running_user_expression,
325+
m_running_utility_function);
326+
}
327+
315328
private:
316329
uint32_t m_stop_id = 0;
317330
uint32_t m_last_natural_stop_id = 0;

‎lldb/source/Commands/CommandObjectProcess.cpp

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1388,6 +1388,9 @@ class CommandObjectProcessStatus : public CommandObjectParsed {
13881388
case 'v':
13891389
m_verbose = true;
13901390
break;
1391+
case 'd':
1392+
m_dump = true;
1393+
break;
13911394
default:
13921395
llvm_unreachable("Unimplemented option");
13931396
}
@@ -1397,6 +1400,7 @@ class CommandObjectProcessStatus : public CommandObjectParsed {
13971400

13981401
void OptionParsingStarting(ExecutionContext *execution_context) override {
13991402
m_verbose = false;
1403+
m_dump = false;
14001404
}
14011405

14021406
llvm::ArrayRef<OptionDefinition> GetDefinitions() override {
@@ -1405,6 +1409,7 @@ class CommandObjectProcessStatus : public CommandObjectParsed {
14051409

14061410
// Instance variables to hold the values for command options.
14071411
bool m_verbose = false;
1412+
bool m_dump = false;
14081413
};
14091414

14101415
protected:
@@ -1459,6 +1464,14 @@ class CommandObjectProcessStatus : public CommandObjectParsed {
14591464
crash_info_sp->GetDescription(strm);
14601465
}
14611466
}
1467+
1468+
if (m_options.m_dump) {
1469+
StateType state = process->GetState();
1470+
if (state == eStateStopped) {
1471+
ProcessModID process_mod_id = process->GetModID();
1472+
process_mod_id.Dump(result.GetOutputStream());
1473+
}
1474+
}
14621475
}
14631476

14641477
private:

‎lldb/source/Commands/Options.td

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -788,6 +788,8 @@ let Command = "process handle" in {
788788
let Command = "process status" in {
789789
def process_status_verbose : Option<"verbose", "v">, Group<1>,
790790
Desc<"Show verbose process status including extended crash information.">;
791+
def process_status_dump : Option<"dump-modification-id", "d">, Group<1>,
792+
Desc<"Dump the state of the ProcessModID of the stopped process.">;
791793
}
792794

793795
let Command = "process save_core" in {

‎lldb/source/Target/Memory.cpp

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -433,3 +433,11 @@ bool AllocatedMemoryCache::DeallocateMemory(lldb::addr_t addr) {
433433
(uint64_t)addr, success);
434434
return success;
435435
}
436+
437+
bool AllocatedMemoryCache::IsInCache(lldb::addr_t addr) const {
438+
std::lock_guard<std::recursive_mutex> guard(m_mutex);
439+
440+
return llvm::any_of(m_memory_map, [addr](const auto &block) {
441+
return block.second->Contains(addr);
442+
});
443+
}

‎lldb/source/Target/Process.cpp

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -370,6 +370,12 @@ FollowForkMode ProcessProperties::GetFollowForkMode() const {
370370
g_process_properties[idx].default_uint_value));
371371
}
372372

373+
bool ProcessProperties::TrackMemoryCacheChanges() const {
374+
const uint32_t idx = ePropertyTrackMemoryCacheChanges;
375+
return GetPropertyAtIndexAs<bool>(
376+
idx, g_process_properties[idx].default_uint_value != 0);
377+
}
378+
373379
ProcessSP Process::FindPlugin(lldb::TargetSP target_sp,
374380
llvm::StringRef plugin_name,
375381
ListenerSP listener_sp,
@@ -2285,6 +2291,7 @@ size_t Process::WriteMemoryPrivate(addr_t addr, const void *buf, size_t size,
22852291
return bytes_written;
22862292
}
22872293

2294+
#define USE_ALLOCATE_MEMORY_CACHE 1
22882295
size_t Process::WriteMemory(addr_t addr, const void *buf, size_t size,
22892296
Status &error) {
22902297
if (ABISP abi_sp = GetABI())
@@ -2297,7 +2304,12 @@ size_t Process::WriteMemory(addr_t addr, const void *buf, size_t size,
22972304
if (buf == nullptr || size == 0)
22982305
return 0;
22992306

2307+
#if defined(USE_ALLOCATE_MEMORY_CACHE)
2308+
if (TrackMemoryCacheChanges() || !m_allocated_memory_cache.IsInCache(addr))
2309+
m_mod_id.BumpMemoryID();
2310+
#else
23002311
m_mod_id.BumpMemoryID();
2312+
#endif
23012313

23022314
// We need to write any data that would go where any current software traps
23032315
// (enabled software breakpoints) any software traps (breakpoints) that we
@@ -2426,7 +2438,6 @@ Status Process::WriteObjectFile(std::vector<ObjectFile::LoadableData> entries) {
24262438
return error;
24272439
}
24282440

2429-
#define USE_ALLOCATE_MEMORY_CACHE 1
24302441
addr_t Process::AllocateMemory(size_t size, uint32_t permissions,
24312442
Status &error) {
24322443
if (GetPrivateState() != eStateStopped) {

‎lldb/source/Target/TargetProperties.td

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -299,6 +299,9 @@ let Definition = "process" in {
299299
DefaultEnumValue<"eFollowParent">,
300300
EnumValues<"OptionEnumValues(g_follow_fork_mode_values)">,
301301
Desc<"Debugger's behavior upon fork or vfork.">;
302+
def TrackMemoryCacheChanges: Property<"track-memory-cache-changes", "Boolean">,
303+
DefaultTrue,
304+
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.">;
302305
}
303306

304307
let Definition = "platform" in {

‎lldb/test/API/commands/settings/TestSettings.py

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -905,6 +905,7 @@ def test_all_settings_exist(self):
905905
"target.use-hex-immediates",
906906
"target.process.disable-memory-cache",
907907
"target.process.extra-startup-command",
908+
"target.process.track-memory-cache-changes",
908909
"target.process.thread.trace-thread",
909910
"target.process.thread.step-avoid-regexp",
910911
],

‎lldb/test/Shell/Expr/TestExprWithSideEffect.cpp

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
1+
// Tests evaluating expressions with side effects.
2+
// Applied side effect should be visible to the debugger.
3+
4+
// RUN: %build %s -o %t
5+
// RUN: %lldb %t \
6+
// RUN: -o "settings set target.process.track-memory-cache-changes false" \
7+
// RUN: -o "run" \
8+
// RUN: -o "frame variable x" \
9+
// RUN: -o "expr x.inc()" \
10+
// RUN: -o "frame variable x" \
11+
// RUN: -o "continue" \
12+
// RUN: -o "frame variable x" \
13+
// RUN: -o "expr x.i = 10" \
14+
// RUN: -o "frame variable x" \
15+
// RUN: -o "continue" \
16+
// RUN: -o "frame variable x" \
17+
// RUN: -o "exit" | FileCheck %s -dump-input=fail
18+
19+
class X {
20+
int i = 0;
21+
22+
public:
23+
void inc() { ++i; }
24+
};
25+
26+
int main() {
27+
X x;
28+
x.inc();
29+
30+
__builtin_debugtrap();
31+
__builtin_debugtrap();
32+
__builtin_debugtrap();
33+
return 0;
34+
}
35+
36+
// CHECK-LABEL: frame variable x
37+
// CHECK: (X) x = (i = 1)
38+
39+
// CHECK-LABEL: expr x.inc()
40+
// CHECK-LABEL: frame variable x
41+
// CHECK: (X) x = (i = 2)
42+
43+
// CHECK-LABEL: continue
44+
// CHECK-LABEL: frame variable x
45+
// CHECK: (X) x = (i = 2)
46+
47+
// CHECK-LABEL: expr x.i = 10
48+
// CHECK: (int) $0 = 10
49+
50+
// CHECK-LABEL: frame variable x
51+
// CHECK: (X) x = (i = 10)
52+
53+
// CHECK-LABEL: continue
54+
// CHECK-LABEL: frame variable x
55+
// CHECK: (X) x = (i = 10)

‎lldb/test/Shell/Expr/TestExprWithSideEffectOnConvenienceVar.cpp

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
1+
// Tests evaluating expressions with side effects on convenience variable.
2+
// Applied side effect should be visible to the debugger.
3+
4+
// UNSUPPORTED: system-windows
5+
6+
// RUN: %build %s -o %t
7+
// RUN: %lldb %t \
8+
// RUN: -o "settings set target.process.track-memory-cache-changes false" \
9+
// RUN: -o "run" \
10+
// RUN: -o "expr int \$y = 11" \
11+
// RUN: -o "expr \$y" \
12+
// RUN: -o "expr \$y = 100" \
13+
// RUN: -o "expr \$y" \
14+
// RUN: -o "continue" \
15+
// RUN: -o "expr \$y" \
16+
// RUN: -o "expr X \$mine = {100, 200}" \
17+
// RUN: -o "expr \$mine.a = 300" \
18+
// RUN: -o "expr \$mine" \
19+
// RUN: -o "exit" | FileCheck %s -dump-input=fail
20+
21+
struct X {
22+
int a;
23+
int b;
24+
};
25+
26+
int main() {
27+
X x;
28+
29+
__builtin_debugtrap();
30+
__builtin_debugtrap();
31+
return 0;
32+
}
33+
34+
// CHECK-LABEL: expr int $y = 11
35+
// CHECK-LABEL: expr $y
36+
// CHECK: (int) $y = 11
37+
38+
// CHECK-LABEL: expr $y = 100
39+
// CHECK: (int) $0 = 100
40+
41+
// CHECK-LABEL: expr $y
42+
// CHECK: (int) $y = 100
43+
44+
// CHECK-LABEL: continue
45+
// CHECK-LABEL: expr $y
46+
// CHECK: (int) $y = 100
47+
48+
// CHECK-LABEL: expr X $mine = {100, 200}
49+
// CHECK-LABEL: expr $mine.a = 300
50+
// CHECK: (int) $1 = 300
51+
// CHECK-LABEL: expr $mine
52+
// CHECK: (X) $mine = (a = 300, b = 200)

0 commit comments

Comments
 (0)