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

[teemperor]

Summary

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
again for the list of declarations to be completed. The SetVector should filter out the duplicates
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.

[teemperor]

FWIW, the case where we get two Imported calls when importing a RecordDecl inside a FunctionDecl seems a bit like a bug. But the only way I see how this could be fixed is by making the ASTImporter keep track when it actually created a new decl in an ASTImporter::Import(Decl *) call. I'll make a follow up patch for that.

[shafik]

LGTM, it is sad we lose the deterministic ordering though. Would it be efficient enough to use a vector but keep the set to prevent duplicates?

[teemperor]

In D85648#2216345, @shafik wrote:

LGTM, it is sad we lose the deterministic ordering though. Would it be efficient enough to use a vector but keep the set to prevent duplicates?

I think that's just a mistake in the review description. Actually the SetVector is deterministic and a set, so we won't reintroduce the non-determinism. (Originally this was a set, then we changed it to a vector to fix the non-determinism and now it's a SetVector which has both wanted properties).

[teemperor]

• Clarify that this isn't just a direct revert, but reverts by establishing a set again instead of just a plain vector (Thanks Shafik!)

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

 namespace {
 /// Completes all imported TagDecls at the end of the scope.
 ///
 /// While in a CompleteTagDeclsScope, every decl that could be completed will
 /// be completed at the end of the scope (including all Decls that are
 /// 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;
   ClangASTImporter &importer;
 
 public:
   /// Constructs a CompleteTagDeclsScope.
   /// \param importer The ClangASTImporter that we should observe.
virtual ~CompleteTagDeclsScope() {
     ClangASTImporter::ASTContextMetadataSP to_context_md =
         importer.GetContextMetadata(m_dst_ctx);
 
     // Complete all decls we collected until now.
     while (!m_decls_to_complete.empty()) {
       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);
 
       Decl *original_decl = to_context_md->getOrigin(decl).decl;
 
       // Complete the decl now.
       TypeSystemClang::GetCompleteDecl(m_src_ctx, original_decl);
       if (auto *tag_decl = dyn_cast<TagDecl>(decl)) {
void NewDeclImported(clang::Decl *from, clang::Decl *to) override {
     // We don't need to complete injected class name decls.
     if (from_record_decl && from_record_decl->isInjectedClassName())
       return;
 
     NamedDecl *to_named_decl = dyn_cast<NamedDecl>(to);
     // 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
 
 CompilerType ClangASTImporter::DeportType(TypeSystemClang &dst,
                                           const CompilerType &src_type) {
   Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_EXPRESSIONS));
 

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

+"""
+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=["= {}"])