[lldb][WIP] Use forward decls with redeclared definitions instead of minimal import for records

This patch is rewriting the way we move Clang types between different ASTS in Clang.

The motivation is that the current approach for 'completing types' in LLDB just doesn't work. We have all kinds of situations where we either have Clang crash due to for example having a Record without DefinitionData but with FieldDecls in it.

The reason for that is that at the moment we create record types ([CXX]RecordDecls and ObjCInterfaceDecls) and we always pretend that a type potentially has a definition that somehow could be lazily pulled in. However, Clang doesn't have any interface (at least none that is consistently called) that turns a RecordDecl without DefinitionData into a RecordDecl with DefinitionData. The only relevant API in the ExternalASTSource is CompleteType is suffering from the fact that it's essentially not used by generic Clang code.

The part of the ExternalASTSource API that is consistently called to pull in a potential definition is CompleteRedeclChain (which is for example automatically called when calling getDefinition on a RecordDecl). The problem with CompleteRedeclChain however is that it's not supposed to add definition data to the Decl its called on, but instead it should provide a redeclaration that is defining the record. That's a very different system than what we currently have and we can't just swap it out under the hood.

To make it short: We probably need to rewrite that part of LLDB.

So the solution here is essentially rewriting all our completion code to do this:

* Instead of creating these weirdly defined records that have fields but maybe not definition and so on, we only create forward decls at the start.
* We then bump the GenerationCounter of the ExternalASTSource of the current AST to force rebuilding of the redeclaration chain.
* When Clang asks for the definition of the record, it will ask the ExternalASTSource to complete the redeclaration chain. At this point we build the definition and add it as a second decl.

The ASTImporter can now also stop using the minimal mode for records as there is no reason anymore. It just imports first the forward declaration and then the definition when needed.

Author: Michael137

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

@@ -6,6 +6,7 @@
 //
 //===----------------------------------------------------------------------===//
 
+#include "Plugins/TypeSystem/Clang/ImporterBackedASTSource.h"
 #include "lldb/Core/Module.h"
 #include "lldb/Utility/LLDBAssert.h"
 #include "lldb/Utility/LLDBLog.h"
@@ -17,6 +18,8 @@
 #include "clang/AST/RecordLayout.h"
 #include "clang/Sema/Lookup.h"
 #include "clang/Sema/Sema.h"
+#include "llvm/Support/Casting.h"
+#include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/raw_ostream.h"
 
 #include "Plugins/ExpressionParser/Clang/ClangASTImporter.h"
@@ -230,35 +233,6 @@ class CompleteTagDeclsScope : public ClangASTImporter::NewDeclListener {
   clang::ASTContext *m_src_ctx;
   ClangASTImporter &importer;
 
-  void CompleteDecl(
-      Decl *decl,
-      lldb_private::ClangASTImporter::ASTContextMetadata const &to_context_md) {
-    // 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)) {
-      if (auto *original_tag_decl = dyn_cast<TagDecl>(original_decl)) {
-        if (original_tag_decl->isCompleteDefinition()) {
-          m_delegate->ImportDefinitionTo(tag_decl, original_tag_decl);
-          tag_decl->setCompleteDefinition(true);
-        }
-      }
-
-      tag_decl->setHasExternalLexicalStorage(false);
-      tag_decl->setHasExternalVisibleStorage(false);
-    } else if (auto *container_decl = dyn_cast<ObjCContainerDecl>(decl)) {
-      container_decl->setHasExternalLexicalStorage(false);
-      container_decl->setHasExternalVisibleStorage(false);
-    }
-  }
-
 public:
   /// Constructs a CompleteTagDeclsScope.
   /// \param importer The ClangASTImporter that we should observe.
@@ -280,9 +254,6 @@ class CompleteTagDeclsScope : public ClangASTImporter::NewDeclListener {
     while (!m_decls_to_complete.empty()) {
       NamedDecl *decl = m_decls_to_complete.pop_back_val();
       m_decls_already_completed.insert(decl);
-
-      CompleteDecl(decl, *to_context_md);
-
       to_context_md->removeOrigin(decl);
     }
 
@@ -296,6 +267,8 @@ class CompleteTagDeclsScope : public ClangASTImporter::NewDeclListener {
     if (!isa<TagDecl>(to) && !isa<ObjCInterfaceDecl>(to))
       return;
 
+    to = ClangUtil::GetFirstDecl(to);
+
     RecordDecl *from_record_decl = dyn_cast<RecordDecl>(from);
     // We don't need to complete injected class name decls.
     if (from_record_decl && from_record_decl->isInjectedClassName())
@@ -510,14 +483,7 @@ bool ClangASTImporter::CompleteType(const CompilerType &compiler_type) {
   if (!CanImport(compiler_type))
     return false;
 
-  if (Import(compiler_type)) {
-    TypeSystemClang::CompleteTagDeclarationDefinition(compiler_type);
-    return true;
-  }
-
-  TypeSystemClang::SetHasExternalStorage(compiler_type.GetOpaqueQualType(),
-                                         false);
-  return false;
+  return Import(compiler_type);
 }
 
 /// Copy layout information from \ref source_map to the \ref destination_map.
@@ -666,8 +632,8 @@ bool ClangASTImporter::importRecordLayoutFromOrigin(
 
   int field_idx = 0, field_count = record_layout.getFieldCount();
 
-  for (RecordDecl::field_iterator fi = origin_record->field_begin(),
-                                  fe = origin_record->field_end();
+  for (RecordDecl::field_iterator fi = definition->field_begin(),
+                                  fe = definition->field_end();
        fi != fe; ++fi) {
     if (field_idx >= field_count)
       return false; // Layout didn't go well.  Bail out.
@@ -808,16 +774,34 @@ bool ClangASTImporter::CompleteTagDecl(clang::TagDecl *decl) {
   if (!decl_origin.Valid())
     return false;
 
-  if (!TypeSystemClang::GetCompleteDecl(decl_origin.ctx, decl_origin.decl))
+  auto *origin_def = llvm::cast<TagDecl>(decl_origin.decl)->getDefinition();
+  if (!origin_def)
     return false;
 
   ImporterDelegateSP delegate_sp(
       GetDelegate(&decl->getASTContext(), decl_origin.ctx));
 
   ASTImporterDelegate::CxxModuleScope std_scope(*delegate_sp,
                                                 &decl->getASTContext());
-  if (delegate_sp)
-    delegate_sp->ImportDefinitionTo(decl, decl_origin.decl);
+
+  // This is expected to pull in a definition for result_decl (if in redecl
+  // completion mode)
+  llvm::Expected<Decl *> result = delegate_sp->Import(origin_def);
+  if (!result) {
+    llvm::handleAllErrors(result.takeError(),
+                          [](const clang::ASTImportError &e) {
+                            llvm::errs() << "ERR: " << e.toString() << "\n";
+                          });
+    return false;
+  }
+
+  // Create redeclaration chain with the 'to' decls.
+  // Only need to do this if the 'result_decl' is a definition outside
+  // of any redeclaration chain and the input 'decl' was a forward declaration
+  TagDecl *result_decl = llvm::cast<TagDecl>(*result);
+  if (!decl->isThisDeclarationADefinition() && result_decl != decl)
+    if (result_decl->getPreviousDecl() == nullptr)
+      result_decl->setPreviousDecl(decl);
 
   return true;
 }
@@ -838,89 +822,30 @@ bool ClangASTImporter::CompleteObjCInterfaceDecl(
   if (!decl_origin.Valid())
     return false;
 
-  if (!TypeSystemClang::GetCompleteDecl(decl_origin.ctx, decl_origin.decl))
+  ObjCInterfaceDecl *origin_decl =
+      llvm::cast<ObjCInterfaceDecl>(decl_origin.decl);
+
+  origin_decl = origin_decl->getDefinition();
+  if (!origin_decl)
     return false;
 
   ImporterDelegateSP delegate_sp(
       GetDelegate(&interface_decl->getASTContext(), decl_origin.ctx));
 
-  if (delegate_sp)
-    delegate_sp->ImportDefinitionTo(interface_decl, decl_origin.decl);
+  llvm::Expected<Decl *> result = delegate_sp->Import(origin_decl);
+  if (result)
+    return true;
 
-  if (ObjCInterfaceDecl *super_class = interface_decl->getSuperClass())
-    RequireCompleteType(clang::QualType(super_class->getTypeForDecl(), 0));
+  llvm::handleAllErrors(result.takeError(),
+                        [](const clang::ASTImportError &e) {
+                          llvm::errs() << "ERR: " << e.toString() << "\n";
+                        });
 
-  return true;
+  return false;
 }
 
 bool ClangASTImporter::CompleteAndFetchChildren(clang::QualType type) {
-  if (!RequireCompleteType(type))
-    return false;
-
-  Log *log = GetLog(LLDBLog::Expressions);
-
-  if (const TagType *tag_type = type->getAs<TagType>()) {
-    TagDecl *tag_decl = tag_type->getDecl();
-
-    DeclOrigin decl_origin = GetDeclOrigin(tag_decl);
-
-    if (!decl_origin.Valid())
-      return false;
-
-    ImporterDelegateSP delegate_sp(
-        GetDelegate(&tag_decl->getASTContext(), decl_origin.ctx));
-
-    ASTImporterDelegate::CxxModuleScope std_scope(*delegate_sp,
-                                                  &tag_decl->getASTContext());
-
-    TagDecl *origin_tag_decl = llvm::dyn_cast<TagDecl>(decl_origin.decl);
-
-    for (Decl *origin_child_decl : origin_tag_decl->decls()) {
-      llvm::Expected<Decl *> imported_or_err =
-          delegate_sp->Import(origin_child_decl);
-      if (!imported_or_err) {
-        LLDB_LOG_ERROR(log, imported_or_err.takeError(),
-                       "Couldn't import decl: {0}");
-        return false;
-      }
-    }
-
-    if (RecordDecl *record_decl = dyn_cast<RecordDecl>(origin_tag_decl))
-      record_decl->setHasLoadedFieldsFromExternalStorage(true);
-
-    return true;
-  }
-
-  if (const ObjCObjectType *objc_object_type = type->getAs<ObjCObjectType>()) {
-    if (ObjCInterfaceDecl *objc_interface_decl =
-            objc_object_type->getInterface()) {
-      DeclOrigin decl_origin = GetDeclOrigin(objc_interface_decl);
-
-      if (!decl_origin.Valid())
-        return false;
-
-      ImporterDelegateSP delegate_sp(
-          GetDelegate(&objc_interface_decl->getASTContext(), decl_origin.ctx));
-
-      ObjCInterfaceDecl *origin_interface_decl =
-          llvm::dyn_cast<ObjCInterfaceDecl>(decl_origin.decl);
-
-      for (Decl *origin_child_decl : origin_interface_decl->decls()) {
-        llvm::Expected<Decl *> imported_or_err =
-            delegate_sp->Import(origin_child_decl);
-        if (!imported_or_err) {
-          LLDB_LOG_ERROR(log, imported_or_err.takeError(),
-                         "Couldn't import decl: {0}");
-          return false;
-        }
-      }
-
-      return true;
-    }
-    return false;
-  }
-
-  return true;
+  return RequireCompleteType(type);
 }
 
 bool ClangASTImporter::RequireCompleteType(clang::QualType type) {
@@ -1102,6 +1027,20 @@ ClangASTImporter::ASTImporterDelegate::ImportImpl(Decl *From) {
     }
   }
 
+  if (auto *source = llvm::dyn_cast<ImporterBackedASTSource>(
+          getToContext().getExternalSource())) {
+    // We added a new declaration (which is not a definition) into the
+    // destination AST context, so bump the declaration chain generation
+    // counter.
+    if (clang::TagDecl *td = dyn_cast<TagDecl>(From))
+      if (!td->isThisDeclarationADefinition())
+        source->MarkRedeclChainsAsOutOfDate(getToContext());
+
+    if (clang::ObjCInterfaceDecl *td = dyn_cast<ObjCInterfaceDecl>(From))
+      if (!td->isThisDeclarationADefinition())
+        source->MarkRedeclChainsAsOutOfDate(getToContext());
+  }
+
   // If we have a forcefully completed type, try to find an actual definition
   // for it in other modules.
   const ClangASTMetadata *md = m_main.GetDeclMetadata(From);
@@ -1122,6 +1061,12 @@ ClangASTImporter::ASTImporterDelegate::ImportImpl(Decl *From) {
     DeclContext::lookup_result lr = dc->lookup(*dn_or_err);
     for (clang::Decl *candidate : lr) {
       if (candidate->getKind() == From->getKind()) {
+        // If we're dealing with redecl chains, we want to find the definition,
+        // so skip if the decl is actually just a forwad decl.
+        if (auto *tag_decl = llvm::dyn_cast<TagDecl>(candidate);
+            !tag_decl || !tag_decl->getDefinition())
+          continue;
+
         RegisterImportedDecl(From, candidate);
         m_decls_to_ignore.insert(candidate);
         return candidate;
@@ -1223,27 +1168,6 @@ void ClangASTImporter::ASTImporterDelegate::ImportDefinitionTo(
   }
 }
 
-/// Takes a CXXMethodDecl and completes the return type if necessary. This
-/// is currently only necessary for virtual functions with covariant return
-/// types where Clang's CodeGen expects that the underlying records are already
-/// completed.
-static void MaybeCompleteReturnType(ClangASTImporter &importer,
-                                        CXXMethodDecl *to_method) {
-  if (!to_method->isVirtual())
-    return;
-  QualType return_type = to_method->getReturnType();
-  if (!return_type->isPointerType() && !return_type->isReferenceType())
-    return;
-
-  clang::RecordDecl *rd = return_type->getPointeeType()->getAsRecordDecl();
-  if (!rd)
-    return;
-  if (rd->getDefinition())
-    return;
-
-  importer.CompleteTagDecl(rd);
-}
-
 /// Recreate a module with its parents in \p to_source and return its id.
 static OptionalClangModuleID
 RemapModule(OptionalClangModuleID from_id,
@@ -1366,53 +1290,12 @@ void ClangASTImporter::ASTImporterDelegate::Imported(clang::Decl *from,
     to_namespace_decl->setHasExternalVisibleStorage();
   }
 
-  MarkDeclImported(from, to);
-}
-
-void ClangASTImporter::ASTImporterDelegate::MarkDeclImported(Decl *from,
-                                                             Decl *to) {
-  Log *log = GetLog(LLDBLog::Expressions);
-
-  if (auto *to_tag_decl = dyn_cast<TagDecl>(to)) {
-    to_tag_decl->setHasExternalLexicalStorage();
-    to_tag_decl->getPrimaryContext()->setMustBuildLookupTable();
-    auto from_tag_decl = cast<TagDecl>(from);
-
-    LLDB_LOG(
-        log,
-        "    [ClangASTImporter] To is a TagDecl - attributes {0}{1} [{2}->{3}]",
-        (to_tag_decl->hasExternalLexicalStorage() ? " Lexical" : ""),
-        (to_tag_decl->hasExternalVisibleStorage() ? " Visible" : ""),
-        (from_tag_decl->isCompleteDefinition() ? "complete" : "incomplete"),
-        (to_tag_decl->isCompleteDefinition() ? "complete" : "incomplete"));
+  if (clang::ObjCInterfaceDecl *td = dyn_cast<ObjCInterfaceDecl>(to)) {
+    if (clang::ExternalASTSource *s = getToContext().getExternalSource())
+      if (td->isThisDeclarationADefinition())
+        s->CompleteRedeclChain(td);
+    td->setHasExternalVisibleStorage();
   }
-
-  if (auto *to_container_decl = dyn_cast<ObjCContainerDecl>(to)) {
-    to_container_decl->setHasExternalLexicalStorage();
-    to_container_decl->setHasExternalVisibleStorage();
-
-    if (log) {
-      if (ObjCInterfaceDecl *to_interface_decl =
-              llvm::dyn_cast<ObjCInterfaceDecl>(to_container_decl)) {
-        LLDB_LOG(
-            log,
-            "    [ClangASTImporter] To is an ObjCInterfaceDecl - attributes "
-            "{0}{1}{2}",
-            (to_interface_decl->hasExternalLexicalStorage() ? " Lexical" : ""),
-            (to_interface_decl->hasExternalVisibleStorage() ? " Visible" : ""),
-            (to_interface_decl->hasDefinition() ? " HasDefinition" : ""));
-      } else {
-        LLDB_LOG(
-            log, "    [ClangASTImporter] To is an {0}Decl - attributes {1}{2}",
-            ((Decl *)to_container_decl)->getDeclKindName(),
-            (to_container_decl->hasExternalLexicalStorage() ? " Lexical" : ""),
-            (to_container_decl->hasExternalVisibleStorage() ? " Visible" : ""));
-      }
-    }
-  }
-
-  if (clang::CXXMethodDecl *to_method = dyn_cast<CXXMethodDecl>(to))
-    MaybeCompleteReturnType(m_main, to_method);
 }
 
 clang::Decl *

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

@@ -327,9 +327,6 @@ void ClangASTSource::CompleteType(clang::ObjCInterfaceDecl *interface_decl) {
 }
 
 void ClangASTSource::CompleteRedeclChain(const Decl *d) {
-  if (!TypeSystemClang::UseRedeclCompletion())
-    return;
-
   if (const clang::TagDecl *td = llvm::dyn_cast<TagDecl>(d)) {
     if (td->isBeingDefined())
       return;
@@ -402,8 +399,7 @@ clang::ObjCInterfaceDecl *ClangASTSource::GetCompleteObjCInterface(
     return nullptr;
 
   ObjCInterfaceDecl *complete_iface_decl(complete_interface_type->getDecl());
-
-  return complete_iface_decl;
+  return complete_iface_decl->getDefinition();
 }
 
 void ClangASTSource::FindExternalLexicalDecls(

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

@@ -29,9 +29,6 @@ void ClangExternalASTSourceCallbacks::CompleteType(
 
 void ClangExternalASTSourceCallbacks::CompleteRedeclChain(
     const clang::Decl *d) {
-  if (!TypeSystemClang::UseRedeclCompletion())
-    return;
-
   if (const clang::TagDecl *td = llvm::dyn_cast<clang::TagDecl>(d)) {
     if (td->isBeingDefined())
       return;