From c03cca7b2f25bcf5c39f405cc4f3400d85ac06cd Mon Sep 17 00:00:00 2001 From: Dan Klishch Date: Sat, 29 Jun 2024 20:27:44 -0400 Subject: [PATCH] AK+LibTest: Choose definition of CO_TRY and CO_TRY_OR_FAIL more robustly There are three compiler bugs that influence this decision: - Clang writing to (validly) destroyed coroutine frame with -O0 and -fsanitize=null,address under some conditions (https://godbolt.org/z/17Efq5Ma5) (AK_COROUTINE_DESTRUCTION_BROKEN); - GCC being unable to handle statement expressions in coroutines (AK_COROUTINE_STATEMENT_EXPRS_BROKEN); - GCC being unable to deduce template type parameter for TryAwaiter with nested CO_TRYs (AK_COROUTINE_TYPE_DEDUCTION_BROKEN). Instead of growing an ifdef soup in AK/Coroutine.h and LibTest/AsyncTestCase.h, define three macros in AK/Platform.h that correspond to these bugs and use them accordingly in the said files. --- AK/Coroutine.h | 37 +++++++++++++----- AK/Platform.h | 10 +++++ Tests/AK/TestCoroutine.cpp | 6 ++- Userland/Libraries/LibHTTP/Http11Connection.h | 2 +- Userland/Libraries/LibTest/AsyncTestCase.h | 38 +++++++++---------- 5 files changed, 62 insertions(+), 31 deletions(-) diff --git a/AK/Coroutine.h b/AK/Coroutine.h index f1cf6019187..0d916793f5e 100644 --- a/AK/Coroutine.h +++ b/AK/Coroutine.h @@ -14,8 +14,12 @@ namespace AK { namespace Detail { -// FIXME: GCC ICEs when a simpler implementation of CO_TRY_OR_FAIL is used. See also LibTest/AsyncTestCase.h. -#ifdef AK_COMPILER_GCC +// FIXME: GCC ICEs when an implementation of CO_TRY_OR_FAIL with statement expressions is used. See also LibTest/AsyncTestCase.h. +#if defined(AK_COROUTINE_STATEMENT_EXPRS_BROKEN) && !defined(AK_COROUTINE_DESTRUCTION_BROKEN) +# define AK_USE_TRY_OR_FAIL_AWAITER +#endif + +#ifdef AK_USE_TRY_OR_FAIL_AWAITER namespace Test { template @@ -123,7 +127,7 @@ private: template friend struct Detail::TryAwaiter; -#ifdef AK_COMPILER_GCC +#ifdef AK_USE_TRY_OR_FAIL_AWAITER template friend struct AK::Detail::Test::TryOrFailAwaiter; #endif @@ -189,6 +193,7 @@ T must_sync(Coroutine>&& coroutine) return object.release_value(); } +#ifndef AK_COROUTINE_DESTRUCTION_BROKEN namespace Detail { template struct TryAwaiter { @@ -243,13 +248,9 @@ struct TryAwaiter { }; } -#ifdef AK_COMPILER_CLANG -# define CO_TRY(expression) (co_await ::AK::Detail::TryAwaiter { (expression) }) -#else -// GCC cannot handle CO_TRY(...CO_TRY(...)...), this hack ensures that it always has the right type information available. -// FIXME: Remove this once GCC can correctly infer the result type of `co_await TryAwaiter { ... }`. -# define CO_TRY(expression) static_cast(co_await ::AK::Detail::TryAwaiter { (expression) }) - +# ifndef AK_COROUTINE_TYPE_DEDUCTION_BROKEN +# define CO_TRY(expression) (co_await ::AK::Detail::TryAwaiter { (expression) }) +# else namespace Detail { template auto declval_coro_result(Coroutine&&) -> T; @@ -257,7 +258,23 @@ template auto declval_coro_result(T&&) -> T; } +// GCC cannot handle CO_TRY(...CO_TRY(...)...), this hack ensures that it always has the right type information available. +// FIXME: Remove this once GCC can correctly infer the result type of `co_await TryAwaiter { ... }`. +# define CO_TRY(expression) static_cast(co_await ::AK::Detail::TryAwaiter { (expression) }) +# endif +#elifndef AK_COROUTINE_STATEMENT_EXPRS_BROKEN +# define CO_TRY(expression) \ + ({ \ + AK_IGNORE_DIAGNOSTIC("-Wshadow", \ + auto _temporary_result = (expression)); \ + if (_temporary_result.is_error()) [[unlikely]] \ + co_return _temporary_result.release_error(); \ + _temporary_result.release_value(); \ + }) +#else +# error Unable to work around compiler bugs in definiton of CO_TRY. #endif + } #ifdef USING_AK_GLOBALLY diff --git a/AK/Platform.h b/AK/Platform.h index ee6f4b2fd2e..2b863501c2a 100644 --- a/AK/Platform.h +++ b/AK/Platform.h @@ -158,6 +158,16 @@ # define AK_BUILTIN_SUBC_BROKEN #endif +#if defined(AK_COMPILER_CLANG) && __clang_major__ < 19 +# define AK_COROUTINE_DESTRUCTION_BROKEN +#endif + +#ifdef AK_COMPILER_GCC +// FIXME: Reduce and report these to GCC. +# define AK_COROUTINE_STATEMENT_EXPRS_BROKEN +# define AK_COROUTINE_TYPE_DEDUCTION_BROKEN +#endif + #ifdef ALWAYS_INLINE # undef ALWAYS_INLINE #endif diff --git a/Tests/AK/TestCoroutine.cpp b/Tests/AK/TestCoroutine.cpp index 44a043d726a..7c037513396 100644 --- a/Tests/AK/TestCoroutine.cpp +++ b/Tests/AK/TestCoroutine.cpp @@ -238,18 +238,22 @@ namespace { Coroutine> co_try_success() { auto c = CO_TRY(co_await return_class_3()); +#ifndef AK_COROUTINE_DESTRUCTION_BROKEN // 1. Construct temporary as an argument for the constructor of a temporary ErrorOr. // 2. Move temporary ErrorOr into Coroutine. // -. Some magic is done in TryAwaiter. // 3. Move Class from ErrorOr inside Coroutine to c. EXPECT_EQ(c.cookie(), 3); +#else + EXPECT_EQ(c.cookie(), 4); +#endif co_return {}; } Coroutine> co_try_fail() { ErrorOr error = Error::from_string_literal("ERROR!"); - CO_TRY(error); + CO_TRY(move(error)); co_return {}; } diff --git a/Userland/Libraries/LibHTTP/Http11Connection.h b/Userland/Libraries/LibHTTP/Http11Connection.h index f8e5dad9c74..d77d3257689 100644 --- a/Userland/Libraries/LibHTTP/Http11Connection.h +++ b/Userland/Libraries/LibHTTP/Http11Connection.h @@ -90,7 +90,7 @@ public: if (response->is_open()) { auto close_result = co_await response->close(); if (!result.is_error()) // Preserve callback error in case it has failed. - CO_TRY(close_result); + CO_TRY(move(close_result)); } co_return result; } diff --git a/Userland/Libraries/LibTest/AsyncTestCase.h b/Userland/Libraries/LibTest/AsyncTestCase.h index 3fcb1bf61a1..49c03782541 100644 --- a/Userland/Libraries/LibTest/AsyncTestCase.h +++ b/Userland/Libraries/LibTest/AsyncTestCase.h @@ -10,7 +10,21 @@ #include #include -#ifdef AK_COMPILER_GCC +#ifndef AK_COROUTINE_STATEMENT_EXPRS_BROKEN +# define CO_TRY_OR_FAIL(expression) \ + ({ \ + /* Ignore -Wshadow to allow nesting the macro. */ \ + AK_IGNORE_DIAGNOSTIC("-Wshadow", \ + auto _temporary_result = (expression)); \ + static_assert(!::AK::Detail::IsLvalueReference, \ + "Do not return a reference from a fallible expression"); \ + if (_temporary_result.is_error()) [[unlikely]] { \ + FAIL(_temporary_result.release_error()); \ + co_return; \ + } \ + _temporary_result.release_value(); \ + }) +#elifndef AK_COROUTINE_DESTRUCTION_BROKEN namespace AK::Detail::Test { // FIXME: Straight-forward way to implement CO_TRY_OR_FAIL we use for Clang causes GCC to ICE. @@ -71,6 +85,10 @@ struct TryOrFailAwaiter { }; } + +# define CO_TRY_OR_FAIL(expression) (co_await ::AK::Detail::Test::TryOrFailAwaiter { (expression) }) +#else +# error Unable to work around compiler bugs in definiton of CO_TRY_OR_FAIL. #endif namespace Test { @@ -95,22 +113,4 @@ namespace Test { static struct __TESTCASE_TYPE(x) __TESTCASE_TYPE(x); \ static Coroutine __ASYNC_TESTCASE_FUNC(x)() -#ifdef AK_COMPILER_GCC -# define CO_TRY_OR_FAIL(expression) (co_await ::AK::Detail::Test::TryOrFailAwaiter { (expression) }) -#else -# define CO_TRY_OR_FAIL(expression) \ - ({ \ - /* Ignore -Wshadow to allow nesting the macro. */ \ - AK_IGNORE_DIAGNOSTIC("-Wshadow", \ - auto _temporary_result = (expression)); \ - static_assert(!::AK::Detail::IsLvalueReference, \ - "Do not return a reference from a fallible expression"); \ - if (_temporary_result.is_error()) [[unlikely]] { \ - FAIL(_temporary_result.release_error()); \ - co_return; \ - } \ - _temporary_result.release_value(); \ - }) -#endif - }