From: Richard Smith Date: Fri, 18 Oct 2013 06:05:18 +0000 (+0000) Subject: Basic ODR checking for C++ modules: X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=3c40a28aa3dde7c5f7e1520c32e7515eda830fef;p=clang 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. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@192950 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/include/clang/Basic/DiagnosticSerializationKinds.td b/include/clang/Basic/DiagnosticSerializationKinds.td index 9f630ae5c1..81509cc188 100644 --- a/include/clang/Basic/DiagnosticSerializationKinds.td +++ b/include/clang/Basic/DiagnosticSerializationKinds.td @@ -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">; + } diff --git a/include/clang/Serialization/ASTReader.h b/include/clang/Serialization/ASTReader.h index 530c54e3cc..00528cc387 100644 --- a/include/clang/Serialization/ASTReader.h +++ b/include/clang/Serialization/ASTReader.h @@ -875,6 +875,14 @@ private: /// been completed. std::deque 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 PendingOdrMergeChecks; + /// \brief The set of Objective-C categories that have been deserialized /// since the last time the declaration chains were linked. llvm::SmallPtrSet CategoriesDeserialized; diff --git a/lib/Serialization/ASTReader.cpp b/lib/Serialization/ASTReader.cpp index 9bb9a7a863..bafe74b174 100644 --- a/lib/Serialization/ASTReader.cpp +++ b/lib/Serialization/ASTReader.cpp @@ -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 > @@ -7400,6 +7401,64 @@ void ASTReader::finishPendingActions() { DeclContext *LexicalDC = cast(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 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(*RI)); + break; + } + } + } + + if (!Found) { + D->setInvalidDecl(); + + Module *CanonDefModule = cast(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(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 diff --git a/lib/Serialization/ASTReaderDecl.cpp b/lib/Serialization/ASTReaderDecl.cpp index 14379f3116..e68c7fc6f9 100644 --- a/lib/Serialization/ASTReaderDecl.cpp +++ b/lib/Serialization/ASTReaderDecl.cpp @@ -2193,6 +2193,9 @@ ASTDeclReader::FindExistingResult ASTDeclReader::findExisting(NamedDecl *D) { 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 ASTDeclReader::findExisting(NamedDecl *D) { 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) { diff --git a/test/Modules/Inputs/odr/a.h b/test/Modules/Inputs/odr/a.h new file mode 100644 index 0000000000..26144b86e8 --- /dev/null +++ b/test/Modules/Inputs/odr/a.h @@ -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; +} diff --git a/test/Modules/Inputs/odr/b.h b/test/Modules/Inputs/odr/b.h new file mode 100644 index 0000000000..b406397947 --- /dev/null +++ b/test/Modules/Inputs/odr/b.h @@ -0,0 +1,9 @@ +struct Y { + int m; + double f; +} y2; +enum E { e2 }; + +int g() { + return y2.m + e2 + y2.f; +} diff --git a/test/Modules/Inputs/odr/module.map b/test/Modules/Inputs/odr/module.map new file mode 100644 index 0000000000..81f396342f --- /dev/null +++ b/test/Modules/Inputs/odr/module.map @@ -0,0 +1,6 @@ +module a { + header "a.h" +} +module b { + header "b.h" +} diff --git a/test/Modules/odr.cpp b/test/Modules/odr.cpp new file mode 100644 index 0000000000..5ab10d2ce4 --- /dev/null +++ b/test/Modules/odr.cpp @@ -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@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'}} +}; + +@import a; +@import b; + +// Trigger the declarations from a and b to be imported. +int x = f() + g(); + +// expected-note@a.h:5 {{definition has no member 'e2'}} +// expected-note@a.h:3 {{declaration of 'f' does not match}} +// expected-note@a.h:1 {{definition has no member 'm'}} + +// expected-error@b.h:5 {{'E::e2' from module 'b' is not present in definition of 'E' in module 'a'}} +// expected-error@b.h:3 {{'Y::f' from module 'b' is not present in definition of 'Y' in module 'a'}} +// expected-error@b.h:2 {{'Y::m' from module 'b' is not present in definition of 'Y' in module 'a'}}