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

349

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

359

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

 }
 
 bool Decl::isUsed(bool CheckUsedAttr) const { 
-  if (Used)
+  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; 
 }
 
 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->getMostRecentDecl()->markUsed(Context);
+
 }
 
 static void

lib/Serialization/ASTReaderDecl.cpp

       Previous->IdentifierNamespace &
       (Decl::IDNS_Ordinary | Decl::IDNS_Tag | Decl::IDNS_Type);
 
+  //Q: Redundant check with the new changes?
   // 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.

       // FIXME: This doesn't send the right notifications if there are
       // ASTMutationListeners other than an ASTWriter.
 
-      // Maintain AST consistency: any later redeclarations are used too.
-      forAllLaterRedecls(D, [](Decl *D) { D->Used = true; });
+      // // Maintain AST consistency: any later redeclarations are used too.
+      D->setIsUsed();
       break;
     }