Propagate lifetimebound from formal parameters to those in the canonical declaration and use that for analysis

[higher-performance]

This partially fixes #62072 by making sure that re-declarations of a function do not have the effect of removing lifetimebound from the canonical declaration.

It doesn't handle the implicit 'this' parameter, but that can be addressed in a separate fix.

[higher-performance]

@ilya-biryukov

[llvmbot]

@llvm/pr-subscribers-clang

Author: None (higher-performance)

Changes

This partially fixes #62072 by making sure that re-declarations of a function do not have the effect of removing lifetimebound from the canonical declaration.

It doesn't handle the implicit 'this' parameter, but that can be addressed in a separate fix.

---

Full diff: https://github.com/llvm/llvm-project/pull/107627.diff

3 Files Affected:

• (modified) clang/lib/Sema/CheckExprLifetime.cpp (+9-5)
• (modified) clang/lib/Sema/SemaAttr.cpp (+23-12)
• (modified) clang/test/SemaCXX/attr-lifetimebound.cpp (+4)

diff --git a/clang/lib/Sema/CheckExprLifetime.cpp b/clang/lib/Sema/CheckExprLifetime.cpp
index f1507ebb9a5068..b484117cecd3f4 100644
--- a/clang/lib/Sema/CheckExprLifetime.cpp
+++ b/clang/lib/Sema/CheckExprLifetime.cpp
@@ -462,14 +462,18 @@ static void visitFunctionCallArguments(IndirectLocalPath &Path, Expr *Call,
     }
   }
 
+  const FunctionDecl *CanonCallee = Callee->getCanonicalDecl();
   for (unsigned I = 0,
-                N = std::min<unsigned>(Callee->getNumParams(), Args.size());
+                N = std::min<unsigned>(std::min(Callee->getNumParams(),
+                                                CanonCallee->getNumParams()),
+                                       Args.size());
        I != N; ++I) {
-    if (CheckCoroCall || Callee->getParamDecl(I)->hasAttr<LifetimeBoundAttr>())
-      VisitLifetimeBoundArg(Callee->getParamDecl(I), Args[I]);
+    if (CheckCoroCall ||
+        CanonCallee->getParamDecl(I)->hasAttr<LifetimeBoundAttr>())
+      VisitLifetimeBoundArg(CanonCallee->getParamDecl(I), Args[I]);
     else if (EnableGSLAnalysis && I == 0) {
-      if (shouldTrackFirstArgument(Callee)) {
-        VisitGSLPointerArg(Callee, Args[0]);
+      if (shouldTrackFirstArgument(CanonCallee)) {
+        VisitGSLPointerArg(CanonCallee, Args[0]);
       } else if (auto *CCE = dyn_cast<CXXConstructExpr>(Call);
                  CCE &&
                  CCE->getConstructor()->getParent()->hasAttr<PointerAttr>()) {
diff --git a/clang/lib/Sema/SemaAttr.cpp b/clang/lib/Sema/SemaAttr.cpp
index fb594e6f13c0b3..e546d031229a77 100644
--- a/clang/lib/Sema/SemaAttr.cpp
+++ b/clang/lib/Sema/SemaAttr.cpp
@@ -217,7 +217,8 @@ void Sema::inferGslOwnerPointerAttribute(CXXRecordDecl *Record) {
 }
 
 void Sema::inferLifetimeBoundAttribute(FunctionDecl *FD) {
-  if (FD->getNumParams() == 0)
+  const unsigned int NumParams = FD->getNumParams();
+  if (NumParams == 0)
     return;
 
   if (unsigned BuiltinID = FD->getBuiltinID()) {
@@ -239,18 +240,13 @@ void Sema::inferLifetimeBoundAttribute(FunctionDecl *FD) {
     default:
       break;
     }
-    return;
-  }
-  if (auto *CMD = dyn_cast<CXXMethodDecl>(FD)) {
-    const auto *CRD = CMD->getParent();
-    if (!CRD->isInStdNamespace() || !CRD->getIdentifier())
-      return;
-
-    if (isa<CXXConstructorDecl>(CMD)) {
+  } else if (auto *CMD = dyn_cast<CXXMethodDecl>(FD)) {
+    const CXXRecordDecl *CRD = CMD->getParent();
+    if (CRD->isInStdNamespace() && CRD->getIdentifier() &&
+        isa<CXXConstructorDecl>(CMD)) {
       auto *Param = CMD->getParamDecl(0);
-      if (Param->hasAttr<LifetimeBoundAttr>())
-        return;
-      if (CRD->getName() == "basic_string_view" &&
+      if (!Param->hasAttr<LifetimeBoundAttr>() &&
+          CRD->getName() == "basic_string_view" &&
           Param->getType()->isPointerType()) {
         // construct from a char array pointed by a pointer.
         //   basic_string_view(const CharT* s);
@@ -266,6 +262,21 @@ void Sema::inferLifetimeBoundAttribute(FunctionDecl *FD) {
               LifetimeBoundAttr::CreateImplicit(Context, FD->getLocation()));
       }
     }
+  } else if (auto *CanonDecl = FD->getCanonicalDecl(); FD != CanonDecl) {
+    // Propagate the lifetimebound attribute from parameters to the canonical
+    // declaration.
+    // Note that this doesn't include the implicit 'this' parameter, as the
+    // attribute is applied to the function type in that case.
+    unsigned int NP = std::min(NumParams, CanonDecl->getNumParams());
+    assert(NP == NumParams);
+    for (unsigned int I = 0; I < NP; ++I) {
+      auto *CanonParam = CanonDecl->getParamDecl(I);
+      if (!CanonParam->hasAttr<LifetimeBoundAttr>() &&
+          FD->getParamDecl(I)->hasAttr<LifetimeBoundAttr>()) {
+        CanonParam->addAttr(LifetimeBoundAttr::CreateImplicit(
+            Context, CanonParam->getLocation()));
+      }
+    }
   }
 }
 
diff --git a/clang/test/SemaCXX/attr-lifetimebound.cpp b/clang/test/SemaCXX/attr-lifetimebound.cpp
index 0fb997a5671085..c0255e9a9f7280 100644
--- a/clang/test/SemaCXX/attr-lifetimebound.cpp
+++ b/clang/test/SemaCXX/attr-lifetimebound.cpp
@@ -19,6 +19,9 @@ namespace usage_invalid {
 namespace usage_ok {
   struct IntRef { int *target; };
 
+  const int &crefparam(const int &param); // Omitted in first decl
+  const int &crefparam(const int &param [[clang::lifetimebound]]); // Add LB
+  const int &crefparam(const int &param) { return param; } // Omit in impl
   int &refparam(int &param [[clang::lifetimebound]]);
   int &classparam(IntRef param [[clang::lifetimebound]]);
 
@@ -41,6 +44,7 @@ namespace usage_ok {
   int *p = A().class_member(); // expected-warning {{temporary whose address is used as value of local variable 'p' will be destroyed at the end of the full-expression}}
   int *q = A(); // expected-warning {{temporary whose address is used as value of local variable 'q' will be destroyed at the end of the full-expression}}
   int *r = A(1); // expected-warning {{temporary whose address is used as value of local variable 'r' will be destroyed at the end of the full-expression}}
+  const int& s = crefparam(2); // expected-warning {{temporary bound to local reference 's' will be destroyed at the end of the full-expression}}
 
   void test_assignment() {
     p = A().class_member(); // expected-warning {{object backing the pointer p will be destroyed at the end of the full-expression}}

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[ilya-biryukov]

I really like the current implementation, but have one major suggestion about usingInheritableParamAttr.

Also, could we add a release note for it?

[ilya-biryukov]

It feels that LifetimeBoundAttr might need to be an InheritableParamAttr itself, which would allow to avoid having a template altogether.

Are there any downsides to that?

(I'm assuming that InheritableParamAttr represents the attributes that should be added from redeclarations, not just from the base methods of overridden members)

[higher-performance]

It doesn't seem like InheritableParamAttr works on types. In any case, other attributes indicate they have this problem too, so let's worry about solving it separately for all of them in the future?

[ilya-biryukov]

It doesn't seem like InheritableParamAttr works on types. In any case, other attributes indicate they have this problem too, so let's worry about solving it separately for all of them in the future?

okay, that's something we can also do in a follow-up.

[higher-performance]

Okay, I believe this PR is ready. Could we merge it?

[ilya-biryukov]

Thanks and sorry for the long review delays.
LGTM, let's work on the details in follow-ups.