[Lldb-commits] [PATCH] D138724: [lldb][Target] Flush the scratch TypeSystem when process gets deleted

[Michael Buch via Phabricator via lldb-commits]

Michael137 added inline comments.


================
Comment at: lldb/source/Target/Target.cpp:207
+    // E.g., this could happen on rebuild & relaunch
+    // of the debugee.
+    m_scratch_type_system_map.Clear();
----------------
aprantl wrote:
> Not opposed to this, but this is leaking an implementation detail of TypeSystemClang into Target. Just out of curiosity — would if be feasible to use weak_ptr in DeclOrigin or is that defined inside of Clang and can't be changed?
I agree with your point about leaking TypeSystem implementation details into Process lifecycle. I briefly considered converting `DeclOrigin`s to hold weak pointers but unfortunately the pointers are handed out by `clang::Decl`. Though thinking about it, `clang::ASTContext`s are refcounted (via `llvm::RefCountedBase`). So perhaps we could keep them alive that way. Though I’m concerned that we then commit to supporting multiple definitions of a type in the scratch AST and never really properly clearing the `DeclOrigin` structures. But I’ll play around with the idea


================
Comment at: lldb/source/Target/Target.cpp:208
+    // of the debugee.
+    m_scratch_type_system_map.Clear();
     m_process_sp.reset();
----------------
kastiglione wrote:
> Do we have some place in the life-cycle where we can perform this only if the target has changed? Ideally this would happen when the binary has a different timestamp, or for mach-o a different UUID.
There is `DidUnloadModules` which gets notified when an lldb_private::Module gets unloaded (e.g., on rebuilt). But this includes JITted modules (e.g., when running AppleObjectiveCRuntimeV2 utility functions) in which case we wouldn’t want to flush the type systems. Also the Clang REPL (and I assume the Swift REPL) rely on the type system being still present after we unloaded the module associated with the evaluated expression. All this is to say, I found it to be quite fiddly to determine when to flush the persistent variables from within that notification. Really we would like a notification to the Target which says “we restarted the debugee AND some module got rebuilt”. In that case it’s not safe to keep the persistent variables around. I’ll double check if there isn’t something like that around.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D138724/new/

https://reviews.llvm.org/D138724