vsapsai created this revision.
vsapsai added reviewers: rsmith, bruno, teemperor.
Herald added a subscriber: ributzka.
vsapsai requested review of this revision.
Herald added a project: clang.
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
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D106994
Files:
clang/include/clang/Serialization/ASTReader.h
clang/lib/Serialization/ASTReaderDecl.cpp
clang/test/Modules/Inputs/merge-record-definition/RecordDef.framework/Headers/RecordDef.h
clang/test/Modules/Inputs/merge-record-definition/RecordDef.framework/Modules/module.modulemap
clang/test/Modules/Inputs/merge-record-definition/RecordDefCopy.framework/Headers/RecordDefCopy.h
clang/test/Modules/Inputs/merge-record-definition/RecordDefCopy.framework/Modules/module.modulemap
clang/test/Modules/Inputs/merge-record-definition/RecordDefHidden.framework/Headers/Hidden.h
clang/test/Modules/Inputs/merge-record-definition/RecordDefHidden.framework/Headers/Visible.h
clang/test/Modules/Inputs/merge-record-definition/RecordDefHidden.framework/Modules/module.modulemap
clang/test/Modules/Inputs/merge-record-definition/RecordDefIncluder.framework/Headers/RecordDefIncluder.h
clang/test/Modules/Inputs/merge-record-definition/RecordDefIncluder.framework/Modules/module.modulemap
clang/test/Modules/merge-record-definition-nonmodular.m
clang/test/Modules/merge-record-definition-visibility.m
clang/test/Modules/merge-record-definition.m
Index: clang/test/Modules/merge-record-definition.m
===================================================================
--- /dev/null
+++ clang/test/Modules/merge-record-definition.m
@@ -0,0 +1,20 @@
+// 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 struct `Buffer` definition is present in two different modules.
+
+#import <RecordDef/RecordDef.h>
+
+void bibi(void) {
+ Buffer buf;
+ buf.b = 1;
+}
+
+#import <RecordDefCopy/RecordDefCopy.h>
+
+void mbap(void) {
+ Buffer buf;
+ buf.c = 2;
+}
Index: clang/test/Modules/merge-record-definition-visibility.m
===================================================================
--- /dev/null
+++ clang/test/Modules/merge-record-definition-visibility.m
@@ -0,0 +1,14 @@
+// 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 struct `Buffer` 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;
+}
Index: clang/test/Modules/merge-record-definition-nonmodular.m
===================================================================
--- /dev/null
+++ clang/test/Modules/merge-record-definition-nonmodular.m
@@ -0,0 +1,30 @@
+// 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 struct `Buffer` 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;
+}
+
+#ifdef MODULAR_BEFORE_TEXTUAL
+ #import <RecordDef/RecordDef.h>
+#else
+ #import <RecordDefIncluder/RecordDefIncluder.h>
+#endif
+
+void mbap(void) {
+ Buffer buf;
+ buf.c = 2;
+}
Index: clang/test/Modules/Inputs/merge-record-definition/RecordDefIncluder.framework/Modules/module.modulemap
===================================================================
--- /dev/null
+++ clang/test/Modules/Inputs/merge-record-definition/RecordDefIncluder.framework/Modules/module.modulemap
@@ -0,0 +1,4 @@
+framework module RecordDefIncluder {
+ header "RecordDefIncluder.h"
+ export *
+}
Index: clang/test/Modules/Inputs/merge-record-definition/RecordDefIncluder.framework/Headers/RecordDefIncluder.h
===================================================================
--- /dev/null
+++ clang/test/Modules/Inputs/merge-record-definition/RecordDefIncluder.framework/Headers/RecordDefIncluder.h
@@ -0,0 +1 @@
+#import <RecordDef/RecordDef.h>
Index: clang/test/Modules/Inputs/merge-record-definition/RecordDefHidden.framework/Modules/module.modulemap
===================================================================
--- /dev/null
+++ 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 *
+ }
+}
Index: clang/test/Modules/Inputs/merge-record-definition/RecordDefHidden.framework/Headers/Visible.h
===================================================================
--- /dev/null
+++ clang/test/Modules/Inputs/merge-record-definition/RecordDefHidden.framework/Headers/Visible.h
@@ -0,0 +1 @@
+// Empty header to create a module.
Index: clang/test/Modules/Inputs/merge-record-definition/RecordDefHidden.framework/Headers/Hidden.h
===================================================================
--- /dev/null
+++ clang/test/Modules/Inputs/merge-record-definition/RecordDefHidden.framework/Headers/Hidden.h
@@ -0,0 +1,7 @@
+// It is important to have a definition *after* non-definition declaration.
+typedef struct _Buffer Buffer;
+struct _Buffer {
+ int a;
+ int b;
+ int c;
+};
Index: clang/test/Modules/Inputs/merge-record-definition/RecordDefCopy.framework/Modules/module.modulemap
===================================================================
--- /dev/null
+++ clang/test/Modules/Inputs/merge-record-definition/RecordDefCopy.framework/Modules/module.modulemap
@@ -0,0 +1,4 @@
+framework module RecordDefCopy {
+ header "RecordDefCopy.h"
+ export *
+}
Index: clang/test/Modules/Inputs/merge-record-definition/RecordDefCopy.framework/Headers/RecordDefCopy.h
===================================================================
--- /dev/null
+++ clang/test/Modules/Inputs/merge-record-definition/RecordDefCopy.framework/Headers/RecordDefCopy.h
@@ -0,0 +1,7 @@
+// It is important to have a definition *after* non-definition declaration.
+typedef struct _Buffer Buffer;
+struct _Buffer {
+ int a;
+ int b;
+ int c;
+};
Index: clang/test/Modules/Inputs/merge-record-definition/RecordDef.framework/Modules/module.modulemap
===================================================================
--- /dev/null
+++ clang/test/Modules/Inputs/merge-record-definition/RecordDef.framework/Modules/module.modulemap
@@ -0,0 +1,4 @@
+framework module RecordDef {
+ header "RecordDef.h"
+ export *
+}
Index: clang/test/Modules/Inputs/merge-record-definition/RecordDef.framework/Headers/RecordDef.h
===================================================================
--- /dev/null
+++ clang/test/Modules/Inputs/merge-record-definition/RecordDef.framework/Headers/RecordDef.h
@@ -0,0 +1,7 @@
+// It is important to have a definition *after* non-definition declaration.
+typedef struct _Buffer Buffer;
+struct _Buffer {
+ int a;
+ int b;
+ int c;
+};
Index: clang/lib/Serialization/ASTReaderDecl.cpp
===================================================================
--- clang/lib/Serialization/ASTReaderDecl.cpp
+++ clang/lib/Serialization/ASTReaderDecl.cpp
@@ -332,7 +332,7 @@
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 @@
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
@@ -2645,7 +2673,7 @@
if (!ND)
return false;
// TODO: implement merge for other necessary decls.
- if (isa<EnumConstantDecl>(ND))
+ if (isa<EnumConstantDecl, FieldDecl>(ND))
return true;
return false;
}
@@ -3315,6 +3343,9 @@
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;
Index: clang/include/clang/Serialization/ASTReader.h
===================================================================
--- clang/include/clang/Serialization/ASTReader.h
+++ clang/include/clang/Serialization/ASTReader.h
@@ -1162,6 +1162,10 @@
/// 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;
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
