[clang] Implement lifetime analysis for lifetime_capture_by(X)

[usx95]

This PR uses the existing lifetime analysis for the capture_by attribute.

In a followup patch, I will

The analysis is behind -Wdangling-capture warning and is disabled by default for now. Once it is found to be stable, it will be default enabled.

Followup:

• add implicit inference of this attribute on STL container methods like std::vector::push_back.
• (consider) warning if capturing X cannot capture anything. It should be a reference, pointer or a view type.
• refactoring temporary visitors and other related handlers.
• start discussing __global vs global in the annotation in a separate PR.

[github-actions]

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

[llvmbot]

@llvm/pr-subscribers-clang

Author: Utkarsh Saxena (usx95)

Changes

---

Patch is 21.29 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/115921.diff

8 Files Affected:

• (modified) clang/include/clang/Basic/DiagnosticGroups.td (+2)
• (modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (+5-2)
• (modified) clang/include/clang/Sema/Sema.h (+3)
• (modified) clang/lib/Sema/CheckExprLifetime.cpp (+52-14)
• (modified) clang/lib/Sema/CheckExprLifetime.h (+13)
• (modified) clang/lib/Sema/SemaChecking.cpp (+46-1)
• (modified) clang/test/Sema/warn-lifetime-analysis-nocfg.cpp (+105)
• (modified) clang/test/SemaCXX/attr-lifetimebound.cpp (+2-2)

diff --git a/clang/include/clang/Basic/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td
index 72eada50a56cc9..df9bf94b5d0398 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -453,6 +453,7 @@ def ShiftOpParentheses: DiagGroup<"shift-op-parentheses">;
 def OverloadedShiftOpParentheses: DiagGroup<"overloaded-shift-op-parentheses">;
 def DanglingAssignment: DiagGroup<"dangling-assignment">;
 def DanglingAssignmentGsl : DiagGroup<"dangling-assignment-gsl">;
+def DanglingCapture : DiagGroup<"dangling-capture">;
 def DanglingElse: DiagGroup<"dangling-else">;
 def DanglingField : DiagGroup<"dangling-field">;
 def DanglingInitializerList : DiagGroup<"dangling-initializer-list">;
@@ -462,6 +463,7 @@ def ReturnStackAddress : DiagGroup<"return-stack-address">;
 def : DiagGroup<"return-local-addr", [ReturnStackAddress]>;
 def Dangling : DiagGroup<"dangling", [DanglingAssignment,
                                       DanglingAssignmentGsl,
+                                      DanglingCapture,
                                       DanglingField,
                                       DanglingInitializerList,
                                       DanglingGsl,
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 2f5d672e2f0035..58745d450ed63f 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -10132,10 +10132,10 @@ def err_lifetimebound_ctor_dtor : Error<
   "%select{constructor|destructor}0">;
 def err_lifetimebound_parameter_void_return_type : Error<
   "'lifetimebound' attribute cannot be applied to a parameter of a function "
-  "that returns void">;
+  "that returns void; did you mean 'lifetime_capture_by(X)'">;
 def err_lifetimebound_implicit_object_parameter_void_return_type : Error<
   "'lifetimebound' attribute cannot be applied to an implicit object "
-  "parameter of a function that returns void">;
+  "parameter of a function that returns void; did you mean 'lifetime_capture_by(X)'">;
 
 // CHECK: returning address/reference of stack memory
 def warn_ret_stack_addr_ref : Warning<
@@ -10230,6 +10230,9 @@ def warn_dangling_pointer_assignment : Warning<
    "object backing %select{|the pointer }0%1 "
    "will be destroyed at the end of the full-expression">,
    InGroup<DanglingAssignment>;
+def warn_dangling_reference_captured : Warning<
+   "object captured by '%0' will be destroyed at the end of the full-expression">,
+   InGroup<DanglingCapture>;
 
 // For non-floating point, expressions of the form x == x or x != x
 // should result in a warning, since these always evaluate to a constant.
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index d6f3508a5243f3..6ea6c67447b6f0 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -2323,6 +2323,9 @@ class Sema final : public SemaBase {
   bool BuiltinVectorMath(CallExpr *TheCall, QualType &Res, bool FPOnly = false);
   bool BuiltinVectorToScalarMath(CallExpr *TheCall);
 
+  void checkLifetimeCaptureBy(FunctionDecl *FDecl, bool IsMemberFunction,
+                              const Expr *ThisArg, ArrayRef<const Expr *> Args);
+
   /// Handles the checks for format strings, non-POD arguments to vararg
   /// functions, NULL arguments passed to non-NULL parameters, diagnose_if
   /// attributes and AArch64 SME attributes.
diff --git a/clang/lib/Sema/CheckExprLifetime.cpp b/clang/lib/Sema/CheckExprLifetime.cpp
index a1a402b4a2b530..81e26f48fb8851 100644
--- a/clang/lib/Sema/CheckExprLifetime.cpp
+++ b/clang/lib/Sema/CheckExprLifetime.cpp
@@ -45,10 +45,14 @@ enum LifetimeKind {
   /// a default member initializer), the program is ill-formed.
   LK_MemInitializer,
 
-  /// The lifetime of a temporary bound to this entity probably ends too soon,
+  /// The lifetime of a temporary bound to this entity may end too soon,
   /// because the entity is a pointer and we assign the address of a temporary
   /// object to it.
   LK_Assignment,
+
+  /// The lifetime of a temporary bound to this entity may end too soon,
+  /// because the entity may capture the reference to a temporary object.
+  LK_LifetimeCapture,
 };
 using LifetimeResult =
     llvm::PointerIntPair<const InitializedEntity *, 3, LifetimeKind>;
@@ -193,6 +197,7 @@ struct IndirectLocalPathEntry {
     VarInit,
     LValToRVal,
     LifetimeBoundCall,
+    LifetimeCapture,
     TemporaryCopy,
     LambdaCaptureInit,
     GslReferenceInit,
@@ -249,9 +254,10 @@ static void visitLocalsRetainedByReferenceBinding(IndirectLocalPath &Path,
                                                   LocalVisitor Visit);
 
 template <typename T> static bool isRecordWithAttr(QualType Type) {
-  if (auto *RD = Type->getAsCXXRecordDecl())
-    return RD->hasAttr<T>();
-  return false;
+  CXXRecordDecl *RD = Type.getNonReferenceType()->getAsCXXRecordDecl();
+  if (!RD)
+    return false;
+  return RD->hasAttr<T>();
 }
 
 // Decl::isInStdNamespace will return false for iterators in some STL
@@ -1049,6 +1055,7 @@ static SourceRange nextPathEntryRange(const IndirectLocalPath &Path, unsigned I,
     case IndirectLocalPathEntry::AddressOf:
     case IndirectLocalPathEntry::LValToRVal:
     case IndirectLocalPathEntry::LifetimeBoundCall:
+    case IndirectLocalPathEntry::LifetimeCapture:
     case IndirectLocalPathEntry::TemporaryCopy:
     case IndirectLocalPathEntry::GslReferenceInit:
     case IndirectLocalPathEntry::GslPointerInit:
@@ -1082,6 +1089,7 @@ static bool pathOnlyHandlesGslPointer(const IndirectLocalPath &Path) {
     case IndirectLocalPathEntry::VarInit:
     case IndirectLocalPathEntry::AddressOf:
     case IndirectLocalPathEntry::LifetimeBoundCall:
+    case IndirectLocalPathEntry::LifetimeCapture:
       continue;
     case IndirectLocalPathEntry::GslPointerInit:
     case IndirectLocalPathEntry::GslReferenceInit:
@@ -1110,12 +1118,13 @@ static bool shouldRunGSLAssignmentAnalysis(const Sema &SemaRef,
            isAssignmentOperatorLifetimeBound(Entity.AssignmentOperator)));
 }
 
-static void checkExprLifetimeImpl(Sema &SemaRef,
-                                  const InitializedEntity *InitEntity,
-                                  const InitializedEntity *ExtendingEntity,
-                                  LifetimeKind LK,
-                                  const AssignedEntity *AEntity, Expr *Init) {
+static void
+checkExprLifetimeImpl(Sema &SemaRef, const InitializedEntity *InitEntity,
+                      const InitializedEntity *ExtendingEntity, LifetimeKind LK,
+                      const AssignedEntity *AEntity,
+                      const CapturingEntity *CapEntity, Expr *Init) {
   assert((AEntity && LK == LK_Assignment) ||
+         (CapEntity && LK == LK_LifetimeCapture) ||
          (InitEntity && LK != LK_Assignment));
   // If this entity doesn't have an interesting lifetime, don't bother looking
   // for temporaries within its initializer.
@@ -1199,6 +1208,17 @@ static void checkExprLifetimeImpl(Sema &SemaRef,
       break;
     }
 
+    case LK_LifetimeCapture: {
+      if (!MTE)
+        return false;
+      assert(shouldLifetimeExtendThroughPath(Path) ==
+                 PathLifetimeKind::NoExtend &&
+             "No lifetime extension in function calls");
+      SemaRef.Diag(DiagLoc, diag::warn_dangling_reference_captured)
+          << CapEntity->Entity << DiagRange;
+      return false;
+    }
+
     case LK_Assignment: {
       if (!MTE || pathContainsInit(Path))
         return false;
@@ -1359,6 +1379,7 @@ static void checkExprLifetimeImpl(Sema &SemaRef,
         break;
 
       case IndirectLocalPathEntry::LifetimeBoundCall:
+      case IndirectLocalPathEntry::LifetimeCapture:
       case IndirectLocalPathEntry::TemporaryCopy:
       case IndirectLocalPathEntry::GslPointerInit:
       case IndirectLocalPathEntry::GslReferenceInit:
@@ -1411,7 +1432,7 @@ static void checkExprLifetimeImpl(Sema &SemaRef,
     // warnings or errors on inner temporaries within this one's initializer.
     return false;
   };
-
+  bool HasReferenceBinding = Init->isGLValue();
   llvm::SmallVector<IndirectLocalPathEntry, 8> Path;
   if (LK == LK_Assignment &&
       shouldRunGSLAssignmentAnalysis(SemaRef, *AEntity)) {
@@ -1420,9 +1441,18 @@ static void checkExprLifetimeImpl(Sema &SemaRef,
              ? IndirectLocalPathEntry::LifetimeBoundCall
              : IndirectLocalPathEntry::GslPointerAssignment,
          Init});
+  } else if (LK == LK_LifetimeCapture) {
+    Path.push_back({IndirectLocalPathEntry::LifetimeCapture, Init});
+    if (isRecordWithAttr<PointerAttr>(Init->getType()))
+      HasReferenceBinding = false;
+    // Skip the top MTE if it is a temporary object of the pointer-like type
+    // itself.
+    if (auto *MTE = dyn_cast<MaterializeTemporaryExpr>(Init);
+        MTE && isPointerLikeType(Init->getType()))
+      Init = MTE->getSubExpr();
   }
 
-  if (Init->isGLValue())
+  if (HasReferenceBinding)
     visitLocalsRetainedByReferenceBinding(Path, Init, RK_ReferenceBinding,
                                           TemporaryVisitor);
   else
@@ -1438,13 +1468,13 @@ void checkExprLifetime(Sema &SemaRef, const InitializedEntity &Entity,
   LifetimeKind LK = LTResult.getInt();
   const InitializedEntity *ExtendingEntity = LTResult.getPointer();
   checkExprLifetimeImpl(SemaRef, &Entity, ExtendingEntity, LK,
-                        /*AEntity*/ nullptr, Init);
+                        /*AEntity*/ nullptr, /*CapEntity=*/nullptr, Init);
 }
 
 void checkExprLifetimeMustTailArg(Sema &SemaRef,
                                   const InitializedEntity &Entity, Expr *Init) {
   checkExprLifetimeImpl(SemaRef, &Entity, nullptr, LK_MustTail,
-                        /*AEntity*/ nullptr, Init);
+                        /*AEntity*/ nullptr, /*CapEntity=*/nullptr, Init);
 }
 
 void checkExprLifetime(Sema &SemaRef, const AssignedEntity &Entity,
@@ -1460,7 +1490,15 @@ void checkExprLifetime(Sema &SemaRef, const AssignedEntity &Entity,
 
   checkExprLifetimeImpl(SemaRef, /*InitEntity=*/nullptr,
                         /*ExtendingEntity=*/nullptr, LK_Assignment, &Entity,
-                        Init);
+                        /*CapEntity=*/nullptr, Init);
+}
+
+void checkExprLifetime(Sema &SemaRef, const CapturingEntity &Entity,
+                       Expr *Init) {
+  return checkExprLifetimeImpl(SemaRef, /*InitEntity=*/nullptr,
+                               /*ExtendingEntity=*/nullptr, LK_LifetimeCapture,
+                               /*AEntity=*/nullptr,
+                               /*CapEntity=*/&Entity, Init);
 }
 
 } // namespace clang::sema
diff --git a/clang/lib/Sema/CheckExprLifetime.h b/clang/lib/Sema/CheckExprLifetime.h
index 903f312f3533e5..e109b73b280e9e 100644
--- a/clang/lib/Sema/CheckExprLifetime.h
+++ b/clang/lib/Sema/CheckExprLifetime.h
@@ -25,6 +25,16 @@ struct AssignedEntity {
   CXXMethodDecl *AssignmentOperator = nullptr;
 };
 
+struct CapturingEntity {
+  //  In an function call involving a lifetime capture, this would be the
+  //  argument capturing the lifetime of another argument.
+  //    void addToSet(std::string_view sv [[clang::lifetime_capture_by(setsv)]],
+  //                  set<std::string_view>& setsv);
+  //    set<std::string_view> setsv;
+  //    addToSet(std::string(), setsv); // Here 'setsv' is the 'Entity'.
+  Expr *Entity = nullptr;
+};
+
 /// Check that the lifetime of the given expr (and its subobjects) is
 /// sufficient for initializing the entity, and perform lifetime extension
 /// (when permitted) if not.
@@ -35,6 +45,9 @@ void checkExprLifetime(Sema &SemaRef, const InitializedEntity &Entity,
 /// sufficient for assigning to the entity.
 void checkExprLifetime(Sema &SemaRef, const AssignedEntity &Entity, Expr *Init);
 
+void checkExprLifetime(Sema &SemaRef, const CapturingEntity &Entity,
+                       Expr *Init);
+
 /// Check that the lifetime of the given expr (and its subobjects) is
 /// sufficient, assuming that it is passed as an argument to a musttail
 /// function.
diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp
index 96008b14225a4c..27edb56df3abc7 100644
--- a/clang/lib/Sema/SemaChecking.cpp
+++ b/clang/lib/Sema/SemaChecking.cpp
@@ -11,6 +11,7 @@
 //
 //===----------------------------------------------------------------------===//
 
+#include "CheckExprLifetime.h"
 #include "clang/AST/APValue.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/Attr.h"
@@ -3229,6 +3230,49 @@ void Sema::CheckArgAlignment(SourceLocation Loc, NamedDecl *FDecl,
         << ParamName << (FDecl != nullptr) << FDecl;
 }
 
+void Sema::checkLifetimeCaptureBy(FunctionDecl *FD, bool IsMemberFunction,
+                                  const Expr *ThisArg,
+                                  ArrayRef<const Expr *> Args) {
+  auto GetArgAt = [&](int Idx) {
+    if (IsMemberFunction && Idx == 0)
+      return const_cast<Expr *>(ThisArg);
+    return const_cast<Expr *>(Args[Idx - int(IsMemberFunction)]);
+  };
+  for (unsigned I = 0; I < FD->getNumParams(); ++I) {
+    auto *CapturedByAttr =
+        FD->getParamDecl(I)->getAttr<LifetimeCaptureByAttr>();
+    if (!CapturedByAttr)
+      continue;
+    for (int CapturingParamIdx : CapturedByAttr->params()) {
+      Expr *Capturing = GetArgAt(CapturingParamIdx);
+      Expr *Captured = GetArgAt(I + IsMemberFunction);
+      CapturingEntity CE{Capturing};
+      // Ensure that 'Captured' outlives the 'Capturing' entity.
+      checkExprLifetime(*this, CE, Captured);
+    }
+  }
+  // Check when the 'this' object is captured.
+  if (IsMemberFunction) {
+    TypeSourceInfo *TSI = FD->getTypeSourceInfo();
+    if (!TSI)
+      return;
+    AttributedTypeLoc ATL;
+    for (TypeLoc TL = TSI->getTypeLoc();
+         (ATL = TL.getAsAdjusted<AttributedTypeLoc>());
+         TL = ATL.getModifiedLoc()) {
+      auto *CapturedByAttr = ATL.getAttrAs<LifetimeCaptureByAttr>();
+      if (!CapturedByAttr)
+        continue;
+      Expr *Captured = GetArgAt(0);
+      for (int CapturingParamIdx : CapturedByAttr->params()) {
+        Expr *Capturing = GetArgAt(CapturingParamIdx);
+        CapturingEntity CE{Capturing};
+        checkExprLifetime(*this, CE, Captured);
+      }
+    }
+  }
+}
+
 void Sema::checkCall(NamedDecl *FDecl, const FunctionProtoType *Proto,
                      const Expr *ThisArg, ArrayRef<const Expr *> Args,
                      bool IsMemberFunction, SourceLocation Loc,
@@ -3269,7 +3313,8 @@ void Sema::checkCall(NamedDecl *FDecl, const FunctionProtoType *Proto,
       }
     }
   }
-
+  if (FD)
+    checkLifetimeCaptureBy(FD, IsMemberFunction, ThisArg, Args);
   if (FDecl || Proto) {
     CheckNonNullArguments(*this, FDecl, Proto, Args, Loc);
 
diff --git a/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp b/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
index 6a2af01ea5116c..49bbc05c009ff9 100644
--- a/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
+++ b/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
@@ -793,3 +793,108 @@ void test13() {
 }
 
 } // namespace GH100526
+
+namespace lifetime_capture_by {
+struct S {
+  const int *x;
+  void captureInt(const int&x [[clang::lifetime_capture_by(this)]]) { this->x = &x; }
+  void captureSV(std::string_view sv [[clang::lifetime_capture_by(this)]]);
+};
+///////////////////////////
+// Detect dangling cases.
+///////////////////////////
+void captureInt(const int&x [[clang::lifetime_capture_by(s)]], S&s);
+void captureRValInt(int&&x [[clang::lifetime_capture_by(s)]], S&s);
+void noCaptureInt(int x [[clang::lifetime_capture_by(s)]], S&s);
+std::string_view substr(const std::string& s [[clang::lifetimebound]]);
+std::string_view strcopy(const std::string& s);
+void captureSV(std::string_view x [[clang::lifetime_capture_by(s)]], S&s);
+void captureRValSV(std::string_view&& x [[clang::lifetime_capture_by(s)]], S&s);
+void noCaptureSV(std::string_view x, S&s);
+void captureS(const std::string& x [[clang::lifetime_capture_by(s)]], S&s);
+void captureRValS(std::string&& x [[clang::lifetime_capture_by(s)]], S&s);
+const std::string* getPointerLB(const std::string& s[[clang::lifetimebound]]);
+const std::string* getPointerNoLB(const std::string& s);
+void capturePointer(const std::string* x [[clang::lifetime_capture_by(s)]], S&s);
+struct ThisIsCaptured {
+  void capture(S& s) [[clang::lifetime_capture_by(s)]];
+  void bar(S& s) [[clang::lifetime_capture_by(abcd)]]; // expected-error {{'lifetime_capture_by' attribute argument 'abcd' is not a known function parameter}}
+  void baz(S& s) [[clang::lifetime_capture_by(this)]]; // expected-error {{'lifetime_capture_by' argument references itself}}
+};
+void use() {
+  std::string_view local_sv;
+  std::string local_s;
+  S s;
+  // Capture an 'int'.
+  int local;
+  captureInt(1, // expected-warning {{object captured by 's' will be destroyed at the end of the full-expression}}
+            s);
+  captureRValInt(1, s); // expected-warning {{object captured by 's'}}
+  captureInt(local, s);
+  noCaptureInt(1, s);
+  noCaptureInt(local, s);
+
+  // Capture lifetimebound pointer.
+  capturePointer(getPointerLB(std::string()), s); // expected-warning {{object captured by 's'}}
+  capturePointer(getPointerLB(*getPointerLB(std::string())), s); // expected-warning {{object captured by 's'}}
+  capturePointer(getPointerNoLB(std::string()), s);
+
+  // Capture using std::string_view.
+  captureSV(local_sv, s);
+  captureSV(std::string(), // expected-warning {{object captured by 's'}}
+            s);
+  captureSV(substr(
+      std::string() // expected-warning {{object captured by 's'}}
+      ), s);
+  captureSV(substr(local_s), s);
+  captureSV(strcopy(std::string()), s);
+  captureRValSV(std::move(local_sv), s);
+  captureRValSV(std::string(), s); // expected-warning {{object captured by 's'}}
+  captureRValSV(std::string_view{"abcd"}, s);
+  captureRValSV(substr(local_s), s);
+  captureRValSV(substr(std::string()), s); // expected-warning {{object captured by 's'}}
+  captureRValSV(strcopy(std::string()), s);
+  noCaptureSV(local_sv, s);
+  noCaptureSV(std::string(), s);
+  noCaptureSV(substr(std::string()), s);
+
+  // Capture using std::string.
+  captureS(std::string(), s); // expected-warning {{object captured by 's'}}
+  captureS(local_s, s);
+  captureRValS(std::move(local_s), s);
+  captureRValS(std::string(), s); // expected-warning {{object captured by 's'}}
+
+  // Member functions.
+  s.captureInt(1); // expected-warning {{object captured by 's'}}
+  s.captureSV(std::string()); // expected-warning {{object captured by 's'}}
+  s.captureSV(substr(std::string())); // expected-warning {{object captured by 's'}}
+  s.captureSV(strcopy(std::string()));
+
+  // 'this' is captured.
+  ThisIsCaptured{}.capture(s); // expected-warning {{object captured by 's'}}
+  ThisIsCaptured TIS;
+  TIS.capture(s);
+}
+class [[gsl::Pointer()]] my_string_view : public std::string_view {};
+class my_string_view_not_pointer : public std::string_view {};
+std::optional<std::string_view> getOptionalSV();
+std::optional<std::string> getOptionalS();
+std::optional<my_string_view> getOptionalMySV();
+std::optional<my_string_view_not_pointer> getOptionalMySVNotP();
+my_string_view getMySV();
+my_string_view_not_pointer getMySVNotP();
+
+template<class T>
+struct MySet {
+void insert(T&& t [[clang::lifetime_capture_by(this)]]);
+void insert(const T& t [[clang::lifetime_capture_by(this)]]);
+};
+void user_defined_containers() {
+  MySet<int> set_of_int;
+  set_of_int.insert(1); // expected-warning {{object captured by 'set_of_int' will be destroyed}}
+  MySet<std::string_view> set_of_sv;
+  set_of_sv.insert(std::string());  // expected-warning {{object captured by 'set_of_sv' will be destroyed}}
+}
+} // namespace lifetime_capture_by
+// Test for templated code.
+// 2 nested function calls foo(sv, bar(sv, setsv));
\ No newline at end of file
diff --git a/clang/test/SemaCXX/attr-lifetimebound.cpp b/clang/test/SemaCXX/attr-lifetimebound.cpp
index 81e9193cf76a04..f89b556f5bba08 100644
--- a/clang/test/SemaCXX/attr-lifetimebound.cpp
+++ b/clang/test/SemaCXX/attr-lifetimebound.cpp
@@ -1,7 +1,7 @@
 // RUN: %clang_cc1 -std=c++23 -verify %s
 
 namespace usage_invalid {
-  void void_return(int &param [[clang::lifetimebound]]); // expected-error {{'lifetimebound' attribute cannot be applied to a parameter of a function that...
[truncated]

[Xazax-hun]

This is a false positive, right? We should probably have a FIXME or something similar.
Also, I wonder if we could suppress warnings from template instantiations until we figure out how to handle these cases.

[usx95]

This is now WAI. The problem is same with lifetimebound annotation as well.
Internally, at Google, we used specialised type traits to distinguish between pointer-type and other element types.
Added a few tests to describe it. I agree it is not the most convenient, but it seems like there is no better solution than this.

[Xazax-hun]

I would argue that either the annotation on MySet is wrong, or if we want that annotation to be correct, we need to handle these cases without false positives. I think this is the case for lifetimebound as well. For the equivalent false positives we either should not have the lifetimebound annotation in the first place since it is incorrect for some of the instantiations, or we should have a way to handle it correctly.

I think currently I am leaning in the direction of saying that the annotation on a template is not correct if it is not correct for some of the instantiations.

We either should tell our users to have ifdefed versions of these APIs and only add these annotations to the overloads where they are correct for all instantiations, or we need to have a conditional version of these annotations where the user can somehow select what specializations should this be applied to.

Overall, I don't think this problem is a blocker for this PR, but I would definitely oppose any automatic inference for templated types where the annotation might be incorrect for some of the instantiations.

[hokein]

I believe the current behavior is working as intended, although it doesn't align with user expectations.

This issue arises when involving a gsl::pointer type with a templated constructor that takes a const reference:

#include <vector>

template <typename T>
struct [[gsl::Pointer]] MySpan2 {
   template <typename Container>
   MySpan2(const Container& container);
};

template <typename T>
struct [[gsl::Pointer]] MySpan {
   template <typename Container>
   MySpan(const Container& container [[clang::lifetimebound]]);
};

void test() {
   MySpan<int> t1(std::vector{1}); // 1) warn
   
   std::vector<int> v;
   MySpan<int> t2 = MySpan2<int>(v); // 2) warn on MySpan2, but we expect no warning
   MySpan<int> t3 = MySpan2<int>(std::vector<int>{}); // 3) warn on MySpan2, but we expect warn on the std::vector<int>
}

Per the lifetimebound documentation, for a reference type, Clang considers it referenced type -- in case 2), this is MySpan2 itself. However, this mismatch user expectations. We want t2 to be lifetimebound to the underlying storage of MySpan2 (which is v). And specifically in case 3), we intend to diagnose the temporary std::vector<int>, rather than flagging the temporary MySpan2 object.

Some options:

1. Add specific overloads for pointer types that are not annotated with lifetimebound. (This is an approach we've used internally);
2. Adjust lifetimebound analysis to accommodate gsl::pointer types. Specifically, for a reference to a GSL pointer type (e.g., const MyPointer&), we could consider its pointee instead. However, this has trade-offs: it would prevent diagnostics when the GSL pointer itself is dangling, I think this is probably fine, this is not an important case.
3. Avoid using lifetimebound in the constructor and use GSLOwner instead. When a GSLPointer is constructed from a GSLOwner object, Clang will diagnose if the GSLOwner is a temporary. In the above, removing clang::lifetimebound would address these issues correctly https://godbolt.org/z/Mq6rKhz99 (std::vector is implicitly annotated as gsl::Owner by default).

CC @higher-performance, who might have some thoughts on this.

[higher-performance]

The fundamental problem here (and the reason you see an expectation mismatch) is that Container is misnamed. It's an unconstrained templated type, so we may as well call it T or U -- it could be anything. It seems the intention here was to have it be Range (a range may or may not actually contain the elements), and once we name it that, it becomes obvious that we can't lifetimebound it -- because that constructor could just be copying the range.

So (1) seems like the correct solution to me. It's certainly verbose, but if that's the concern then the fix would be to find some way to make lifetimebound conditional (maybe like noexcept(noexcept(...))?) rather than by changing the analysis.

[bricknerb]

First pass, still didn't dive fully into the logic.

[Xazax-hun]

LGTM!

[bricknerb]

Please request another review after comments are addressed.

[hokein]

I know that this code originated from my patch that added assignment support for "container." That patch has some issues: although it passed the unit tests, it failed to diagnose cases in real STL headers.

I'm not sure I fully understand the implementation here (my initial idea was to leverage the existing code paths for lifetimebound and gsl::Pointer, so the need for IndirectLocalPathEntry::LifetimeCapture isn’t clear). I experimented this patch, and made some tweaks to simplify the logic, and here’s the diff I came up with:

--- a/clang/lib/Sema/CheckExprLifetime.cpp
+++ b/clang/lib/Sema/CheckExprLifetime.cpp
@@ -1212,9 +1212,9 @@ checkExprLifetimeImpl(Sema &SemaRef, const InitializedEntity *InitEntity,
       // and the capturing entity does too, so don't warn.
       if (!MTE)
         return false;
-      assert(shouldLifetimeExtendThroughPath(Path) ==
-                 PathLifetimeKind::NoExtend &&
-             "No lifetime extension in function calls");
+      // assert(shouldLifetimeExtendThroughPath(Path) ==
+      //            PathLifetimeKind::NoExtend &&
+      //        "No lifetime extension in function calls");
       if (CapEntity->Entity)
         SemaRef.Diag(DiagLoc, diag::warn_dangling_reference_captured)
             << CapEntity->Entity << DiagRange;
@@ -1451,14 +1451,8 @@ checkExprLifetimeImpl(Sema &SemaRef, const InitializedEntity *InitEntity,
     break;
   }
   case LK_LifetimeCapture: {
-    Path.push_back({IndirectLocalPathEntry::LifetimeCapture, Init});
-    if (isRecordWithAttr<PointerAttr>(Init->getType()))
-      HasReferenceBinding = false;
-    // Skip the top MTE if it is a temporary object of the pointer-like type
-    // itself.
-    if (auto *MTE = dyn_cast<MaterializeTemporaryExpr>(Init);
-        MTE && isPointerLikeType(Init->getType()))
-      Init = MTE->getSubExpr();
+    if (isPointerLikeType(Init->getType()))
+      Path.push_back({IndirectLocalPathEntry::GslPointerInit, Init});
     break;
   }
   default:

The tests passed, and it also fixes the known false positives:

error: 'expected-warning' diagnostics expected but not seen: 
  File /workspace/llvm-project/clang/test/Sema/warn-lifetime-analysis-capture-by.cpp Line 313: captured
  File /workspace/llvm-project/clang/test/Sema/warn-lifetime-analysis-capture-by.cpp Line 314: captured
  File /workspace/llvm-project/clang/test/Sema/warn-lifetime-analysis-capture-by.cpp Line 318: captured

Would you mind taking it further?

[usx95]

Great. Simplified. I have kept the assert though since it is important to not lifetimeextend.

So the need for IndirectLocalPathEntry::LifetimeCapture isn’t clear.

I think it is important to stop lifetime extension.

[hokein]

The assert is being triggered because Path is empty. It seems that lifetime extension is only relevant in the LK_Extended case, and doesn't apply to other cases where we don’t check it. I think we can just remove the assert. (we should probably do the same thing for GSLPointerAssignment, I think this one can be merged with the GSLPointerInit).

Additionally, I’d prefer not to introduce a new LifetimeCapture kind unless necessary. Adding it would increase code complexity, requiring updates across multiple switch (Kind) statements.

[usx95]

Hmm. I see this method being used only for LK_Extended so this seems safe. I always disliked dealing with lifetime extension as part of this analysis as it is the only AST/codegen-side effect of this analysis.

But this feels safe now. Thanks for digging into this.

[hokein]

It seems that the latest revision doesn't contain the change.

[bricknerb]

Please note that there a few unreplied comments from previous iterations.

[hokein]

Thanks, this looks good to me! I think we should ship it. Let’s check if @bricknerb has any additional concerns before landing it.

From my understanding, the follow-ups outlined in the description should cover everything during the code review.

warn if capturing X cannot capture anything. It should be a reference, pointer or a view type.

I think this one is not right any more, right?

[bricknerb]

Thanks!