[Lldb-commits] [lldb] 7866b91 - [lldb] Fix a crash when the ASTImporter is giving us two Imported callbacks for the same target decl

[Raphael Isemann via lldb-commits]

Author: Raphael Isemann
Date: 2020-09-09T10:31:39+02:00
New Revision: 7866b91405693df5b4cf6ba770b3a92d48b0c508

URL: https://github.com/llvm/llvm-project/commit/7866b91405693df5b4cf6ba770b3a92d48b0c508
DIFF: https://github.com/llvm/llvm-project/commit/7866b91405693df5b4cf6ba770b3a92d48b0c508.diff

LOG: [lldb] Fix a crash when the ASTImporter is giving us two Imported callbacks for the same target decl

The ASTImporter has an `Imported(From, To)` callback that notifies subclasses
that a declaration has been imported in some way. LLDB uses this in the
`CompleteTagDeclsScope` to see which records have been imported into the scratch
context. If the record was declared inside the expression, then the
`CompleteTagDeclsScope` will forcibly import the full definition of that record
to the scratch context so that the expression AST can safely be disposed later
(otherwise we might end up going back to the deleted AST to complete the
minimally imported record). The way this is implemented is that there is a list
of decls that need to be imported (`m_decls_to_complete`) and we keep completing
the declarations inside that list until the list is empty. Every `To` Decl we
get via the `Imported` callback will be added to the list of Decls to be
completed.

There are some situations where the ASTImporter will actually give us two
`Imported` calls with the same `To` Decl. One way where this happens is if the
ASTImporter decides to merge an imported definition into an already imported
one. Another way is that the ASTImporter just happens to get two calls to
`ASTImporter::Import` for the same Decl. This for example happens when importing
the DeclContext of a Decl requires importing the Decl itself, such as when
importing a RecordDecl that was declared inside a function.

The bug addressed in this patch is that when we end up getting two `Imported`
calls for the same `To` Decl, then we would crash in the
`CompleteTagDeclsScope`.  That's because the first time we complete the Decl we
remove the Origin tracking information (that maps the Decl back to from where it
came from). The next time we try to complete the same `To` Decl the Origin
tracking information is gone and we hit the `to_context_md->getOrigin(decl).ctx
== m_src_ctx` assert (`getOrigin(decl).ctx` is a nullptr the second time as the
Origin was deleted).

This is actually a regression coming from D72495. Before D72495
`m_decls_to_complete` was actually a set so every declaration in there could
only be queued once to be completed. The set was changed to a vector to make the
iteration over it deterministic, but that also causes that we now potentially
end up trying to complete a Decl twice.

This patch essentially just reverts D72495 and makes the `CompleteTagDeclsScope`
use a SetVector for the list of declarations to be completed. The SetVector
should filter out the duplicates (as the original `set` did) and also ensure that
the completion order is deterministic. I actually couldn't find any way to cause
LLDB to reproduce this bug by merging declarations (this would require that we
for example declare two namespaces in a non-top-level expression which isn't
possible). But the bug reproduces very easily by just declaring a class in an
expression, so that's what the test is doing.

Reviewed By: shafik

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

Added: 
    lldb/test/API/lang/c/record_decl_in_expr/TestRecordDeclInExpr.py

Modified: 
    lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp

Removed: 
    


################################################################################
diff  --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp b/lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp
index 73042c205a5a..e2601a059bb7 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp
+++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp
@@ -216,7 +216,12 @@ namespace {
 /// imported while completing the original Decls).
 class CompleteTagDeclsScope : public ClangASTImporter::NewDeclListener {
   ClangASTImporter::ImporterDelegateSP m_delegate;
-  llvm::SmallVector<NamedDecl *, 32> m_decls_to_complete;
+  /// List of declarations in the target context that need to be completed.
+  /// Every declaration should only be completed once and therefore should only
+  /// be once in this list.
+  llvm::SetVector<NamedDecl *> m_decls_to_complete;
+  /// Set of declarations that already were successfully completed (not just
+  /// added to m_decls_to_complete).
   llvm::SmallPtrSet<NamedDecl *, 32> m_decls_already_completed;
   clang::ASTContext *m_dst_ctx;
   clang::ASTContext *m_src_ctx;
@@ -244,6 +249,9 @@ class CompleteTagDeclsScope : public ClangASTImporter::NewDeclListener {
       NamedDecl *decl = m_decls_to_complete.pop_back_val();
       m_decls_already_completed.insert(decl);
 
+      // The decl that should be completed has to be imported into the target
+      // context from some other context.
+      assert(to_context_md->hasOrigin(decl));
       // We should only complete decls coming from the source context.
       assert(to_context_md->getOrigin(decl).ctx == m_src_ctx);
 
@@ -287,7 +295,8 @@ class CompleteTagDeclsScope : public ClangASTImporter::NewDeclListener {
     // Check if we already completed this type.
     if (m_decls_already_completed.count(to_named_decl) != 0)
       return;
-    m_decls_to_complete.push_back(to_named_decl);
+    // Queue this type to be completed.
+    m_decls_to_complete.insert(to_named_decl);
   }
 };
 } // namespace

diff  --git a/lldb/test/API/lang/c/record_decl_in_expr/TestRecordDeclInExpr.py b/lldb/test/API/lang/c/record_decl_in_expr/TestRecordDeclInExpr.py
new file mode 100644
index 000000000000..16bf098dce8f
--- /dev/null
+++ b/lldb/test/API/lang/c/record_decl_in_expr/TestRecordDeclInExpr.py
@@ -0,0 +1,34 @@
+"""
+Tests declaring RecordDecls in non-top-level expressions.
+"""
+
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class TestCase(TestBase):
+
+    mydir = TestBase.compute_mydir(__file__)
+
+    @no_debug_info_test
+    def test_fwd_decl(self):
+        # Declare a forward decl and import it to the scratch AST.
+        self.expect_expr("struct S; S *s = nullptr; s", result_type="S *")
+
+    @no_debug_info_test
+    def test_struct(self):
+        # Declare a struct and import it to the scratch AST.
+        self.expect("expr struct S {}; S s; s", substrs=["= {}"])
+
+    @no_debug_info_test
+    def test_struct_with_fwd_decl(self):
+        # Import the forward decl to the scratch AST.
+        self.expect_expr("struct S; S *s = nullptr; s", result_type="S *")
+        # Merge the definition into the scratch AST.
+        self.expect("expr struct S {}; S s; s", substrs=["= {}"])
+
+    @no_debug_info_test
+    def test_struct_with_fwd_decl_same_expr(self):
+        # Test both a forward decl and a definition in one expression and
+        # import them into the scratch AST.
+        self.expect("expr struct S; struct S{}; S s; s", substrs=["= {}"])