[lldb][Target] Flush the scratch TypeSystem when owning lldb_private::Module gets...

**Summary**

This patch addresses #59128, where LLDB would crash when evaluating
importing a type that has been imported before into the same target.
The proposed solution is to clear the scratch AST (and associated
persistent variables, `ClangASTImporter`, etc.) whenever a module that
could've owned one of the stale `TypeSystem`s gets unloaded/destroyed.

Details:
1. The first time we evaluate the expression we import the decl for Foo into the Targets scratch AST
   context (lives in m_scratch_type_system_map). During this process we also create a ClangASTImporter
   that lives in the ClangPersistentVariables::m_ast_importer_sp. This importer has decl tracking
   structures which reference the source AST that the decl got imported from. This importer also gets
   re-used for all calls to DeportType (which we use to copy the final decl into the Targets scratch AST).
2. Rebuilding the executable triggers a tear-down of the Module that was backing the ASTContext that
   we originally got the Foo decl from (which lived in the Module::m_type_system_map). However, the Target’s scratch AST lives on.
3. Re-running the same expression will now create a new ASTImporterDelegate where the destination TranslationUnitDecl is
   the same as the one from step (1).
4. When importing the new Foo decl we first try to find it in the destination DeclContext, which happens to be
   the scratch destination TranslationUnitDecl. The `Foo` decl exists in this context since we copied it into
   the scratch AST in the first run. The ASTImporter then queries LLDB for the origin of that decl. Using the
   same persistent variable ClangASTImporter we claim the decl has an origin in the AST context that got torn
   down with the Module. This faulty origin leads to a use-after-free.

**Testing**

- Added API test

Differential Revision: https://reviews.llvm.org/D138724

diff --git a/lldb/source/Target/Target.cpp b/lldb/source/Target/Target.cpp
index 22071b7..8a197bf 100644 (file)
--- a/lldb/source/Target/Target.cpp
+++ b/lldb/source/Target/Target.cpp
@@ -1682,6 +1682,30 @@ void Target::ModulesDidUnload(ModuleList &module_list, bool delete_locations) {
     m_breakpoint_list.UpdateBreakpoints(module_list, false, delete_locations);
     m_internal_breakpoint_list.UpdateBreakpoints(module_list, false,
                                                  delete_locations);
+
+    // If a module was torn down it will have torn down the 'TypeSystemClang's
+    // that we used as source 'ASTContext's for the persistent variables in
+    // the current target. Those would now be unsafe to access because the
+    // 'DeclOrigin' are now possibly stale. Thus clear all persistent
+    // variables. We only want to flush 'TypeSystem's if the module being
+    // unloaded was capable of describing a source type. JITted module unloads
+    // happen frequently for Objective-C utility functions or the REPL and rely
+    // on the persistent variables to stick around.
+    const bool should_flush_type_systems =
+        module_list.AnyOf([](lldb_private::Module &module) {
+          auto *object_file = module.GetObjectFile();
+
+          if (!object_file)
+            return true;
+
+          auto type = object_file->GetType();
+
+          return type == ObjectFile::eTypeObjectFile ||
+                 type == ObjectFile::eTypeExecutable;
+        });
+
+    if (should_flush_type_systems)
+      m_scratch_type_system_map.Clear();
   }
 }
 

diff --git a/lldb/test/API/functionalities/rerun_and_expr/Makefile b/lldb/test/API/functionalities/rerun_and_expr/Makefile
new file mode 100644 (file)
index 0000000..22f1051
--- /dev/null
+++ b/lldb/test/API/functionalities/rerun_and_expr/Makefile
@@ -0,0 +1 @@
+include Makefile.rules

diff --git a/lldb/test/API/functionalities/rerun_and_expr/TestRerunAndExpr.py b/lldb/test/API/functionalities/rerun_and_expr/TestRerunAndExpr.py
new file mode 100644 (file)
index 0000000..19907ce
--- /dev/null
+++ b/lldb/test/API/functionalities/rerun_and_expr/TestRerunAndExpr.py
@@ -0,0 +1,57 @@
+"""
+Test that re-running a process from within the same target
+after rebuilding the executable flushes the scratch TypeSystems
+tied to that process.
+"""
+
+import lldb
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class TestRerun(TestBase):
+    def test(self):
+        """
+        Tests whether re-launching a process without destroying
+        the owning target keeps invalid ASTContexts in the
+        scratch AST's importer.
+
+        We test this by:
+        1. Evaluating an expression to import 'struct Foo' into
+           the scratch AST
+        2. Change the definition of 'struct Foo' and rebuild the executable
+        3. Re-launch the process
+        4. Evaluate the same expression in (1). We expect to have only
+           the latest definition of 'struct Foo' in the scratch AST.
+        """
+        self.build(dictionary={'CXX_SOURCES':'main.cpp', 'EXE':'a.out'})
+        (target, _, _, bkpt) = \
+                lldbutil.run_to_source_breakpoint(self, 'return', lldb.SBFileSpec('main.cpp'))
+
+        target.BreakpointCreateBySourceRegex('return', lldb.SBFileSpec('rebuild.cpp', False))
+
+        self.expect_expr('foo', result_type='Foo', result_children=[
+                ValueCheck(name='m_val', value='42')
+            ])
+
+        self.build(dictionary={'CXX_SOURCES':'rebuild.cpp', 'EXE':'a.out'})
+
+        self.runCmd('process launch')
+
+        self.expect_expr('foo', result_type='Foo', result_children=[
+            ValueCheck(name='Base', children=[
+                ValueCheck(name='m_base_val', value='42')
+            ]),
+            ValueCheck(name='m_derived_val', value='137')
+        ])
+
+        self.filecheck("target module dump ast", __file__)
+
+        # The new definition 'struct Foo' is in the scratch AST
+        # CHECK:      |-CXXRecordDecl {{.*}} struct Foo definition
+        # CHECK:      | |-public 'Base'
+        # CHECK-NEXT: | `-FieldDecl {{.*}} m_derived_val 'int'
+        # CHECK-NEXT: `-CXXRecordDecl {{.*}} struct Base definition
+
+        # ...but the original definition of 'struct Foo' is not in the scratch AST anymore
+        # CHECK-NOT: FieldDecl {{.*}} m_val 'int'
+

diff --git a/lldb/test/API/functionalities/rerun_and_expr/main.cpp b/lldb/test/API/functionalities/rerun_and_expr/main.cpp
new file mode 100644 (file)
index 0000000..a19dac0
--- /dev/null
+++ b/lldb/test/API/functionalities/rerun_and_expr/main.cpp
@@ -0,0 +1,8 @@
+struct Foo {
+  int m_val = 42;
+};
+
+int main() {
+  Foo foo;
+  return 0;
+}

diff --git a/lldb/test/API/functionalities/rerun_and_expr/rebuild.cpp b/lldb/test/API/functionalities/rerun_and_expr/rebuild.cpp
new file mode 100644 (file)
index 0000000..745a2ce
--- /dev/null
+++ b/lldb/test/API/functionalities/rerun_and_expr/rebuild.cpp
@@ -0,0 +1,12 @@
+struct Base {
+  int m_base_val = 42;
+};
+
+struct Foo : public Base {
+  int m_derived_val = 137;
+};
+
+int main() {
+  Foo foo;
+  return 0;
+}

diff --git a/lldb/test/API/functionalities/rerun_and_expr_dylib/Makefile b/lldb/test/API/functionalities/rerun_and_expr_dylib/Makefile
new file mode 100644 (file)
index 0000000..99998b2
--- /dev/null
+++ b/lldb/test/API/functionalities/rerun_and_expr_dylib/Makefile
@@ -0,0 +1,3 @@
+CXX_SOURCES := main.cpp
+
+include Makefile.rules

diff --git a/lldb/test/API/functionalities/rerun_and_expr_dylib/TestRerunAndExprDylib.py b/lldb/test/API/functionalities/rerun_and_expr_dylib/TestRerunAndExprDylib.py
new file mode 100644 (file)
index 0000000..df75cbe
--- /dev/null
+++ b/lldb/test/API/functionalities/rerun_and_expr_dylib/TestRerunAndExprDylib.py
@@ -0,0 +1,72 @@
+"""
+Test that re-running a process from within the same target
+after rebuilding the a dynamic library flushes the scratch
+TypeSystems tied to that process.
+"""
+
+import lldb
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class TestRerun(TestBase):
+    def test(self):
+        """
+        Tests whether re-launching a process without destroying
+        the owning target keeps invalid ASTContexts in the
+        scratch AST's importer.
+
+        We test this by:
+        1. Evaluating an expression to import 'struct Foo' into
+           the scratch AST
+        2. Change the definition of 'struct Foo' and rebuild the dylib
+        3. Re-launch the process
+        4. Evaluate the same expression in (1). We expect to have only
+           the latest definition of 'struct Foo' in the scratch AST.
+        """
+
+        # Build a.out
+        self.build(dictionary={'EXE':'a.out',
+                               'CXX_SOURCES':'main.cpp'})
+
+        # Build libfoo.dylib
+        self.build(dictionary={'DYLIB_CXX_SOURCES':'lib.cpp',
+                               'DYLIB_ONLY':'YES',
+                               'DYLIB_NAME':'foo',
+                               'USE_LIBDL':'1',
+                               'LD_EXTRAS':'-L.'})
+
+        (target, _, _, bkpt) = \
+                lldbutil.run_to_source_breakpoint(self, 'return', lldb.SBFileSpec('main.cpp'))
+
+        self.expect_expr('*foo', result_type='Foo', result_children=[
+                ValueCheck(name='m_val', value='42')
+            ])
+
+        # Re-build libfoo.dylib
+        self.build(dictionary={'DYLIB_CXX_SOURCES':'rebuild.cpp',
+                               'DYLIB_ONLY':'YES',
+                               'DYLIB_NAME':'foo',
+                               'USE_LIBDL':'1',
+                               'LD_EXTRAS':'-L.'})
+
+        self.runCmd('process launch')
+        (target, _, _, bkpt) = \
+                lldbutil.run_to_source_breakpoint(self, 'return', lldb.SBFileSpec('main.cpp'))
+
+        self.expect_expr('*foo', result_type='Foo', result_children=[
+            ValueCheck(name='Base', children=[
+                ValueCheck(name='m_base_val', value='42')
+            ]),
+            ValueCheck(name='m_derived_val', value='137')
+        ])
+
+        self.filecheck("target module dump ast", __file__)
+
+        # The new definition 'struct Foo' is in the scratch AST
+        # CHECK:      |-CXXRecordDecl {{.*}} struct Foo definition
+        # CHECK:      | |-public 'Base'
+        # CHECK-NEXT: | `-FieldDecl {{.*}} m_derived_val 'int'
+        # CHECK-NEXT: `-CXXRecordDecl {{.*}} struct Base definition
+
+        # ...but the original definition of 'struct Foo' is not in the scratch AST anymore
+        # CHECK-NOT: FieldDecl {{.*}} m_val 'int'

diff --git a/lldb/test/API/functionalities/rerun_and_expr_dylib/lib.cpp b/lldb/test/API/functionalities/rerun_and_expr_dylib/lib.cpp
new file mode 100644 (file)
index 0000000..aad9816
--- /dev/null
+++ b/lldb/test/API/functionalities/rerun_and_expr_dylib/lib.cpp
@@ -0,0 +1 @@
+LLDB_DYLIB_EXPORT struct Foo { int m_val = 42; } global_foo;

diff --git a/lldb/test/API/functionalities/rerun_and_expr_dylib/main.cpp b/lldb/test/API/functionalities/rerun_and_expr_dylib/main.cpp
new file mode 100644 (file)
index 0000000..144e8ba
--- /dev/null
+++ b/lldb/test/API/functionalities/rerun_and_expr_dylib/main.cpp
@@ -0,0 +1,12 @@
+#include <cassert>
+#include <dlfcn.h>
+
+extern struct Foo imported;
+
+int main() {
+  void *handle = dlopen("libfoo.dylib", RTLD_NOW);
+  struct Foo *foo = (struct Foo *)dlsym(handle, "global_foo");
+  assert(foo != nullptr);
+
+  return 0;
+}

diff --git a/lldb/test/API/functionalities/rerun_and_expr_dylib/rebuild.cpp b/lldb/test/API/functionalities/rerun_and_expr_dylib/rebuild.cpp
new file mode 100644 (file)
index 0000000..b9d8351
--- /dev/null
+++ b/lldb/test/API/functionalities/rerun_and_expr_dylib/rebuild.cpp
@@ -0,0 +1,7 @@
+struct Base {
+  int m_base_val = 42;
+};
+
+LLDB_DYLIB_EXPORT struct Foo : public Base {
+  int m_derived_val = 137;
+} global_foo;