[clang] Lifetime of locals must end before musttail call

[ostannard]

The lifetimes of local variables and function parameters must end before the call to a [[clang::musttail]] function, instead of before the return, because we will not have a stack frame to hold them when doing the call.

This documents this limitation, and adds diagnostics to warn about some code which is invalid because of it.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Oliver Stannard (ostannard)

Changes

The lifetimes of local variables and function parameters must end before the call to a [[clang::musttail]] function, instead of before the return, because we will not have a stack frame to hold them when doing the call.

This documents this limitation, and adds diagnostics to warn about some code which is invalid because of it.

---

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

6 Files Affected:

• (modified) clang/include/clang/Basic/AttrDocs.td (+5)
• (modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (+2-1)
• (modified) clang/lib/Sema/CheckExprLifetime.cpp (+14-2)
• (modified) clang/lib/Sema/CheckExprLifetime.h (+6)
• (modified) clang/lib/Sema/SemaStmt.cpp (+10)
• (modified) clang/test/SemaCXX/attr-musttail.cpp (+27)

diff --git a/clang/include/clang/Basic/AttrDocs.td b/clang/include/clang/Basic/AttrDocs.td
index 8ef151b3f2fddb..7226871074ee7e 100644
--- a/clang/include/clang/Basic/AttrDocs.td
+++ b/clang/include/clang/Basic/AttrDocs.td
@@ -637,6 +637,11 @@ return value must be trivially destructible. The calling convention of the
 caller and callee must match, and they must not be variadic functions or have
 old style K&R C function declarations.
 
+The lifetimes of all local variables and function parameters end immediately
+before the call to the function. This means that it is undefined behaviour to
+pass a pointer or reference to a local variable to the called function, which
+is not the case without the attribute.
+
 ``clang::musttail`` provides assurances that the tail call can be optimized on
 all targets, not just one.
   }];
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index ba813af960af6f..75c7f9e0eb7de0 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -10101,7 +10101,8 @@ def err_lifetimebound_ctor_dtor : Error<
 // CHECK: returning address/reference of stack memory
 def warn_ret_stack_addr_ref : Warning<
   "%select{address of|reference to}0 stack memory associated with "
-  "%select{local variable|parameter|compound literal}2 %1 returned">,
+  "%select{local variable|parameter|compound literal}2 %1 "
+  "%select{returned|passed to musttail function}3">,
   InGroup<ReturnStackAddress>;
 def warn_ret_local_temp_addr_ref : Warning<
   "returning %select{address of|reference to}0 local temporary object">,
diff --git a/clang/lib/Sema/CheckExprLifetime.cpp b/clang/lib/Sema/CheckExprLifetime.cpp
index c98fbca849faba..211c1cc7bc81f9 100644
--- a/clang/lib/Sema/CheckExprLifetime.cpp
+++ b/clang/lib/Sema/CheckExprLifetime.cpp
@@ -33,6 +33,10 @@ enum LifetimeKind {
   /// the entity is a return object.
   LK_Return,
 
+  /// The lifetime of a temporary bound to this entity ends too soon, because
+  /// the entity passed to a musttail function call.
+  LK_MustTail,
+
   /// The lifetime of a temporary bound to this entity ends too soon, because
   /// the entity is the result of a statement expression.
   LK_StmtExprResult,
@@ -1150,6 +1154,7 @@ static void checkExprLifetimeImpl(Sema &SemaRef,
       break;
 
     case LK_Return:
+    case LK_MustTail:
     case LK_StmtExprResult:
       if (auto *DRE = dyn_cast<DeclRefExpr>(L)) {
         // We can't determine if the local variable outlives the statement
@@ -1158,7 +1163,8 @@ static void checkExprLifetimeImpl(Sema &SemaRef,
           return false;
         SemaRef.Diag(DiagLoc, diag::warn_ret_stack_addr_ref)
             << InitEntity->getType()->isReferenceType() << DRE->getDecl()
-            << isa<ParmVarDecl>(DRE->getDecl()) << DiagRange;
+            << isa<ParmVarDecl>(DRE->getDecl()) << (LK == LK_MustTail)
+            << DiagRange;
       } else if (isa<BlockExpr>(L)) {
         SemaRef.Diag(DiagLoc, diag::err_ret_local_block) << DiagRange;
       } else if (isa<AddrLabelExpr>(L)) {
@@ -1170,7 +1176,7 @@ static void checkExprLifetimeImpl(Sema &SemaRef,
       } else if (auto *CLE = dyn_cast<CompoundLiteralExpr>(L)) {
         SemaRef.Diag(DiagLoc, diag::warn_ret_stack_addr_ref)
             << InitEntity->getType()->isReferenceType() << CLE->getInitializer()
-            << 2 << DiagRange;
+            << 2 << (LK == LK_MustTail) << DiagRange;
       } else {
         // P2748R5: Disallow Binding a Returned Glvalue to a Temporary.
         // [stmt.return]/p6: In a function whose return type is a reference,
@@ -1265,6 +1271,12 @@ void checkExprLifetime(Sema &SemaRef, const InitializedEntity &Entity,
                         /*AEntity*/ nullptr, Init);
 }
 
+void checkExprLifetimeMustTailArg(Sema &SemaRef, const InitializedEntity &Entity,
+                       Expr *Init) {
+  checkExprLifetimeImpl(SemaRef, &Entity, nullptr, LK_MustTail,
+                        /*AEntity*/ nullptr, Init);
+}
+
 void checkExprLifetime(Sema &SemaRef, const AssignedEntity &Entity,
                        Expr *Init) {
   bool EnableDanglingPointerAssignment = !SemaRef.getDiagnostics().isIgnored(
diff --git a/clang/lib/Sema/CheckExprLifetime.h b/clang/lib/Sema/CheckExprLifetime.h
index 8c8d0806dee0a3..903f312f3533e5 100644
--- a/clang/lib/Sema/CheckExprLifetime.h
+++ b/clang/lib/Sema/CheckExprLifetime.h
@@ -35,6 +35,12 @@ void checkExprLifetime(Sema &SemaRef, const InitializedEntity &Entity,
 /// sufficient for assigning to the entity.
 void checkExprLifetime(Sema &SemaRef, const AssignedEntity &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.
+void checkExprLifetimeMustTailArg(Sema &SemaRef,
+                                  const InitializedEntity &Entity, Expr *Init);
+
 } // namespace clang::sema
 
 #endif // LLVM_CLANG_SEMA_CHECK_EXPR_LIFETIME_H
diff --git a/clang/lib/Sema/SemaStmt.cpp b/clang/lib/Sema/SemaStmt.cpp
index 9664287b9a3fe9..9e235a46707cd4 100644
--- a/clang/lib/Sema/SemaStmt.cpp
+++ b/clang/lib/Sema/SemaStmt.cpp
@@ -10,6 +10,7 @@
 //
 //===----------------------------------------------------------------------===//
 
+#include "CheckExprLifetime.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/ASTDiagnostic.h"
 #include "clang/AST/ASTLambda.h"
@@ -889,6 +890,15 @@ bool Sema::checkMustTailAttr(const Stmt *St, const Attr &MTA) {
     return false;
   }
 
+  // The lifetimes of locals and incoming function parameters must end before
+  // the call, because we can't have a stack frame to store them, so diagnose
+  // any pointers or references to them passed into the musttail call.
+  for (auto ArgExpr : CE->arguments()) {
+    InitializedEntity Entity = InitializedEntity::InitializeParameter(
+        Context, ArgExpr->getType(), false);
+    checkExprLifetimeMustTailArg(*this, Entity, const_cast<Expr *>(ArgExpr));
+  }
+
   return true;
 }
 
diff --git a/clang/test/SemaCXX/attr-musttail.cpp b/clang/test/SemaCXX/attr-musttail.cpp
index 561184e7a24f94..d84b97a4f04d86 100644
--- a/clang/test/SemaCXX/attr-musttail.cpp
+++ b/clang/test/SemaCXX/attr-musttail.cpp
@@ -267,3 +267,30 @@ namespace ns {}
 void TestCallNonValue() {
   [[clang::musttail]] return ns; // expected-error {{unexpected namespace name 'ns': expected expression}}
 }
+
+// Test diagnostics for lifetimes of local variables, which end earlier for a
+// musttail call than for a nowmal one.
+
+void TakesIntAndPtr(int, int *);
+void PassAddressOfLocal(int a, int *b) {
+  int c;
+  [[clang::musttail]] return TakesIntAndPtr(0, &c); // expected-warning {{address of stack memory associated with local variable 'c' passed to musttail function}}
+}
+void PassAddressOfParam(int a, int *b) {
+  [[clang::musttail]] return TakesIntAndPtr(0, &a); // expected-warning {{address of stack memory associated with parameter 'a' passed to musttail function}}
+}
+void PassValues(int a, int *b) {
+  [[clang::musttail]] return TakesIntAndPtr(a, b);
+}
+
+void TakesIntAndRef(int, int &);
+void PassRefOfLocal(int a, int &b) {
+  int c;
+  [[clang::musttail]] return TakesIntAndRef(0, c); // expected-warning {{address of stack memory associated with local variable 'c' passed to musttail function}}
+}
+void PassRefOfParam(int a, int &b) {
+  [[clang::musttail]] return TakesIntAndRef(0, a); // expected-warning {{address of stack memory associated with parameter 'a' passed to musttail function}}
+}
+void PassValuesRef(int a, int &b) {
+  [[clang::musttail]] return TakesIntAndRef(a, b);
+}

[github-actions]

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

[hokein]

I think it warrants a mention in the release notes.

[hokein]

Thanks, it looks good, just a few nits.

[hokein]

can you add a test where a function call is used in the argument? something like

int foo();
void test(int a, const int &b) {
  int c;
  [[clang::musttail]] return test(0, foo()); 
}

[ostannard]

Done, and there was a different diagnostic message used for temporaries, so added a better wording for that.

[hokein]

nit: maybe mention that clang will give a diagnostic on this case.

[ostannard]

Done.

[hokein]

Thanks, still looks good.