From 8f8f6cfae4ff021e9ca098c1ed3d337bc41c1510 Mon Sep 17 00:00:00 2001
From: "Wei Wang (Server LLVM)" <wangwei@meta.com>
Date: Tue, 10 Oct 2023 22:29:01 -0700
Subject: [PATCH] revise Optional and Expected coroutine return-object tracking
Summary:
LLVM 15 built code crashes n the existing coroutine implementation for `folly::Expected`.
LLVM 15 has changed how it handles the coroutine return-object in https://reviews.llvm.org/D117087, in a way which is incompatible with the existing code for `folly::Expected`. It looks like the new behavior is the same eager-conversion alternative that MSVC 2019 update 16.5 finally retired, so the new behavior might be classified as a regression.
Fix this regression by revising how Optional and Expected manage the coroutine promise return-object and by detecting which behavior is required. Lift the behavior-detection facility to the base coroutine header.
We have a patched LLVM 15 with the offending upstream patch reverted so, under LLVM, the detection must be more complex than a simple version check; we do the detection at runtime since, conveniently, LLVM manages to optimize all of the runtime detection down to a constant. Other alternatives we considered are: (1) to return to the upstream behavior and do a straightforward version check or (2) to have the patched compiler, or builds using the patched compiler, predefine a preprocessor symbol and have the detection look for that symbol.
Reviewed By: bcardosolopes
Differential Revision: D42260201
fbshipit-source-id: cbef347e373c8580b3ad53b1c78c6bab8e8a7db6
folly/Expected.h | 55 ++++----
folly/Optional.h | 37 ++++--
folly/experimental/coro/Coroutine.h | 118 ++++++++++++++++++
.../experimental/coro/test/CoroutineTest.cpp | 50 ++++++++
4 files changed, 227 insertions(+), 33 deletions(-)
create mode 100644 folly/experimental/coro/test/CoroutineTest.cpp
@@ -22,6 +22,7 @@
#pragma once
+#include <cassert>
#include <cstddef>
#include <initializer_list>
#include <new>
@@ -1095,12 +1096,6 @@ class Expected final : expected_detail::ExpectedStorage<Value, Error> {
return *this;
}
-#ifndef __clang__
- // Used only when an Expected is used with coroutines on MSVC/GCC
- /* implicit */ Expected(expected_detail::PromiseReturn<Value, Error>&& p)
- : Expected{std::move(*p.storage_)} {}
-#endif
-
template <class... Ts FOLLY_REQUIRES_TRAILING(
std::is_constructible<Value, Ts&&...>::value)>
void emplace(Ts&&... ts) {
@@ -1342,6 +1337,15 @@ class Expected final : expected_detail::ExpectedStorage<Value, Error> {
}
private:
+ friend struct expected_detail::PromiseReturn<Value, Error>;
+ using EmptyTag = expected_detail::EmptyTag;
+
+ explicit Expected(EmptyTag tag) noexcept : Base{tag} {}
+ // for when coroutine promise return-object conversion is eager
+ Expected(EmptyTag tag, Expected*& pointer) noexcept : Base{tag} {
+ pointer = this;
+ }
+
void requireValue() const {
if (FOLLY_UNLIKELY(!hasValue())) {
if (FOLLY_LIKELY(hasError())) {
@@ -1513,35 +1517,42 @@ struct Promise;
template <typename Value, typename Error>
struct PromiseReturn {
- Optional<Expected<Value, Error>> storage_;
- Promise<Value, Error>* promise_;
- /* implicit */ PromiseReturn(Promise<Value, Error>& promise) noexcept
- : promise_(&promise) {
- promise_->value_ = &storage_;
- }
- PromiseReturn(PromiseReturn&& that) noexcept
- : PromiseReturn{*that.promise_} {}
+ Expected<Value, Error> storage_{EmptyTag{}};
+ Expected<Value, Error>*& pointer_;
+
+ /* implicit */ PromiseReturn(Promise<Value, Error>& p) noexcept
+ : pointer_{p.value_} {
+ pointer_ = &storage_;
+ }
+ PromiseReturn(PromiseReturn const&) = delete;
+ // letting dtor be trivial makes the coroutine crash
+ // TODO: fix clang/llvm codegen
~PromiseReturn() {}
- /* implicit */ operator Expected<Value, Error>() & {
- return std::move(*storage_);
+ /* implicit */ operator Expected<Value, Error>() {
+ // handle both deferred and eager return-object conversion behaviors
+ // see docs for detect_promise_return_object_eager_conversion
+ if (folly::coro::detect_promise_return_object_eager_conversion()) {
+ assert(storage_.which_ == expected_detail::Which::eEmpty);
+ return Expected<Value, Error>{EmptyTag{}, pointer_}; // eager
+ } else {
+ assert(storage_.which_ != expected_detail::Which::eEmpty);
+ return std::move(storage_); // deferred
+ }
}
};
template <typename Value, typename Error>
struct Promise {
- Optional<Expected<Value, Error>>* value_ = nullptr;
+ Expected<Value, Error>* value_ = nullptr;
+
Promise() = default;
Promise(Promise const&) = delete;
- // This should work regardless of whether the compiler generates:
- // folly::Expected<Value, Error> retobj{ p.get_return_object(); } // MSVC
- // or:
- // auto retobj = p.get_return_object(); // clang
PromiseReturn<Value, Error> get_return_object() noexcept { return *this; }
coro::suspend_never initial_suspend() const noexcept { return {}; }
coro::suspend_never final_suspend() const noexcept { return {}; }
template <typename U = Value>
void return_value(U&& u) {
- value_->emplace(static_cast<U&&>(u));
+ *value_ = static_cast<U&&>(u);
}
void unhandled_exception() {
// Technically, throwing from unhandled_exception is underspecified:
@@ -54,6 +54,7 @@
* }
*/
+#include <cassert>
#include <cstddef>
#include <functional>
#include <new>
@@ -76,6 +77,7 @@ template <class Value>
class Optional;
namespace detail {
+struct OptionalEmptyTag {};
template <class Value>
struct OptionalPromise;
template <class Value>
@@ -396,6 +398,8 @@ class Optional {
}
private:
+ friend struct detail::OptionalPromiseReturn<Value>;
+
template <class T>
friend constexpr Optional<std::decay_t<T>> make_optional(T&&);
template <class T, class... Args>
@@ -419,6 +423,10 @@ class Optional {
std::is_nothrow_constructible<Value, Args&&...>::value) {
construct(std::forward<Args>(args)...);
}
+ // for when coroutine promise return-object conversion is eager
+ explicit Optional(detail::OptionalEmptyTag, Optional*& pointer) noexcept {
+ pointer = this;
+ }
void require_value() const {
if (!storage_.hasValue) {
@@ -662,15 +670,26 @@ struct OptionalPromise;
template <typename Value>
struct OptionalPromiseReturn {
Optional<Value> storage_;
- OptionalPromise<Value>* promise_;
- /* implicit */ OptionalPromiseReturn(OptionalPromise<Value>& promise) noexcept
- : promise_(&promise) {
- promise.value_ = &storage_;
+ Optional<Value>*& pointer_;
+
+ /* implicit */ OptionalPromiseReturn(OptionalPromise<Value>& p) noexcept
+ : pointer_{p.value_} {
+ pointer_ = &storage_;
}
- OptionalPromiseReturn(OptionalPromiseReturn&& that) noexcept
- : OptionalPromiseReturn{*that.promise_} {}
+ OptionalPromiseReturn(OptionalPromiseReturn const&) = delete;
+ // letting dtor be trivial makes the coroutine crash
+ // TODO: fix clang/llvm codegen
~OptionalPromiseReturn() {}
- /* implicit */ operator Optional<Value>() & { return std::move(storage_); }
+ /* implicit */ operator Optional<Value>() {
+ // handle both deferred and eager return-object conversion behaviors
+ // see docs for detect_promise_return_object_eager_conversion
+ if (folly::coro::detect_promise_return_object_eager_conversion()) {
+ assert(!storage_.has_value());
+ return Optional{OptionalEmptyTag{}, pointer_}; // eager
+ } else {
+ return std::move(storage_); // deferred
+ }
+ }
};
template <typename Value>
@@ -678,10 +697,6 @@ struct OptionalPromise {
Optional<Value>* value_ = nullptr;
OptionalPromise() = default;
OptionalPromise(OptionalPromise const&) = delete;
- // This should work regardless of whether the compiler generates:
- // folly::Optional<Value> retobj{ p.get_return_object(); } // MSVC
- // or:
- // auto retobj = p.get_return_object(); // clang
OptionalPromiseReturn<Value> get_return_object() noexcept { return *this; }
coro::suspend_never initial_suspend() const noexcept { return {}; }
coro::suspend_never final_suspend() const noexcept { return {}; }
@@ -181,6 +181,124 @@ class variant_awaitable : private std::variant<A...> {
#endif // __has_include(<variant>)
+// ----
+
+namespace detail {
+
+struct detect_promise_return_object_eager_conversion_ {
+ struct promise_type {
+ struct return_object {
+ /* implicit */ return_object(promise_type& p) noexcept : promise{&p} {
+ promise->object = this;
+ }
+ ~return_object() {
+ if (promise) {
+ promise->object = nullptr;
+ }
+ }
+
+ promise_type* promise;
+ };
+
+ ~promise_type() {
+ if (object) {
+ object->promise = nullptr;
+ }
+ }
+
+ suspend_never initial_suspend() const noexcept { return {}; }
+ suspend_never final_suspend() const noexcept { return {}; }
+ void unhandled_exception() {}
+
+ return_object get_return_object() noexcept { return {*this}; }
+ void return_void() {}
+
+ return_object* object = nullptr;
+ };
+
+ /* implicit */ detect_promise_return_object_eager_conversion_(
+ promise_type::return_object const& o) noexcept
+ : eager{!!o.promise} {}
+ // letting the coroutine type be trivially-copyable makes the coroutine crash
+ // under clang; to work around, provide an empty but not trivial destructor
+ ~detect_promise_return_object_eager_conversion_() {}
+
+ bool eager = false;
+
+// FIXME: when building against Apple SDKs using c++17, we hit this all over
+// the place on complex testing infrastructure for iOS. Since it's not clear
+// how to fix the issue properly right now, force ignore this warnings and help
+// unblock expected/optional coroutines. This should be removed once the build
+// configuration is changed to use -Wno-deprecated-experimental-coroutine.
+#if defined(__clang__) && (__clang_major__ < 17 && __clang_major__ > 13)
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wdeprecated-experimental-coroutine"
+ static detect_promise_return_object_eager_conversion_ go() noexcept {
+ co_return;
+ }
+#pragma clang diagnostic pop
+#else
+ static detect_promise_return_object_eager_conversion_ go() noexcept {
+ co_return;
+ }
+#endif
+};
+
+} // namespace detail
+
+// detect_promise_return_object_eager_conversion
+//
+// Returns true if the compiler implements coroutine promise return-object
+// conversion eagerly and returns false if the compiler defers conversion.
+//
+// It is expected that the caller holds the promise return-object until the
+// promise is fulfilled, even when it is not the same type as the coroutine.
+//
+// auto ret = promise.get_return_object();
+// initial-suspend, etc...
+// return ret;
+//
+// But this expected behavior was, mistakenly, never specified.
+//
+// Some compilers misbehave, where the caller holds precisely the coroutine
+// type by converting the promise return-object eagerly when it is of some
+// type different from the coroutine type.
+//
+// coro-type ret = promise.get_return_object();
+// initial-suspend, etc...
+// return ret;
+//
+// Known behaviors are as follows:
+// * For msvc, conversion is eager for vs < 2019 update 16.5 (msc ver 1925) and
+// is deferred for vs >= 2019 update 16.5 (msc ver 1925).
+// References:
+// https://developercommunity.visualstudio.com/t/c-coroutine-get-return-object-converted-too-early/222420
+// * For g++, conversion is deferred.
+// * For clang++, conversion is eager for 15 <= llvm < 17 and is deferred for
+// llvm < 15 or llvm >= 17.
+// References:
+// https://reviews.llvm.org/D117087
+// https://github.com/llvm/llvm-project/issues/56532
+// https://reviews.llvm.org/D145639
+//
+// Meta sometimes uses llvm patched to have deferred conversion where the
+// corresponding upstream implements eager conversion. So version numbers do
+// not tell the whole story.
+//
+// This function detects which behavior the compiler implements at a mix of
+// compile time and run time, depending on the compiler. It is only necessary
+// to do the runtime detection for llvm but, conveniently, llvm is able to do
+// full heap-allocation elision ("HALO") and optimize the detection down to a
+// constant.
+//
+// TODO: Remove this detection once the behavior is specified.
+inline bool detect_promise_return_object_eager_conversion() {
+ using coro = detail::detect_promise_return_object_eager_conversion_;
+ constexpr auto t = kMscVer && kMscVer < 1925;
+ constexpr auto f = (kGnuc && !kIsClang) || (kMscVer >= 1925);
+ return t ? true : f ? false : coro::go().eager;
+}
+
class ExtendedCoroutineHandle;
// Extended promise interface folly::coro types are expected to implement
new file mode 100644
@@ -0,0 +1,50 @@
+/*
+ * Copyright (c) Meta Platforms, Inc. and affiliates.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include <folly/experimental/coro/Coroutine.h>
+
+#include <folly/lang/Keep.h>
+#include <folly/portability/GTest.h>
+
+#if FOLLY_HAS_COROUTINES
+
+extern "C" FOLLY_KEEP bool
+check_folly_coro_detect_promise_return_object_eager_conversion() {
+ return folly::coro::detect_promise_return_object_eager_conversion();
+}
+
+struct CoroutineTest : testing::Test {};
+
+TEST_F(CoroutineTest, detect_promise_return_object_eager_conversion) {
+ auto const eager =
+ folly::coro::detect_promise_return_object_eager_conversion();
+ if (folly::kMscVer) {
+ EXPECT_EQ(folly::kMscVer < 1925, eager);
+ }
+ if (folly::kGnuc && !folly::kIsClang) {
+ EXPECT_FALSE(eager);
+ }
+ if (folly::kIsClang) {
+ constexpr auto ver = folly::kClangVerMajor;
+ if (ver <= 14 || ver >= 17) {
+ EXPECT_FALSE(eager);
+ } else {
+ SUCCEED(); // we sometimes use patched llvm-15/llvm-16
+ }
+ }
+}
+
+#endif
--
2.47.0.windows.1
@@ -85,7 +85,7 @@ struct StampedPtr {
// to us, which is difficult to prove). Signed right-shift of a
// negative number is implementation-defined in C++ (not undefined!),
// but actually does the right thing on all the platforms I can find.
- auto extended = static_cast<int64_t>(raw) >> kInternalStampBits;
+ auto extended = static_cast<uint64_t>(raw) >> kInternalStampBits;
return reinterpret_cast<T*>(static_cast<intptr_t>(extended));
}