Skip to content

clang allocates the coroutine promise on the stack although it should put the promise on the heap #56513

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
cppdev123 opened this issue Jul 13, 2022 · 6 comments
Assignees
Labels
clang:codegen IR generation bugs: mangling, exceptions, etc. coroutines C++20 coroutines duplicate Resolved as duplicate miscompilation

Comments

@cppdev123
Copy link

cppdev123 commented Jul 13, 2022

So I opened this issue #56455 but didn't receive any response and I thought this is to due lack of reproducible code that demonstrates the bug and the somewhat strange title so I opened this new issue after had a code to reproduce the problem:

The problem occurs with clang 14.0.6 with max optimizations due to allocating a promise of a coroutine on the stack while the coroutine return object task has escaped but clang didn't detect that !
The exception allocated on the stack turned to be not a problem but it the Microsoft c++ abi which clang adapts on windows.
The problem is tricky because it only happens when escaping the pointer with a pure virtual noexcept method using the abstract base

code to demonstrate the problem:
on godbolt: https://godbolt.org/z/qTdaMEj5o

code here on github:

#include <atomic>
#include <thread>
#include <condition_variable>
#include <coroutine>
#include <variant>
#include <deque>
#include <cassert>

// executor and operation base

class bug_any_executor;

struct bug_async_op_base
{
	void invoke()
	{
		invoke_operation();
	}

protected:

	~bug_async_op_base() = default;

	virtual void invoke_operation() = 0;
};

class bug_any_executor
{
	using op_type = bug_async_op_base;

public:

	virtual ~bug_any_executor() = default;

	// removing noexcept enables clang to find that the pointer has escaped
	virtual void post(op_type& op) noexcept = 0;

	virtual void wait() noexcept = 0;
};

class bug_thread_executor : public bug_any_executor
{
	void work_thd()
	{
		while (!ops_.empty())
		{
			std::unique_lock<std::mutex> lock{ lock_ };
			cv_.wait(lock, [this] { return !ops_.empty(); });

			while (!ops_.empty())
			{
				bug_async_op_base* op = ops_.front();
				ops_.pop_front();
				op->invoke();
			}
		}

		cv_.notify_all();
	}

	std::mutex lock_;
	std::condition_variable cv_;
	std::deque<bug_async_op_base*> ops_;
	std::thread thd_;

public:

	void start()
	{
		thd_ = std::thread(&bug_thread_executor::work_thd, this);
	}

	~bug_thread_executor()
	{
		if (thd_.joinable())
			thd_.join();
	}

	// although this implementation is not realy noexcept due to allocation but I have a real one that is and required to be noexcept
	virtual void post(bug_async_op_base& op) noexcept override
	{
		{
			std::unique_lock<std::mutex> lock{ lock_ };
			ops_.push_back(&op);
		}
		cv_.notify_all();
	}

	virtual void wait() noexcept override
	{
		std::unique_lock<std::mutex> lock{ lock_ };
		cv_.wait(lock, [this] { return ops_.empty(); });
	}
};

// task and promise

struct bug_final_suspend_notification
{
	virtual std::coroutine_handle<> get_waiter() = 0;
};

class bug_task;

class bug_resume_waiter
{
public:
	bug_resume_waiter(std::coroutine_handle<> waiter) noexcept : waiter_{ waiter } {}

	constexpr bool await_ready() const noexcept { return false; }

	std::coroutine_handle<> await_suspend(std::coroutine_handle<>) noexcept
	{
		return waiter_;
	}

	constexpr void await_resume() const noexcept {}

private:
	std::coroutine_handle<> waiter_;
};

class bug_task_promise
{
	friend bug_task;
public:

	bug_task get_return_object() noexcept;

	constexpr std::suspend_always initial_suspend() noexcept { return {}; }

	bug_resume_waiter final_suspend() noexcept 
	{
		return bug_resume_waiter{ waiter_.index() == 0 ? std::get<0>(waiter_) : std::get<1>(waiter_)->get_waiter() };
	}

	void unhandled_exception() noexcept
	{
		ex_ptr = std::current_exception();
	}

	constexpr void return_void() const noexcept {}

	void get_result() const
	{
		if (ex_ptr)
			std::rethrow_exception(ex_ptr);
	}

	std::variant<std::monostate, std::exception_ptr> result_or_error() const noexcept
	{
		if (ex_ptr)
			return ex_ptr;
		return {};
	}

private:
	std::variant<std::coroutine_handle<>, bug_final_suspend_notification*> waiter_;
	std::exception_ptr ex_ptr = nullptr;
};

class bug_task
{
	friend bug_task_promise;
	using handle = std::coroutine_handle<>;
	using promise_t = bug_task_promise;

	bug_task(handle coro, promise_t* p) noexcept : this_coro{ coro }, this_promise{ p }
	{
		//printf("task(%p) coroutine(%p) promise(%p)\n", this, this_coro.address(), this_promise);
	}

public:

	using promise_type = bug_task_promise;

	bug_task(bug_task&& other) noexcept
		: this_coro{ std::exchange(other.this_coro, nullptr) }, this_promise{ std::exchange(other.this_promise, nullptr) }
	{ 
		printf("task(task&&: %p) coroutine(%p) promise(%p)\n", this, this_coro.address(), this_promise); 
	}

	~bug_task()
	{
		if (this_coro) {
			//printf("~task(%p) coroutine(%p) promise(%p)\n", this, this_coro.address(), this_promise);
			this_coro.destroy();
		}
	}

	constexpr bool await_ready() const noexcept
	{
		return false;
	}

	handle await_suspend(handle waiter) noexcept
	{
		assert(this_coro != nullptr && this_promise != nullptr);
		this_promise->waiter_ = waiter;
		return this_coro;
	}

	void await_resume() 
	{
		return this_promise->get_result();
	}

	bool is_valid() const noexcept
	{
		return this_promise != nullptr && this_coro != nullptr;
	}

	void start_coro(bug_final_suspend_notification& w) noexcept
	{
		assert(this_promise != nullptr && this_coro != nullptr);
		this_promise->waiter_ = &w;
		this_coro.resume(); // never throws since all exceptions are caught by the promise
	}

private:
	handle this_coro;
	promise_t* this_promise;
};

bug_task bug_task_promise::get_return_object() noexcept
{
	return { std::coroutine_handle<bug_task_promise>::from_promise(*this), this };
}

// spawn operation and spawner

template<class Handler>
class bug_spawn_op final : public bug_async_op_base, bug_final_suspend_notification
{
	Handler handler;
	bug_task task_;

public:

	bug_spawn_op(Handler handler, bug_task&& t)
		: handler { handler }, task_{ std::move(t) } {}

	virtual void invoke_operation() override
	{
		printf("starting the coroutine\n");
		task_.start_coro(*this);
		printf("started the coroutine\n");
	}

	virtual std::coroutine_handle<> get_waiter() override
	{
		handler();
		return std::noop_coroutine();
	}
};

struct dummy_spawn_handler_t
{
	constexpr void operator()() const noexcept {}
};

void bug_spawn(bug_any_executor& ex, bug_task&& t)
{
	using op_t = bug_spawn_op<dummy_spawn_handler_t>;
	op_t* op = new op_t{ dummy_spawn_handler_t{}, std::move(t) };
	ex.post(*op);
}

class bug_spawner;

struct bug_spawner_awaiter
{
	bug_spawner& s;
	std::coroutine_handle<> waiter;

	bug_spawner_awaiter(bug_spawner& s) : s{ s } {}

	bool await_ready() const noexcept;

	void await_suspend(std::coroutine_handle<> coro);

	void await_resume() {}
};

class bug_spawner
{
	friend bug_spawner_awaiter;

	struct final_handler_t
	{
		bug_spawner& s;

		void operator()()
		{
			s.on_spawn_finished();
		}
	};

public:

	bug_spawner(bug_any_executor& ex) : ex_{ ex } {}

	void spawn(bug_task&& t)
	{
		using op_t = bug_spawn_op<final_handler_t>;
		// move task into ptr
		op_t* ptr = new op_t(final_handler_t{ *this }, std::move(t));
		++count_;
		ex_.post(*ptr); // ptr escapes here thus task escapes but clang can't deduce that unless post() is not noexcept
	}

	bug_spawner_awaiter wait() noexcept { return { *this }; }

	void on_spawn_finished()
	{
		if (!--count_ && awaiter_)
		{
			auto a = std::exchange(awaiter_, nullptr);
			a->waiter.resume();
		}
	}

private:

	bug_any_executor& ex_; // if bug_thread_executor& is used instead enables clang to detect the escape of the promise
	bug_spawner_awaiter* awaiter_ = nullptr;
	std::atomic<std::size_t> count_ = 0;
};

bool bug_spawner_awaiter::await_ready() const noexcept
{
	return s.count_ == 0;
}

void bug_spawner_awaiter::await_suspend(std::coroutine_handle<> coro)
{
	waiter = coro;
	s.awaiter_ = this;
}

template<std::invocable<bug_spawner&> Fn>
bug_task scoped_spawn(bug_any_executor& ex, Fn fn)
{
	bug_spawner s{ ex };
	std::exception_ptr ex_ptr;

	try
	{
		fn(s);
	}
	catch (const std::exception& ex) // ex instead of ... to observe the address of ex
	{
		printf("caught an exception from fn(s): %p\n", std::addressof(ex));
		ex_ptr = std::current_exception();
	}

	co_await s.wait();
	if (ex_ptr)
		std::rethrow_exception(ex_ptr);
}

// forked task to start the coroutine from sync code

struct bug_forked_task_promise;

class bug_forked_task
{
	friend struct bug_forked_task_promise;
	bug_forked_task() = default;
public:
	using promise_type = bug_forked_task_promise;
};

struct bug_forked_task_promise
{
	bug_forked_task get_return_object() noexcept { return {}; }

	constexpr std::suspend_never initial_suspend() noexcept { return {}; }

	constexpr std::suspend_never final_suspend() noexcept { return {}; }

	void unhandled_exception() noexcept
	{
		std::terminate();
	}

	constexpr void return_void() const noexcept {}
};

// test case

bug_task bug_spawned_task(int id, int inc, std::atomic<int>& n)
{
	int result = n += inc;
	std::string msg = "count in coro (" + std::to_string(id) + ") = " + std::to_string(result);
	printf("%s\n", msg.c_str());
	co_return;
}

bug_forked_task run_coros(bug_any_executor& ex)
{
	std::atomic<int> count = 0;
	auto throwing_fn = [&](bug_spawner& s)
	{
		int stack_ptr = 0;
		printf("stack ptr: %p\n", std::addressof(stack_ptr));
		s.spawn(bug_spawned_task(1, 2, count)); // the coroutine frame is allocated on the stack !
		s.spawn(bug_spawned_task(2, 3, count));
		s.spawn(bug_spawned_task(3, 5, count));
		throw std::runtime_error{ "catch this !" }; // allocated on the stack as required by msvc c++ abi
	};

	try
	{
		co_await scoped_spawn(ex, throwing_fn);
	}
	catch (const std::exception& ex)
	{
		printf("scoped_spawn propagated exception: %s\n", ex.what());
	}

	printf("count after scoped_spawn: %d\n", count.load());
}


int main()
{
	int var = 0;
	bug_thread_executor ex;
	printf("stack address: %p\n", std::addressof(var));
	run_coros(ex);
	ex.start();
	ex.wait();
	return 0;
}
@cppdev123
Copy link
Author

ChuanqiXu9 can you take a look at this ?

@EugeneZelenko EugeneZelenko added clang:codegen IR generation bugs: mangling, exceptions, etc. and removed new issue labels Jul 13, 2022
@llvmbot
Copy link
Member

llvmbot commented Jul 13, 2022

@llvm/issue-subscribers-clang-codegen

@cppdev123
Copy link
Author

this issue should have the coroutine tag

@llvmbot
Copy link
Member

llvmbot commented Mar 14, 2023

@llvm/issue-subscribers-coroutines

@ChuanqiXu9
Copy link
Member

Sorry for the slow response, (I probably forgot this before). IIUC, this may be a duplicate of #56455?

@ChuanqiXu9
Copy link
Member

It looks like this is a duplication of #59723 too after I look into them actually.

@ChuanqiXu9 ChuanqiXu9 added the duplicate Resolved as duplicate label Aug 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:codegen IR generation bugs: mangling, exceptions, etc. coroutines C++20 coroutines duplicate Resolved as duplicate miscompilation
Projects
Status: Done
Development

No branches or pull requests

5 participants