r192950 - Basic ODR checking for C++ modules:
Richard Smith
richard-llvm at metafoo.co.uk
Thu Oct 17 23:05:18 PDT 2013
Author: rsmith
Date: Fri Oct 18 01:05:18 2013
New Revision: 192950
URL: http://llvm.org/viewvc/llvm-project?rev=192950&view=rev
Log:
Basic ODR checking for C++ modules:
If we have multiple definitions of the same entity from different modules, we
nominate the first definition which we see as being the canonical definition.
If we load a declaration from a different definition and we can't find a
corresponding declaration in the canonical definition, issue a diagnostic.
This is insufficient to prevent things from going horribly wrong in all cases
-- we might be in the middle of emitting IR for a function when we trigger some
deserialization and discover that it refers to an incoherent piece of the AST,
by which point it's probably too late to bail out -- but we'll at least produce
a diagnostic.
Added:
cfe/trunk/test/Modules/Inputs/odr/
cfe/trunk/test/Modules/Inputs/odr/a.h
cfe/trunk/test/Modules/Inputs/odr/b.h
cfe/trunk/test/Modules/Inputs/odr/module.map
cfe/trunk/test/Modules/odr.cpp
Modified:
cfe/trunk/include/clang/Basic/DiagnosticSerializationKinds.td
cfe/trunk/include/clang/Serialization/ASTReader.h
cfe/trunk/lib/Serialization/ASTReader.cpp
cfe/trunk/lib/Serialization/ASTReaderDecl.cpp
Modified: cfe/trunk/include/clang/Basic/DiagnosticSerializationKinds.td
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSerializationKinds.td?rev=192950&r1=192949&r2=192950&view=diff
==============================================================================
--- cfe/trunk/include/clang/Basic/DiagnosticSerializationKinds.td (original)
+++ cfe/trunk/include/clang/Basic/DiagnosticSerializationKinds.td Fri Oct 18 01:05:18 2013
@@ -67,4 +67,13 @@ def err_pch_pp_detailed_record : Error<
def err_not_a_pch_file : Error<
"'%0' does not appear to be a precompiled header file">, DefaultFatal;
+
+def err_module_odr_violation_missing_decl : Error<
+ "%q0 from module '%1' is not present in definition of %q2"
+ "%select{ in module '%4'| provided earlier}3">, NoSFINAE;
+def note_module_odr_violation_no_possible_decls : Note<
+ "definition has no member %0">;
+def note_module_odr_violation_possible_decl : Note<
+ "declaration of %0 does not match">;
+
}
Modified: cfe/trunk/include/clang/Serialization/ASTReader.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Serialization/ASTReader.h?rev=192950&r1=192949&r2=192950&view=diff
==============================================================================
--- cfe/trunk/include/clang/Serialization/ASTReader.h (original)
+++ cfe/trunk/include/clang/Serialization/ASTReader.h Fri Oct 18 01:05:18 2013
@@ -875,6 +875,14 @@ private:
/// been completed.
std::deque<PendingDeclContextInfo> PendingDeclContextInfos;
+ /// \brief 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.
+ ///
+ /// We will check whether the corresponding declaration is in fact missing
+ /// once recursing loading has been completed.
+ llvm::SmallVector<NamedDecl *, 16> PendingOdrMergeChecks;
+
/// \brief The set of Objective-C categories that have been deserialized
/// since the last time the declaration chains were linked.
llvm::SmallPtrSet<ObjCCategoryDecl *, 16> CategoriesDeserialized;
Modified: cfe/trunk/lib/Serialization/ASTReader.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Serialization/ASTReader.cpp?rev=192950&r1=192949&r2=192950&view=diff
==============================================================================
--- cfe/trunk/lib/Serialization/ASTReader.cpp (original)
+++ cfe/trunk/lib/Serialization/ASTReader.cpp Fri Oct 18 01:05:18 2013
@@ -7337,7 +7337,8 @@ void ASTReader::ReadComments() {
void ASTReader::finishPendingActions() {
while (!PendingIdentifierInfos.empty() || !PendingDeclChains.empty() ||
- !PendingMacroIDs.empty() || !PendingDeclContextInfos.empty()) {
+ !PendingMacroIDs.empty() || !PendingDeclContextInfos.empty() ||
+ !PendingOdrMergeChecks.empty()) {
// If any identifiers with corresponding top-level declarations have
// been loaded, load those declarations now.
typedef llvm::DenseMap<IdentifierInfo *, SmallVector<Decl *, 2> >
@@ -7400,6 +7401,64 @@ void ASTReader::finishPendingActions() {
DeclContext *LexicalDC = cast<DeclContext>(GetDecl(Info.LexicalDC));
Info.D->setDeclContextsImpl(SemaDC, LexicalDC, getContext());
}
+
+ // For each declaration from a merged context, check that the canonical
+ // definition of that context also contains a declaration of the same
+ // entity.
+ while (!PendingOdrMergeChecks.empty()) {
+ NamedDecl *D = PendingOdrMergeChecks.pop_back_val();
+
+ // FIXME: Skip over implicit declarations for now. This matters for things
+ // like implicitly-declared special member functions. This isn't entirely
+ // correct; we can end up with multiple unmerged declarations of the same
+ // implicit entity.
+ if (D->isImplicit())
+ continue;
+
+ DeclContext *CanonDef = D->getDeclContext();
+ DeclContext::lookup_result R = CanonDef->lookup(D->getDeclName());
+
+ bool Found = false;
+ const Decl *DCanon = D->getCanonicalDecl();
+
+ llvm::SmallVector<const NamedDecl*, 4> Candidates;
+ for (DeclContext::lookup_iterator I = R.begin(), E = R.end();
+ !Found && I != E; ++I) {
+ for (Decl::redecl_iterator RI = (*I)->redecls_begin(),
+ RE = (*I)->redecls_end();
+ RI != RE; ++RI) {
+ if ((*RI)->getLexicalDeclContext() == CanonDef) {
+ // This declaration is present in the canonical definition. If it's
+ // in the same redecl chain, it's the one we're looking for.
+ if ((*RI)->getCanonicalDecl() == DCanon)
+ Found = true;
+ else
+ Candidates.push_back(cast<NamedDecl>(*RI));
+ break;
+ }
+ }
+ }
+
+ if (!Found) {
+ D->setInvalidDecl();
+
+ Module *CanonDefModule = cast<Decl>(CanonDef)->getOwningModule();
+ Diag(D->getLocation(), diag::err_module_odr_violation_missing_decl)
+ << D << D->getOwningModule()->getFullModuleName()
+ << CanonDef << !CanonDefModule
+ << (CanonDefModule ? CanonDefModule->getFullModuleName() : "");
+
+ if (Candidates.empty())
+ Diag(cast<Decl>(CanonDef)->getLocation(),
+ diag::note_module_odr_violation_no_possible_decls) << D;
+ else {
+ for (unsigned I = 0, N = Candidates.size(); I != N; ++I)
+ Diag(Candidates[I]->getLocation(),
+ diag::note_module_odr_violation_possible_decl)
+ << Candidates[I];
+ }
+ }
+ }
}
// If we deserialized any C++ or Objective-C class definitions, any
Modified: cfe/trunk/lib/Serialization/ASTReaderDecl.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Serialization/ASTReaderDecl.cpp?rev=192950&r1=192949&r2=192950&view=diff
==============================================================================
--- cfe/trunk/lib/Serialization/ASTReaderDecl.cpp (original)
+++ cfe/trunk/lib/Serialization/ASTReaderDecl.cpp Fri Oct 18 01:05:18 2013
@@ -2193,6 +2193,9 @@ ASTDeclReader::FindExistingResult ASTDec
return Result;
}
+ // FIXME: Bail out for non-canonical declarations. We will have performed any
+ // necessary merging already.
+
DeclContext *DC = D->getDeclContext()->getRedeclContext();
if (DC->isTranslationUnit() && Reader.SemaObj) {
IdentifierResolver &IdResolver = Reader.SemaObj->IdResolver;
@@ -2226,17 +2229,23 @@ ASTDeclReader::FindExistingResult ASTDec
if (isSameEntity(*I, D))
return FindExistingResult(Reader, D, *I);
}
- return FindExistingResult(Reader, D, /*Existing=*/0);
} else if (DeclContext *MergeDC = getPrimaryContextForMerging(DC)) {
DeclContext::lookup_result R = MergeDC->noload_lookup(Name);
for (DeclContext::lookup_iterator I = R.begin(), E = R.end(); I != E; ++I) {
if (isSameEntity(*I, D))
return FindExistingResult(Reader, D, *I);
}
- return FindExistingResult(Reader, D, /*Existing=*/0);
+ } else {
+ // Not in a mergeable context.
+ return FindExistingResult(Reader);
}
- return FindExistingResult(Reader);
+ // If this declaration is from a merged context, make a note that we need to
+ // check that the canonical definition of that context contains the decl.
+ if (Reader.MergedDeclContexts.count(D->getLexicalDeclContext()))
+ Reader.PendingOdrMergeChecks.push_back(D);
+
+ return FindExistingResult(Reader, D, /*Existing=*/0);
}
void ASTDeclReader::attachPreviousDecl(Decl *D, Decl *previous) {
Added: cfe/trunk/test/Modules/Inputs/odr/a.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/odr/a.h?rev=192950&view=auto
==============================================================================
--- cfe/trunk/test/Modules/Inputs/odr/a.h (added)
+++ cfe/trunk/test/Modules/Inputs/odr/a.h Fri Oct 18 01:05:18 2013
@@ -0,0 +1,13 @@
+extern struct Y {
+ int n;
+ float f;
+} y1;
+enum E { e1 };
+
+struct X {
+ int n;
+} x1;
+
+int f() {
+ return y1.n + e1 + y1.f + x1.n;
+}
Added: cfe/trunk/test/Modules/Inputs/odr/b.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/odr/b.h?rev=192950&view=auto
==============================================================================
--- cfe/trunk/test/Modules/Inputs/odr/b.h (added)
+++ cfe/trunk/test/Modules/Inputs/odr/b.h Fri Oct 18 01:05:18 2013
@@ -0,0 +1,9 @@
+struct Y {
+ int m;
+ double f;
+} y2;
+enum E { e2 };
+
+int g() {
+ return y2.m + e2 + y2.f;
+}
Added: cfe/trunk/test/Modules/Inputs/odr/module.map
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/odr/module.map?rev=192950&view=auto
==============================================================================
--- cfe/trunk/test/Modules/Inputs/odr/module.map (added)
+++ cfe/trunk/test/Modules/Inputs/odr/module.map Fri Oct 18 01:05:18 2013
@@ -0,0 +1,6 @@
+module a {
+ header "a.h"
+}
+module b {
+ header "b.h"
+}
Added: cfe/trunk/test/Modules/odr.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/odr.cpp?rev=192950&view=auto
==============================================================================
--- cfe/trunk/test/Modules/odr.cpp (added)
+++ cfe/trunk/test/Modules/odr.cpp Fri Oct 18 01:05:18 2013
@@ -0,0 +1,20 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -x objective-c++ -fmodules -fmodules-cache-path=%t -I %S/Inputs/odr %s -verify -std=c++11
+
+// expected-error at a.h:8 {{'X::n' from module 'a' is not present in definition of 'X' provided earlier}}
+struct X { // expected-note {{definition has no member 'n'}}
+};
+
+ at import a;
+ at import b;
+
+// Trigger the declarations from a and b to be imported.
+int x = f() + g();
+
+// expected-note at a.h:5 {{definition has no member 'e2'}}
+// expected-note at a.h:3 {{declaration of 'f' does not match}}
+// expected-note at a.h:1 {{definition has no member 'm'}}
+
+// expected-error at b.h:5 {{'E::e2' from module 'b' is not present in definition of 'E' in module 'a'}}
+// expected-error at b.h:3 {{'Y::f' from module 'b' is not present in definition of 'Y' in module 'a'}}
+// expected-error at b.h:2 {{'Y::m' from module 'b' is not present in definition of 'Y' in module 'a'}}
More information about the cfe-commits
mailing list