Merge pull request swiftlang#7921 from Michael137/bgufix/astimporter-objc-crash-to-20230725

This reverts commit e953669.

Prior to this patch, the import re-ordering only applied to `RecordDecl`. This patch changed the `decls()` we were iterating over from a `RecordDecl` to those of the `DeclContext`, which meant we were now also doing this re-ordering for members of `ObjCInterfaceDecl`s. This caused crashes when debugging Objective-C applications with LLDB.

For Objective-C interfaces this re-ordering doesn't actually make sense because those don't have function bodies, so there's no risk of importing a method body that references a yet-to-be-imported ivar, which is what [D154764](http://108.170.204.19/D154764) attempted to fix.

Revert for now until we find a way to support `ObjCInterfaceDecl`s properly.

rdar://119343184
rdar://119636274
rdar://119832131

Author: Michael137

clang/lib/AST/ASTImporter.cpp

@@ -433,7 +433,6 @@ namespace clang {
         Decl *From, DeclContext *&ToDC, DeclContext *&ToLexicalDC);
     Error ImportImplicitMethods(const CXXRecordDecl *From, CXXRecordDecl *To);
 
-    Error ImportFieldDeclDefinition(const FieldDecl *From, const FieldDecl *To);
     Expected<CXXCastPath> ImportCastPath(CastExpr *E);
     Expected<APValue> ImportAPValue(const APValue &FromValue);
 
@@ -1851,33 +1850,52 @@ ASTNodeImporter::ImportDeclContext(DeclContext *FromDC, bool ForceImport) {
   // different values in two distinct translation units.
   ChildErrorHandlingStrategy HandleChildErrors(FromDC);
 
-  auto MightNeedReordering = [](const Decl *D) {
-    return isa<FieldDecl>(D) || isa<IndirectFieldDecl>(D) || isa<FriendDecl>(D);
-  };
-
-  // Import everything that might need reordering first.
   Error ChildErrors = Error::success();
   for (auto *From : FromDC->decls()) {
-    if (!MightNeedReordering(From))
-      continue;
-
     ExpectedDecl ImportedOrErr = import(From);
 
     // If we are in the process of ImportDefinition(...) for a RecordDecl we
     // want to make sure that we are also completing each FieldDecl. There
     // are currently cases where this does not happen and this is correctness
     // fix since operations such as code generation will expect this to be so.
-    if (!ImportedOrErr) {
+    if (ImportedOrErr) {
+      FieldDecl *FieldFrom = dyn_cast_or_null<FieldDecl>(From);
+      Decl *ImportedDecl = *ImportedOrErr;
+      FieldDecl *FieldTo = dyn_cast_or_null<FieldDecl>(ImportedDecl);
+      if (FieldFrom && FieldTo) {
+        RecordDecl *FromRecordDecl = nullptr;
+        RecordDecl *ToRecordDecl = nullptr;
+        // If we have a field that is an ArrayType we need to check if the array
+        // element is a RecordDecl and if so we need to import the definition.
+        if (FieldFrom->getType()->isArrayType()) {
+          // getBaseElementTypeUnsafe(...) handles multi-dimensonal arrays for us.
+          FromRecordDecl = FieldFrom->getType()->getBaseElementTypeUnsafe()->getAsRecordDecl();
+          ToRecordDecl = FieldTo->getType()->getBaseElementTypeUnsafe()->getAsRecordDecl();
+        }
+
+        if (!FromRecordDecl || !ToRecordDecl) {
+          const RecordType *RecordFrom =
+              FieldFrom->getType()->getAs<RecordType>();
+          const RecordType *RecordTo = FieldTo->getType()->getAs<RecordType>();
+
+          if (RecordFrom && RecordTo) {
+            FromRecordDecl = RecordFrom->getDecl();
+            ToRecordDecl = RecordTo->getDecl();
+          }
+        }
+
+        if (FromRecordDecl && ToRecordDecl) {
+          if (FromRecordDecl->isCompleteDefinition() &&
+              !ToRecordDecl->isCompleteDefinition()) {
+            Error Err = ImportDefinition(FromRecordDecl, ToRecordDecl);
+            HandleChildErrors.handleChildImportResult(ChildErrors,
+                                                      std::move(Err));
+          }
+        }
+      }
+    } else {
       HandleChildErrors.handleChildImportResult(ChildErrors,
                                                 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));
     }
   }
 
@@ -1892,7 +1910,7 @@ ASTNodeImporter::ImportDeclContext(DeclContext *FromDC, bool ForceImport) {
   // During the import of `a` we import first the dependencies in sequence,
   // thus the order would be `c`, `b`, `a`. We will get the normal order by
   // first removing the already imported members and then adding them in the
-  // order as they appear in the "from" context.
+  // order as they apper in the "from" context.
   //
   // Keeping field order is vital because it determines structure layout.
   //
@@ -1904,6 +1922,9 @@ ASTNodeImporter::ImportDeclContext(DeclContext *FromDC, bool ForceImport) {
   // interface in LLDB is implemented by the means of the ASTImporter. However,
   // calling an import at this point would result in an uncontrolled import, we
   // must avoid that.
+  const auto *FromRD = dyn_cast<RecordDecl>(FromDC);
+  if (!FromRD)
+    return ChildErrors;
 
   auto ToDCOrErr = Importer.ImportContext(FromDC);
   if (!ToDCOrErr) {
@@ -1914,70 +1935,26 @@ ASTNodeImporter::ImportDeclContext(DeclContext *FromDC, bool ForceImport) {
   DeclContext *ToDC = *ToDCOrErr;
   // Remove all declarations, which may be in wrong order in the
   // lexical DeclContext and then add them in the proper order.
-  for (auto *D : FromDC->decls()) {
-    if (!MightNeedReordering(D))
-      continue;
-
-    assert(D && "DC contains a null decl");
-    if (Decl *ToD = Importer.GetAlreadyImportedOrNull(D)) {
+  for (auto *D : FromRD->decls()) {
+    if (isa<FieldDecl>(D) || isa<IndirectFieldDecl>(D) || isa<FriendDecl>(D)) {
+      assert(D && "DC contains a null decl");
+      Decl *ToD = Importer.GetAlreadyImportedOrNull(D);
       // Remove only the decls which we successfully imported.
-      assert(ToDC == ToD->getLexicalDeclContext() && ToDC->containsDecl(ToD));
-      // Remove the decl from its wrong place in the linked list.
-      ToDC->removeDecl(ToD);
-      // Add the decl to the end of the linked list.
-      // This time it will be at the proper place because the enclosing for
-      // loop iterates in the original (good) order of the decls.
-      ToDC->addDeclInternal(ToD);
+      if (ToD) {
+        assert(ToDC == ToD->getLexicalDeclContext() && ToDC->containsDecl(ToD));
+        // Remove the decl from its wrong place in the linked list.
+        ToDC->removeDecl(ToD);
+        // Add the decl to the end of the linked list.
+        // This time it will be at the proper place because the enclosing for
+        // loop iterates in the original (good) order of the decls.
+        ToDC->addDeclInternal(ToD);
+      }
     }
   }
 
-  // Import everything else.
-  for (auto *From : FromDC->decls()) {
-    if (MightNeedReordering(From))
-      continue;
-
-    ExpectedDecl ImportedOrErr = import(From);
-    if (!ImportedOrErr)
-      HandleChildErrors.handleChildImportResult(ChildErrors,
-                                                ImportedOrErr.takeError());
-  }
-
   return ChildErrors;
 }
 
-Error ASTNodeImporter::ImportFieldDeclDefinition(const FieldDecl *From,
-                                                 const FieldDecl *To) {
-  RecordDecl *FromRecordDecl = nullptr;
-  RecordDecl *ToRecordDecl = nullptr;
-  // If we have a field that is an ArrayType we need to check if the array
-  // element is a RecordDecl and if so we need to import the definition.
-  QualType FromType = From->getType();
-  QualType ToType = To->getType();
-  if (FromType->isArrayType()) {
-    // getBaseElementTypeUnsafe(...) handles multi-dimensonal arrays for us.
-    FromRecordDecl = FromType->getBaseElementTypeUnsafe()->getAsRecordDecl();
-    ToRecordDecl = ToType->getBaseElementTypeUnsafe()->getAsRecordDecl();
-  }
-
-  if (!FromRecordDecl || !ToRecordDecl) {
-    const RecordType *RecordFrom = FromType->getAs<RecordType>();
-    const RecordType *RecordTo = ToType->getAs<RecordType>();
-
-    if (RecordFrom && RecordTo) {
-      FromRecordDecl = RecordFrom->getDecl();
-      ToRecordDecl = RecordTo->getDecl();
-    }
-  }
-
-  if (FromRecordDecl && ToRecordDecl) {
-    if (FromRecordDecl->isCompleteDefinition() &&
-        !ToRecordDecl->isCompleteDefinition())
-      return ImportDefinition(FromRecordDecl, ToRecordDecl);
-  }
-
-  return Error::success();
-}
-
 Error ASTNodeImporter::ImportDeclContext(
     Decl *FromD, DeclContext *&ToDC, DeclContext *&ToLexicalDC) {
   auto ToDCOrErr = Importer.ImportContext(FromD->getDeclContext());

clang/unittests/AST/ASTImporterTest.cpp

@@ -8048,27 +8048,6 @@ TEST_P(ASTImporterOptionSpecificTestBase, ImportDeductionGuideDifferentOrder) {
             ToDGOther);
 }
 
-TEST_P(ASTImporterOptionSpecificTestBase,
-       ImportFieldsFirstForCorrectRecordLayoutTest) {
-  // UnaryOperator(&) triggers RecordLayout computation, which relies on
-  // correctly imported fields.
-  auto Code =
-      R"(
-      class A {
-        int m() {
-          return &((A *)0)->f1 - &((A *)0)->f2;
-        }
-        int f1;
-        int f2;
-      };
-      )";
-  Decl *FromTU = getTuDecl(Code, Lang_CXX11);
-
-  auto *FromF = FirstDeclMatcher<CXXMethodDecl>().match(
-      FromTU, cxxMethodDecl(hasName("A::m")));
-  Import(FromF, Lang_CXX11);
-}
-
 TEST_P(ASTImporterOptionSpecificTestBase,
        ImportRecordWithLayoutRequestingExpr) {
   TranslationUnitDecl *FromTU = getTuDecl(