[LifetimeSafety] Merge lifetimebound attribute on implicit 'this' across method redeclarations

[usx95]

Followup on #107627
Fixes #62072
Fixes #172013
Fixes #175391

This PR adds support for merging the lifetimebound attribute on the implicit this parameter when merging method declarations. Previously, if a method was declared with lifetimebound on its function type (which represents the implicit this parameter), this attribute would not be propagated to the method definition, causing lifetime safety warnings to be missed.

The implementation adds helper functions to extract the lifetimebound attribute from a function type and to merge this attribute from an old method declaration to a new one when appropriate.

[usx95]

• [LifetimeSafety] Merge lifetimebound attribute on implicit 'this' across method redeclarations #172146 👈 (View in Graphite)
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[llvmbot]

@llvm/pr-subscribers-clang-analysis

@llvm/pr-subscribers-clang

Author: Utkarsh Saxena (usx95)

Changes

Followup on #107627
Fixes #62072
Fixes #172013

This PR adds support for merging the lifetimebound attribute on the implicit this parameter when merging method declarations. Previously, if a method was declared with lifetimebound on its function type (which represents the implicit this parameter), this attribute would not be propagated to the method definition, causing lifetime safety warnings to be missed.

The implementation adds helper functions to extract the lifetimebound attribute from a function type and to merge this attribute from an old method declaration to a new one when appropriate.

---

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

3 Files Affected:

• (modified) clang/lib/Sema/SemaDecl.cpp (+57-6)
• (modified) clang/test/Sema/warn-lifetime-safety.cpp (+22)
• (modified) clang/test/SemaCXX/attr-lifetimebound.cpp (+12)

diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index 47bd7295e93f6..3dd62c1428271 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -4469,6 +4469,52 @@ bool Sema::MergeFunctionDecl(FunctionDecl *New, NamedDecl *&OldD, Scope *S,
   return true;
 }
 
+/// Check if a function has a lifetimebound attribute on its function type
+/// (which represents the implicit 'this' parameter for methods).
+/// Returns the attribute if found, nullptr otherwise.
+static const LifetimeBoundAttr *
+getLifetimeBoundAttrFromType(const TypeSourceInfo *TSI) {
+  if (!TSI)
+    return nullptr;
+
+  for (TypeLoc TL = TSI->getTypeLoc();;) {
+    auto ATL = TL.getAsAdjusted<AttributedTypeLoc>();
+    if (!ATL)
+      break;
+    if (auto *LBAttr = ATL.getAttrAs<LifetimeBoundAttr>())
+      return LBAttr;
+    TL = ATL.getModifiedLoc();
+  }
+  return nullptr;
+}
+
+/// Merge lifetimebound attribute on function type (implicit 'this')
+/// from Old to New method declaration.
+static void mergeLifetimeBoundAttrOnMethod(Sema &S, CXXMethodDecl *New,
+                                           const CXXMethodDecl *Old) {
+  const TypeSourceInfo *OldTSI = Old->getTypeSourceInfo();
+  const TypeSourceInfo *NewTSI = New->getTypeSourceInfo();
+
+  if (!OldTSI || !NewTSI)
+    return;
+
+  const LifetimeBoundAttr *OldLBAttr = getLifetimeBoundAttrFromType(OldTSI);
+  const LifetimeBoundAttr *NewLBAttr = getLifetimeBoundAttrFromType(NewTSI);
+
+  // If Old has lifetimebound but New doesn't, add it to New
+  if (OldLBAttr && !NewLBAttr) {
+    QualType NewMethodType = New->getType();
+    QualType AttributedType =
+        S.Context.getAttributedType(OldLBAttr, NewMethodType, NewMethodType);
+    TypeLocBuilder TLB;
+    TLB.pushFullCopy(NewTSI->getTypeLoc());
+    AttributedTypeLoc TyLoc = TLB.push<AttributedTypeLoc>(AttributedType);
+    TyLoc.setAttr(OldLBAttr);
+    New->setType(AttributedType);
+    New->setTypeSourceInfo(TLB.getTypeSourceInfo(S.Context, AttributedType));
+  }
+}
+
 bool Sema::MergeCompatibleFunctionDecls(FunctionDecl *New, FunctionDecl *Old,
                                         Scope *S, bool MergeTypeWithOld) {
   // Merge the attributes
@@ -4485,12 +4531,17 @@ bool Sema::MergeCompatibleFunctionDecls(FunctionDecl *New, FunctionDecl *Old,
   // Merge attributes from the parameters.  These can mismatch with K&R
   // declarations.
   if (New->getNumParams() == Old->getNumParams())
-      for (unsigned i = 0, e = New->getNumParams(); i != e; ++i) {
-        ParmVarDecl *NewParam = New->getParamDecl(i);
-        ParmVarDecl *OldParam = Old->getParamDecl(i);
-        mergeParamDeclAttributes(NewParam, OldParam, *this);
-        mergeParamDeclTypes(NewParam, OldParam, *this);
-      }
+    for (unsigned i = 0, e = New->getNumParams(); i != e; ++i) {
+      ParmVarDecl *NewParam = New->getParamDecl(i);
+      ParmVarDecl *OldParam = Old->getParamDecl(i);
+      mergeParamDeclAttributes(NewParam, OldParam, *this);
+      mergeParamDeclTypes(NewParam, OldParam, *this);
+    }
+
+  // Merge function type attributes (e.g., lifetimebound on implicit 'this').
+  if (auto *NewMethod = dyn_cast<CXXMethodDecl>(New))
+    if (auto *OldMethod = dyn_cast<CXXMethodDecl>(Old))
+      mergeLifetimeBoundAttrOnMethod(*this, NewMethod, OldMethod);
 
   if (getLangOpts().CPlusPlus)
     return MergeCXXFunctionDecl(New, Old, S);
diff --git a/clang/test/Sema/warn-lifetime-safety.cpp b/clang/test/Sema/warn-lifetime-safety.cpp
index 1191469e23df1..658d68db44369 100644
--- a/clang/test/Sema/warn-lifetime-safety.cpp
+++ b/clang/test/Sema/warn-lifetime-safety.cpp
@@ -943,3 +943,25 @@ void parentheses(bool cond) {
   }  // expected-note 4 {{destroyed here}}
   (void)*p;  // expected-note 4 {{later used here}}
 }
+
+// Implicit this annotations with redecls.
+namespace GH172013 {
+// https://github.com/llvm/llvm-project/issues/62072
+// https://github.com/llvm/llvm-project/issues/172013
+struct S {
+    View x() const [[clang::lifetimebound]];
+    MyObj i;
+};
+
+View S::x() const { return i; }
+
+void bar() {
+    View x;
+    {
+        S s;
+        x = s.x(); // expected-warning {{object whose reference is captured does not live long enough}}
+        View y = S().x(); // FIXME: Handle temporaries.
+    } // expected-note {{destroyed here}}
+    (void)x; // expected-note {{used here}}
+}
+}
diff --git a/clang/test/SemaCXX/attr-lifetimebound.cpp b/clang/test/SemaCXX/attr-lifetimebound.cpp
index 111bad65f7e1b..5b0aa0e518cb3 100644
--- a/clang/test/SemaCXX/attr-lifetimebound.cpp
+++ b/clang/test/SemaCXX/attr-lifetimebound.cpp
@@ -75,6 +75,18 @@ namespace usage_ok {
     r = A(1); // expected-warning {{object backing the pointer 'r' will be destroyed at the end of the full-expression}}
   }
 
+  // Test that lifetimebound on implicit 'this' is propagated across redeclarations
+  struct B {
+    int *method() [[clang::lifetimebound]];
+    int i;
+  };
+  int *B::method() { return &i; }
+
+  void test_lifetimebound_on_implicit_this() {
+    int *t = B().method(); // expected-warning {{temporary whose address is used as value of local variable 't' will be destroyed at the end of the full-expression}}
+    t = {B().method()}; // expected-warning {{object backing the pointer 't' will be destroyed at the end of the full-expression}}
+  }
+
   struct FieldCheck {
     struct Set {
       int a;

[llvmbot]

@llvm/pr-subscribers-clang-temporal-safety

Author: Utkarsh Saxena (usx95)

Changes

Followup on #107627
Fixes #62072
Fixes #172013

This PR adds support for merging the lifetimebound attribute on the implicit this parameter when merging method declarations. Previously, if a method was declared with lifetimebound on its function type (which represents the implicit this parameter), this attribute would not be propagated to the method definition, causing lifetime safety warnings to be missed.

The implementation adds helper functions to extract the lifetimebound attribute from a function type and to merge this attribute from an old method declaration to a new one when appropriate.

---

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

3 Files Affected:

• (modified) clang/lib/Sema/SemaDecl.cpp (+57-6)
• (modified) clang/test/Sema/warn-lifetime-safety.cpp (+22)
• (modified) clang/test/SemaCXX/attr-lifetimebound.cpp (+12)

diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index 47bd7295e93f6..3dd62c1428271 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -4469,6 +4469,52 @@ bool Sema::MergeFunctionDecl(FunctionDecl *New, NamedDecl *&OldD, Scope *S,
   return true;
 }
 
+/// Check if a function has a lifetimebound attribute on its function type
+/// (which represents the implicit 'this' parameter for methods).
+/// Returns the attribute if found, nullptr otherwise.
+static const LifetimeBoundAttr *
+getLifetimeBoundAttrFromType(const TypeSourceInfo *TSI) {
+  if (!TSI)
+    return nullptr;
+
+  for (TypeLoc TL = TSI->getTypeLoc();;) {
+    auto ATL = TL.getAsAdjusted<AttributedTypeLoc>();
+    if (!ATL)
+      break;
+    if (auto *LBAttr = ATL.getAttrAs<LifetimeBoundAttr>())
+      return LBAttr;
+    TL = ATL.getModifiedLoc();
+  }
+  return nullptr;
+}
+
+/// Merge lifetimebound attribute on function type (implicit 'this')
+/// from Old to New method declaration.
+static void mergeLifetimeBoundAttrOnMethod(Sema &S, CXXMethodDecl *New,
+                                           const CXXMethodDecl *Old) {
+  const TypeSourceInfo *OldTSI = Old->getTypeSourceInfo();
+  const TypeSourceInfo *NewTSI = New->getTypeSourceInfo();
+
+  if (!OldTSI || !NewTSI)
+    return;
+
+  const LifetimeBoundAttr *OldLBAttr = getLifetimeBoundAttrFromType(OldTSI);
+  const LifetimeBoundAttr *NewLBAttr = getLifetimeBoundAttrFromType(NewTSI);
+
+  // If Old has lifetimebound but New doesn't, add it to New
+  if (OldLBAttr && !NewLBAttr) {
+    QualType NewMethodType = New->getType();
+    QualType AttributedType =
+        S.Context.getAttributedType(OldLBAttr, NewMethodType, NewMethodType);
+    TypeLocBuilder TLB;
+    TLB.pushFullCopy(NewTSI->getTypeLoc());
+    AttributedTypeLoc TyLoc = TLB.push<AttributedTypeLoc>(AttributedType);
+    TyLoc.setAttr(OldLBAttr);
+    New->setType(AttributedType);
+    New->setTypeSourceInfo(TLB.getTypeSourceInfo(S.Context, AttributedType));
+  }
+}
+
 bool Sema::MergeCompatibleFunctionDecls(FunctionDecl *New, FunctionDecl *Old,
                                         Scope *S, bool MergeTypeWithOld) {
   // Merge the attributes
@@ -4485,12 +4531,17 @@ bool Sema::MergeCompatibleFunctionDecls(FunctionDecl *New, FunctionDecl *Old,
   // Merge attributes from the parameters.  These can mismatch with K&R
   // declarations.
   if (New->getNumParams() == Old->getNumParams())
-      for (unsigned i = 0, e = New->getNumParams(); i != e; ++i) {
-        ParmVarDecl *NewParam = New->getParamDecl(i);
-        ParmVarDecl *OldParam = Old->getParamDecl(i);
-        mergeParamDeclAttributes(NewParam, OldParam, *this);
-        mergeParamDeclTypes(NewParam, OldParam, *this);
-      }
+    for (unsigned i = 0, e = New->getNumParams(); i != e; ++i) {
+      ParmVarDecl *NewParam = New->getParamDecl(i);
+      ParmVarDecl *OldParam = Old->getParamDecl(i);
+      mergeParamDeclAttributes(NewParam, OldParam, *this);
+      mergeParamDeclTypes(NewParam, OldParam, *this);
+    }
+
+  // Merge function type attributes (e.g., lifetimebound on implicit 'this').
+  if (auto *NewMethod = dyn_cast<CXXMethodDecl>(New))
+    if (auto *OldMethod = dyn_cast<CXXMethodDecl>(Old))
+      mergeLifetimeBoundAttrOnMethod(*this, NewMethod, OldMethod);
 
   if (getLangOpts().CPlusPlus)
     return MergeCXXFunctionDecl(New, Old, S);
diff --git a/clang/test/Sema/warn-lifetime-safety.cpp b/clang/test/Sema/warn-lifetime-safety.cpp
index 1191469e23df1..658d68db44369 100644
--- a/clang/test/Sema/warn-lifetime-safety.cpp
+++ b/clang/test/Sema/warn-lifetime-safety.cpp
@@ -943,3 +943,25 @@ void parentheses(bool cond) {
   }  // expected-note 4 {{destroyed here}}
   (void)*p;  // expected-note 4 {{later used here}}
 }
+
+// Implicit this annotations with redecls.
+namespace GH172013 {
+// https://github.com/llvm/llvm-project/issues/62072
+// https://github.com/llvm/llvm-project/issues/172013
+struct S {
+    View x() const [[clang::lifetimebound]];
+    MyObj i;
+};
+
+View S::x() const { return i; }
+
+void bar() {
+    View x;
+    {
+        S s;
+        x = s.x(); // expected-warning {{object whose reference is captured does not live long enough}}
+        View y = S().x(); // FIXME: Handle temporaries.
+    } // expected-note {{destroyed here}}
+    (void)x; // expected-note {{used here}}
+}
+}
diff --git a/clang/test/SemaCXX/attr-lifetimebound.cpp b/clang/test/SemaCXX/attr-lifetimebound.cpp
index 111bad65f7e1b..5b0aa0e518cb3 100644
--- a/clang/test/SemaCXX/attr-lifetimebound.cpp
+++ b/clang/test/SemaCXX/attr-lifetimebound.cpp
@@ -75,6 +75,18 @@ namespace usage_ok {
     r = A(1); // expected-warning {{object backing the pointer 'r' will be destroyed at the end of the full-expression}}
   }
 
+  // Test that lifetimebound on implicit 'this' is propagated across redeclarations
+  struct B {
+    int *method() [[clang::lifetimebound]];
+    int i;
+  };
+  int *B::method() { return &i; }
+
+  void test_lifetimebound_on_implicit_this() {
+    int *t = B().method(); // expected-warning {{temporary whose address is used as value of local variable 't' will be destroyed at the end of the full-expression}}
+    t = {B().method()}; // expected-warning {{object backing the pointer 't' will be destroyed at the end of the full-expression}}
+  }
+
   struct FieldCheck {
     struct Set {
       int a;

[cor3ntin]

What about the opposite scenario?

[usx95]

The idea is to read mostRecentDecl for analysis and it would reflect all the merged attributes seen until now.

[Xazax-hun]

Do we have an established convention how to deal with attributes? One way is just what @usx95 described, but another way is to use the canonical decl as the single source of truth.

[usx95]

As far as merging declarations are concerned, it looks like this is the way that it has been done historically throughout this file, i.e., merging old decl into the new decl. E.g. docs for MergeCompatibleFunctionDecls

  /// Completes the merge of two function declarations that are
  /// known to be compatible.
  ///
  /// This routine handles the merging of attributes and other
  /// properties of function declarations from the old declaration to
  /// the new declaration, once we know that New is in fact a
  /// redeclaration of Old.
  ///
  /// \returns false
  bool MergeCompatibleFunctionDecls(FunctionDecl *New, FunctionDecl *Old,
                                    Scope *S, bool MergeTypeWithOld);

IIUC mostRecentDecl is the default decl that is retrieved:

/// Represents a function declaration or definition.
///
/// Since a given function can be declared several times in a program,
/// there may be several FunctionDecls that correspond to that
/// function. Only one of those FunctionDecls will be found when
/// traversing the list of declarations in the context of the
/// FunctionDecl (e.g., the translation unit); this FunctionDecl
/// contains all of the information known about the function. Other,
/// previous declarations of the function are available via the
/// getPreviousDecl() chain.
class FunctionDecl : public DeclaratorDecl,
                     public DeclContext,
                     public Redeclarable<FunctionDecl> {

I would be inclined to keep the same behaviour here and always use the mostRecentDecl (which should also be the default) to read the annotations.

[Xazax-hun]

Do we have an established convention how to deal with attributes? One way is just what @usx95 described, but another way is to use the canonical decl as the single source of truth.

[ymand]

Overall LGTM, but I defer to Gabor and Corentin on approval since I'm not familiar with Sema.

[usx95]

Merge activity

• Jan 13, 8:19 AM UTC: A user started a stack merge that includes this pull request via Graphite.
• Jan 13, 8:21 AM UTC: @usx95 merged this pull request with Graphite.

[zmodem]

We're hitting assertion failures after this:

clang: /work/llvm-project/clang/lib/Sema/TypeLocBuilder.cpp:89: TypeLoc clang::TypeLocBuilder::pushImpl(QualType, size_t, unsigned int): Assertion `TLast == LastTy && "mismatch between last type and new type's inner type"' failed.

See https://crbug.com/475583794#comment3 for a reproducer.

[zmodem]

Reverting to green..

[mstorsjo]

Thanks for the revert! I also ran into this, in building libcxx. My repro is at https://martin.st/temp/libcxx-fs-path-preproc.cpp, triggered with clang -target i686-w64-mingw32 -c libcxx-fs-path-preproc.cpp -fsized-deallocation -std=c++23 -w.

With cvise, I reduced it down to this reproducer:

struct a {
  a &b() [[_Clang::__lifetimebound__]];
};
a &a::b() {}

Which reproduces with e.g. clang -c repro.cpp -target i686-windows.