[lldb] Do not bump memory modificator ID when "internal" debugger memory is updated

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;
 

lldb/include/lldb/Target/Process.h

@@ -111,6 +111,7 @@ class ProcessProperties : public Properties {
   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 @@ class ProcessModID {
     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;

lldb/source/Commands/CommandObjectProcess.cpp

@@ -1827,6 +1827,33 @@ class CommandObjectMultiwordProcessTrace : public CommandObjectMultiword {
   ~CommandObjectMultiwordProcessTrace() override = default;
 };
 
+// CommandObjectProcessDumpModificationId
+class CommandObjectProcessDumpModificationId : public CommandObjectParsed {
+public:
+  CommandObjectProcessDumpModificationId(CommandInterpreter &interpreter)
+      : CommandObjectParsed(interpreter, "process dump-modification-id",
+                            "Dump the state of the ProcessModID. Intended to "
+                            "be used for debugging LLDB itself.",
+                            "process dump-modification-id",
+                            eCommandRequiresProcess) {}
+
+  ~CommandObjectProcessDumpModificationId() override = default;
+
+protected:
+  void DoExecute(Args &command, CommandReturnObject &result) override {
+    Process *process = m_exe_ctx.GetProcessPtr();
+    StateType state = process->GetState();
+    if (state != eStateStopped) {
+      result.SetStatus(eReturnStatusFailed);
+      return;
+    }
+
+    ProcessModID process_mod_id = process->GetModID();
+    process_mod_id.Dump(result.GetOutputStream());
+    result.SetStatus(eReturnStatusSuccessFinishResult);
+  }
+};
+
 // CommandObjectMultiwordProcess
 
 CommandObjectMultiwordProcess::CommandObjectMultiwordProcess(
@@ -1866,6 +1893,9 @@ CommandObjectMultiwordProcess::CommandObjectMultiwordProcess(
   LoadSubCommand(
       "trace",
       CommandObjectSP(new CommandObjectMultiwordProcessTrace(interpreter)));
+  LoadSubCommand(
+      "dump-modification-id",
+      CommandObjectSP(new CommandObjectProcessDumpModificationId(interpreter)));
 }
 
 CommandObjectMultiwordProcess::~CommandObjectMultiwordProcess() = default;

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);
+  });
+}

lldb/source/Target/Process.cpp

@@ -369,6 +369,12 @@ FollowForkMode ProcessProperties::GetFollowForkMode() const {
                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,
@@ -2267,6 +2273,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 +2286,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 (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
@@ -2408,7 +2420,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) {

lldb/source/Target/TargetProperties.td

@@ -296,6 +296,9 @@ let Definition = "process" in {
     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, but convenience variables won't reevaluate synthetic children automatically.">;
 }
 
 let Definition = "platform" in {

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

@@ -905,6 +905,7 @@ def test_all_settings_exist(self):
                 "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",
             ],

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)

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)

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)

lldb/test/Shell/Expr/TestProcessModificationIdOnExpr.cpp

@@ -0,0 +1,67 @@
+// Tests that ProcessModID.m_memory_id is not bumped when evaluating expressions without side effects.
+
+// REQUIRES: target-windows
+// Due to different implementations exact numbers (m_stop_id) are different on different OSs. So we lock this test to specific platform.
+
+// 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 dump-modification-id" \
+// RUN:   -o "expr x.i != 42" \
+// RUN:   -o "process dump-modification-id" \
+// RUN:   -o "expr x.get()" \
+// RUN:   -o "process dump-modification-id" \
+// RUN:   -o "expr x.i = 10" \
+// RUN:   -o "process dump-modification-id" \
+// RUN:   -o "continue" \
+// RUN:   -o "process dump-modification-id" \
+// 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 dump-modification-id
+// 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 dump-modification-id
+// 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
+
+// CHECK-LABEL: process dump-modification-id
+// 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 dump-modification-id
+// 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 dump-modification-id
+// CHECK: m_stop_id: 4
+// CHECK: m_memory_id: 2