GCC 10.1 support #158

dutow · 2020-07-19T20:34:16Z

This pull requests adds GCC support on top of @mmha's CMake pull request.

I cherry picked his commit to the current master branch for this, I will rebase my pull request once that PR is merged.

I opened this for now only so my changes can be reviewed.

As I explained in my last commit message GCC 10.1 seems to have remaining bugs causing test failures. I found workarounds for some of them, but I had to completely remove one testcase for GCC (not supported syntax), and leave a test failure as is (missing destructor call). Otherwise the library seems to be working.

Starting with GCC 10.1, GCC/libstdc++ supports coroutines, but includes them in their final location in <coroutine>. This commit generalizes the location of the header, and the name of the types (either in the std or std::experimental namespace), so that both the current Clang/MSVC and the GCC approach can be supported. This should also guarantee that both current and later Clang/MSVC releases will be supported by the library.

While clang uses the -fcoroutines-ts flag to add support for the Coroutines TS, GCC uses the -fcoroutines flag. This commit refactors the cmake finder to support both.

* Included missing headers * Disabled a testcase for GCC that uses a syntax not supported by GCC10.1 * Reordered a class so GCC is able to find a data member

After these changes, there's one failure remaining with GCC 10.1, where it doesn't call a destructor for an object, and calls another later than it should. Other tests required workarounds for bugs in the GCC coroutine implementation, but were possible to fix with minor code modifications.

nanoric · 2020-08-14T09:02:01Z

Great job!
When can this be merged?

lewissbaker

Thanks very much for the PR!
CMake and GCC support have been requested by several people.
My apologies for taking so long to review.

A few minor things to address:

Some accidental changes of std::experimental::filesystem to cppcoro::filesystem (which will break the Windows builds)
Missing reexport of std::noop_coroutine()
There is some debug spam still in test/counted.hpp
I'm not sure that some of the workarounds to get tests passing with GCC 10.1 are worthwhile.
GCC 10.1 coroutines support was quite broken.
I'd probably recommend requiring GCC 10.2 as a minimum compiler version for use with cppcoro.

Also, it would be good to hook up the CI configs to exercise the CMake builds to avoid regressions.
This can be deferred to a later PR though.

lewissbaker · 2020-08-22T17:30:10Z

cmake/FindCppcoroCoroutines.cmake

+
+find_package_handle_standard_args(CppcoroCoroutines
+    REQUIRED_VARS Coroutines_LIBRARY_SUPPORT Coroutines_COMPILER_SUPPORT
+    FAIL_MESSAGE "Verify that the compiler and the standard library both support the Coroutines TS")


nit: Maybe reword to also be inclusive of C++20 coroutines, not just Coroutines TS?
"... both support either C++20 coroutines or the Coroutines TS"

lewissbaker · 2020-08-22T17:55:41Z

include/cppcoro/coroutine.hpp

+
+namespace cppcoro {
+  template<typename Promise=void>
+  using coroutine_handle = std::coroutine_handle<Promise>;


There is also one place that makes use of std::experimental::noop_coroutine() (round_robin_scheduler.hpp) which also needs to be made available if it's provided by the platform headers.

It should be supported by all <coroutine> headers (since that's required by the C++20 spec).
However, compilers that just implemented the coroutines TS may not have it (e.g. early versions of clang and all MSVC versions).

We may need some kind of detection/macro to enable this (or maybe we can use feature-macros to detect it?)

lewissbaker · 2020-08-22T18:01:32Z

include/cppcoro/file_read_operation.hpp

-#include <experimental/coroutine>
+#include <cppcoro/coroutine.hpp>


It doesn't seem like this include is needed.
Remove it instead?

lewissbaker · 2020-08-22T18:08:40Z

include/cppcoro/file_write_operation.hpp

-#include <experimental/coroutine>
+#include <cppcoro/coroutine.hpp>


Similarly here, include seems to be unused. Maybe just remove this include?

lewissbaker · 2020-08-23T14:58:26Z

include/cppcoro/round_robin_scheduler.hpp

@@ -61,7 +61,7 @@ namespace cppcoro
 	public:
 		round_robin_scheduler() noexcept
 			: m_index(0)
-			, m_noop(std::experimental::noop_coroutine())
+			, m_noop(cppcoro::noop_coroutine())


The noop_coroutine() function has not been exported from cppcoro yet.

lewissbaker · 2020-08-23T15:58:25Z

test/recursive_generator_tests.cpp

+      // GCC 10.1 workaround: "co_yield start++ always returns the same value, resulting in an infinite loop
+      // ((++start)-1) seems to have the same issue, while ++start works, but breaks the test
+      start++;


GCC 10.1 had a large number of coroutine bugs. I wouldn't recommend it for any coroutine code in production.
This issue should hopefully have been fixed in 10.2, although I'm not sure whether this particular issue was reported.

Possibly related to https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95017 or https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95599 ?

lewissbaker · 2020-08-23T16:21:32Z

test/shared_task_tests.cpp

+    // GCC 10.1 workaround: GCC doesn't generate any code for a for loop with only a co_await in it
+      [](){}();


This should be fixed in GCC 10.2.
See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95017

lewissbaker · 2020-08-23T16:26:26Z

test/task_tests.cpp

+    // GCC 10.1 performs 2 copies
+		CHECK(counted::copy_construction_count >= 1);


This should be fixed in GCC 10.2
See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95350

lewissbaker · 2020-08-23T16:35:52Z

test/when_all_tests.cpp

@@ -112,6 +112,9 @@ TEST_CASE("when_all() with all task types")

 		CHECK(a == "foo");
 		CHECK(b.id == 0);
+    // GCC 10.1 fails this check: at this point there are 3 objects alive
+    // * One will be destructed later
+    // * One object is completely leaked


Hmm, I wonder why this one is failing.
There were a few destructor-related bugs I found in 10.1 most of which should be fixed in 10.2.
Have you tested this with 10.2 to see if it passes?

lewissbaker · 2020-08-23T16:38:01Z

test/file_tests.cpp

-namespace fs = std::experimental::filesystem;
+namespace fs = cppcoro::filesystem;


Accidental search+replace change?

I couldn't see any changes that would have reexported std::experimental::filesystem under cppcoro.
Also see other headers: file.hpp/cpp, read_write_file.hpp/cpp, read_only_file.hpp/cpp, write_only_file.hpp/cpp

OleksandrKvl · 2020-08-26T19:38:23Z

I've done similar CMake+GCC support for local experiments and implemented several things differently, not quite clean though.
Consider replacing for co_await with manual implementation because it hasn't been approved for C++20. Also, why cppcoroConfig.cmake and FindCppcoroCoroutines.cmake are needed there? IIRC, they should be generated outside of project directory where others can reach them. Here's my installation part:

include(GNUInstallDirs)

# install headers
install(DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/../include/cppcoro 
    TYPE INCLUDE
)

# install binary
install(TARGETS cppcoro_lib
    EXPORT coro
    RUNTIME DESTINATION ${CMAKE_INSTALL_BINDIR}
    LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR}
    ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR}
    INCLUDES DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}
)

# install config for find_package()
install(EXPORT coro
    DESTINATION ${CMAKE_INSTALL_LIBDIR}/cmake/cppcoro
    NAMESPACE cppcoro::
    FILE cppcoroConfig.cmake
)

# generate export file
export(EXPORT coro
    NAMESPACE cppcoro::
    FILE cppcoroExport.cmake
)

It's usually good to set default CMAKE_BUILD_TYPE to Release. People then can use canonical CMake installation mkdir build && cd build && cmake .. && cmake --build . && cmake --install .

pbrady · 2020-09-08T20:24:31Z

Just want to say that I'm excited about this.

dutow · 2020-09-09T06:15:11Z

@OleksandrKvl the cmake files are from #110, I didn't change those. And I'm not sure if defaulting to release is a good idea, as that would effect multi-buildtype IDE setups also (like visual studio or xcode). If a projects wants to set a default build type correctly, it usually requires several conditions (don't do anything if using a multi config generator ; use debug if the .git directory is present ; use relwithdebinfo otherwise)

@lewissbaker I'll updat my PR based on your comments, but do you have plans about #110? As currently this PR includes that commit too.

lewissbaker · 2020-10-09T20:04:19Z

I've added some comments to #110.
Perhaps @OleksandrKvl 's comments above could be added to that PR and CMake-related discussion moved there?

@dutow Would it be possible to separate this PR out into just the code-changes needed for GCC support and not have it build upon the CMake changes, so that they can be merged independently?

dutow · 2020-10-10T07:08:26Z

@lewissbaker Then we don't have a build system I'm actually familiar with. Cake seems to be a custom tool written by you, and the cake script currently forces clang on linux/osx. Seems like adding gcc there would require more changes than the cmake script (adding a config to choose the compiler, then refactoring the entire linux/darwin section to handle both clang and gcc)

Also, then we'll have to add d99be58 to the cmake pullrequest instead.

mmha and others added 5 commits July 18, 2020 20:18

Added CMake build system

16ae7f1

Generalize CMake flag handling to support GCC

d99be58

While clang uses the -fcoroutines-ts flag to add support for the Coroutines TS, GCC uses the -fcoroutines flag. This commit refactors the cmake finder to support both.

Fixing GCC compilation issues

3e08a7d

* Included missing headers * Disabled a testcase for GCC that uses a syntax not supported by GCC10.1 * Reordered a class so GCC is able to find a data member

dutow marked this pull request as draft July 19, 2020 20:34

lewissbaker requested changes Aug 23, 2020

View reviewed changes

GIGte mentioned this pull request Aug 27, 2020

File I/O for Linux #160

Open

This was referenced Oct 10, 2020

Unify experimental includes #171

Open

Small fixes to make gcc happy #173

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GCC 10.1 support #158

GCC 10.1 support #158

dutow commented Jul 19, 2020

nanoric commented Aug 14, 2020

lewissbaker left a comment

lewissbaker Aug 22, 2020

lewissbaker Aug 22, 2020

lewissbaker Aug 22, 2020

lewissbaker Aug 22, 2020

lewissbaker Aug 23, 2020

lewissbaker Aug 23, 2020

lewissbaker Aug 23, 2020

lewissbaker Aug 23, 2020

lewissbaker Aug 23, 2020

lewissbaker Aug 23, 2020

OleksandrKvl commented Aug 26, 2020 •

edited

pbrady commented Sep 8, 2020

dutow commented Sep 9, 2020

lewissbaker commented Oct 9, 2020

dutow commented Oct 10, 2020

		#include <experimental/coroutine>
		#include <cppcoro/coroutine.hpp>

		// GCC 10.1 workaround: GCC doesn't generate any code for a for loop with only a co_await in it
		[](){}();

		// GCC 10.1 performs 2 copies
		CHECK(counted::copy_construction_count >= 1);

		namespace fs = std::experimental::filesystem;
		namespace fs = cppcoro::filesystem;

GCC 10.1 support #158

Are you sure you want to change the base?

GCC 10.1 support #158

Conversation

dutow commented Jul 19, 2020

nanoric commented Aug 14, 2020

lewissbaker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OleksandrKvl commented Aug 26, 2020 • edited

pbrady commented Sep 8, 2020

dutow commented Sep 9, 2020

lewissbaker commented Oct 9, 2020

dutow commented Oct 10, 2020

OleksandrKvl commented Aug 26, 2020 •

edited