From b1cb4319ec77bc9a0e8eb5495852eba77c9b01f8 Mon Sep 17 00:00:00 2001 From: Casey Carter Date: Mon, 5 Oct 2020 09:46:51 -0700 Subject: [PATCH 1/4] generator refactoring Restore kernel mode operation by defining `unhandled_exception` only when `_KERNEL_MODE` is not defined. (When `unhandled_exception` is defined, the compiler will wrap the coroutine body in a `try...catch` as depicted in [dcl.fct.def.coroutine]/5, which it then diagnoses as ill-formed when unwinding is disabled as in `/kernel`.) Fixes #1323. Some cleanup while we're here: * Use STL style: * Prefer not to deduce simple return types * `const` west * introduce template type parameters with `class` * Annotate functions that don't throw exceptions `noexcept` * remove excess empy lines * Use `_Capital_snake` names for non-template-parameters * `generator::promise_type` only has members `_Exception` and `_Rethrow_if_exception` to "delay" exception delivery so as to avoid triggering VSO-1172852. They aren't needed in kernel mode. * `generator::iterator` should have _explicit_ conversion from `coroutine_handle` rather than implicit for good design hygiene. The conversion from `nullptr_t` is unnecessary. Note that the story for `/EHs-` without `/kernel` is less clear and currently not supported. --- stl/inc/experimental/generator | 118 +++++++++++++++------------------ 1 file changed, 54 insertions(+), 64 deletions(-) diff --git a/stl/inc/experimental/generator b/stl/inc/experimental/generator index e19a80142d3..6ba43f61548 100644 --- a/stl/inc/experimental/generator +++ b/stl/inc/experimental/generator @@ -22,7 +22,6 @@ #error requires /std:c++latest or /await compiler options #endif // ^^^ no coroutine support ^^^ - #pragma pack(push, _CRT_PACKING) #pragma warning(push, _STL_WARNING_LEVEL) #pragma warning(disable : _STL_DISABLED_WARNINGS) @@ -33,18 +32,18 @@ _STL_DISABLE_CLANG_WARNINGS _STD_BEGIN namespace experimental { - - template > + // NOTE WELL: _CPPUNWIND currently affects the ABI of generator. Linking together /EHs + // translation units and /EHs- translation units that use the same specialization of generator + // violates the One Definition Rule. This could result in fantastic failures or obscure bugs. + template > struct generator { struct promise_type { - _Ty const* _CurrentValue; -#if 1 // TRANSITION, VSO-1172852 -#ifdef _CPPUNWIND - exception_ptr _Eptr; -#endif // _CPPUNWIND + const _Ty* _Value; +#ifdef _CPPUNWIND // TRANSITION, VSO-1172852 + exception_ptr _Exception; #endif // TRANSITION, VSO-1172852 - auto get_return_object() { + generator get_return_object() noexcept { return generator{*this}; } @@ -56,38 +55,38 @@ namespace experimental { return {}; } -#if defined(_CPPUNWIND) || defined(__cpp_impl_coroutine) - void unhandled_exception() noexcept { -#if 1 // TRANSITION, VSO-1172852 +#ifndef _KERNEL_MODE #ifdef _CPPUNWIND - _Eptr = _STD current_exception(); -#else // ^^^ _CPPUNWIND / !_CPPUNWIND vvv - abort(); -#endif // _CPPUNWIND +#if 1 // TRANSITION, VSO-1172852 + void unhandled_exception() noexcept { + _Exception = _STD current_exception(); + } #else // ^^^ workaround / no workaround vvv + void unhandled_exception() { throw; -#endif // TRANSITION, VSO-1172852 } -#endif // defined(_CPPUNWIND) || defined(__cpp_impl_coroutine) +#endif // TRANSITION, VSO-1172852 +#else // ^^^ defined(_CPPUNWIND / !defined(_CPPUNWIND) + void unhandled_exception() noexcept {} +#endif // _CPPUNWIND +#endif // _KERNEL_MODE -#if 1 // TRANSITION, VSO-1172852 +#ifdef _CPPUNWIND // TRANSITION, VSO-1172852 void _Rethrow_if_exception() { -#ifdef _CPPUNWIND - if (_Eptr) { - _STD rethrow_exception(_Eptr); + if (_Exception) { + _STD rethrow_exception(_Exception); } -#endif // _CPPUNWIND } #endif // TRANSITION, VSO-1172852 - auto yield_value(_Ty const& _Value) { - _CurrentValue = _STD addressof(_Value); - return suspend_always{}; + suspend_always yield_value(const _Ty& _Val) noexcept { + _Value = _STD addressof(_Val); + return {}; } - void return_void() {} + void return_void() noexcept {} - template + template _Uty&& await_transform(_Uty&& _Whatever) { static_assert(_Always_false<_Uty>, "co_await is not supported in coroutines of type std::experimental::generator"); @@ -98,15 +97,16 @@ namespace experimental { static_assert(is_same_v::pointer>, "generator does not support allocators with fancy pointer types"); static_assert( - allocator_traits<_Alloc_char>::is_always_equal::value, "generator only supports stateless allocators"); + allocator_traits<_Alloc_char>::is_always_equal::value && is_default_constructible_v<_Alloc_char>, + "generator supports only stateless allocators"); static void* operator new(size_t _Size) { - _Alloc_char _Al; + _Alloc_char _Al{}; return allocator_traits<_Alloc_char>::allocate(_Al, _Size); } static void operator delete(void* _Ptr, size_t _Size) noexcept { - _Alloc_char _Al; + _Alloc_char _Al{}; return allocator_traits<_Alloc_char>::deallocate(_Al, static_cast(_Ptr), _Size); } }; @@ -115,21 +115,19 @@ namespace experimental { using iterator_category = input_iterator_tag; using difference_type = ptrdiff_t; using value_type = _Ty; - using reference = _Ty const&; - using pointer = _Ty const*; + using reference = const _Ty&; + using pointer = const _Ty*; coroutine_handle _Coro = nullptr; iterator() = default; - iterator(nullptr_t) : _Coro(nullptr) {} - - iterator(coroutine_handle _CoroArg) : _Coro(_CoroArg) {} + explicit iterator(coroutine_handle _CoroArg) noexcept : _Coro(_CoroArg) {} iterator& operator++() { _Coro.resume(); if (_Coro.done()) { -#if 1 // TRANSITION, VSO-1172852 - _STD exchange(_Coro, {}).promise()._Rethrow_if_exception(); +#ifdef _CPPUNWIND // TRANSITION, VSO-1172852 + _STD exchange(_Coro, nullptr).promise()._Rethrow_if_exception(); #else // ^^^ workaround / no workaround vvv _Coro = nullptr; #endif // TRANSITION, VSO-1172852 @@ -139,25 +137,25 @@ namespace experimental { } void operator++(int) { - // This postincrement operator meets the requirements of the Ranges TS - // InputIterator concept, but not those of Standard C++ InputIterator. + // This operator meets the requirements of the C++20 input_iterator concept, + // but not the Cpp17InputIterator requirements. ++*this; } - _NODISCARD bool operator==(iterator const& _Right) const { + _NODISCARD bool operator==(const iterator& _Right) const noexcept { return _Coro == _Right._Coro; } - _NODISCARD bool operator!=(iterator const& _Right) const { + _NODISCARD bool operator!=(const iterator& _Right) const noexcept { return !(*this == _Right); } - _NODISCARD reference operator*() const { - return *_Coro.promise()._CurrentValue; + _NODISCARD reference operator*() const noexcept { + return *_Coro.promise()._Value; } - _NODISCARD pointer operator->() const { - return _Coro.promise()._CurrentValue; + _NODISCARD pointer operator->() const noexcept { + return _Coro.promise()._Value; } }; @@ -165,35 +163,28 @@ namespace experimental { if (_Coro) { _Coro.resume(); if (_Coro.done()) { -#if 1 // TRANSITION, VSO-1172852 +#ifdef _CPPUNWIND // TRANSITION, VSO-1172852 _Coro.promise()._Rethrow_if_exception(); #endif // TRANSITION, VSO-1172852 - return {nullptr}; + return {}; } } - return {_Coro}; + return iterator{_Coro}; } - _NODISCARD iterator end() { - return {nullptr}; + _NODISCARD iterator end() noexcept { + return {}; } - explicit generator(promise_type& _Prom) : _Coro(coroutine_handle::from_promise(_Prom)) {} + explicit generator(promise_type& _Prom) noexcept : _Coro(coroutine_handle::from_promise(_Prom)) {} - generator() = default; - generator(generator const&) = delete; - generator& operator=(generator const&) = delete; + generator() = default; - generator(generator&& _Right) : _Coro(_Right._Coro) { - _Right._Coro = nullptr; - } + generator(generator&& _Right) noexcept : _Coro(_STD exchange(_Right._Coro, nullptr)) {} - generator& operator=(generator&& _Right) { - if (this != _STD addressof(_Right)) { - _Coro = _Right._Coro; - _Right._Coro = nullptr; - } + generator& operator=(generator&& _Right) noexcept { + _Coro = _STD exchange(_Right._Coro, nullptr); return *this; } @@ -206,7 +197,6 @@ namespace experimental { private: coroutine_handle _Coro = nullptr; }; - } // namespace experimental _STD_END From 38a634671adc529cdd197ed0fb2eda08a2139749 Mon Sep 17 00:00:00 2001 From: Casey Carter Date: Wed, 14 Oct 2020 15:26:30 -0700 Subject: [PATCH 2/4] Curtis's review comments Co-authored-by: Curtis J Bezault --- stl/inc/experimental/generator | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/stl/inc/experimental/generator b/stl/inc/experimental/generator index 6ba43f61548..cc6918cf575 100644 --- a/stl/inc/experimental/generator +++ b/stl/inc/experimental/generator @@ -66,7 +66,7 @@ namespace experimental { throw; } #endif // TRANSITION, VSO-1172852 -#else // ^^^ defined(_CPPUNWIND / !defined(_CPPUNWIND) +#else // ^^^ defined(_CPPUNWIND) / !defined(_CPPUNWIND) vvv void unhandled_exception() noexcept {} #endif // _CPPUNWIND #endif // _KERNEL_MODE From 209720a4eab8b813e0c7677d9c8cdad1b9674a2b Mon Sep 17 00:00:00 2001 From: Casey Carter Date: Wed, 14 Oct 2020 15:26:56 -0700 Subject: [PATCH 3/4] Casey's review comments --- stl/inc/experimental/generator | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/stl/inc/experimental/generator b/stl/inc/experimental/generator index cc6918cf575..4f20e477c90 100644 --- a/stl/inc/experimental/generator +++ b/stl/inc/experimental/generator @@ -32,9 +32,7 @@ _STL_DISABLE_CLANG_WARNINGS _STD_BEGIN namespace experimental { - // NOTE WELL: _CPPUNWIND currently affects the ABI of generator. Linking together /EHs - // translation units and /EHs- translation units that use the same specialization of generator - // violates the One Definition Rule. This could result in fantastic failures or obscure bugs. + // NOTE WELL: _CPPUNWIND currently affects the ABI of generator. template > struct generator { struct promise_type { From 41fc5c14412e4ca7bd4d43fd9bd0458434dc6b22 Mon Sep 17 00:00:00 2001 From: Casey Carter Date: Thu, 15 Oct 2020 13:09:53 -0700 Subject: [PATCH 4/4] STL's review comments --- stl/inc/experimental/generator | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/stl/inc/experimental/generator b/stl/inc/experimental/generator index 4f20e477c90..6a13039c5dd 100644 --- a/stl/inc/experimental/generator +++ b/stl/inc/experimental/generator @@ -119,7 +119,7 @@ namespace experimental { coroutine_handle _Coro = nullptr; iterator() = default; - explicit iterator(coroutine_handle _CoroArg) noexcept : _Coro(_CoroArg) {} + explicit iterator(coroutine_handle _Coro_) noexcept : _Coro(_Coro_) {} iterator& operator++() { _Coro.resume();