Merge pull request #8222 from Michael137/lldb/type-completion-rework/WIP/to-20230725

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

clang/include/clang/AST/ASTImporter.h

@@ -211,6 +211,7 @@ class TypeSourceInfo;
 
     /// Whether to perform a minimal import.
     bool Minimal;
+    bool LLDBRedeclCompletion = false;
 
     ODRHandlingType ODRHandling;
 
@@ -296,8 +297,10 @@ class TypeSourceInfo;
     /// Whether the importer will perform a minimal import, creating
     /// to-be-completed forward declarations when possible.
     bool isMinimalImport() const { return Minimal; }
+    bool hasLLDBRedeclCompletion() const { return LLDBRedeclCompletion; }
 
     void setODRHandling(ODRHandlingType T) { ODRHandling = T; }
+    void setLLDBRedeclCompletion(bool Val) { LLDBRedeclCompletion = Val; }
 
     /// \brief Import the given object, returns the result.
     ///

clang/include/clang/AST/DeclObjC.h

@@ -1238,7 +1238,8 @@ class ObjCInterfaceDecl : public ObjCContainerDecl
   /// which will be NULL if this class has not yet been defined.
   ///
   /// The bit indicates when we don't need to check for out-of-date
-  /// declarations. It will be set unless modules are enabled.
+  /// declarations. It will be set unless there is an ExternalASTSource that
+  /// could provide a definition.
   llvm::PointerIntPair<DefinitionData *, 1, bool> Data;
 
   ObjCInterfaceDecl(const ASTContext &C, DeclContext *DC, SourceLocation AtLoc,
@@ -1527,7 +1528,7 @@ class ObjCInterfaceDecl : public ObjCContainerDecl
     // If the name of this class is out-of-date, bring it up-to-date, which
     // might bring in a definition.
     // Note: a null value indicates that we don't have a definition and that
-    // modules are enabled.
+    // there is a ExternalASTSource that could provide a definition.
     if (!Data.getOpaqueValue())
       getMostRecentDecl();
 
@@ -2095,7 +2096,8 @@ class ObjCProtocolDecl : public ObjCContainerDecl,
   /// which will be NULL if this class has not yet been defined.
   ///
   /// The bit indicates when we don't need to check for out-of-date
-  /// declarations. It will be set unless modules are enabled.
+  /// declarations. It will be set unless there is an ExternalASTSource that
+  /// could provide a definition.
   llvm::PointerIntPair<DefinitionData *, 1, bool> Data;
 
   ObjCProtocolDecl(ASTContext &C, DeclContext *DC, IdentifierInfo *Id,
@@ -2232,7 +2234,7 @@ class ObjCProtocolDecl : public ObjCContainerDecl,
     // If the name of this protocol is out-of-date, bring it up-to-date, which
     // might bring in a definition.
     // Note: a null value indicates that we don't have a definition and that
-    // modules are enabled.
+    // there is a ExternalASTSource that could provide a definition.
     if (!Data.getOpaqueValue())
       getMostRecentDecl();
 

clang/lib/AST/ASTImporter.cpp

@@ -1031,7 +1031,8 @@ Expected<LambdaCapture> ASTNodeImporter::import(const LambdaCapture &From) {
 
 template <typename T>
 bool ASTNodeImporter::hasSameVisibilityContextAndLinkage(T *Found, T *From) {
-  if (Found->getLinkageInternal() != From->getLinkageInternal())
+  if (!Importer.hasLLDBRedeclCompletion() &&
+      Found->getLinkageInternal() != From->getLinkageInternal())
     return false;
 
   if (From->hasExternalFormalLinkage())
@@ -1482,7 +1483,9 @@ ExpectedType ASTNodeImporter::VisitInjectedClassNameType(
 }
 
 ExpectedType ASTNodeImporter::VisitRecordType(const RecordType *T) {
+  // getCanonicalDecl in order to not trigger redeclaration completion
   Expected<RecordDecl *> ToDeclOrErr = import(T->getDecl());
+
   if (!ToDeclOrErr)
     return ToDeclOrErr.takeError();
 
@@ -1771,8 +1774,9 @@ Error ASTNodeImporter::ImportDefinitionIfNeeded(Decl *FromD, Decl *ToD) {
 
   if (RecordDecl *FromRecord = dyn_cast<RecordDecl>(FromD)) {
     if (RecordDecl *ToRecord = cast<RecordDecl>(ToD)) {
-      if (FromRecord->getDefinition() && FromRecord->isCompleteDefinition() &&
-          !ToRecord->getDefinition()) {
+      if (FromRecord->getDefinition() && !ToRecord->getDefinition() &&
+          (Importer.hasLLDBRedeclCompletion() ||
+           FromRecord->isCompleteDefinition())) {
         if (Error Err = ImportDefinition(FromRecord, ToRecord))
           return Err;
       }
@@ -1873,12 +1877,15 @@ ASTNodeImporter::ImportDeclContext(DeclContext *FromDC, bool ForceImport) {
                                                 ImportedOrErr.takeError());
       continue;
     }
-    FieldDecl *FieldFrom = dyn_cast_or_null<FieldDecl>(From);
-    Decl *ImportedDecl = *ImportedOrErr;
-    FieldDecl *FieldTo = dyn_cast_or_null<FieldDecl>(ImportedDecl);
-    if (FieldFrom && FieldTo) {
-      Error Err = ImportFieldDeclDefinition(FieldFrom, FieldTo);
-      HandleChildErrors.handleChildImportResult(ChildErrors, std::move(Err));
+
+    if (Importer.hasLLDBRedeclCompletion()) {
+      FieldDecl *FieldFrom = dyn_cast_or_null<FieldDecl>(From);
+      Decl *ImportedDecl = *ImportedOrErr;
+      FieldDecl *FieldTo = dyn_cast_or_null<FieldDecl>(ImportedDecl);
+      if (FieldFrom && FieldTo) {
+        Error Err = ImportFieldDeclDefinition(FieldFrom, FieldTo);
+        HandleChildErrors.handleChildImportResult(ChildErrors, std::move(Err));
+      }
     }
   }
 
@@ -2039,7 +2046,11 @@ Error ASTNodeImporter::ImportDefinition(
     To->completeDefinition();
   };
 
-  if (To->getDefinition() || To->isBeingDefined()) {
+  bool hasDef = (Importer.hasLLDBRedeclCompletion() &&
+                 To->isThisDeclarationADefinition()) ||
+                To->getDefinition();
+
+  if (hasDef || To->isBeingDefined()) {
     if (Kind == IDK_Everything ||
         // In case of lambdas, the class already has a definition ptr set, but
         // the contained decls are not imported yet. Also, isBeingDefined was
@@ -2558,6 +2569,9 @@ ASTNodeImporter::VisitTypedefNameDecl(TypedefNameDecl *D, bool IsAlias) {
                 !hasSameVisibilityContextAndLinkage(FoundR, FromR))
               continue;
           }
+
+          if (Importer.hasLLDBRedeclCompletion() && Importer.isMinimalImport())
+            return Importer.MapImported(D, FoundTypedef);
           // If the "From" context has a complete underlying type but we
           // already have a complete underlying type then return with that.
           if (!FromUT->isIncompleteType() && !FoundUT->isIncompleteType())
@@ -2822,9 +2836,11 @@ ExpectedDecl ASTNodeImporter::VisitEnumDecl(EnumDecl *D) {
       return POIOrErr.takeError();
   }
 
+  auto Kind = Importer.hasLLDBRedeclCompletion() ? IDK_Everything : IDK_Default;
+
   // Import the definition
   if (D->isCompleteDefinition())
-    if (Error Err = ImportDefinition(D, D2))
+    if (Error Err = ImportDefinition(D, D2, Kind))
       return std::move(Err);
 
   return D2;
@@ -2902,7 +2918,8 @@ ExpectedDecl ASTNodeImporter::VisitRecordDecl(RecordDecl *D) {
 
         if (IsStructuralMatch(D, FoundRecord)) {
           RecordDecl *FoundDef = FoundRecord->getDefinition();
-          if (D->isThisDeclarationADefinition() && FoundDef) {
+          if (!Importer.hasLLDBRedeclCompletion() &&
+              D->isThisDeclarationADefinition() && FoundDef) {
             // FIXME: Structural equivalence check should check for same
             // user-defined methods.
             Importer.MapImported(D, FoundDef);
@@ -3078,8 +3095,9 @@ ExpectedDecl ASTNodeImporter::VisitRecordDecl(RecordDecl *D) {
   if (D->isAnonymousStructOrUnion())
     D2->setAnonymousStructOrUnion(true);
 
+  auto Kind = Importer.hasLLDBRedeclCompletion() ? IDK_Everything : IDK_Default;
   if (D->isCompleteDefinition())
-    if (Error Err = ImportDefinition(D, D2, IDK_Default))
+    if (Error Err = ImportDefinition(D, D2, Kind))
       return std::move(Err);
 
   return D2;
@@ -5203,7 +5221,8 @@ Error ASTNodeImporter::ImportDefinition(
                           diag::note_odr_objc_missing_superclass);
     }
 
-    if (shouldForceImportDeclContext(Kind))
+    if (Importer.hasLLDBRedeclCompletion() ||
+        shouldForceImportDeclContext(Kind))
       if (Error Err = ImportDeclContext(From))
         return Err;
     return Error::success();
@@ -6084,8 +6103,9 @@ ExpectedDecl ASTNodeImporter::VisitClassTemplateSpecializationDecl(
     }
   }
 
+  auto Kind = Importer.hasLLDBRedeclCompletion() ? IDK_Everything : IDK_Default;
   if (D->isCompleteDefinition())
-    if (Error Err = ImportDefinition(D, D2))
+    if (Error Err = ImportDefinition(D, D2, Kind))
       return std::move(Err);
 
   return D2;

clang/lib/AST/DeclObjC.cpp

@@ -1546,7 +1546,7 @@ ObjCInterfaceDecl *ObjCInterfaceDecl::Create(const ASTContext &C,
   auto *Result = new (C, DC)
       ObjCInterfaceDecl(C, DC, atLoc, Id, typeParamList, ClassLoc, PrevDecl,
                         isInternal);
-  Result->Data.setInt(!C.getLangOpts().Modules);
+  Result->Data.setInt(!C.getExternalSource());
   C.getObjCInterfaceType(Result, PrevDecl);
   return Result;
 }
@@ -1556,7 +1556,7 @@ ObjCInterfaceDecl *ObjCInterfaceDecl::CreateDeserialized(const ASTContext &C,
   auto *Result = new (C, ID)
       ObjCInterfaceDecl(C, nullptr, SourceLocation(), nullptr, nullptr,
                         SourceLocation(), nullptr, false);
-  Result->Data.setInt(!C.getLangOpts().Modules);
+  Result->Data.setInt(!C.getExternalSource());
   return Result;
 }
 
@@ -1947,7 +1947,7 @@ ObjCProtocolDecl *ObjCProtocolDecl::Create(ASTContext &C, DeclContext *DC,
                                            ObjCProtocolDecl *PrevDecl) {
   auto *Result =
       new (C, DC) ObjCProtocolDecl(C, DC, Id, nameLoc, atStartLoc, PrevDecl);
-  Result->Data.setInt(!C.getLangOpts().Modules);
+  Result->Data.setInt(!C.getExternalSource());
   return Result;
 }
 
@@ -1956,7 +1956,7 @@ ObjCProtocolDecl *ObjCProtocolDecl::CreateDeserialized(ASTContext &C,
   ObjCProtocolDecl *Result =
       new (C, ID) ObjCProtocolDecl(C, nullptr, nullptr, SourceLocation(),
                                    SourceLocation(), nullptr);
-  Result->Data.setInt(!C.getLangOpts().Modules);
+  Result->Data.setInt(!C.getExternalSource());
   return Result;
 }
 

lldb/include/lldb/Core/PluginManager.h

@@ -474,6 +474,7 @@ class PluginManager {
   // TypeSystem
   static bool RegisterPlugin(llvm::StringRef name, llvm::StringRef description,
                              TypeSystemCreateInstance create_callback,
+                             DebuggerInitializeCallback debugger_callback,
                              LanguageSet supported_languages_for_types,
                              LanguageSet supported_languages_for_expressions);
 
@@ -528,6 +529,13 @@ class PluginManager {
       Debugger &debugger, const lldb::OptionValuePropertiesSP &properties_sp,
       llvm::StringRef description, bool is_global_property);
 
+  static lldb::OptionValuePropertiesSP
+  GetSettingForTypeSystemPlugin(Debugger &debugger, ConstString setting_name);
+
+  static bool CreateSettingForTypeSystemPlugin(
+      Debugger &debugger, const lldb::OptionValuePropertiesSP &properties_sp,
+      ConstString description, bool is_global_property);
+
   static bool CreateSettingForTracePlugin(
       Debugger &debugger, const lldb::OptionValuePropertiesSP &properties_sp,
       llvm::StringRef description, bool is_global_property);

lldb/source/Core/PluginManager.cpp

@@ -1423,10 +1423,11 @@ PluginManager::GetInstrumentationRuntimeCreateCallbackAtIndex(uint32_t idx) {
 struct TypeSystemInstance : public PluginInstance<TypeSystemCreateInstance> {
   TypeSystemInstance(llvm::StringRef name, llvm::StringRef description,
                      CallbackType create_callback,
+                     DebuggerInitializeCallback debugger_init_callback,
                      LanguageSet supported_languages_for_types,
                      LanguageSet supported_languages_for_expressions)
-      : PluginInstance<TypeSystemCreateInstance>(name, description,
-                                                 create_callback),
+      : PluginInstance<TypeSystemCreateInstance>(
+            name, description, create_callback, debugger_init_callback),
         supported_languages_for_types(supported_languages_for_types),
         supported_languages_for_expressions(
             supported_languages_for_expressions) {}
@@ -1445,11 +1446,12 @@ static TypeSystemInstances &GetTypeSystemInstances() {
 bool PluginManager::RegisterPlugin(
     llvm::StringRef name, llvm::StringRef description,
     TypeSystemCreateInstance create_callback,
+    DebuggerInitializeCallback debugger_init_callback,
     LanguageSet supported_languages_for_types,
     LanguageSet supported_languages_for_expressions) {
   return GetTypeSystemInstances().RegisterPlugin(
-      name, description, create_callback, supported_languages_for_types,
-      supported_languages_for_expressions);
+      name, description, create_callback, debugger_init_callback,
+      supported_languages_for_types, supported_languages_for_expressions);
 }
 
 bool PluginManager::UnregisterPlugin(TypeSystemCreateInstance create_callback) {
@@ -1536,6 +1538,7 @@ void PluginManager::DebuggerInitialize(Debugger &debugger) {
   GetOperatingSystemInstances().PerformDebuggerCallback(debugger);
   GetStructuredDataPluginInstances().PerformDebuggerCallback(debugger);
   GetTracePluginInstances().PerformDebuggerCallback(debugger);
+  GetTypeSystemInstances().PerformDebuggerCallback(debugger);
 }
 
 // This is the preferred new way to register plugin specific settings.  e.g.
@@ -1660,6 +1663,7 @@ static constexpr llvm::StringLiteral kProcessPluginName("process");
 static constexpr llvm::StringLiteral kTracePluginName("trace");
 static constexpr llvm::StringLiteral kObjectFilePluginName("object-file");
 static constexpr llvm::StringLiteral kSymbolFilePluginName("symbol-file");
+static constexpr llvm::StringLiteral kTypeSystemPluginName("typesystem");
 static constexpr llvm::StringLiteral kJITLoaderPluginName("jit-loader");
 static constexpr llvm::StringLiteral
     kStructuredDataPluginName("structured-data");
@@ -1744,6 +1748,22 @@ bool PluginManager::CreateSettingForSymbolFilePlugin(
                                 properties_sp, description, is_global_property);
 }
 
+lldb::OptionValuePropertiesSP
+PluginManager::GetSettingForTypeSystemPlugin(Debugger &debugger,
+                                             ConstString setting_name) {
+  return GetSettingForPlugin(debugger, setting_name,
+                             ConstString(kTypeSystemPluginName));
+}
+
+bool PluginManager::CreateSettingForTypeSystemPlugin(
+    Debugger &debugger, const lldb::OptionValuePropertiesSP &properties_sp,
+    ConstString description, bool is_global_property) {
+  return CreateSettingForPlugin(
+      debugger, ConstString(kTypeSystemPluginName),
+      ConstString("Settings for type system plug-ins"), properties_sp,
+      description, is_global_property);
+}
+
 lldb::OptionValuePropertiesSP
 PluginManager::GetSettingForJITLoaderPlugin(Debugger &debugger,
                                             llvm::StringRef setting_name) {

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

@@ -9,6 +9,7 @@
 #ifndef LLDB_SOURCE_PLUGINS_EXPRESSIONPARSER_CLANG_ASTUTILS_H
 #define LLDB_SOURCE_PLUGINS_EXPRESSIONPARSER_CLANG_ASTUTILS_H
 
+#include "Plugins/TypeSystem/Clang/ImporterBackedASTSource.h"
 #include "clang/Basic/Module.h"
 #include "clang/Sema/Lookup.h"
 #include "clang/Sema/MultiplexExternalSemaSource.h"
@@ -20,7 +21,7 @@ namespace lldb_private {
 
 /// Wraps an ExternalASTSource into an ExternalSemaSource. Doesn't take
 /// ownership of the provided source.
-class ExternalASTSourceWrapper : public clang::ExternalSemaSource {
+class ExternalASTSourceWrapper : public ImporterBackedASTSource {
   ExternalASTSource *m_Source;
 
 public:
@@ -240,7 +241,7 @@ class ASTConsumerForwarder : public clang::SemaConsumer {
 /// provide more accurate replies to the requests, but might not be able to
 /// answer all requests. The debug information will be used as a fallback then
 /// to provide information that is not in the C++ module.
-class SemaSourceWithPriorities : public clang::ExternalSemaSource {
+class SemaSourceWithPriorities : public ImporterBackedASTSource {
 
 private:
   /// The sources ordered in decreasing priority.
@@ -273,9 +274,16 @@ class SemaSourceWithPriorities : public clang::ExternalSemaSource {
     return nullptr;
   }
 
+  /// Call ExternalASTSource::CompleteRedeclChain(D)
+  /// on each AST source. Returns as soon as we got
+  /// a definition for D.
   void CompleteRedeclChain(const clang::Decl *D) override {
-    for (size_t i = 0; i < Sources.size(); ++i)
+    for (size_t i = 0; i < Sources.size(); ++i) {
       Sources[i]->CompleteRedeclChain(D);
+      if (auto *td = llvm::dyn_cast<clang::TagDecl>(D))
+        if (td->getDefinition())
+          return;
+    }
   }
 
   clang::Selector GetExternalSelector(uint32_t ID) override {