[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.

(cherry picked from commit b4cfd2d)

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"
@@ -15,6 +16,8 @@
 #include "clang/AST/DeclObjC.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"
@@ -278,7 +281,8 @@ class CompleteTagDeclsScope : public ClangASTImporter::NewDeclListener {
       NamedDecl *decl = m_decls_to_complete.pop_back_val();
       m_decls_already_completed.insert(decl);
 
-      CompleteDecl(decl, *to_context_md);
+      if (!TypeSystemClang::UseRedeclCompletion())
+        CompleteDecl(decl, *to_context_md);
 
       to_context_md->removeOrigin(decl);
     }
@@ -293,6 +297,9 @@ class CompleteTagDeclsScope : public ClangASTImporter::NewDeclListener {
     if (!isa<TagDecl>(to) && !isa<ObjCInterfaceDecl>(to))
       return;
 
+    if (TypeSystemClang::UseRedeclCompletion())
+      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())
@@ -507,7 +514,14 @@ bool ClangASTImporter::CompleteType(const CompilerType &compiler_type) {
   if (!CanImport(compiler_type))
     return false;
 
-  if (Import(compiler_type)) {
+  auto const success = Import(compiler_type);
+
+  // With redecl completion we don't need to manually complete
+  // the definition.
+  if (TypeSystemClang::UseRedeclCompletion())
+    return success;
+
+  if (success) {
     TypeSystemClang::CompleteTagDeclarationDefinition(compiler_type);
     return true;
   }
@@ -568,16 +582,42 @@ bool ClangASTImporter::CompleteTagDecl(clang::TagDecl *decl) {
   if (!decl_origin.Valid())
     return false;
 
-  if (!TypeSystemClang::GetCompleteDecl(decl_origin.ctx, decl_origin.decl))
-    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);
+
+  if (!TypeSystemClang::UseRedeclCompletion()) {
+    if (!TypeSystemClang::GetCompleteDecl(decl_origin.ctx, decl_origin.decl))
+      return false;
+
+    if (delegate_sp)
+      delegate_sp->ImportDefinitionTo(decl, decl_origin.decl);
+  } else {
+    auto *origin_def = llvm::cast<TagDecl>(decl_origin.decl)->getDefinition();
+    if (!origin_def)
+      return false;
+
+    // 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;
 }
@@ -598,24 +638,48 @@ bool ClangASTImporter::CompleteObjCInterfaceDecl(
   if (!decl_origin.Valid())
     return false;
 
-  if (!TypeSystemClang::GetCompleteDecl(decl_origin.ctx, decl_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);
+  if (!TypeSystemClang::UseRedeclCompletion()) {
+    if (!TypeSystemClang::GetCompleteDecl(decl_origin.ctx, decl_origin.decl))
+      return false;
+
+    if (delegate_sp)
+      delegate_sp->ImportDefinitionTo(interface_decl, decl_origin.decl);
 
-  if (ObjCInterfaceDecl *super_class = interface_decl->getSuperClass())
-    RequireCompleteType(clang::QualType(super_class->getTypeForDecl(), 0));
+    if (ObjCInterfaceDecl *super_class = interface_decl->getSuperClass())
+      RequireCompleteType(clang::QualType(super_class->getTypeForDecl(), 0));
+  } else {
+    ObjCInterfaceDecl *origin_decl =
+        llvm::cast<ObjCInterfaceDecl>(decl_origin.decl);
+
+    origin_decl = origin_decl->getDefinition();
+    if (!origin_decl)
+      return false;
+
+    auto delegate_sp(
+        GetDelegate(&interface_decl->getASTContext(), decl_origin.ctx));
+
+    llvm::Expected<Decl *> result = delegate_sp->Import(origin_decl);
+    if (result)
+      return true;
+
+    llvm::handleAllErrors(result.takeError(),
+                          [](const clang::ASTImportError &e) {
+                            llvm::errs() << "ERR: " << e.toString() << "\n";
+                          });
+
+    return false;
+  }
 
   return true;
 }
 
 bool ClangASTImporter::CompleteAndFetchChildren(clang::QualType type) {
-  if (!RequireCompleteType(type))
-    return false;
+  const auto ret = RequireCompleteType(type);
+  if (TypeSystemClang::UseRedeclCompletion() || !ret)
+    return ret;
 
   Log *log = GetLog(LLDBLog::Expressions);
 
@@ -862,6 +926,22 @@ ClangASTImporter::ASTImporterDelegate::ImportImpl(Decl *From) {
     }
   }
 
+  if (TypeSystemClang::UseRedeclCompletion()) {
+    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);
@@ -883,6 +963,14 @@ ClangASTImporter::ASTImporterDelegate::ImportImpl(Decl *From) {
     for (clang::Decl *candidate : lr) {
       if (candidate->getKind() == From->getKind()) {
         RegisterImportedDecl(From, candidate);
+
+        // 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 (TypeSystemClang::UseRedeclCompletion())
+          if (auto *tag_decl = llvm::dyn_cast<TagDecl>(candidate);
+              !tag_decl || !tag_decl->getDefinition())
+            continue;
+
         m_decls_to_ignore.insert(candidate);
         return candidate;
       }
@@ -1128,7 +1216,16 @@ void ClangASTImporter::ASTImporterDelegate::Imported(clang::Decl *from,
     to_namespace_decl->setHasExternalVisibleStorage();
   }
 
-  MarkDeclImported(from, to);
+  if (TypeSystemClang::UseRedeclCompletion()) {
+    if (clang::ObjCInterfaceDecl *td = dyn_cast<ObjCInterfaceDecl>(to)) {
+      if (clang::ExternalASTSource *s = getToContext().getExternalSource())
+        if (td->isThisDeclarationADefinition())
+          s->CompleteRedeclChain(td);
+      td->setHasExternalVisibleStorage();
+    }
+  } else {
+    MarkDeclImported(from, to);
+  }
 }
 
 void ClangASTImporter::ASTImporterDelegate::MarkDeclImported(Decl *from,
@@ -1199,4 +1296,5 @@ ClangASTImporter::ASTImporterDelegate::ASTImporterDelegate(
   // do a minimal import and the imported declarations won't be completed.
   assert(target_ctx->getExternalSource() && "Missing ExternalSource");
   setODRHandling(clang::ASTImporter::ODRHandlingType::Liberal);
+  setLLDBRedeclCompletion(TypeSystemClang::UseRedeclCompletion());
 }

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

@@ -76,6 +76,18 @@ clang::Decl *ClangUtil::GetFirstDecl(clang::Decl *d) {
   return d;
 }
 
+clang::ObjCInterfaceDecl *ClangUtil::GetAsObjCDecl(const CompilerType &type) {
+  clang::QualType qual_type = ClangUtil::GetCanonicalQualType(type);
+  if (qual_type.isNull())
+    return nullptr;
+
+  if (const auto *ot = qual_type->getAsObjCInterfaceType())
+    return ot->getInterface();
+  if (const auto *ot = qual_type->getAsObjCInterfacePointerType())
+    return ot->getInterfaceDecl();
+  return nullptr;
+}
+
 std::string ClangUtil::DumpDecl(const clang::Decl *d) {
   if (!d)
     return "nullptr";

lldb/source/Plugins/ExpressionParser/Clang/ClangUtil.h

@@ -45,6 +45,8 @@ struct ClangUtil {
     return GetFirstDecl(const_cast<clang::Decl *>(d));
   }
 
+  static clang::ObjCInterfaceDecl *GetAsObjCDecl(const CompilerType &type);
+
   /// Returns a textual representation of the given Decl's AST. Does not
   /// deserialize any child nodes.
   static std::string DumpDecl(const clang::Decl *d);

lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCDeclVendor.cpp

@@ -30,6 +30,7 @@ class lldb_private::AppleObjCExternalASTSource
   AppleObjCExternalASTSource(AppleObjCDeclVendor &decl_vendor)
       : m_decl_vendor(decl_vendor) {}
 
+  // FIXME: unused when 'TypeSystemClang::UseRedeclCompletion == true'
   bool FindExternalVisibleDeclsByName(const clang::DeclContext *decl_ctx,
                                       clang::DeclarationName name) override {
 
@@ -179,8 +180,13 @@ AppleObjCDeclVendor::GetDeclForISA(ObjCLanguageRuntime::ObjCISA isa) {
   meta_data.SetISAPtr(isa);
   m_ast_ctx->SetMetadata(new_iface_decl, meta_data);
 
-  new_iface_decl->setHasExternalVisibleStorage();
-  new_iface_decl->setHasExternalLexicalStorage();
+  if (TypeSystemClang::UseRedeclCompletion()) {
+    m_interface_to_isa[new_iface_decl] = isa;
+    m_external_source->MarkRedeclChainsAsOutOfDate(m_ast_ctx->getASTContext());
+  } else {
+    new_iface_decl->setHasExternalVisibleStorage();
+    new_iface_decl->setHasExternalLexicalStorage();
+  }
 
   ast_ctx.getTranslationUnitDecl()->addDecl(new_iface_decl);
 
@@ -408,6 +414,21 @@ bool AppleObjCDeclVendor::FinishDecl(clang::ObjCInterfaceDecl *interface_decl) {
   Log *log(
       GetLog(LLDBLog::Expressions)); // FIXME - a more appropriate log channel?
 
+  clang::ObjCInterfaceDecl *iface_def = nullptr;
+  if (TypeSystemClang::UseRedeclCompletion()) {
+    // Already completed.
+    if (interface_decl->hasDefinition())
+      return true;
+
+    clang::QualType qt(interface_decl->getTypeForDecl(), 0U);
+    CompilerType type(m_ast_ctx, qt.getAsOpaquePtr());
+    CompilerType def_type = m_ast_ctx->CreateRedeclaration(type);
+    iface_def = ClangUtil::GetAsObjCDecl(def_type);
+
+    auto isa = m_interface_to_isa[interface_decl];
+    m_isa_to_interface[isa] = iface_def;
+  }
+
   ClangASTMetadata *metadata = m_ast_ctx->GetMetadata(interface_decl);
   ObjCLanguageRuntime::ObjCISA objc_isa = 0;
   if (metadata)
@@ -416,13 +437,20 @@ bool AppleObjCDeclVendor::FinishDecl(clang::ObjCInterfaceDecl *interface_decl) {
   if (!objc_isa)
     return false;
 
-  if (!interface_decl->hasExternalVisibleStorage())
+  if (!TypeSystemClang::UseRedeclCompletion() &&
+      !interface_decl->hasExternalVisibleStorage())
+    return true;
+
+  // Could've completed during CreateRedeclaration above
+  if (TypeSystemClang::UseRedeclCompletion() && interface_decl->hasDefinition())
     return true;
 
   interface_decl->startDefinition();
 
-  interface_decl->setHasExternalVisibleStorage(false);
-  interface_decl->setHasExternalLexicalStorage(false);
+  if (!TypeSystemClang::UseRedeclCompletion()) {
+    interface_decl->setHasExternalVisibleStorage(false);
+    interface_decl->setHasExternalLexicalStorage(false);
+  }
 
   ObjCLanguageRuntime::ClassDescriptorSP descriptor =
       m_runtime.GetClassDescriptorFromISA(objc_isa);

lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCDeclVendor.h

@@ -46,6 +46,10 @@ class AppleObjCDeclVendor : public ClangDeclVendor {
       ISAToInterfaceMap;
 
   ISAToInterfaceMap m_isa_to_interface;
+
+  using InterfaceToISAMap =
+      llvm::DenseMap<clang::ObjCInterfaceDecl *, ObjCLanguageRuntime::ObjCISA>;
+  InterfaceToISAMap m_interface_to_isa;
 };
 
 } // namespace lldb_private