Skip to content

[coroutines] Introduce [[clang::coro_lifetimebound]] #72851

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 10 commits into from
Nov 22, 2023

Conversation

usx95
Copy link
Contributor

@usx95 usx95 commented Nov 20, 2023

Adds attribute [[clang::coro_lifetimebound]].

All arguments to a function are considered to be lifetime bound if the function
returns a type annotated with [[clang::coro_lifetimebound]] and [[clang::coro_return_type]].

@usx95 usx95 marked this pull request as ready for review November 20, 2023 15:23
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Nov 20, 2023
@usx95 usx95 requested a review from ilya-biryukov November 20, 2023 15:23
@llvmbot
Copy link
Member

llvmbot commented Nov 20, 2023

@llvm/pr-subscribers-clang

Author: Utkarsh Saxena (usx95)

Changes

Adds attribute [[clang::coro_lifetimebound]].

All arguments to a function are considered to be lifetime bound if the function
returns a type annotated with [[clang::coro_lifetimebound]] and [[clang::coro_return_type]].


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

6 Files Affected:

  • (modified) clang/docs/ReleaseNotes.rst (+4)
  • (modified) clang/include/clang/Basic/Attr.td (+8)
  • (modified) clang/include/clang/Basic/AttrDocs.td (+54-1)
  • (modified) clang/lib/Sema/SemaInit.cpp (+6-1)
  • (modified) clang/test/Misc/pragma-attribute-supported-attributes-list.test (+2-1)
  • (added) clang/test/SemaCXX/coro-lifetimebound.cpp (+100)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 52c9d6eb69617b0..3fdb93fda8eec7f 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -311,6 +311,10 @@ Attribute Changes in Clang
   should be a coroutine. A non-coroutine function marked with ``[[clang::coro_wrapper]]``
   is still allowed to return the such a type. This is helpful for analyzers to recognize coroutines from the function signatures.
 
+- Clang now introduced ``[[clang::coro_lifetimebound]]`` attribute.
+  All arguments to a function are considered to be lifetime bound if the function 
+  returns a type annotated with ``[[clang::coro_lifetimebound]]`` and ``[[clang::coro_return_type]]``.
+
 Improvements to Clang's diagnostics
 -----------------------------------
 - Clang constexpr evaluator now prints template arguments when displaying
diff --git a/clang/include/clang/Basic/Attr.td b/clang/include/clang/Basic/Attr.td
index f7a2b83b15ef5bc..dfe6f8999f832a3 100644
--- a/clang/include/clang/Basic/Attr.td
+++ b/clang/include/clang/Basic/Attr.td
@@ -1110,6 +1110,14 @@ def CoroWrapper : InheritableAttr {
   let SimpleHandler = 1;
 }
 
+def CoroLifetimeBound : InheritableAttr {
+  let Spellings = [Clang<"coro_lifetimebound">];
+  let Subjects = SubjectList<[CXXRecord]>;
+  let LangOpts = [CPlusPlus];
+  let Documentation = [CoroLifetimeBoundDoc];
+  let SimpleHandler = 1;
+}
+
 // OSObject-based attributes.
 def OSConsumed : InheritableParamAttr {
   let Spellings = [Clang<"os_consumed">];
diff --git a/clang/include/clang/Basic/AttrDocs.td b/clang/include/clang/Basic/AttrDocs.td
index 438d846c39eaa57..516ef76615f40f1 100644
--- a/clang/include/clang/Basic/AttrDocs.td
+++ b/clang/include/clang/Basic/AttrDocs.td
@@ -7483,7 +7483,6 @@ generation of the other destruction cases, optimizing the above `foo.destroy` to
   }];
 }
 
-
 def CoroReturnTypeAndWrapperDoc : Documentation {
   let Category = DocCatDecl;
   let Content = [{
@@ -7540,3 +7539,57 @@ Note: ``a_promise_type::get_return_object`` is exempted from this analysis as it
 implementation detail of any coroutine library.
 }];
 }
+
+def CoroLifetimeBoundDoc : Documentation {
+  let Category = DocCatDecl;
+  let Content = [{
+The ``[[clang::coro_lifetimebound]]`` is a class attribute which can be applied
+to a `coroutine return type (CRT) <https://clang.llvm.org/docs/AttributeReference.html#coro-return-type>` _ (i.e.
+it should also be annotated with ``[[clang::coro_return_type]]``).
+
+All arguments to a function are considered to be `lifetime bound <https://clang.llvm.org/docs/AttributeReference.html#lifetimebound> _`
+if the function returns a coroutine return type (CRT) annotated with ``[[clang::coro_lifetimebound]]``.
+
+Reference parameters of a coroutine are susceptible to capturing references to temporaries or local variables.
+
+For example,
+
+.. code-block:: c++
+  task<int> coro(const int& a) { co_return a + 1; }
+  task<int> dangling_refs(int a) {
+    // `coro` captures reference to a temporary. `foo` would now contain a dangling reference to `a`.
+    auto foo = coro(1);
+    // `coro` captures reference to local variable `a` which is destroyed after the return. 
+    return coro(a);
+  }
+
+`Lifetime bound <https://clang.llvm.org/docs/AttributeReference.html#lifetimebound> _` static analysis
+can be used to detect such instances when coroutines capture references which may die earlier than the
+coroutine frame itself. In the above example, if the CRT `task` is annotated with 
+``[[clang::coro_lifetimebound]]``, then lifetime bound analysis would detect capturing reference to
+temporaries or return address of a local variable.
+
+Both coroutines and coroutine wrappers are part of this analysis.
+
+.. code-block:: c++
+  template <typename T> struct [[clang::coro_return_type, clang::coro_lifetimebound]] Task {
+    using promise_type = some_promise_type;
+  };
+
+  Task<int> coro(const int& a) { co_return a + 1; }
+  Task<int> [[clang::coro_wrapper]] coro_wrapper(const int& a, const int& b) { 
+    return a > b ? coro(a) : coro(b);
+  }
+  Task<int> temporary_reference() {
+    auto foo = coro(1); // warning: capturing reference to a temporary which would die after the expression.
+
+    int a = 1;
+    auto bar = coro_wrapper(a, 0); // warning: `b` captures reference to a temporary.
+
+    co_return co_await coro(1); // fine.
+  }
+  Task<int> stack_reference(int a) {
+    return coro(a); // returning address of stack variable `a`.
+  }
+}];
+}
diff --git a/clang/lib/Sema/SemaInit.cpp b/clang/lib/Sema/SemaInit.cpp
index 80b51b09bf5445f..631b6a266412ccb 100644
--- a/clang/lib/Sema/SemaInit.cpp
+++ b/clang/lib/Sema/SemaInit.cpp
@@ -7580,10 +7580,15 @@ static void visitLifetimeBoundArguments(IndirectLocalPath &Path, Expr *Call,
   if (ObjectArg && implicitObjectParamIsLifetimeBound(Callee))
     VisitLifetimeBoundArg(Callee, ObjectArg);
 
+  bool checkCoroCall = false;
+  if (const auto *RD = Callee->getReturnType()->getAsRecordDecl()) {
+    checkCoroCall |= RD->hasAttr<CoroLifetimeBoundAttr>() &&
+                     RD->hasAttr<CoroReturnTypeAttr>();
+  }
   for (unsigned I = 0,
                 N = std::min<unsigned>(Callee->getNumParams(), Args.size());
        I != N; ++I) {
-    if (Callee->getParamDecl(I)->hasAttr<LifetimeBoundAttr>())
+    if (checkCoroCall || Callee->getParamDecl(I)->hasAttr<LifetimeBoundAttr>())
       VisitLifetimeBoundArg(Callee->getParamDecl(I), Args[I]);
   }
 }
diff --git a/clang/test/Misc/pragma-attribute-supported-attributes-list.test b/clang/test/Misc/pragma-attribute-supported-attributes-list.test
index a57bc011c059483..dd91f4f88ad685b 100644
--- a/clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ b/clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -56,9 +56,10 @@
 // CHECK-NEXT: ConsumableAutoCast (SubjectMatchRule_record)
 // CHECK-NEXT: ConsumableSetOnRead (SubjectMatchRule_record)
 // CHECK-NEXT: Convergent (SubjectMatchRule_function)
+// CHECK-NEXT: CoroLifetimeBound (SubjectMatchRule_record)
 // CHECK-NEXT: CoroOnlyDestroyWhenComplete (SubjectMatchRule_record)
 // CHECK-NEXT: CoroReturnType (SubjectMatchRule_record)
-// CHECK-NEXT: CoroWrapper
+// CHECK-NEXT: CoroWrapper (SubjectMatchRule_function)
 // CHECK-NEXT: CountedBy (SubjectMatchRule_field)
 // CHECK-NEXT: DLLExport (SubjectMatchRule_function, SubjectMatchRule_variable, SubjectMatchRule_record, SubjectMatchRule_objc_interface)
 // CHECK-NEXT: DLLImport (SubjectMatchRule_function, SubjectMatchRule_variable, SubjectMatchRule_record, SubjectMatchRule_objc_interface)
diff --git a/clang/test/SemaCXX/coro-lifetimebound.cpp b/clang/test/SemaCXX/coro-lifetimebound.cpp
new file mode 100644
index 000000000000000..a12315d17096199
--- /dev/null
+++ b/clang/test/SemaCXX/coro-lifetimebound.cpp
@@ -0,0 +1,100 @@
+// RUN: %clang_cc1 -triple x86_64-apple-darwin9 %s -std=c++20 -fsyntax-only -verify -Wall -Wextra -Wno-error=unreachable-code -Wno-unused
+
+#include "Inputs/std-coroutine.h"
+
+using std::suspend_always;
+using std::suspend_never;
+
+template <typename T> struct [[clang::coro_lifetimebound, clang::coro_return_type]] Co {
+  struct promise_type {
+    Co<T> get_return_object() {
+      return {};
+    }
+    suspend_always initial_suspend();
+    suspend_always final_suspend() noexcept;
+    void unhandled_exception();
+    void return_value(const T &t);
+
+    template <typename U>
+    auto await_transform(const Co<U> &) {
+      struct awaitable {
+        bool await_ready() noexcept { return false; }
+        void await_suspend(std::coroutine_handle<>) noexcept {}
+        U await_resume() noexcept { return {}; }
+      };
+      return awaitable{};
+    }
+  };
+};
+
+Co<int> foo_coro(const int& b) {
+  if (b > 0)
+    co_return 1;
+  co_return 2;
+}
+
+int getInt() { return 0; }
+
+Co<int> bar_coro(const int &b, int c) {
+  int x = co_await foo_coro(b);
+  int y = co_await foo_coro(1);
+  int z = co_await foo_coro(getInt());
+  auto unsafe1 = foo_coro(1); // expected-warning {{temporary whose address is used as value of local variable}}
+  auto unsafe2 = foo_coro(getInt()); // expected-warning {{temporary whose address is used as value of local variable}}
+  auto  safe1 = foo_coro(b);
+  auto  safe2 = foo_coro(c);
+  co_return co_await foo_coro(co_await foo_coro(1));
+}
+
+[[clang::coro_wrapper]] Co<int> plain_return_co(int b) {
+  return foo_coro(b); // expected-warning {{address of stack memory associated with parameter}}
+}
+
+[[clang::coro_wrapper]] Co<int> safe_forwarding(const int& b) {
+  return foo_coro(b);
+}
+
+[[clang::coro_wrapper]] Co<int> unsafe_wrapper(int b) {
+  return safe_forwarding(b); // expected-warning {{address of stack memory associated with parameter}}
+}
+
+[[clang::coro_wrapper]] Co<int> complex_plain_return(int b) {
+  return b > 0 
+      ? foo_coro(1)   // expected-warning {{returning address of local temporary object}}
+      : bar_coro(0, 1); // expected-warning {{returning address of local temporary object}}
+}
+
+#define CORO_WRAPPER \
+  _Pragma("clang diagnostic push") \
+  _Pragma("clang diagnostic ignored \"-Wc++23-extensions\"") \
+  [[clang::coro_wrapper]] \
+  _Pragma("clang diagnostic pop")
+
+void lambdas() {
+  auto unsafe_lambda = [] CORO_WRAPPER (int b) {
+    return foo_coro(b); // expected-warning {{address of stack memory associated with parameter}}
+  };
+  auto coro_lambda = [] (const int&) -> Co<int> {
+    co_return 0;
+  };
+  auto unsafe_coro_lambda = [&] (const int& b) -> Co<int> {
+    int x = co_await coro_lambda(b);
+    auto safe = coro_lambda(b);
+    auto unsafe1 = coro_lambda(1); // expected-warning {{temporary whose address is used as value of local variable}}
+    auto unsafe2 = coro_lambda(getInt()); // expected-warning {{temporary whose address is used as value of local variable}}
+    auto unsafe3 = coro_lambda(co_await coro_lambda(b)); // expected-warning {{temporary whose address is used as value of local variable}}
+    co_return co_await safe;
+  };
+  auto safe_lambda = [](int b) -> Co<int> {
+    int x = co_await foo_coro(1);
+    co_return x + co_await foo_coro(b);
+  };
+}
+// =============================================================================
+// Safe usage when parameters are value
+// =============================================================================
+namespace by_value {
+Co<int> value_coro(int b) { co_return co_await foo_coro(b); }
+[[clang::coro_wrapper]] Co<int> wrapper1(int b) { return value_coro(b); }
+[[clang::coro_wrapper]] Co<int> wrapper2(const int& b) { return value_coro(b); }
+}

@usx95 usx95 requested review from hokein and ChuanqiXu9 November 20, 2023 15:24
Copy link
Member

@ChuanqiXu9 ChuanqiXu9 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Please wait for few days in case there are other comments.

Copy link

github-actions bot commented Nov 21, 2023

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

@usx95 usx95 closed this Nov 21, 2023
(cherry picked from commit 28e9fda4b78e1e60287048891cc92bafdef3ac4c)
(cherry picked from commit cfc5fa4e59e551a391ecc9be8db6d4b98d37e45a)
(cherry picked from commit 3f5f1a370300ff41483efd6b68c4dd9d85ed025a)
(cherry picked from commit 54c62257caaa3c5070b977bad9bc96bdd6c8c69e)
(cherry picked from commit 932ddd98e6b46b21f1d85834d2586c6de8e024aa)
@usx95 usx95 reopened this Nov 21, 2023
@usx95 usx95 removed request for a team, nikic, antiagainst, JDevlieghere and kuhar November 21, 2023 03:22
Copy link
Collaborator

@hokein hokein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, looks good from my side. Some nits.

Copy link
Contributor

@ilya-biryukov ilya-biryukov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with a few suggestiosn.

One is in the comment.
I also suggest to add a warning when users mark the type as coro_lifetimebound without coro_return_type.
The analysis will be disabled in this case and it may be hard to understand why.

However, I don't think that's a reason to block this change. This could be a follow-up.

@usx95
Copy link
Contributor Author

usx95 commented Nov 21, 2023

I would add the mentioned warning in a follow-up change.

@usx95 usx95 merged commit fbfd2c9 into llvm:main Nov 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants