Fix for Bug 27401. Always mark the canonical Decl as used.

[CrisCristescu]

Summary

As discussed, when checking for the usage of a Decl, the module merging might not detect
that it has been already marked as used in the Redeclarables chain.
Always marking the canonical Decl ensures consistency.
https://llvm.org/bugs/show_bug.cgi?id=27401

[CrisCristescu]

Refinements

[rsmith]

You also need to update ASTDeclReader::VisitDecl to set the Used flag on the canonical declaration. ASTWriter::DeclarationMarkedUsed should also be updated to only create an update record when necessary; if there is *any* declaration of the entity that's not from an AST file, we can skip writing the update record (but we should make sure that isUsed() triggers completion of the redeclaration chain of the entity if we're going to serialize / deserialize the flag on individual Decls).

lib/AST/DeclBase.cpp

350

You should check for this attribute on the most recent declaration instead (attributes accumulate along the redeclaration chain).

360

I think it's fine either way.

lib/Serialization/ASTReaderDecl.cpp

3863

You need to be careful to avoid this causing a downstream ASTWriter to write an update record. You can set the Used flag directly, as we did before (and keep the FIXME above which notes that doing so isn't entirely correct).

include/clang/AST/DeclBase.h

   /// \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; }
+  void setIsUsed() { getCanonicalDecl()->Used = true; }
 
   /// \brief Mark the declaration used, in the sense of odr-use.
   ///

lib/AST/DeclBase.cpp

   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>())
+  if (CheckUsedAttr && canonD->hasAttr<UsedAttr>())
     return true;
 
-  return false; 
+  return false;
 }
 
 void Decl::markUsed(ASTContext &C) {
-  if (Used)
+  if (isUsed())
     return;
 
+  // Q: Do we need to call the callback with the canonical decl?
   if (C.getASTMutationListener())
     C.getASTMutationListener()->DeclarationMarkedUsed(this);
 
-  Used = true;
+  getCanonicalDecl()->Used = true;
 }
 
 bool Decl::isReferenced() const { 

lib/Sema/SemaExpr.cpp

       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;
-  }
+  // It is sufficient for the canonical decl to be marked as used.
+  Func->markUsed(Context);
+
 }
 
 static void

lib/Serialization/ASTReaderDecl.cpp

       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))

       // ASTMutationListeners other than an ASTWriter.
 
       // Maintain AST consistency: any later redeclarations are used too.
-      forAllLaterRedecls(D, [](Decl *D) { D->Used = true; });
+      D->setIsUsed();
       break;
     }
 

test/Modules/Inputs/PR27401/a.h

+#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

test/Modules/Inputs/PR27401/b.h

+#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; }
+

test/Modules/Inputs/PR27401/module.modulemap

+module "b" { header "b.h" export * }

test/Modules/pr27401.cpp

+// 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