[clang] [clang][ASTImporter] Only reorder fields of RecordDecls (PR #77079)

[Michael Buch via cfe-commits]

https://github.com/Michael137 updated https://github.com/llvm/llvm-project/pull/77079

>From 544e2b12695fa572b08abc8efdceeadd63ebbde5 Mon Sep 17 00:00:00 2001
From: Michael Buch <michaelbuch12 at gmail.com>
Date: Fri, 5 Jan 2024 10:41:55 +0000
Subject: [PATCH 1/2] [clang][ASTImporter] Only reorder fields of RecordDecls

Prior to `e9536698720ec524cc8b72599363622bc1a31558`
(https://reviews.llvm.org/D154764) we only re-ordered
the fields of `RecordDecl`s. The change refactored this
logic to make sure `FieldDecl`s are imported before other
member decls. However, this change also widened the types
of `DeclContext`s we consider for re-ordering from `RecordDecl`
to anything that's a `DeclContext`. This seems to have been
just a drive-by cleanup.

Internally we've seen numerous crashes in LLDB where we try to
perform this re-ordering fields of `ObjCInterfaceDecl`s.

This patch restores old behaviour where we limit the re-ordering
to just `RecordDecl`s.
---
 clang/lib/AST/ASTImporter.cpp | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/clang/lib/AST/ASTImporter.cpp b/clang/lib/AST/ASTImporter.cpp
index 9ffae72346f2af..f03ea6e09e8b7e 100644
--- a/clang/lib/AST/ASTImporter.cpp
+++ b/clang/lib/AST/ASTImporter.cpp
@@ -2034,10 +2034,14 @@ ASTNodeImporter::ImportDeclContext(DeclContext *FromDC, bool ForceImport) {
     return ToDCOrErr.takeError();
   }
 
+  const auto *FromRD = dyn_cast<RecordDecl>(FromDC);
+  if (!FromRD)
+    return ChildErrors;
+
   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()) {
+  for (auto *D : FromRD->decls()) {
     if (!MightNeedReordering(D))
       continue;
 
@@ -2055,7 +2059,7 @@ ASTNodeImporter::ImportDeclContext(DeclContext *FromDC, bool ForceImport) {
   }
 
   // Import everything else.
-  for (auto *From : FromDC->decls()) {
+  for (auto *From : FromRD->decls()) {
     if (MightNeedReordering(From))
       continue;
 

>From a33d2fe1dda862956bcc25455239e830fa2040ed Mon Sep 17 00:00:00 2001
From: Michael Buch <michaelbuch12 at gmail.com>
Date: Fri, 5 Jan 2024 13:23:50 +0000
Subject: [PATCH 2/2] fixup! make sure we do import non-FieldDecls for FromDC

---
 clang/lib/AST/ASTImporter.cpp | 40 +++++++++++++++++------------------
 1 file changed, 19 insertions(+), 21 deletions(-)

diff --git a/clang/lib/AST/ASTImporter.cpp b/clang/lib/AST/ASTImporter.cpp
index f03ea6e09e8b7e..5e5570bb42a1ef 100644
--- a/clang/lib/AST/ASTImporter.cpp
+++ b/clang/lib/AST/ASTImporter.cpp
@@ -2034,32 +2034,30 @@ ASTNodeImporter::ImportDeclContext(DeclContext *FromDC, bool ForceImport) {
     return ToDCOrErr.takeError();
   }
 
-  const auto *FromRD = dyn_cast<RecordDecl>(FromDC);
-  if (!FromRD)
-    return ChildErrors;
-
-  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 (!MightNeedReordering(D))
-      continue;
+  if (const auto *FromRD = dyn_cast<RecordDecl>(FromDC)) {
+    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 (!MightNeedReordering(D))
+        continue;
 
-    assert(D && "DC contains a null decl");
-    if (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);
+      assert(D && "DC contains a null decl");
+      if (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);
+      }
     }
   }
 
   // Import everything else.
-  for (auto *From : FromRD->decls()) {
+  for (auto *From : FromDC->decls()) {
     if (MightNeedReordering(From))
       continue;