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

[vsapsai]

Summary

When deserializing a RecordDecl we don't enforce that redeclaration
chain contains only a single definition. So if the canonical decl is not
a definition itself, RecordType::getDecl can return different objects
before and after an include. It means we can build CGRecordLayout for
one RecordDecl with its set of FieldDecl but try to use it with
FieldDecl belonging to a different RecordDecl. With assertions enabled
it results in

Assertion failed: (FieldInfo.count(FD) && "Invalid field for record!"),
function getLLVMFieldNo, file llvm-project/clang/lib/CodeGen/CGRecordLayout.h, line 199.

and with assertions disabled a bunch of fields are treated as their
memory is located at offset 0.

Fix by keeping the first encountered RecordDecl definition and marking
the subsequent ones as non-definitions. Also need to merge FieldDecl
properly, so that getPrimaryMergedDecl works correctly and during name
lookup we don't treat fields from same-name RecordDecl as ambiguous.

rdar://80184238

[vsapsai]

Added John McCall in case I'm missing some CodeGen pieces.

[vsapsai]

clang/lib/Serialization/ASTReaderDecl.cpp

832

Here is the perfect place to compare if RD and OldDef are equivalent and emit diagnostic if they are not. I've tried StructuralEquivalenceContext for this purpose but it compares canonical decls which doesn't work in this case. I think the best approach for this task would be ODR hash comparison proposed in https://reviews.llvm.org/D71734 It will need some tweaks to work with the current patch but overall the plan is to use ODR hash instead of any other decl comparison.

[rsmith]

For what it's worth, I think the right way to handle this in C is to properly implement the "compatible types" rule instead of trying to invent something ODR-ish: in C, struct definitions in different translation units are different types, but if they're structurally equivalent then they're compatible and you can implicitly pass an object of some type to a function accepting a compatible type. This would mean that we could have multiple, different types with the same name, and we'd need name lookup to deduplicate compatible types, but we wouldn't need to do any cross-module ODR-like struct merging.

But assuming we want to keep the current ODR-in-C approach, this looks OK. There might be some places that assume the lexical and semantic DeclContext for a C FieldDecl are the same (etc) but I don't think there's a good way to find such things other than by testing this patch broadly.

clang/lib/Serialization/ASTReaderDecl.cpp

832

Just a minor note: it's not safe to emit diagnostics from here in general; in particular, emitting a diagnostic that refers to a declaration can trigger deserialization, which can reenter the AST reader in unfortunate ways and crash. But we can make a note to do the structural equivalence check here and then actually perform the check when we finish deserialization (with the other merging checks).

[vsapsai]

In D106994#2911508, @rsmith wrote:

For what it's worth, I think the right way to handle this in C is to properly implement the "compatible types" rule instead of trying to invent something ODR-ish: in C, struct definitions in different translation units are different types, but if they're structurally equivalent then they're compatible and you can implicitly pass an object of some type to a function accepting a compatible type. This would mean that we could have multiple, different types with the same name, and we'd need name lookup to deduplicate compatible types, but we wouldn't need to do any cross-module ODR-like struct merging.

I agree that implementing the "compatible types" looks better as it models the language more faithfully. And in the long run we might need to do that anyway. Is there any work done for "compatible types" already? Or I can start by creating a new type for a new definition with the same name and see how it breaks the lookup?

From pragmatic perspective we are pretty invested into this ODR-ish approach and it's not clear how much work switching to "compatible types" would take. So I'd like to continue with the definition merging and evaluate the effort for "compatible types". That's why I'm curious what work is done already.

But assuming we want to keep the current ODR-in-C approach, this looks OK. There might be some places that assume the lexical and semantic DeclContext for a C FieldDecl are the same (etc) but I don't think there's a good way to find such things other than by testing this patch broadly.

Are there any known signs for mixing lexical and semantic DeclContext? I plan to test the change on our internal codebase, hopefully it'll help to catch any remaining issues.

clang/lib/Serialization/ASTReaderDecl.cpp

832

Thanks for pointing it out, I didn't realize diagnostic can trigger deserialization. Was planning to do something like

if (OldDef->getODRHash() != RD->getODRHash())
  Reader.PendingRecordOdrMergeFailures[OldDef].push_back(RD);

[rsmith]

In D106994#2911903, @vsapsai wrote:

In D106994#2911508, @rsmith wrote:

For what it's worth, I think the right way to handle this in C is to properly implement the "compatible types" rule instead of trying to invent something ODR-ish: in C, struct definitions in different translation units are different types, but if they're structurally equivalent then they're compatible and you can implicitly pass an object of some type to a function accepting a compatible type. This would mean that we could have multiple, different types with the same name, and we'd need name lookup to deduplicate compatible types, but we wouldn't need to do any cross-module ODR-like struct merging.

I agree that implementing the "compatible types" looks better as it models the language more faithfully. And in the long run we might need to do that anyway. Is there any work done for "compatible types" already? Or I can start by creating a new type for a new definition with the same name and see how it breaks the lookup?

From pragmatic perspective we are pretty invested into this ODR-ish approach and it's not clear how much work switching to "compatible types" would take. So I'd like to continue with the definition merging and evaluate the effort for "compatible types". That's why I'm curious what work is done already.

I don't think there's been any real work done on a cross-TU implementation of compatible types. I also don't want that idea to get in the way of this patch, which seems like a clear improvement following our current approach.

But assuming we want to keep the current ODR-in-C approach, this looks OK. There might be some places that assume the lexical and semantic DeclContext for a C FieldDecl are the same (etc) but I don't think there's a good way to find such things other than by testing this patch broadly.

Are there any known signs for mixing lexical and semantic DeclContext? I plan to test the change on our internal codebase, hopefully it'll help to catch any remaining issues.

The kinds of things I saw go wrong when we were bringing this up on the C++ side were generally in code that would walk the list of (say) fields of a record building up some information, and then attempt to look up a given FieldDecl* in that data structure. That can fail if fields get merged, because the lookup key may be a different redeclaration of the same field than the one found by walking the class's members. The fix is usually to add getCanonicalDecl calls in the right places. The sign of this kind of bug happening was usually a crash or assert, usually pretty close to where the problem was.

clang/lib/Serialization/ASTReaderDecl.cpp

832

That seems reasonable to me.

[vsapsai]

Are there any known signs for mixing lexical and semantic DeclContext? I plan to test the change on our internal codebase, hopefully it'll help to catch any remaining issues.

The kinds of things I saw go wrong when we were bringing this up on the C++ side were generally in code that would walk the list of (say) fields of a record building up some information, and then attempt to look up a given FieldDecl* in that data structure. That can fail if fields get merged, because the lookup key may be a different redeclaration of the same field than the one found by walking the class's members. The fix is usually to add getCanonicalDecl calls in the right places. The sign of this kind of bug happening was usually a crash or assert, usually pretty close to where the problem was.

Thanks, that's helpful.

[vsapsai]

Discovered ambiguous name lookup for IndirectFieldDecl in anonymous structs.

[vsapsai]

Handle nested anonymous structs and IndirectFieldDecl; more tests to cover unions and bitfields.

[vsapsai]

Add missing changes back.

[vsapsai]

clang/lib/Serialization/ASTReaderDecl.cpp

3346–3347

In D71734 we have

if (auto *RD = dyn_cast<RecordDecl>(DC))
  if (!RD->getASTContext().getLangOpts().CPlusPlus)
    return RD->getCanonicalDecl()->getDefinition();

I've verified that in C++ unions are also CXXRecordDecl, so I think CPlusPlus check is not required. Locally I'm testing with

if (auto *RD = dyn_cast<RecordDecl>(DC)) {
  assert(!RD->getASTContext().getLangOpts().CPlusPlus &&
         "Unexpected RecordDecl in C++");
  return RD->getDefinition();
}

to get extra reassurance. But don't think it should be in the final version.

[vsapsai]

Tested clang with this change on internal code and there were no regressions. Also have done limited testing of runtime behavior of projects built with this clang - no errors encountered. So the testing so far hasn't found any issues.

[vsapsai]

Ping.

[rsmith]

LGTM

clang/lib/Serialization/ASTReaderDecl.cpp

3328–3331

I believe there's no need to have logic matching this case in C because the only way that a class definition can be added by an update record is due to template instantiation. So we can use the simpler logic below for C.

[vsapsai]

Thanks for the review!

clang/lib/Serialization/ASTReaderDecl.cpp

3328–3331

Thanks, it's good to know.

clang/include/clang/Serialization/ASTReader.h

private:
   /// are treating as the definition of the entity. This is used, for instance,
   /// when merging implicit instantiations of class templates across modules.
   llvm::DenseMap<DeclContext *, DeclContext *> MergedDeclContexts;
 
   /// A mapping from canonical declarations of enums to their canonical
   /// 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;
 
   /// What kind of records we are reading.
   enum ReadingKind {
     Read_None, Read_Decl, Read_Type, Read_Stmt
   };
 

clang/lib/Serialization/ASTCommon.cpp

if (auto *VD = dyn_cast<VarDecl>(D))
       return VD->isStaticLocal();
     // FIXME: What about CapturedDecls (and declarations nested within them)?
     return isa<TagDecl>(D) || isa<BlockDecl>(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

public:
     RedeclarableResult VisitTypedefNameDecl(TypedefNameDecl *TD);
     void VisitTypedefDecl(TypedefDecl *TD);
     void VisitTypeAliasDecl(TypeAliasDecl *TD);
     void VisitUnresolvedUsingTypenameDecl(UnresolvedUsingTypenameDecl *D);
     void VisitUnresolvedUsingIfExistsDecl(UnresolvedUsingIfExistsDecl *D);
     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(
                                             ClassTemplateSpecializationDecl *D);
 
     void VisitClassTemplateSpecializationDecl(
         ClassTemplateSpecializationDecl *D) {
       VisitClassTemplateSpecializationDeclImpl(D);
ASTDeclReader::VisitRecordDeclImpl(RecordDecl *RD) {
   RD->setHasNonTrivialToPrimitiveDefaultInitializeCUnion(Record.readInt());
   RD->setHasNonTrivialToPrimitiveDestructCUnion(Record.readInt());
   RD->setHasNonTrivialToPrimitiveCopyCUnion(Record.readInt());
   RD->setParamDestroyedInCallee(Record.readInt());
   RD->setArgPassingRestrictions((RecordDecl::ArgPassingKind)Record.readInt());
   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
   // a deduced return type that references an entity declared within the
   // function.
   if (isa<FunctionDecl>(VD))
     DeferredTypeID = Record.getGlobalTypeID(Record.readInt());
   else

 /// check in Sema guarantees the types can be merged (see C11 6.2.7/1 or C89
 /// 6.1.2.6/1). Although most merging is done in Sema, we need to guarantee
 /// that some types are mergeable during deserialization, otherwise name
 /// lookup fails. This is the case for EnumConstantDecl.
 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;
 }
 
 /// Attempts to merge LifetimeExtendedTemporaryDecl with
 /// identical class definitions from two different modules.
 void ASTDeclReader::mergeMergeable(LifetimeExtendedTemporaryDecl *D) {
   // If modules are not available, there is no reason to perform this merge.
if (auto *ND = dyn_cast<NamespaceDecl>(DC))
     return ND->getOriginalNamespace();
 
   if (auto *RD = dyn_cast<CXXRecordDecl>(DC)) {
     // Try to dig out the definition.
     auto *DD = RD->DefinitionData;
     if (!DD)
       DD = RD->getCanonicalDecl()->DefinitionData;
 
     // If there's no definition yet, then DC's definition is added by an update
     // record, but we've not yet loaded that update record. In this case, we
     // commit to DC being the canonical definition now, and will fix this when
     // we load the update record.
     if (!DD) {
       DD = new (Reader.getContext()) struct CXXRecordDecl::DefinitionData(RD);
       RD->setCompleteDefinition(true);
       RD->DefinitionData = DD;
       RD->getCanonicalDecl()->DefinitionData = DD;
 
       // Track that we did this horrible thing so that we can fix it later.
       Reader.PendingFakeDefinitionData.insert(
           std::make_pair(DD, ASTReader::PendingFakeDefinitionKind::Fake));
     }
 
     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;
 
   // We can see the TU here only if we have no Sema object. In that case,
   // there's no TU scope to look in, so using the DC alone is sufficient.
   if (auto *TU = dyn_cast<TranslationUnitDecl>(DC))
     return TU;
ASTDeclReader::getPrimaryDCForAnonymousDecl(DeclContext *LexicalDC) {
   // chain isn't wired up.
   for (auto *D : merged_redecls(cast<Decl>(LexicalDC))) {
     if (auto *FD = dyn_cast<FunctionDecl>(D))
       if (FD->isThisDeclarationADefinition())
         return FD;
     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.
   return nullptr;
 }
 
 NamedDecl *ASTDeclReader::getAnonymousDeclForMerging(ASTReader &Reader,
                                                      DeclContext *DC,

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

+// 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

+framework module RecordDef {
+  header "RecordDef.h"
+  export *
+}

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

+// 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

+framework module RecordDefCopy {
+  header "RecordDefCopy.h"
+  export *
+}

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

+// 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

+// Empty header to create a module.

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

+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

+#import <RecordDef/RecordDef.h>

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

+framework module RecordDefIncluder {
+  header "RecordDefIncluder.h"
+  export *
+}

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

+// 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

+// 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

+// 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;
+}