[ASTImporter] Fields are imported first and reordered for correct layout.

[danix800]

Summary

Fields are imported first and reordered for correct layout.

For partially imported record, layout computation is incorrect.

[danix800]

Apply git-clang-format.

[danix800]

Fields are imported first and reordered for correct layout.

[balazske]

If I see correctly this change does that the re-ordered members are imported before all other, specially fields come before functions. This way order of fields is already correct when a function is imported.

clang/lib/AST/ASTImporter.cpp

1863

Braces are not required here (and at the other similar places).

[danix800]

In D154764#4505339, @balazske wrote:

If I see correctly this change does that the re-ordered members are imported before all other, specially fields come before functions. This way order of fields is already correct when a function is imported.

That's correct.

[danix800]

Remove unnecessary braces.

[shafik]

clang/unittests/AST/ASTImporterTest.cpp

8018

So is it the case that this caused f2 to be imported first and then f1? Which is the opposite of the example in the comments:

struct declToImport {
         int a = c + b;
         int b = 1;
         int c = 2;
     };

which causes the order to be c, b and then a.

[danix800]

clang/unittests/AST/ASTImporterTest.cpp

8018

The issue caused by this case

int m() {
   return &((A *)0)->f1 - &((A *)0)->f2;
}
int f1;
int f2;

is that before fixing, method m is imported first, which triggers expr (UnaryOperator) dependency computation, further relies on the correct RecordLayout of declToImport, but f1 & f2 is not imported at this moment.

This is not the opposite to the case like int a = c + b;, but both are caused by circular refs.

Similar record layout issue will also be triggered like:

class B;
class A {
  B* b;
  int c;
};
class B {
   A *f() { return &((B *)0)->a; }
   A a;
};

https://reviews.llvm.org/D156201 removes the unnecessary record layout as another fix to this kind of circular refs
(which cannot be fixed simply by re-adjusting import ordering).

[danix800]

clang/unittests/AST/ASTImporterTest.cpp

8018

Sorry for typos, they are both import ordering issues (not circular ref issues).

clang/lib/AST/ASTImporter.cpp

public:
     Error ImportDefinitionIfNeeded(Decl *FromD, Decl *ToD = nullptr);
     Error ImportDeclarationNameLoc(
         const DeclarationNameInfo &From, DeclarationNameInfo &To);
     Error ImportDeclContext(DeclContext *FromDC, bool ForceImport = false);
     Error ImportDeclContext(
         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);
 
     using Designator = DesignatedInitExpr::Designator;
 
     /// What we should import from the definition.
     enum ImportDefinitionKind {
       /// Import the default subset of the definition, which might be
ASTNodeImporter::ImportDeclContext(DeclContext *FromDC, bool ForceImport) {
   // FIXME Clients of the ASTImporter should be able to choose an
   // appropriate error handling strategy for their needs.  For instance,
   // they may not want to mark an entire namespace as erroneous merely
   // because there is an ODR error with two typedefs.  As another example,
   // the client may allow EnumConstantDecls with same names but with
   // 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) {
-      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());
+    if (!ImportedOrErr) {
+      HandleChildErrors.handleChildImportResult(ChildErrors,
+                                                ImportedOrErr.takeError());
+      continue;
+    }
+      Error Err = ImportFieldDeclDefinition(FieldFrom, FieldTo);
+      HandleChildErrors.handleChildImportResult(ChildErrors, std::move(Err));
     }
   }
 
   // We reorder declarations in RecordDecls because they may have another order
   // in the "to" context than they have in the "from" context. This may happen
   // e.g when we import a class like this:
   //    struct declToImport {
   //        int a = c + b;
   //        int b = 1;
   //        int c = 2;
   //    };
   // 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 apper in the "from" context.
+  // order as they appear in the "from" context.
   //
   // Keeping field order is vital because it determines structure layout.
   //
   // Here and below, we cannot call field_begin() method and its callers on
   // ToDC if it has an external storage. Calling field_begin() will
   // automatically load all the fields by calling
   // LoadFieldsFromExternalStorage(). LoadFieldsFromExternalStorage() would
   // call ASTImporter::Import(). This is because the ExternalASTSource
   // 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) {
     consumeError(std::move(ChildErrors));
     return ToDCOrErr.takeError();
   }
 
   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 : FromRD->decls()) {
-    if (isa<FieldDecl>(D) || isa<IndirectFieldDecl>(D) || isa<FriendDecl>(D)) {
-      assert(D && "DC contains a null decl");
-      Decl *ToD = Importer.GetAlreadyImportedOrNull(D);
+  for (auto *D : FromDC->decls()) {
+    if (!MightNeedReordering(D))
+      continue;
+
+    if (Decl *ToD = Importer.GetAlreadyImportedOrNull(D)) {
       // Remove only the decls which we successfully imported.
-      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());
   if (!ToDCOrErr)
     return ToDCOrErr.takeError();
   ToDC = *ToDCOrErr;
 
   if (FromD->getDeclContext() != FromD->getLexicalDeclContext()) {

clang/unittests/AST/ASTImporterTest.cpp

TEST_P(ASTImporterOptionSpecificTestBase, ImportDeductionGuideDifferentOrder) {
   EXPECT_EQ(ToDGOther->getDescribedTemplate()
                 ->getTemplateParameters()
                 ->getParam(0)
                 ->getDeclContext(),
             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(
       R"(
       struct A {
         int idx;
         static void foo(A x) {
           (void)&"text"[x.idx];
         }