bruno created this revision.
bruno added reviewers: rsmith, arphaman, vsapsai, martong.
Herald added subscribers: cfe-commits, ributzka, dexonsmith, jkorous, rnkovacs.
Herald added a reviewer: jdoerfert.
Herald added a project: clang.
Take `struct Z {...}` defined differently and imported from both modules
X and Y. While in C/Objective-C mode, clang used to pick one of the
definitions and ignore the other even though they are not structurally
equivalent. Hook up a mechanism to check for it and reject such
conflicts. Instead of silently compiling, clang now emits:
In module 'Y' imported from t.m:2:
./y.h:2:8: error: type 'struct Z' has incompatible definitions in different
translation units
struct Z {
^
./y.h:3:10: note: field 'm' has type 'double' here
double m;
^
./x.h:3:7: note: field 'm' has type 'int' here
int m;
^
rdar://problem/56764293
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D71734
Files:
clang/include/clang/AST/ASTStructuralEquivalence.h
clang/include/clang/Serialization/ASTReader.h
clang/lib/AST/ASTStructuralEquivalence.cpp
clang/lib/Serialization/ASTReader.cpp
clang/lib/Serialization/ASTReaderDecl.cpp
clang/test/Modules/Inputs/merge-in-c/tag-types/module.modulemap
clang/test/Modules/Inputs/merge-in-c/tag-types/x.h
clang/test/Modules/Inputs/merge-in-c/tag-types/y.h
clang/test/Modules/merge-tag-types.c
Index: clang/test/Modules/merge-tag-types.c
===================================================================
--- /dev/null
+++ clang/test/Modules/merge-tag-types.c
@@ -0,0 +1,14 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/cache -x objective-c -I%S/Inputs/merge-in-c/tag-types -verify %s
+
+@import Y;
+@import X;
+
+void foo() {
+ struct Z z;
+ z.m = 2.0;
+}
+
+// expected-error@y.h:2 {{type 'struct Z' has incompatible definitions in different translation units}}
+// expected-note@y.h:3 {{field 'm' has type 'double' here}}
+// expected-note@x.h:3 {{field 'm' has type 'int' here}}
Index: clang/test/Modules/Inputs/merge-in-c/tag-types/y.h
===================================================================
--- /dev/null
+++ clang/test/Modules/Inputs/merge-in-c/tag-types/y.h
@@ -0,0 +1,4 @@
+
+struct Z {
+ double m;
+};
Index: clang/test/Modules/Inputs/merge-in-c/tag-types/x.h
===================================================================
--- /dev/null
+++ clang/test/Modules/Inputs/merge-in-c/tag-types/x.h
@@ -0,0 +1,4 @@
+
+struct Z {
+ int m;
+};
Index: clang/test/Modules/Inputs/merge-in-c/tag-types/module.modulemap
===================================================================
--- /dev/null
+++ clang/test/Modules/Inputs/merge-in-c/tag-types/module.modulemap
@@ -0,0 +1,10 @@
+
+module X {
+ header "x.h"
+ export *
+}
+
+module Y {
+ header "y.h"
+ export *
+}
Index: clang/lib/Serialization/ASTReaderDecl.cpp
===================================================================
--- clang/lib/Serialization/ASTReaderDecl.cpp
+++ clang/lib/Serialization/ASTReaderDecl.cpp
@@ -14,6 +14,7 @@
#include "ASTCommon.h"
#include "ASTReaderInternals.h"
#include "clang/AST/ASTContext.h"
+#include "clang/AST/ASTStructuralEquivalence.h"
#include "clang/AST/Attr.h"
#include "clang/AST/AttrIterator.h"
#include "clang/AST/Decl.h"
@@ -723,8 +724,22 @@
llvm_unreachable("unexpected tag info kind");
}
- if (!isa<CXXRecordDecl>(TD))
- mergeRedeclarable(TD, Redecl);
+ if (isa<CXXRecordDecl>(TD))
+ return Redecl;
+
+ mergeRedeclarable(TD, Redecl);
+ // Handle merging in C/Objective-C
+ if (!Reader.getContext().getLangOpts().CPlusPlus) {
+ auto *Canon = TD->getCanonicalDecl();
+ if (TD == Canon)
+ return Redecl;
+
+ // Only check decls loaded from different external sources and
+ // delay the actual checking using PendingStructuralMismatches.
+ if (TD->getGlobalID() != Canon->getGlobalID())
+ Reader.PendingStructuralMismatches[Canon].push_back(TD);
+ }
+
return Redecl;
}
Index: clang/lib/Serialization/ASTReader.cpp
===================================================================
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -10,14 +10,14 @@
//
//===----------------------------------------------------------------------===//
-#include "clang/Serialization/ASTRecordReader.h"
#include "ASTCommon.h"
#include "ASTReaderInternals.h"
-#include "clang/AST/AbstractTypeReader.h"
#include "clang/AST/ASTConsumer.h"
#include "clang/AST/ASTContext.h"
#include "clang/AST/ASTMutationListener.h"
+#include "clang/AST/ASTStructuralEquivalence.h"
#include "clang/AST/ASTUnresolvedSet.h"
+#include "clang/AST/AbstractTypeReader.h"
#include "clang/AST/Decl.h"
#include "clang/AST/DeclBase.h"
#include "clang/AST/DeclCXX.h"
@@ -30,8 +30,8 @@
#include "clang/AST/ExprCXX.h"
#include "clang/AST/ExternalASTSource.h"
#include "clang/AST/NestedNameSpecifier.h"
-#include "clang/AST/OpenMPClause.h"
#include "clang/AST/ODRHash.h"
+#include "clang/AST/OpenMPClause.h"
#include "clang/AST/RawCommentList.h"
#include "clang/AST/TemplateBase.h"
#include "clang/AST/TemplateName.h"
@@ -75,6 +75,7 @@
#include "clang/Sema/Weak.h"
#include "clang/Serialization/ASTBitCodes.h"
#include "clang/Serialization/ASTDeserializationListener.h"
+#include "clang/Serialization/ASTRecordReader.h"
#include "clang/Serialization/ContinuousRangeMap.h"
#include "clang/Serialization/GlobalModuleIndex.h"
#include "clang/Serialization/InMemoryModuleCache.h"
@@ -9263,6 +9264,30 @@
PendingMergedDefinitionsToDeduplicate.clear();
}
+void ASTReader::diagnoseStructuralMismatches() {
+ if (PendingStructuralMismatches.empty())
+ return;
+
+ // Updates while doing structural equivalence checks may in turn find and
+ // diagnose other failures, so take ownership of it first.
+ auto StructuralMismatches = std::move(PendingStructuralMismatches);
+ PendingStructuralMismatches.clear();
+
+ for (auto &Mismatches : StructuralMismatches) {
+ auto *Canon = Mismatches.first;
+ for (auto *D : Mismatches.second) {
+ llvm::DenseSet<std::pair<Decl *, Decl *>> NonEquivalentDecls;
+
+ StructuralEquivalenceContext Ctx(
+ D->getASTContext(), Canon->getASTContext(), NonEquivalentDecls,
+ StructuralEquivalenceKind::Default, false /*StrictTypeSpelling*/,
+ true /*Complain*/, true /*ErrorOnTagTypeMismatch*/,
+ false /*UseCanonicalDecls*/);
+ (void)Ctx.IsEquivalent(D, Canon);
+ }
+ }
+}
+
void ASTReader::diagnoseOdrViolations() {
if (PendingOdrMergeFailures.empty() && PendingOdrMergeChecks.empty() &&
PendingFunctionOdrMergeFailures.empty() &&
@@ -11381,6 +11406,7 @@
ReadTimer->stopTimer();
diagnoseOdrViolations();
+ diagnoseStructuralMismatches();
// We are not in recursive loading, so it's safe to pass the "interesting"
// decls to the consumer.
Index: clang/lib/AST/ASTStructuralEquivalence.cpp
===================================================================
--- clang/lib/AST/ASTStructuralEquivalence.cpp
+++ clang/lib/AST/ASTStructuralEquivalence.cpp
@@ -1574,8 +1574,10 @@
Decl *D1, Decl *D2) {
// FIXME: Check for known structural equivalences via a callback of some sort.
- D1 = D1->getCanonicalDecl();
- D2 = D2->getCanonicalDecl();
+ if (Context.UseCanonicalDecls) {
+ D1 = D1->getCanonicalDecl();
+ D2 = D2->getCanonicalDecl();
+ }
std::pair<Decl *, Decl *> P{D1, D2};
// Check whether we already know that these two declarations are not
Index: clang/include/clang/Serialization/ASTReader.h
===================================================================
--- clang/include/clang/Serialization/ASTReader.h
+++ clang/include/clang/Serialization/ASTReader.h
@@ -1084,6 +1084,10 @@
/// been completed.
std::deque<PendingDeclContextInfo> PendingDeclContextInfos;
+ /// C/ObjC definitions in which the structural equivalence check fails
+ llvm::SmallDenseMap<NamedDecl *, llvm::SmallVector<NamedDecl *, 2>, 2>
+ PendingStructuralMismatches;
+
/// The set of NamedDecls that have been loaded, but are members of a
/// context that has been merged into another context where the corresponding
/// declaration is either missing or has not yet been loaded.
@@ -1412,6 +1416,7 @@
void finishPendingActions();
void diagnoseOdrViolations();
+ void diagnoseStructuralMismatches();
void pushExternalDeclIntoScope(NamedDecl *D, DeclarationName Name);
Index: clang/include/clang/AST/ASTStructuralEquivalence.h
===================================================================
--- clang/include/clang/AST/ASTStructuralEquivalence.h
+++ clang/include/clang/AST/ASTStructuralEquivalence.h
@@ -69,15 +69,20 @@
/// \c true if the last diagnostic came from ToCtx.
bool LastDiagFromC2 = false;
+ /// \c true if canonical types should be used for each Decl before
+ /// comparing for structural equivalnce
+ bool UseCanonicalDecls;
+
StructuralEquivalenceContext(
ASTContext &FromCtx, ASTContext &ToCtx,
llvm::DenseSet<std::pair<Decl *, Decl *>> &NonEquivalentDecls,
- StructuralEquivalenceKind EqKind,
- bool StrictTypeSpelling = false, bool Complain = true,
- bool ErrorOnTagTypeMismatch = false)
+ StructuralEquivalenceKind EqKind, bool StrictTypeSpelling = false,
+ bool Complain = true, bool ErrorOnTagTypeMismatch = false,
+ bool UseCanonicalDecls = true)
: FromCtx(FromCtx), ToCtx(ToCtx), NonEquivalentDecls(NonEquivalentDecls),
EqKind(EqKind), StrictTypeSpelling(StrictTypeSpelling),
- ErrorOnTagTypeMismatch(ErrorOnTagTypeMismatch), Complain(Complain) {}
+ ErrorOnTagTypeMismatch(ErrorOnTagTypeMismatch), Complain(Complain),
+ UseCanonicalDecls(UseCanonicalDecls) {}
DiagnosticBuilder Diag1(SourceLocation Loc, unsigned DiagID);
DiagnosticBuilder Diag2(SourceLocation Loc, unsigned DiagID);
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
