From: Vassil Vassilev Date: Wed, 27 Apr 2016 10:46:06 +0000 (+0000) Subject: [modules] Fix Decl's Used invariant. X-Git-Tag: llvmorg-3.9.0-rc1~7794 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=a4d7d783d04f15b65c749daa33baa3d94b1e5904;p=platform%2Fupstream%2Fllvm.git [modules] Fix Decl's Used invariant. The Decl::isUsed has a value for every decl. In non-module builds it is very difficult (but possible) to break this invariant but when we walk up the redecl chain we find the neccessary information. When deserializing the decls from a module it is much more difficult to update correctly this invariant. The patch centralizes the information whether a decl is used in the canonical decl marking the entire entity as being used. Fixes https://llvm.org/bugs/show_bug.cgi?id=27401 Patch by Cristina Cristescu and me. Thanks to Richard Smith who helped to debug and understand the issue! Reviewed by Richard Smith. llvm-svn: 267691 --- diff --git a/clang/include/clang/AST/DeclBase.h b/clang/include/clang/AST/DeclBase.h index f7ddb5b..26cbd38 100644 --- a/clang/include/clang/AST/DeclBase.h +++ b/clang/include/clang/AST/DeclBase.h @@ -518,8 +518,8 @@ public: bool isImplicit() const { return Implicit; } void setImplicit(bool I = true) { Implicit = I; } - /// \brief Whether this declaration was used, meaning that a definition - /// is required. + /// \brief Whether *any* (re-)declaration of the entity was used, meaning that + /// a definition is required. /// /// \param CheckUsedAttr When true, also consider the "used" attribute /// (in addition to the "used" bit set by \c setUsed()) when determining @@ -529,7 +529,8 @@ public: /// \brief Set whether the declaration is used, in the sense of odr-use. /// /// This should only be used immediately after creating a declaration. - void setIsUsed() { Used = true; } + /// It intentionally doesn't notify any listeners. + void setIsUsed() { getCanonicalDecl()->Used = true; } /// \brief Mark the declaration used, in the sense of odr-use. /// diff --git a/clang/include/clang/Serialization/ASTWriter.h b/clang/include/clang/Serialization/ASTWriter.h index 72af48c..bf53cf0 100644 --- a/clang/include/clang/Serialization/ASTWriter.h +++ b/clang/include/clang/Serialization/ASTWriter.h @@ -565,6 +565,17 @@ public: /// decl. const Decl *getFirstLocalDecl(const Decl *D); + /// \brief Is this a local declaration (that is, one that will be written to + /// our AST file)? This is the case for declarations that are neither imported + /// from another AST file nor predefined. + bool IsLocalDecl(const Decl *D) { + if (D->isFromASTFile()) + return false; + auto I = DeclIDs.find(D); + return (I == DeclIDs.end() || + I->second >= serialization::NUM_PREDEF_DECL_IDS); + }; + /// \brief Emit a reference to a declaration. void AddDeclRef(const Decl *D, RecordDataImpl &Record); diff --git a/clang/lib/AST/DeclBase.cpp b/clang/lib/AST/DeclBase.cpp index aec3b7c..338b059 100644 --- a/clang/lib/AST/DeclBase.cpp +++ b/clang/lib/AST/DeclBase.cpp @@ -340,25 +340,29 @@ unsigned Decl::getMaxAlignment() const { return Align; } -bool Decl::isUsed(bool CheckUsedAttr) const { - if (Used) +bool Decl::isUsed(bool CheckUsedAttr) const { + const Decl *CanonD = getCanonicalDecl(); + if (CanonD->Used) return true; - + // Check for used attribute. - if (CheckUsedAttr && hasAttr()) + // Ask the most recent decl, since attributes accumulate in the redecl chain. + if (CheckUsedAttr && getMostRecentDecl()->hasAttr()) return true; - return false; + // The information may have not been deserialized yet. Force deserialization + // to complete the needed information. + return getMostRecentDecl()->getCanonicalDecl()->Used; } void Decl::markUsed(ASTContext &C) { - if (Used) + if (isUsed()) return; if (C.getASTMutationListener()) C.getASTMutationListener()->DeclarationMarkedUsed(this); - Used = true; + setIsUsed(); } bool Decl::isReferenced() const { diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp index 0f47e9e..065c236 100644 --- a/clang/lib/Sema/SemaExpr.cpp +++ b/clang/lib/Sema/SemaExpr.cpp @@ -13011,17 +13011,7 @@ void Sema::MarkFunctionReferenced(SourceLocation Loc, FunctionDecl *Func, UndefinedButUsed.insert(std::make_pair(Func->getCanonicalDecl(), Loc)); } - // Normally the most current decl is marked used while processing the use and - // any subsequent decls are marked used by decl merging. This fails with - // template instantiation since marking can happen at the end of the file - // and, because of the two phase lookup, this function is called with at - // decl in the middle of a decl chain. We loop to maintain the invariant - // that once a decl is used, all decls after it are also used. - for (FunctionDecl *F = Func->getMostRecentDecl();; F = F->getPreviousDecl()) { - F->markUsed(Context); - if (F == Func) - break; - } + Func->markUsed(Context); } static void diff --git a/clang/lib/Serialization/ASTReaderDecl.cpp b/clang/lib/Serialization/ASTReaderDecl.cpp index d81d73d..a6e9938 100644 --- a/clang/lib/Serialization/ASTReaderDecl.cpp +++ b/clang/lib/Serialization/ASTReaderDecl.cpp @@ -51,6 +51,11 @@ namespace clang { bool HasPendingBody; + ///\brief A flag to carry the information for a decl from the entity is + /// used. We use it to delay the marking of the canonical decl as used until + /// the entire declaration is deserialized and merged. + bool IsDeclMarkedUsed; + uint64_t GetCurrentCursorOffset(); uint64_t ReadLocalOffset(const RecordData &R, unsigned &I) { @@ -217,7 +222,8 @@ namespace clang { : Reader(Reader), F(*Loc.F), Offset(Loc.Offset), ThisDeclID(thisDeclID), ThisDeclLoc(ThisDeclLoc), Record(Record), Idx(Idx), TypeIDForTypeDecl(0), NamedDeclForTagDecl(0), - TypedefNameForLinkage(nullptr), HasPendingBody(false) {} + TypedefNameForLinkage(nullptr), HasPendingBody(false), + IsDeclMarkedUsed(false) {} template static Decl *getMostRecentDeclImpl(Redeclarable *D); @@ -444,6 +450,11 @@ uint64_t ASTDeclReader::GetCurrentCursorOffset() { void ASTDeclReader::Visit(Decl *D) { DeclVisitor::Visit(D); + // At this point we have deserialized and merged the decl and it is safe to + // update its canonical decl to signal that the entire entity is used. + D->getCanonicalDecl()->Used |= IsDeclMarkedUsed; + IsDeclMarkedUsed = false; + if (DeclaratorDecl *DD = dyn_cast(D)) { if (DD->DeclInfo) { DeclaratorDecl::ExtInfo *Info = @@ -524,6 +535,7 @@ void ASTDeclReader::VisitDecl(Decl *D) { } D->setImplicit(Record[Idx++]); D->Used = Record[Idx++]; + IsDeclMarkedUsed |= D->Used; D->setReferenced(Record[Idx++]); D->setTopLevelDeclInObjCContainer(Record[Idx++]); D->setAccess((AccessSpecifier)Record[Idx++]); @@ -548,7 +560,7 @@ void ASTDeclReader::VisitDecl(Decl *D) { if (Owner->NameVisibility != Module::AllVisible) { // The owning module is not visible. Mark this declaration as hidden. D->Hidden = true; - + // Note that this declaration was hidden because its owning module is // not yet visible. Reader.HiddenNamesMap[Owner].push_back(D); @@ -2355,6 +2367,8 @@ void ASTDeclReader::mergeRedeclarable(Redeclarable *DBase, T *Existing, // appropriate canonical declaration. D->RedeclLink = Redeclarable::PreviousDeclLink(ExistingCanon); D->First = ExistingCanon; + ExistingCanon->Used |= D->Used; + D->Used = false; // When we merge a namespace, update its pointer to the first namespace. // We cannot have loaded any redeclarations of this declaration yet, so @@ -3112,11 +3126,6 @@ void ASTDeclReader::attachPreviousDecl(ASTReader &Reader, Decl *D, Previous->IdentifierNamespace & (Decl::IDNS_Ordinary | Decl::IDNS_Tag | Decl::IDNS_Type); - // If the previous declaration is marked as used, then this declaration should - // be too. - if (Previous->Used) - D->Used = true; - // If the declaration declares a template, it may inherit default arguments // from the previous declaration. if (TemplateDecl *TD = dyn_cast(D)) @@ -3865,7 +3874,7 @@ void ASTDeclReader::UpdateDecl(Decl *D, ModuleFile &ModuleFile, // ASTMutationListeners other than an ASTWriter. // Maintain AST consistency: any later redeclarations are used too. - forAllLaterRedecls(D, [](Decl *D) { D->Used = true; }); + D->setIsUsed(); break; } diff --git a/clang/lib/Serialization/ASTWriter.cpp b/clang/lib/Serialization/ASTWriter.cpp index 073ed67..32c9c47 100644 --- a/clang/lib/Serialization/ASTWriter.cpp +++ b/clang/lib/Serialization/ASTWriter.cpp @@ -5784,8 +5784,13 @@ void ASTWriter::AddedObjCCategoryToInterface(const ObjCCategoryDecl *CatD, void ASTWriter::DeclarationMarkedUsed(const Decl *D) { assert(!WritingAST && "Already writing the AST!"); - if (!D->isFromASTFile()) - return; + + // If there is *any* declaration of the entity that's not from an AST file, + // we can skip writing the update record. We make sure that isUsed() triggers + // completion of the redeclaration chain of the entity. + for (auto Prev = D->getMostRecentDecl(); Prev; Prev = Prev->getPreviousDecl()) + if (IsLocalDecl(Prev)) + return; DeclUpdates[D].push_back(DeclUpdate(UPD_DECL_MARKED_USED)); } diff --git a/clang/lib/Serialization/ASTWriterDecl.cpp b/clang/lib/Serialization/ASTWriterDecl.cpp index e2b03ba..1e26f0f 100644 --- a/clang/lib/Serialization/ASTWriterDecl.cpp +++ b/clang/lib/Serialization/ASTWriterDecl.cpp @@ -1541,16 +1541,6 @@ void ASTDeclWriter::VisitDeclContext(DeclContext *DC) { } const Decl *ASTWriter::getFirstLocalDecl(const Decl *D) { - /// \brief Is this a local declaration (that is, one that will be written to - /// our AST file)? This is the case for declarations that are neither imported - /// from another AST file nor predefined. - auto IsLocalDecl = [&](const Decl *D) -> bool { - if (D->isFromASTFile()) - return false; - auto I = DeclIDs.find(D); - return (I == DeclIDs.end() || I->second >= NUM_PREDEF_DECL_IDS); - }; - assert(IsLocalDecl(D) && "expected a local declaration"); const Decl *Canon = D->getCanonicalDecl(); diff --git a/clang/test/Modules/Inputs/PR27401/a.h b/clang/test/Modules/Inputs/PR27401/a.h new file mode 100644 index 0000000..63d6b70 --- /dev/null +++ b/clang/test/Modules/Inputs/PR27401/a.h @@ -0,0 +1,17 @@ +#ifndef _LIBCPP_ALGORITHM +#define _LIBCPP_ALGORITHM +template +struct integral_constant { + static const _Tp value = _Tp(); +}; + +template +struct is_nothrow_default_constructible + : integral_constant {}; + +template +struct is_nothrow_move_constructible + : integral_constant {}; + +class allocator {}; +#endif diff --git a/clang/test/Modules/Inputs/PR27401/b.h b/clang/test/Modules/Inputs/PR27401/b.h new file mode 100644 index 0000000..2b4e7f1 --- /dev/null +++ b/clang/test/Modules/Inputs/PR27401/b.h @@ -0,0 +1,21 @@ +#include "a.h" +#ifndef _LIBCPP_VECTOR +template +class __vector_base { +protected: + _Allocator __alloc() const; + __vector_base(_Allocator); +}; + +template +class vector : __vector_base<_Tp, _Allocator> { +public: + vector() noexcept(is_nothrow_default_constructible<_Allocator>::value); + vector(const vector &); + vector(vector &&) + noexcept(is_nothrow_move_constructible<_Allocator>::value); +}; + +#endif +void GetUniquePtrType() { vector v; } + diff --git a/clang/test/Modules/Inputs/PR27401/module.modulemap b/clang/test/Modules/Inputs/PR27401/module.modulemap new file mode 100644 index 0000000..a0efada --- /dev/null +++ b/clang/test/Modules/Inputs/PR27401/module.modulemap @@ -0,0 +1 @@ +module "b" { header "b.h" export * } diff --git a/clang/test/Modules/pr27401.cpp b/clang/test/Modules/pr27401.cpp new file mode 100644 index 0000000..7d5479c --- /dev/null +++ b/clang/test/Modules/pr27401.cpp @@ -0,0 +1,38 @@ +// RUN: rm -rf %t +// RUN: %clang_cc1 -std=c++11 -I%S/Inputs/PR27401 -verify %s +// RUN: %clang_cc1 -std=c++11 -fmodules -fmodule-map-file=%S/Inputs/PR27401/module.modulemap -fmodules-cache-path=%t -I%S/Inputs/PR27401 -verify %s + +#include "a.h" +#define _LIBCPP_VECTOR +template +class __vector_base { +protected: + _Allocator __alloc() const; + __vector_base(_Allocator); +}; + +template +class vector : __vector_base<_Tp, _Allocator> { +public: + vector() noexcept(is_nothrow_default_constructible<_Allocator>::value); + vector(const vector &); + vector(vector &&) + noexcept(is_nothrow_move_constructible<_Allocator>::value); +}; + +template +vector<_Tp, _Allocator>::vector(const vector &__x) : __vector_base<_Tp, _Allocator>(__x.__alloc()) {} + + struct CommentOptions { + vector ParseAllComments; + CommentOptions() {} + }; + struct PrintingPolicy { + PrintingPolicy(CommentOptions LO) : LangOpts(LO) {} + CommentOptions LangOpts; + }; + +#include "b.h" +CommentOptions fn1() { return fn1(); } + +// expected-no-diagnostics