[clang] 93764ff - [modules] Fix miscompilation when using two RecordDecl definitions with the same name.
Volodymyr Sapsai via cfe-commits
cfe-commits at lists.llvm.org
Mon Aug 30 17:52:31 PDT 2021
Author: Volodymyr Sapsai
Date: 2021-08-30T17:51:38-07:00
New Revision: 93764ff6e2005b92057cffa9b3866307e2de260f
URL: https://github.com/llvm/llvm-project/commit/93764ff6e2005b92057cffa9b3866307e2de260f
DIFF: https://github.com/llvm/llvm-project/commit/93764ff6e2005b92057cffa9b3866307e2de260f.diff
LOG: [modules] Fix miscompilation when using two RecordDecl definitions with the same name.
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
Differential Revision: https://reviews.llvm.org/D106994
Added:
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
Modified:
clang/include/clang/Serialization/ASTReader.h
clang/lib/Serialization/ASTCommon.cpp
clang/lib/Serialization/ASTReaderDecl.cpp
Removed:
################################################################################
diff --git a/clang/include/clang/Serialization/ASTReader.h b/clang/include/clang/Serialization/ASTReader.h
index e342a061379eb..f24ccf579aa8f 100644
--- a/clang/include/clang/Serialization/ASTReader.h
+++ b/clang/include/clang/Serialization/ASTReader.h
@@ -1162,6 +1162,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;
diff --git a/clang/lib/Serialization/ASTCommon.cpp b/clang/lib/Serialization/ASTCommon.cpp
index 5fe1f96327ddc..db118d8b00776 100644
--- a/clang/lib/Serialization/ASTCommon.cpp
+++ b/clang/lib/Serialization/ASTCommon.cpp
@@ -474,7 +474,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);
}
diff --git a/clang/lib/Serialization/ASTReaderDecl.cpp b/clang/lib/Serialization/ASTReaderDecl.cpp
index ff79f91e5db1b..ef7921212f21b 100644
--- a/clang/lib/Serialization/ASTReaderDecl.cpp
+++ b/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
@@ -2645,7 +2673,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;
}
@@ -3315,6 +3343,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;
@@ -3398,6 +3429,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.
diff --git a/clang/test/Modules/Inputs/merge-record-definition/RecordDef.framework/Headers/RecordDef.h b/clang/test/Modules/Inputs/merge-record-definition/RecordDef.framework/Headers/RecordDef.h
new file mode 100644
index 0000000000000..7ff8449d429a9
--- /dev/null
+++ b/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;
+};
diff --git a/clang/test/Modules/Inputs/merge-record-definition/RecordDef.framework/Modules/module.modulemap b/clang/test/Modules/Inputs/merge-record-definition/RecordDef.framework/Modules/module.modulemap
new file mode 100644
index 0000000000000..3cb3da0baf019
--- /dev/null
+++ b/clang/test/Modules/Inputs/merge-record-definition/RecordDef.framework/Modules/module.modulemap
@@ -0,0 +1,4 @@
+framework module RecordDef {
+ header "RecordDef.h"
+ export *
+}
diff --git a/clang/test/Modules/Inputs/merge-record-definition/RecordDefCopy.framework/Headers/RecordDefCopy.h b/clang/test/Modules/Inputs/merge-record-definition/RecordDefCopy.framework/Headers/RecordDefCopy.h
new file mode 100644
index 0000000000000..7ff8449d429a9
--- /dev/null
+++ b/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;
+};
diff --git a/clang/test/Modules/Inputs/merge-record-definition/RecordDefCopy.framework/Modules/module.modulemap b/clang/test/Modules/Inputs/merge-record-definition/RecordDefCopy.framework/Modules/module.modulemap
new file mode 100644
index 0000000000000..2905828e9ae5d
--- /dev/null
+++ b/clang/test/Modules/Inputs/merge-record-definition/RecordDefCopy.framework/Modules/module.modulemap
@@ -0,0 +1,4 @@
+framework module RecordDefCopy {
+ header "RecordDefCopy.h"
+ export *
+}
diff --git a/clang/test/Modules/Inputs/merge-record-definition/RecordDefHidden.framework/Headers/Hidden.h b/clang/test/Modules/Inputs/merge-record-definition/RecordDefHidden.framework/Headers/Hidden.h
new file mode 100644
index 0000000000000..7ff8449d429a9
--- /dev/null
+++ b/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;
+};
diff --git a/clang/test/Modules/Inputs/merge-record-definition/RecordDefHidden.framework/Headers/Visible.h b/clang/test/Modules/Inputs/merge-record-definition/RecordDefHidden.framework/Headers/Visible.h
new file mode 100644
index 0000000000000..5b570cc0c4dda
--- /dev/null
+++ b/clang/test/Modules/Inputs/merge-record-definition/RecordDefHidden.framework/Headers/Visible.h
@@ -0,0 +1 @@
+// Empty header to create a module.
diff --git a/clang/test/Modules/Inputs/merge-record-definition/RecordDefHidden.framework/Modules/module.modulemap b/clang/test/Modules/Inputs/merge-record-definition/RecordDefHidden.framework/Modules/module.modulemap
new file mode 100644
index 0000000000000..b9eb312ec7003
--- /dev/null
+++ b/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 *
+ }
+}
diff --git a/clang/test/Modules/Inputs/merge-record-definition/RecordDefIncluder.framework/Headers/RecordDefIncluder.h b/clang/test/Modules/Inputs/merge-record-definition/RecordDefIncluder.framework/Headers/RecordDefIncluder.h
new file mode 100644
index 0000000000000..b1239cfef680f
--- /dev/null
+++ b/clang/test/Modules/Inputs/merge-record-definition/RecordDefIncluder.framework/Headers/RecordDefIncluder.h
@@ -0,0 +1 @@
+#import <RecordDef/RecordDef.h>
diff --git a/clang/test/Modules/Inputs/merge-record-definition/RecordDefIncluder.framework/Modules/module.modulemap b/clang/test/Modules/Inputs/merge-record-definition/RecordDefIncluder.framework/Modules/module.modulemap
new file mode 100644
index 0000000000000..46cdbe79a60d1
--- /dev/null
+++ b/clang/test/Modules/Inputs/merge-record-definition/RecordDefIncluder.framework/Modules/module.modulemap
@@ -0,0 +1,4 @@
+framework module RecordDefIncluder {
+ header "RecordDefIncluder.h"
+ export *
+}
diff --git a/clang/test/Modules/merge-record-definition-nonmodular.m b/clang/test/Modules/merge-record-definition-nonmodular.m
new file mode 100644
index 0000000000000..84d777b1d84b8
--- /dev/null
+++ b/clang/test/Modules/merge-record-definition-nonmodular.m
@@ -0,0 +1,38 @@
+// 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;
+}
diff --git a/clang/test/Modules/merge-record-definition-visibility.m b/clang/test/Modules/merge-record-definition-visibility.m
new file mode 100644
index 0000000000000..03075457fb58d
--- /dev/null
+++ b/clang/test/Modules/merge-record-definition-visibility.m
@@ -0,0 +1,18 @@
+// 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;
+}
diff --git a/clang/test/Modules/merge-record-definition.m b/clang/test/Modules/merge-record-definition.m
new file mode 100644
index 0000000000000..abdcc15f12dd8
--- /dev/null
+++ b/clang/test/Modules/merge-record-definition.m
@@ -0,0 +1,28 @@
+// 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
diff erent 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;
+}
More information about the cfe-commits
mailing list