Reland "[modules] Fix miscompilation when using two RecordDecl definitions with the same name."

This reverts commit a0423cc and
re-applies commit 9454716.

Re-applying the change as it is consistent with Objective-C class
behavior where we keep only a single definition in a redeclaration chain.

Failing projects should be updated to use entities only from a single
module. Mixing multiple definitions still poses a risk of
miscompilations and rejecting such code is preferable to silent memory
corruption.

rdar://86548228

Author: vsapsai

clang/include/clang/Serialization/ASTReader.h

@@ -1153,6 +1153,10 @@ class ASTReader
   /// definitions. Only populated when using modules in C++.
   llvm::DenseMap<EnumDecl *, EnumDecl *> EnumDefinitions;
 
+  /// A mapping from canonical declarations of records to their canonical
+  /// definitions. Doesn't cover CXXRecordDecl.
+  llvm::DenseMap<RecordDecl *, RecordDecl *> RecordDefinitions;
+
   /// When reading a Stmt tree, Stmt operands are placed in this stack.
   SmallVector<Stmt *, 16> StmtStack;
 

clang/lib/Serialization/ASTCommon.cpp

@@ -477,7 +477,7 @@ bool serialization::needsAnonymousDeclarationNumber(const NamedDecl *D) {
   // Otherwise, we only care about anonymous class members / block-scope decls.
   // FIXME: We need to handle lambdas and blocks within inline / templated
   // variables too.
-  if (D->getDeclName() || !isa<CXXRecordDecl>(D->getLexicalDeclContext()))
+  if (D->getDeclName() || !isa<RecordDecl>(D->getLexicalDeclContext()))
     return false;
   return isa<TagDecl>(D) || isa<FieldDecl>(D);
 }

clang/lib/Serialization/ASTReaderDecl.cpp

@@ -332,7 +332,7 @@ namespace clang {
     RedeclarableResult VisitTagDecl(TagDecl *TD);
     void VisitEnumDecl(EnumDecl *ED);
     RedeclarableResult VisitRecordDeclImpl(RecordDecl *RD);
-    void VisitRecordDecl(RecordDecl *RD) { VisitRecordDeclImpl(RD); }
+    void VisitRecordDecl(RecordDecl *RD);
     RedeclarableResult VisitCXXRecordDeclImpl(CXXRecordDecl *D);
     void VisitCXXRecordDecl(CXXRecordDecl *D) { VisitCXXRecordDeclImpl(D); }
     RedeclarableResult VisitClassTemplateSpecializationDeclImpl(
@@ -808,6 +808,34 @@ ASTDeclReader::VisitRecordDeclImpl(RecordDecl *RD) {
   return Redecl;
 }
 
+void ASTDeclReader::VisitRecordDecl(RecordDecl *RD) {
+  VisitRecordDeclImpl(RD);
+
+  // Maintain the invariant of a redeclaration chain containing only
+  // a single definition.
+  if (RD->isCompleteDefinition()) {
+    RecordDecl *Canon = static_cast<RecordDecl *>(RD->getCanonicalDecl());
+    RecordDecl *&OldDef = Reader.RecordDefinitions[Canon];
+    if (!OldDef) {
+      // This is the first time we've seen an imported definition. Look for a
+      // local definition before deciding that we are the first definition.
+      for (auto *D : merged_redecls(Canon)) {
+        if (!D->isFromASTFile() && D->isCompleteDefinition()) {
+          OldDef = D;
+          break;
+        }
+      }
+    }
+    if (OldDef) {
+      Reader.MergedDeclContexts.insert(std::make_pair(RD, OldDef));
+      RD->setCompleteDefinition(false);
+      Reader.mergeDefinitionVisibility(OldDef, RD);
+    } else {
+      OldDef = RD;
+    }
+  }
+}
+
 void ASTDeclReader::VisitValueDecl(ValueDecl *VD) {
   VisitNamedDecl(VD);
   // For function declarations, defer reading the type in case the function has
@@ -2661,7 +2689,7 @@ static bool allowODRLikeMergeInC(NamedDecl *ND) {
   if (!ND)
     return false;
   // TODO: implement merge for other necessary decls.
-  if (isa<EnumConstantDecl>(ND))
+  if (isa<EnumConstantDecl, FieldDecl, IndirectFieldDecl>(ND))
     return true;
   return false;
 }
@@ -3337,6 +3365,9 @@ DeclContext *ASTDeclReader::getPrimaryContextForMerging(ASTReader &Reader,
     return DD->Definition;
   }
 
+  if (auto *RD = dyn_cast<RecordDecl>(DC))
+    return RD->getDefinition();
+
   if (auto *ED = dyn_cast<EnumDecl>(DC))
     return ED->getASTContext().getLangOpts().CPlusPlus? ED->getDefinition()
                                                       : nullptr;
@@ -3423,6 +3454,9 @@ ASTDeclReader::getPrimaryDCForAnonymousDecl(DeclContext *LexicalDC) {
     if (auto *MD = dyn_cast<ObjCMethodDecl>(D))
       if (MD->isThisDeclarationADefinition())
         return MD;
+    if (auto *RD = dyn_cast<RecordDecl>(D))
+      if (RD->isThisDeclarationADefinition())
+        return RD;
   }
 
   // No merged definition yet.

clang/test/Modules/Inputs/merge-record-definition/RecordDef.framework/Headers/RecordDef.h

@@ -0,0 +1,21 @@
+// It is important to have a definition *after* non-definition declaration.
+typedef struct _Buffer Buffer;
+struct _Buffer {
+  int a;
+  int b;
+  int c;
+};
+
+typedef struct _AnonymousStruct AnonymousStruct;
+struct _AnonymousStruct {
+  struct {
+    int x;
+    int y;
+  };
+};
+
+typedef union _UnionRecord UnionRecord;
+union _UnionRecord {
+  int u: 2;
+  int v: 4;
+};

clang/test/Modules/Inputs/merge-record-definition/RecordDef.framework/Modules/module.modulemap

@@ -0,0 +1,4 @@
+framework module RecordDef {
+  header "RecordDef.h"
+  export *
+}

clang/test/Modules/Inputs/merge-record-definition/RecordDefCopy.framework/Headers/RecordDefCopy.h

@@ -0,0 +1,21 @@
+// It is important to have a definition *after* non-definition declaration.
+typedef struct _Buffer Buffer;
+struct _Buffer {
+  int a;
+  int b;
+  int c;
+};
+
+typedef struct _AnonymousStruct AnonymousStruct;
+struct _AnonymousStruct {
+  struct {
+    int x;
+    int y;
+  };
+};
+
+typedef union _UnionRecord UnionRecord;
+union _UnionRecord {
+  int u: 2;
+  int v: 4;
+};

clang/test/Modules/Inputs/merge-record-definition/RecordDefCopy.framework/Modules/module.modulemap

@@ -0,0 +1,4 @@
+framework module RecordDefCopy {
+  header "RecordDefCopy.h"
+  export *
+}

clang/test/Modules/Inputs/merge-record-definition/RecordDefHidden.framework/Headers/Hidden.h

@@ -0,0 +1,21 @@
+// It is important to have a definition *after* non-definition declaration.
+typedef struct _Buffer Buffer;
+struct _Buffer {
+  int a;
+  int b;
+  int c;
+};
+
+typedef struct _AnonymousStruct AnonymousStruct;
+struct _AnonymousStruct {
+  struct {
+    int x;
+    int y;
+  };
+};
+
+typedef union _UnionRecord UnionRecord;
+union _UnionRecord {
+  int u: 2;
+  int v: 4;
+};

clang/test/Modules/Inputs/merge-record-definition/RecordDefHidden.framework/Headers/Visible.h

@@ -0,0 +1 @@
+// Empty header to create a module.

clang/test/Modules/Inputs/merge-record-definition/RecordDefHidden.framework/Modules/module.modulemap

@@ -0,0 +1,9 @@
+framework module RecordDefHidden {
+  header "Visible.h"
+  export *
+
+  explicit module Hidden {
+    header "Hidden.h"
+    export *
+  }
+}

clang/test/Modules/Inputs/merge-record-definition/RecordDefIncluder.framework/Headers/RecordDefIncluder.h

@@ -0,0 +1 @@
+#import <RecordDef/RecordDef.h>

clang/test/Modules/Inputs/merge-record-definition/RecordDefIncluder.framework/Modules/module.modulemap

@@ -0,0 +1,4 @@
+framework module RecordDefIncluder {
+  header "RecordDefIncluder.h"
+  export *
+}

clang/test/Modules/merge-record-definition-nonmodular.m

@@ -0,0 +1,39 @@
+// UNSUPPORTED: -zos, -aix
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: %clang_cc1 -emit-llvm -o %t/test.bc -F%S/Inputs/merge-record-definition %s \
+// RUN:            -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/modules.cache -fmodule-name=RecordDef
+// RUN: %clang_cc1 -emit-llvm -o %t/test.bc -F%S/Inputs/merge-record-definition %s -DMODULAR_BEFORE_TEXTUAL \
+// RUN:            -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/modules.cache -fmodule-name=RecordDef
+
+// Test a case when a struct definition once is included from a textual header and once from a module.
+
+#ifdef MODULAR_BEFORE_TEXTUAL
+  #import <RecordDefIncluder/RecordDefIncluder.h>
+#else
+  #import <RecordDef/RecordDef.h>
+#endif
+
+void bibi(void) {
+  Buffer buf;
+  buf.b = 1;
+  AnonymousStruct strct;
+  strct.x = 1;
+  UnionRecord rec;
+  rec.u = 1;
+}
+
+#ifdef MODULAR_BEFORE_TEXTUAL
+  #import <RecordDef/RecordDef.h>
+#else
+  #import <RecordDefIncluder/RecordDefIncluder.h>
+#endif
+
+void mbap(void) {
+  Buffer buf;
+  buf.c = 2;
+  AnonymousStruct strct;
+  strct.y = 2;
+  UnionRecord rec;
+  rec.v = 2;
+}

clang/test/Modules/merge-record-definition-visibility.m

@@ -0,0 +1,19 @@
+// UNSUPPORTED: -zos, -aix
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: %clang_cc1 -emit-llvm -o %t/test.bc -F%S/Inputs/merge-record-definition %s \
+// RUN:            -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/modules.cache
+
+// Test a case when a struct definition is first imported as invisible and then as visible.
+
+#import <RecordDefHidden/Visible.h>
+#import <RecordDef/RecordDef.h>
+
+void bibi(void) {
+  Buffer buf;
+  buf.b = 1;
+  AnonymousStruct strct;
+  strct.y = 1;
+  UnionRecord rec;
+  rec.u = 1;
+}

clang/test/Modules/merge-record-definition.m

@@ -0,0 +1,29 @@
+// UNSUPPORTED: -zos, -aix
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: %clang_cc1 -emit-llvm -o %t/test.bc -F%S/Inputs/merge-record-definition %s \
+// RUN:            -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/modules.cache
+
+// Test a case when a struct definition is present in two different modules.
+
+#import <RecordDef/RecordDef.h>
+
+void bibi(void) {
+  Buffer buf;
+  buf.b = 1;
+  AnonymousStruct strct;
+  strct.x = 1;
+  UnionRecord rec;
+  rec.u = 1;
+}
+
+#import <RecordDefCopy/RecordDefCopy.h>
+
+void mbap(void) {
+  Buffer buf;
+  buf.c = 2;
+  AnonymousStruct strct;
+  strct.y = 2;
+  UnionRecord rec;
+  rec.v = 2;
+}