Make [[clang::lifetimebound]] work for expressions coming from default arguments

[higher-performance]

This is an attempt to fix #68596.

I'm very unfamiliar with this code, so it is very likely that I have made mistakes or overlooked a lot of things, especially things that might introduce unintended false-positives or false-negatives. CC @zygoloid

Please review carefully!

[github-actions]

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

[llvmbot]

@llvm/pr-subscribers-clang

Author: None (higher-performance)

Changes

This is an attempt to fix #68596.

I'm very unfamiliar with this code, so it is very likely that I have made mistakes or overlooked a lot of things, especially things that might introduce unintended false-positives or false-negatives. CC @zygoloid

Please review carefully!

---

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

2 Files Affected:

• (modified) clang/lib/Sema/CheckExprLifetime.cpp (+20-1)
• (modified) clang/test/SemaCXX/attr-lifetimebound.cpp (+6)

diff --git a/clang/lib/Sema/CheckExprLifetime.cpp b/clang/lib/Sema/CheckExprLifetime.cpp
index f62e18543851c1..0388efe3f182fd 100644
--- a/clang/lib/Sema/CheckExprLifetime.cpp
+++ b/clang/lib/Sema/CheckExprLifetime.cpp
@@ -194,6 +194,7 @@ struct IndirectLocalPathEntry {
     GslReferenceInit,
     GslPointerInit,
     GslPointerAssignment,
+    DefaultArg,
   } Kind;
   Expr *E;
   union {
@@ -532,6 +533,11 @@ static void visitLocalsRetainedByReferenceBinding(IndirectLocalPath &Path,
     }
   } while (Init != Old);
 
+  if (auto *DAE = dyn_cast<CXXDefaultArgExpr>(Init)) {
+    Path.push_back({IndirectLocalPathEntry::DefaultArg, DAE, DAE->getParam()});
+    Init = DAE->getExpr();
+  }
+
   if (auto *MTE = dyn_cast<MaterializeTemporaryExpr>(Init)) {
     if (Visit(Path, Local(MTE), RK))
       visitLocalsRetainedByInitializer(Path, MTE->getSubExpr(), Visit, true);
@@ -917,6 +923,9 @@ static SourceRange nextPathEntryRange(const IndirectLocalPath &Path, unsigned I,
         continue;
       return Path[I].E->getSourceRange();
     }
+
+    case IndirectLocalPathEntry::DefaultArg:
+      return cast<CXXDefaultArgExpr>(Path[I].E)->getUsedLocation();
   }
   return E->getSourceRange();
 }
@@ -1221,7 +1230,7 @@ static void checkExprLifetimeImpl(Sema &SemaRef,
         break;
       }
 
-      case IndirectLocalPathEntry::LambdaCaptureInit:
+      case IndirectLocalPathEntry::LambdaCaptureInit: {
         if (!Elem.Capture->capturesVariable())
           break;
         // FIXME: We can't easily tell apart an init-capture from a nested
@@ -1234,6 +1243,16 @@ static void checkExprLifetimeImpl(Sema &SemaRef,
             << nextPathEntryRange(Path, I + 1, L);
         break;
       }
+
+      case IndirectLocalPathEntry::DefaultArg: {
+        const auto *DAE = cast<CXXDefaultArgExpr>(Elem.E);
+        SemaRef.Diag(DAE->getParam()->getDefaultArgRange().getBegin(),
+                     diag::note_default_argument_declared_here)
+            << DAE->getParam() << nextPathEntryRange(Path, I + 1, L);
+        break;
+      }
+      }
+      }
     }
 
     // We didn't lifetime-extend, so don't go any further; we don't need more
diff --git a/clang/test/SemaCXX/attr-lifetimebound.cpp b/clang/test/SemaCXX/attr-lifetimebound.cpp
index 0fb997a5671085..d39d7e9eb54bff 100644
--- a/clang/test/SemaCXX/attr-lifetimebound.cpp
+++ b/clang/test/SemaCXX/attr-lifetimebound.cpp
@@ -19,8 +19,10 @@ namespace usage_invalid {
 namespace usage_ok {
   struct IntRef { int *target; };
 
+  const int *defaultparam(const int &def1 [[clang::lifetimebound]] = 0); // #def1
   int &refparam(int &param [[clang::lifetimebound]]);
   int &classparam(IntRef param [[clang::lifetimebound]]);
+  const int *c = defaultparam(); // expected-warning {{temporary whose address is used as value of local variable 'c' will be destroyed at the end of the full-expression}} expected-note@#def1 {{default argument declared here}}
 
   // Do not diagnose non-void return types; they can still be lifetime-bound.
   long long ptrintcast(int &param [[clang::lifetimebound]]) {
@@ -34,13 +36,17 @@ namespace usage_ok {
   struct A {
     A();
     A(int);
+    A(const char*, const int& def2 [[clang::lifetimebound]] = 0); // #def2
     int *class_member() [[clang::lifetimebound]];
     operator int*() [[clang::lifetimebound]];
+    const int *defaulted_param(const int &def3 [[clang::lifetimebound]] = 0); // #def3
   };
 
   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}}
+  A a = A(""); // expected-warning {{temporary whose address is used as value of local variable 'a' will be destroyed at the end of the full-expression}} expected-note@#def2 {{default argument declared here}}
+  const int *s = A().defaulted_param(); // expected-warning {{temporary whose address is used as value of local variable 's' will be destroyed at the end of the full-expression}} expected-note@#def3 {{default argument declared here}}
 
   void test_assignment() {
     p = A().class_member(); // expected-warning {{object backing the pointer p will be destroyed at the end of the full-expression}}

[zygoloid]

Thanks, this seems reasonable.

[zygoloid]

Please can you also add tests for the case where the default argument is a pointer, eg:

using T = int[];
int *f([[clang::lifetimebound]] int *p = T{1, 2, 3});
int *p = f();

I expect that won't be caught, and you'll need to make a similar change to visitLocalsRetainedByInitializer to handle it.

[zygoloid]

I expect that won't be caught, and you'll need to make a similar change to visitLocalsRetainedByInitializer to handle it.

Actually, maybe the best thing would be to change visitFunctionCallArguments to step over CXXDefaultArgExpr in Args. That should cover both cases, and the only place we should see a CXXDefaultArgExpr is at the top level in some form of function call.

[higher-performance]

Thanks so much, I made all those changes, please take another look.

[ilya-biryukov]

cc @usx95 @bricknerb who were looking at the lifetime annotations recently too.

[usx95]

Thanks for working on this. Mostly LG. Couple of nits and more ideas for tests.

[Xazax-hun]

Some small nits inline, but it looks good to me.

[higher-performance]

Thanks for the reviews! @Xazax-hun to confirm, can I merge this?

[Xazax-hun]

Yup, feel free to merge it!

[higher-performance]

Actually @Xazax-hun looks like I don't have merge permission, would you mind clicking it (or giving me permission if that makes sense)?

[zygoloid]

Thanks, looks good to me too.

[higher-performance]

Thanks all!