[clang] [clang] Lifetime of locals must end before musttail call (PR #109255)

Oliver Stannard via cfe-commits cfe-commits at lists.llvm.org
Thu Sep 19 07:18:14 PDT 2024


https://github.com/ostannard updated https://github.com/llvm/llvm-project/pull/109255

>From 85ac319257785f88fd27a533fffde7aab20c8d8d Mon Sep 17 00:00:00 2001
From: Oliver Stannard <oliver.stannard at arm.com>
Date: Wed, 18 Sep 2024 16:22:41 +0100
Subject: [PATCH 1/3] [clang] Lifetime of locals must end before musttail call

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.
---
 clang/include/clang/Basic/AttrDocs.td         |  5 ++++
 .../clang/Basic/DiagnosticSemaKinds.td        |  3 ++-
 clang/lib/Sema/CheckExprLifetime.cpp          | 16 +++++++++--
 clang/lib/Sema/CheckExprLifetime.h            |  6 +++++
 clang/lib/Sema/SemaStmt.cpp                   | 10 +++++++
 clang/test/SemaCXX/attr-musttail.cpp          | 27 +++++++++++++++++++
 6 files changed, 64 insertions(+), 3 deletions(-)

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);
+}

>From c9ed9b8bd0fc065fa4c17cee41a1804897176068 Mon Sep 17 00:00:00 2001
From: Oliver Stannard <oliver.stannard at arm.com>
Date: Thu, 19 Sep 2024 10:12:24 +0100
Subject: [PATCH 2/3] clang-format

---
 clang/lib/Sema/CheckExprLifetime.cpp | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/clang/lib/Sema/CheckExprLifetime.cpp b/clang/lib/Sema/CheckExprLifetime.cpp
index 211c1cc7bc81f9..149880301d4a93 100644
--- a/clang/lib/Sema/CheckExprLifetime.cpp
+++ b/clang/lib/Sema/CheckExprLifetime.cpp
@@ -1271,8 +1271,8 @@ void checkExprLifetime(Sema &SemaRef, const InitializedEntity &Entity,
                         /*AEntity*/ nullptr, Init);
 }
 
-void checkExprLifetimeMustTailArg(Sema &SemaRef, const InitializedEntity &Entity,
-                       Expr *Init) {
+void checkExprLifetimeMustTailArg(Sema &SemaRef,
+                                  const InitializedEntity &Entity, Expr *Init) {
   checkExprLifetimeImpl(SemaRef, &Entity, nullptr, LK_MustTail,
                         /*AEntity*/ nullptr, Init);
 }

>From 78075ad7f5adc47fa8e30878a378854df3cf1751 Mon Sep 17 00:00:00 2001
From: Oliver Stannard <oliver.stannard at arm.com>
Date: Thu, 19 Sep 2024 15:16:41 +0100
Subject: [PATCH 3/3] Add release notes

---
 clang/docs/ReleaseNotes.rst | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index b8816d7b555e87..b7fa28e3ae1f57 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -260,6 +260,11 @@ Attribute Changes in Clang
 - Introduced a new attribute ``[[clang::coro_await_elidable_argument]]`` on function parameters
   to propagate safe elide context to arguments if such function is also under a safe elide context.
 
+- The documentation of the ``[[clang::musttail]]`` attribute was updated to
+  note that the lifetimes of all local variables end before the call. This does
+  not change the behaviour of the compiler, as this was true for previous
+  versions.
+
 Improvements to Clang's diagnostics
 -----------------------------------
 
@@ -316,6 +321,10 @@ Improvements to Clang's diagnostics
 
 - Don't emit bogus dangling diagnostics when ``[[gsl::Owner]]`` and `[[clang::lifetimebound]]` are used together (#GH108272).
 
+- The ``-Wreturn-stack-address`` warning now also warns about addresses of
+  local variables passed to function calls using the ``[[clang::musttail]]``
+  attribute.
+
 Improvements to Clang's time-trace
 ----------------------------------
 



More information about the cfe-commits mailing list