[LifetimeSafety] Extend suggestions for lifetimebound to also warn on canonical declarations (llvm#198784)

With this patch, we suggest adding the `clang::lifetimebound` attribute
on the canonical declaration and on the earliest redeclaration in each
other file, preserving diagnostics for declarations visible from other
translation units while avoiding duplicate suggestions within the same
file.

Fixes llvm#198624
Fixes llvm#198628

Author: NeKon69

clang/lib/Analysis/LifetimeSafety/Checker.cpp

@@ -316,103 +316,83 @@ class LifetimeChecker {
     }
   }
 
-  std::pair<const FunctionDecl *, WarningScope>
-  getCanonicalFunctionDeclForAttr(const FunctionDecl *FDef) {
+  // Returns declarations that should be annotated with lifetime attributes
+  // in order to annotate FDef: the canonical declaration and the earliest
+  // redeclarations in each other file. This defines the placement policy for
+  // lifetime annotations. Each target is paired with its corresponding warning
+  // scope.
+  llvm::SmallVector<std::pair<const FunctionDecl *, WarningScope>, 2>
+  getTargetDeclsForAttr(const FunctionDecl *FDef) {
     if (!FDef)
-      return {nullptr, WarningScope::IntraTU};
+      return {};
 
     assert(FDef->isThisDeclarationADefinition() &&
            "Expected FunctionDecl to be a definition");
 
     const auto &SM = FDef->getASTContext().getSourceManager();
-    const FileID DefFile =
-        SM.getFileID(SM.getExpansionLoc(FDef->getLocation()));
-    const FunctionDecl *CanonicalDecl = FDef->getCanonicalDecl();
-    WarningScope Scope = WarningScope::IntraTU;
-
-    Scope = SM.getFileID(SM.getExpansionLoc(CanonicalDecl->getLocation())) !=
-                    DefFile
-                ? WarningScope::CrossTU
-                : WarningScope::IntraTU;
-
-    return {CanonicalDecl, Scope};
-  }
 
-  std::pair<const CXXMethodDecl *, WarningScope>
-  getCanonicalDeclForAttr(const CXXMethodDecl *MDef) {
-    auto [CanonicalFDecl, Scope] = getCanonicalFunctionDeclForAttr(MDef);
-    return {cast_or_null<CXXMethodDecl>(CanonicalFDecl), Scope};
-  }
+    auto GetFile = [&SM](const FunctionDecl *FD) {
+      return SM.getFileID(SM.getExpansionLoc(FD->getLocation()));
+    };
 
-  std::pair<const ParmVarDecl *, WarningScope>
-  getCanonicalDeclForAttr(const FunctionDecl *FDef, const ParmVarDecl *PVDDef) {
-    auto [CanonicalFDecl, Scope] = getCanonicalFunctionDeclForAttr(FDef);
-    if (!CanonicalFDecl)
-      return {nullptr, Scope};
-    return {CanonicalFDecl->getParamDecl(PVDDef->getFunctionScopeIndex()),
-            Scope};
-  }
+    const FileID DefFile = GetFile(FDef);
+    const FunctionDecl *CanonicalDecl = FDef->getCanonicalDecl();
+    llvm::SmallVector<std::pair<const FunctionDecl *, WarningScope>, 2> Targets{
+        {CanonicalDecl, GetFile(CanonicalDecl) == DefFile
+                            ? WarningScope::IntraTU
+                            : WarningScope::CrossTU}};
+
+    // Find the earliest redeclaration in each file other than the definition
+    // file.
+    auto AddCrossTUDecl = [&](const FunctionDecl *FD) {
+      FileID File = GetFile(FD);
+      if (File == DefFile)
+        return;
+      for (auto [SeenFD, _] : Targets)
+        if (GetFile(SeenFD) == File)
+          return;
+      Targets.push_back({FD, WarningScope::CrossTU});
+    };
 
-  /// Returns the declaration of a function that is visible across translation
-  /// units, if such a declaration exists and is different from the definition.
-  static const FunctionDecl *getCrossTUDecl(const FunctionDecl &FD,
-                                            SourceManager &SM) {
-    if (!FD.isExternallyVisible())
-      return nullptr;
-    const FileID DefinitionFile = SM.getFileID(FD.getLocation());
-    for (const FunctionDecl *Redecl : FD.redecls())
-      if (SM.getFileID(Redecl->getLocation()) != DefinitionFile)
-        return Redecl;
-
-    return nullptr;
-  }
+    // We iterate in reverse order (from most recent to oldest) to find
+    // the first declaration in each file.
+    for (const FunctionDecl *Redecl :
+         llvm::reverse(llvm::to_vector(FDef->redecls())))
+      AddCrossTUDecl(Redecl);
 
-  static const FunctionDecl *getCrossTUDecl(const ParmVarDecl &PVD,
-                                            SourceManager &SM) {
-    if (const auto *FD = dyn_cast<FunctionDecl>(PVD.getDeclContext()))
-      return getCrossTUDecl(*FD, SM);
-    return nullptr;
+    return Targets;
   }
 
-  static void suggestWithScopeForParmVar(LifetimeSafetySemaHelper *SemaHelper,
-                                         const ParmVarDecl *PVD,
-                                         SourceManager &SM,
-                                         EscapingTarget EscapeTarget) {
+  void suggestWithScopeForParmVar(const ParmVarDecl *PVD,
+                                  EscapingTarget EscapeTarget) {
     if (llvm::isa<const VarDecl *>(EscapeTarget))
       return;
 
-    if (const FunctionDecl *CrossTUDecl = getCrossTUDecl(*PVD, SM))
-      SemaHelper->suggestLifetimeboundToParmVar(
-          WarningScope::CrossTU,
-          CrossTUDecl->getParamDecl(PVD->getFunctionScopeIndex()),
-          EscapeTarget);
-    else
-      SemaHelper->suggestLifetimeboundToParmVar(WarningScope::IntraTU, PVD,
+    for (auto [Decl, Scope] : getTargetDeclsForAttr(cast<FunctionDecl>(FD))) {
+      const auto *ParmToAnnotate =
+          Decl->getParamDecl(PVD->getFunctionScopeIndex());
+      SemaHelper->suggestLifetimeboundToParmVar(Scope, ParmToAnnotate,
                                                 EscapeTarget);
+    }
   }
 
-  static void
-  suggestWithScopeForImplicitThis(LifetimeSafetySemaHelper *SemaHelper,
-                                  const CXXMethodDecl *MD, SourceManager &SM,
-                                  const Expr *EscapeExpr) {
-    if (const FunctionDecl *CrossTUDecl = getCrossTUDecl(*MD, SM))
+  void suggestWithScopeForImplicitThis(const CXXMethodDecl *MD,
+                                       const Expr *EscapeExpr) {
+    for (auto [Decl, Scope] : getTargetDeclsForAttr(MD)) {
       SemaHelper->suggestLifetimeboundToImplicitThis(
-          WarningScope::CrossTU, cast<CXXMethodDecl>(CrossTUDecl), EscapeExpr);
-    else
-      SemaHelper->suggestLifetimeboundToImplicitThis(WarningScope::IntraTU, MD,
-                                                     EscapeExpr);
+          Scope, cast<CXXMethodDecl>(Decl), EscapeExpr);
+    }
   }
 
   void suggestAnnotations() {
     if (!SemaHelper)
       return;
-    SourceManager &SM = AST.getSourceManager();
     for (auto [Target, EscapeTarget] : AnnotationWarningsMap) {
       if (const auto *PVD = Target.dyn_cast<const ParmVarDecl *>())
-        suggestWithScopeForParmVar(SemaHelper, PVD, SM, EscapeTarget);
+        suggestWithScopeForParmVar(PVD, EscapeTarget);
       else if (const auto *MD = Target.dyn_cast<const CXXMethodDecl *>()) {
         if (const auto *EscapeExpr = EscapeTarget.dyn_cast<const Expr *>())
-          suggestWithScopeForImplicitThis(SemaHelper, MD, SM, EscapeExpr);
+          suggestWithScopeForImplicitThis(MD, EscapeExpr);
         else
           llvm_unreachable("Implicit this can only escape via Expr (return)");
       }
@@ -458,23 +438,29 @@ class LifetimeChecker {
     const FunctionDecl *FDef = dyn_cast<FunctionDecl>(FD);
     if (!FDef)
       return;
+
+    auto TargetDecls = getTargetDeclsForAttr(FDef);
     // Check if implicit 'this' has lifetimebound on definition but not on
     // declaration.
     if (const auto *MDef = dyn_cast<CXXMethodDecl>(FDef);
         MDef && getDirectImplicitObjectLifetimeBoundAttr(MDef))
-      if (auto [MDecl, Scope] = getCanonicalDeclForAttr(MDef);
-          MDecl && !getDirectImplicitObjectLifetimeBoundAttr(MDecl))
-        SemaHelper->reportMisplacedLifetimebound(Scope, MDef, MDecl);
+      for (auto [Decl, Scope] : TargetDecls) {
+        const auto *MDecl = cast<CXXMethodDecl>(Decl);
+        if (!getDirectImplicitObjectLifetimeBoundAttr(MDecl))
+          SemaHelper->reportMisplacedLifetimebound(Scope, MDef, MDecl);
+      }
 
     // Check each parameter for explicit lifetimebound on definition but not on
     // declaration.
     for (const auto *PDef : FDef->parameters()) {
       const auto *Attr = PDef->getAttr<LifetimeBoundAttr>();
       if (!Attr || Attr->isImplicit())
         continue;
-      if (auto [PDecl, Scope] = getCanonicalDeclForAttr(FDef, PDef);
-          PDecl && !PDecl->hasAttr<LifetimeBoundAttr>())
-        SemaHelper->reportMisplacedLifetimebound(Scope, PDef, PDecl);
+      for (auto [Decl, Scope] : TargetDecls) {
+        const auto *PDecl = Decl->getParamDecl(PDef->getFunctionScopeIndex());
+        if (!PDecl->hasAttr<LifetimeBoundAttr>())
+          SemaHelper->reportMisplacedLifetimebound(Scope, PDef, PDecl);
+      }
     }
   }
 

clang/test/Sema/warn-lifetime-safety-fixits.cpp

@@ -69,12 +69,11 @@ int *arr_default(int a[2] = nullptr) {
   return a;
 }
 
-// FIXME: Iterate over redecls and add [[clang::lifetimebound]]
 View multi_decl(View a);
+// CHECK: :[[@LINE-1]]:17: warning: parameter in intra-TU function should be marked
+// CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:23-[[@LINE-2]]:23}:" {{\[\[}}clang::lifetimebound]]"
 View multi_decl(View a);
 View multi_decl(View a) {
-  // CHECK: :[[@LINE-1]]:17: warning: parameter in intra-TU function should be marked
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:23-[[@LINE-2]]:23}:" {{\[\[}}clang::lifetimebound]]"
   return a;
 }
 
@@ -145,10 +144,10 @@ struct OutOfLine {
   OutOfLine() {}
   ~OutOfLine() {}
   const OutOfLine &get() const;
+  // CHECK: :[[@LINE-1]]:31: warning: implicit this in intra-TU function should be marked
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:31-[[@LINE-2]]:31}:" {{\[\[}}clang::lifetimebound]]"
 };
 const OutOfLine &OutOfLine::get() const {
-  // CHECK: :[[@LINE-1]]:40: warning: implicit this in intra-TU function should be marked
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:40-[[@LINE-2]]:40}:" {{\[\[}}clang::lifetimebound]]"
   return *this;
 }
 

clang/test/Sema/warn-lifetime-safety-misplaced-lifetimebound-cross-tu.cpp

@@ -14,8 +14,18 @@ struct HeaderS {
   HeaderObj &header_this(); // expected-warning {{'lifetimebound' attribute on this definition is not visible to callers in other translation units; add it to the declaration instead}}
 };
 
+//--- cross_1.h
+struct HeaderObj;
+HeaderObj &multi_header_param(HeaderObj &obj); // expected-warning {{'lifetimebound' attribute on this definition is not visible to callers in other translation units; add it to the declaration instead}}
+
+//--- cross_2.h
+struct HeaderObj;
+HeaderObj &multi_header_param(HeaderObj &obj); // expected-warning {{'lifetimebound' attribute on this definition is not visible to callers in other translation units; add it to the declaration instead}}
+
 //--- cross.cpp
 #include "cross.h"
+#include "cross_1.h"
+#include "cross_2.h"
 
 HeaderObj &header_param(HeaderObj &obj [[clang::lifetimebound]]) { // expected-note {{'lifetimebound' attribute appears here on the definition}}
   return obj;
@@ -24,3 +34,7 @@ HeaderObj &header_param(HeaderObj &obj [[clang::lifetimebound]]) { // expected-n
 HeaderObj &HeaderS::header_this() [[clang::lifetimebound]] { // expected-note {{'lifetimebound' attribute appears here on the definition}}
   return data;
 }
+
+HeaderObj &multi_header_param(HeaderObj &obj [[clang::lifetimebound]]) { // expected-note 2 {{'lifetimebound' attribute appears here on the definition}}
+  return obj;
+}

clang/test/Sema/warn-lifetime-safety-misplaced-lifetimebound-intra-tu.cpp

@@ -9,6 +9,12 @@ MyObj &free_param(MyObj &obj [[clang::lifetimebound]]) { // expected-note {{'lif
   return obj;
 }
 
+MyObj &earliest_redecl_param(MyObj &obj); // expected-warning {{'lifetimebound' attribute on this definition is not visible to callers before the definition; add it to the declaration instead}}
+MyObj &earliest_redecl_param(MyObj &obj);
+MyObj &earliest_redecl_param(MyObj &obj [[clang::lifetimebound]]) { // expected-note {{'lifetimebound' attribute appears here on the definition}}
+  return obj;
+}
+
 struct S {
   MyObj data;
   const MyObj &implicit_this_only();         // expected-warning {{'lifetimebound' attribute on this definition is not visible to callers before the definition; add it to the declaration instead}}