This revision was automatically updated to reflect the committed changes.
Closed by commit rG93764ff6e200: [modules] Fix miscompilation when using two
RecordDecl definitions with the… (authored by vsapsai).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D106994/new/
https://reviews.llvm.org/D106994
Files:
clang/include/clang/Serialization/ASTReader.h
clang/lib/Serialization/ASTCommon.cpp
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,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 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;
+}
Index: clang/test/Modules/merge-record-definition-visibility.m
===================================================================
--- /dev/null
+++ 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;
+}
Index: clang/test/Modules/merge-record-definition-nonmodular.m
===================================================================
--- /dev/null
+++ 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;
+}
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,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;
+};
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,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;
+};
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,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;
+};
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, IndirectFieldDecl>(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;
@@ -3398,6 +3429,9 @@
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.
Index: clang/lib/Serialization/ASTCommon.cpp
===================================================================
--- clang/lib/Serialization/ASTCommon.cpp
+++ clang/lib/Serialization/ASTCommon.cpp
@@ -474,7 +474,7 @@
// 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);
}
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
