[clang][ASTImporter] Only reorder fields of RecordDecls

[Michael137]

Prior to e9536698720ec524cc8b72599363622bc1a31558 (https://reviews.llvm.org/D154764) we only re-ordered the fields of RecordDecls. The change refactored this logic to make sure FieldDecls are imported before other member decls. However, this change also widened the types of DeclContexts 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 on fields of ObjCInterfaceDecls.

This patch restores old behaviour where we limit the re-ordering to just RecordDecls.

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

[llvmbot]

@llvm/pr-subscribers-clang

Author: Michael Buch (Michael137)

Changes

Prior to e9536698720ec524cc8b72599363622bc1a31558 (https://reviews.llvm.org/D154764) we only re-ordered the fields of RecordDecls. The change refactored this logic to make sure FieldDecls are imported before other member decls. However, this change also widened the types of DeclContexts 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 ObjCInterfaceDecls.

This patch restores old behaviour where we limit the re-ordering to just RecordDecls.

---

Full diff: https://github.com/llvm/llvm-project/pull/77079.diff

1 Files Affected:

• (modified) clang/lib/AST/ASTImporter.cpp (+6-2)

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;
 

[Michael137]

Need to sort out some test failures

[balazske]

I can not check what caused exactly the problems but the change looks correct and in change D154764 there seems to be no reason for re-ordering at non-record objects. As an improvement, some tests could be added that check for re-ordering at RecordDecl and (this is the more important) no re-ordering at ObjCInterfaceDecl.

[Michael137]

I can not check what caused exactly the problems but the change looks correct and in change D154764 there seems to be no reason for re-ordering at non-record objects. As an improvement, some tests could be added that check for re-ordering at RecordDecl and (this is the more important) no re-ordering at ObjCInterfaceDecl.

Agreed it would be nice to get a test for this. We already have a test that checks re-ordering of RecordDecls (CXXRecordDeclFieldOrderShouldNotDependOnImportOrder). For ObjCInterfaceDecls it's trickier because, AFAIK, they neither support in-class member initialisers nor function bodies that could trigger a different import order. Any ideas?

[Michael137]

I guess if there's some other type of ObjC decl that could trigger the re-ordering that would be useful.

[adrian-prantl]

Test would be nice, but generally this looks like a reasonable modification.

[Michael137]

Tried to find a way to test the "objective-c fields aren't reordered" case. But after talking to some objective-c experts, there doesn't seem to be a good way to force such "incorrect field order", because it's strictly an in-order language. Reducing the LLDB crashes has been tricky because the reproducers require a somewhat complex setup. @balazske If you're ok with it, I'll go ahead and merge it like this, given it's essentially a minor revert.

Separately I'll try to see if I can simplify the LLDB reproducer, but that'll take some time

[balazske]

This reordering is not a significant feature (and may change in the future) and no test existed before, it is enough (if this exists) if there is a GDB test for the crash.