Skip to content

[RFC] Perform lifetime bound checks for arguments to coroutine #69360

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

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion clang/lib/Sema/SemaInit.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -7580,10 +7580,22 @@ static void visitLifetimeBoundArguments(IndirectLocalPath &Path, Expr *Call,
if (ObjectArg && implicitObjectParamIsLifetimeBound(Callee))
VisitLifetimeBoundArg(Callee, ObjectArg);

bool checkCoroCall = false;
if (const auto *RD = Callee->getReturnType()->getAsRecordDecl()) {
for (const auto &attr :
RD->getUnderlyingDecl()->specific_attrs<clang::AnnotateAttr>()) {
// Only for demonstration: Get feedback and add a clang annotation as an
// extension.
if (attr->getAnnotation() == "coro_type") {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think you need this to find if a record is a "coroutine type", there are other ways, for example:

if (rec->getDeclName().isIdentifier() && rec->getName() == "promise_type") {
....

checkCoroCall = true;
break;
}
}
}
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]);
}
}
Expand Down
184 changes: 184 additions & 0 deletions clang/test/SemaCXX/coroutine-lifetimebound-args.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,184 @@
// 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;


#define CORO_TYPE [[clang::annotate("coro_type")]]
#define CORO_UNSAFE [[clang::annotate("coro_unsafe")]]

template <typename T> struct CORO_TYPE Gen {
struct promise_type {
Gen<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 Gen<U> &) {
struct awaitable {
bool await_ready() noexcept { return false; }
void await_suspend(std::coroutine_handle<>) noexcept {}
U await_resume() noexcept { return {}; }
};
return awaitable{};
}
};
};

template <typename T> using Co = Gen<T>;

Gen<int> foo_coro(const int& b);

Gen<int> plain_return_foo_decl(int b) {
return foo_coro(b); // expected-warning {{address of stack memory associated with parameter}}
}

Gen<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);
Copy link
Member

Choose a reason for hiding this comment

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

How is this implemented? I mean how can we don't emit warnings to co_await foo_coro(1) but emit warnings to auto unsafe1 = foo_coro(1);? This is not showed in this patch itself.

I envision the reason may be that we will copy the parameters at the beginning of the coroutines. If it is indeed the reason, then it smells bad. Since the behavior depends that behavior implicitly.

For example, the behavior of co_await foo_coro(); depends on the implementation of the corresponding awaiter. It is completely possible that co_await foo_coro(); doing exactly the same thing with a standalone foo_coro();. But we may emit warnings for one place but not the other one.

Copy link
Contributor Author

@usx95 usx95 Oct 27, 2023

Choose a reason for hiding this comment

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

This is because co_await expr is equivalent to auto x = SomeFunc(foo_coro(1));
(func is await_transform for co_await).
This is safe because temporary lives as long as the SomeFunc call. This same reasoning can be applied to co_await as the temporary lives in the coroutine frame as long as the coroutine co_awaits.

The only implementation of awaiter that could be problematic and go undetected is when it schedules the foo_coro on another thread and resumes the awaiting coroutine before foo_coro is finished.

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

Gen<int> plain_return_co(int b) {
return foo_coro(b); // expected-warning {{address of stack memory associated with parameter}}
}

Gen<int> safe_forwarding(const int& b) {
return foo_coro(b);
}

Gen<int> unsafe_wrapper(int b) {
return safe_forwarding(b); // expected-warning {{address of stack memory associated with parameter}}
}

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}}
}

void lambdas() {
auto unsafe_lambda = [](int b) {
return foo_coro(b); // expected-warning {{address of stack memory associated with parameter}}
};
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 {
Gen<int> value_coro(int b) { co_return co_await foo_coro(b); }

Gen<int> wrapper1(int b) { return value_coro(b); }
Gen<int> wrapper2(const int& b) { return value_coro(b); }
}

// =============================================================================
// std::function like wrappers. (Eg: https://godbolt.org/z/x3PfG3Gfb)
// =============================================================================
namespace std {

template <class T>
T&& forward(typename remove_reference<T>::type& t) noexcept;
template <class T>
T&& forward(typename remove_reference<T>::type&& t) noexcept;

template <bool, typename>
class function;

template <bool UseFp, typename ReturnValue, typename... Args>
class function<UseFp, ReturnValue(Args...)> {
public:
class Callable {
public:
ReturnValue operator()(Args&&...) const { return {}; }
};
Callable* callable_;
ReturnValue operator()(Args... args) const
requires (!UseFp)
{
return (*callable_)(std::forward<Args>(args)...); // expected-warning 3 {{address of stack memory}}
}

// Callable can also be a function pointer type.
using FpCallableType = ReturnValue(Args&&...);
FpCallableType* fp_callable_;
ReturnValue operator()(Args... args) const
requires(UseFp)
{
return fp_callable_(std::forward<Args>(args)...);
}

template <typename T>
function& operator=(T) {}
template <typename T>
function(T) {}
function() {}
};
} // namespace std

namespace without_function_pointers {
template <typename T>
using fn = std::function<false, T>;

void use_std_function() {
fn<Co<int>(const int&, const int&)> pass;
pass(1, 1);
// Lifetime issue with one parameter.
fn<Co<int>(const int&, int)> fail;
fail(1, 1); // expected-note {{in instantiation of}}
// Lifetime issue with both parameters.
fn<Co<int>(int, int)> fail_twice;
fail_twice(1, 1); // expected-note {{in instantiation of}}
}
} // namespace without_function_pointers

// =============================================================================
// Future work: Function pointers needs to be fixed.
// =============================================================================
namespace with_function_pointers {
template <typename T>
using fn = std::function<true, T>;

void use_std_function() {
fn<Co<int>(int, int)> fail;
fail(1, 1); // FIXME: Must error.
}
} // namespace function_pointers

// =============================================================================
// Future work: Reference wrappers needs to be fixed.
// =============================================================================
namespace with_reference_wrappers {
struct RefWrapper {
RefWrapper(const int &a): b(a) {}
const int &b;
};
Co<int> RefWrapperCoro(RefWrapper a) { co_return a.b; }

Co<int> UnsafeWrapper(int a) {
return RefWrapperCoro(a); // FIXME: Must error.
}
}