[Lldb-commits] [lldb] 4df1139 - [lldb][Target] Flush the scratch TypeSystem when owning lldb_private::Module gets unloaded
Michael Buch via lldb-commits
lldb-commits at lists.llvm.org
Fri Dec 2 02:53:11 PST 2022
Author: Michael Buch
Date: 2022-12-02T10:52:41Z
New Revision: 4df11394a10b3b15d2fb9bde8b831cf68785aa45
URL: https://github.com/llvm/llvm-project/commit/4df11394a10b3b15d2fb9bde8b831cf68785aa45
DIFF: https://github.com/llvm/llvm-project/commit/4df11394a10b3b15d2fb9bde8b831cf68785aa45.diff
LOG: [lldb][Target] Flush the scratch TypeSystem when owning lldb_private::Module gets unloaded
**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
Added:
lldb/test/API/functionalities/rerun_and_expr/Makefile
lldb/test/API/functionalities/rerun_and_expr/TestRerunAndExpr.py
lldb/test/API/functionalities/rerun_and_expr/main.cpp
lldb/test/API/functionalities/rerun_and_expr/rebuild.cpp
lldb/test/API/functionalities/rerun_and_expr_dylib/Makefile
lldb/test/API/functionalities/rerun_and_expr_dylib/TestRerunAndExprDylib.py
lldb/test/API/functionalities/rerun_and_expr_dylib/lib.cpp
lldb/test/API/functionalities/rerun_and_expr_dylib/main.cpp
lldb/test/API/functionalities/rerun_and_expr_dylib/rebuild.cpp
Modified:
lldb/source/Target/Target.cpp
Removed:
################################################################################
diff --git a/lldb/source/Target/Target.cpp b/lldb/source/Target/Target.cpp
index 22071b76a805b..8a197bf6e502a 100644
--- 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
index 0000000000000..22f1051530f87
--- /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
index 0000000000000..19907ce62de69
--- /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
index 0000000000000..a19dac03ffa98
--- /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
index 0000000000000..745a2ce954e89
--- /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
index 0000000000000..99998b20bcb05
--- /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
index 0000000000000..df75cbeba01fa
--- /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
index 0000000000000..aad9816a4be27
--- /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
index 0000000000000..144e8bac699b5
--- /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
index 0000000000000..b9d835184dde8
--- /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;
More information about the lldb-commits
mailing list