Michael137 created this revision.
Michael137 added reviewers: jingham, aprantl.
Herald added a project: All.
Michael137 requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.
**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.) that were created for
the process.
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
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D138724
Files:
lldb/source/Target/Target.cpp
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
Index: lldb/test/API/functionalities/rerun_and_expr/rebuild.cpp
===================================================================
--- /dev/null
+++ 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;
+}
Index: lldb/test/API/functionalities/rerun_and_expr/main.cpp
===================================================================
--- /dev/null
+++ 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;
+}
Index: lldb/test/API/functionalities/rerun_and_expr/TestRerunAndExpr.py
===================================================================
--- /dev/null
+++ lldb/test/API/functionalities/rerun_and_expr/TestRerunAndExpr.py
@@ -0,0 +1,71 @@
+"""
+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-NEXT: | |-DefinitionData pass_in_registers standard_layout trivially_copyable trivial literal
+ # CHECK-NEXT: | | |-DefaultConstructor exists trivial needs_implicit
+ # CHECK-NEXT: | | |-CopyConstructor simple trivial has_const_param needs_implicit implicit_has_const_param
+ # CHECK-NEXT: | | |-MoveConstructor exists simple trivial needs_implicit
+ # CHECK-NEXT: | | |-CopyAssignment simple trivial has_const_param needs_implicit implicit_has_const_param
+ # CHECK-NEXT: | | |-MoveAssignment exists simple trivial needs_implicit
+ # CHECK-NEXT: | | `-Destructor simple irrelevant trivial needs_implicit
+ # CHECK-NEXT: | |-public 'Base'
+ # CHECK-NEXT: | `-FieldDecl {{.*}} m_derived_val 'int'
+ # CHECK-NEXT: `-CXXRecordDecl {{.*}} struct Base definition
+ # CHECK-NEXT: |-DefinitionData pass_in_registers aggregate standard_layout trivially_copyable pod trivial literal
+ # CHECK-NEXT: | |-DefaultConstructor exists trivial needs_implicit
+ # CHECK-NEXT: | |-CopyConstructor simple trivial has_const_param needs_implicit implicit_has_const_param
+ # CHECK-NEXT: | |-MoveConstructor exists simple trivial needs_implicit
+ # CHECK-NEXT: | |-CopyAssignment simple trivial has_const_param needs_implicit implicit_has_const_param
+ # CHECK-NEXT: | |-MoveAssignment exists simple trivial needs_implicit
+ # CHECK-NEXT: | `-Destructor simple irrelevant trivial needs_implicit
+
+ # ...but the original definition of 'struct Foo' is not in the scratch AST anymore
+ # CHECK-NOT: FieldDecl {{.*}} m_val 'int'
+
Index: lldb/test/API/functionalities/rerun_and_expr/Makefile
===================================================================
--- /dev/null
+++ lldb/test/API/functionalities/rerun_and_expr/Makefile
@@ -0,0 +1 @@
+include Makefile.rules
Index: lldb/source/Target/Target.cpp
===================================================================
--- lldb/source/Target/Target.cpp
+++ lldb/source/Target/Target.cpp
@@ -200,6 +200,10 @@
CleanupProcess();
+ // Flush the scratch 'TypeSystem's because
+ // we may have stale DeclOrigin's in the
+ // scratch ClangASTImporter.
+ m_scratch_type_system_map.Clear();
m_process_sp.reset();
}
}
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
