[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.

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@267691 91177308-0d34-0410-b5e6-96231b3b80d8

diff --git a/include/clang/AST/DeclBase.h b/include/clang/AST/DeclBase.h
index f7ddb5bd6412e45b01f65972e515001c43fc9a5e..26cbd3802fcb61aa45d411d0308d20417f9fc2d2 100644 (file)
--- a/include/clang/AST/DeclBase.h
+++ b/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/include/clang/Serialization/ASTWriter.h b/include/clang/Serialization/ASTWriter.h
index 72af48c27279956c10bfda188ff09cfaa1af64ea..bf53cf09bef0691abd52e9b57d2caab8e08a1d0e 100644 (file)
--- a/include/clang/Serialization/ASTWriter.h
+++ b/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/lib/AST/DeclBase.cpp b/lib/AST/DeclBase.cpp
index aec3b7cd3bd20037f209b7ff7da2cce3922d60a8..338b059e53cb66b79d47e1196ca87a640021256a 100644 (file)
--- a/lib/AST/DeclBase.cpp
+++ b/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<UsedAttr>())
+  // Ask the most recent decl, since attributes accumulate in the redecl chain.
+  if (CheckUsedAttr && getMostRecentDecl()->hasAttr<UsedAttr>())
     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/lib/Sema/SemaExpr.cpp b/lib/Sema/SemaExpr.cpp
index 0f47e9e0994407d667db4f28311e3a72c5b1466f..065c236d6cc5058fc70d703e84e4f4bbdad90c63 100644 (file)
--- a/lib/Sema/SemaExpr.cpp
+++ b/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/lib/Serialization/ASTReaderDecl.cpp b/lib/Serialization/ASTReaderDecl.cpp
index d81d73d4ba7b2a28ba906062592dd62ddada52ee..a6e993885a7dd24bc1557d19f2a88485540e5a3f 100644 (file)
--- a/lib/Serialization/ASTReaderDecl.cpp
+++ b/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 <typename DeclT>
     static Decl *getMostRecentDeclImpl(Redeclarable<DeclT> *D);
@@ -444,6 +450,11 @@ uint64_t ASTDeclReader::GetCurrentCursorOffset() {
 void ASTDeclReader::Visit(Decl *D) {
   DeclVisitor<ASTDeclReader, void>::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<DeclaratorDecl>(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<T> *DBase, T *Existing,
     // appropriate canonical declaration.
     D->RedeclLink = Redeclarable<T>::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<TemplateDecl>(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/lib/Serialization/ASTWriter.cpp b/lib/Serialization/ASTWriter.cpp
index 073ed6747419fda89c984595750f961dae8e31de..32c9c4787c4e0c2f6028f0f83fc9cef2d5207464 100644 (file)
--- a/lib/Serialization/ASTWriter.cpp
+++ b/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/lib/Serialization/ASTWriterDecl.cpp b/lib/Serialization/ASTWriterDecl.cpp
index e2b03ba98ee7593354e771aab6ffbb25e161610a..1e26f0f9c0db373ca53f955a02b48b3126dbbb65 100644 (file)
--- a/lib/Serialization/ASTWriterDecl.cpp
+++ b/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/test/Modules/Inputs/PR27401/a.h b/test/Modules/Inputs/PR27401/a.h
new file mode 100644 (file)
index 0000000..63d6b70
--- /dev/null
+++ b/test/Modules/Inputs/PR27401/a.h
@@ -0,0 +1,17 @@
+#ifndef _LIBCPP_ALGORITHM
+#define _LIBCPP_ALGORITHM
+template <class _Tp, _Tp>
+struct integral_constant {
+  static const _Tp value = _Tp();
+};
+
+template <class _Tp>
+struct is_nothrow_default_constructible
+       : integral_constant<bool, __is_constructible(_Tp)> {};
+
+template <class _Tp>
+struct is_nothrow_move_constructible
+    : integral_constant<bool, __is_constructible(_Tp, _Tp)> {};
+
+class allocator {};
+#endif

diff --git a/test/Modules/Inputs/PR27401/b.h b/test/Modules/Inputs/PR27401/b.h
new file mode 100644 (file)
index 0000000..2b4e7f1
--- /dev/null
+++ b/test/Modules/Inputs/PR27401/b.h
@@ -0,0 +1,21 @@
+#include "a.h"
+#ifndef _LIBCPP_VECTOR
+template <class, class _Allocator>
+class __vector_base {
+protected:
+  _Allocator __alloc() const;
+  __vector_base(_Allocator);
+};
+
+template <class _Tp, class _Allocator = allocator>
+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<char> v; }
+

diff --git a/test/Modules/Inputs/PR27401/module.modulemap b/test/Modules/Inputs/PR27401/module.modulemap
new file mode 100644 (file)
index 0000000..a0efada
--- /dev/null
+++ b/test/Modules/Inputs/PR27401/module.modulemap
@@ -0,0 +1 @@
+module "b" { header "b.h" export * }

diff --git a/test/Modules/pr27401.cpp b/test/Modules/pr27401.cpp
new file mode 100644 (file)
index 0000000..7d5479c
--- /dev/null
+++ b/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, class _Allocator>
+class __vector_base {
+protected:
+  _Allocator __alloc() const;
+  __vector_base(_Allocator);
+};
+
+template <class _Tp, class _Allocator = allocator>
+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 <class _Tp, class _Allocator>
+vector<_Tp, _Allocator>::vector(const vector &__x) : __vector_base<_Tp, _Allocator>(__x.__alloc()) {}
+
+  struct CommentOptions {
+    vector<char>  ParseAllComments;
+    CommentOptions() {}
+  };
+  struct PrintingPolicy {
+    PrintingPolicy(CommentOptions LO) : LangOpts(LO) {}
+    CommentOptions LangOpts;
+  };
+
+#include "b.h"
+CommentOptions fn1() { return fn1(); }
+
+// expected-no-diagnostics