Skip to content
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

Add cmake support #15

Merged

Conversation

Garcia6l20
Copy link

adds missing parts for MSVC support

  • /await:heapelide for x64 target
  • static tests-main library
  • missing windows library dependencies

@Garcia6l20
Copy link
Author

also includes remove CTest commit, so,

cmake -DBUILD_TESTING=ON <src_dir>

is now required to compile tests.

Tell me if you don't want it, I'll drop it.

@Garcia6l20 Garcia6l20 force-pushed the add_cmake_support branch 2 times, most recently from 780058b to 53a1c12 Compare October 22, 2020 16:48
@Garcia6l20
Copy link
Author

  • compiles successfully on MSVC 2019
  • one test fails:
37 - async auto reset event tests-multi-threaded (SEGFAULT)

This is because were running into sack overflow (on x64 only with this particular number of event), see my comment in this commit.
We can workaround it but the whole implementation of cppcoro::static_thread_pool should be reviewed to avoid stack overflows.

@andreasbuhr
Copy link
Owner

Hi, thank you so much for your work.
Could you please remove the iostream includes for doctest? I'd like to take care of that problem in the "update_doctest" branch, see lewissbaker#178 . For your local testing, just merge the "update_doctest" branch.

@Garcia6l20
Copy link
Author

Ok

@@ -115,6 +115,9 @@ check_cxx_compiler_flag(-fcoroutines _CXX_COROUTINES_SUPPORTS_CORO_FLAG)

if(_CXX_COROUTINES_SUPPORTS_MS_FLAG)
set(_CXX_COROUTINES_EXTRA_FLAGS "/await")
if(CMAKE_CL_64)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there some documentation which versions of MSVC support the "/await:heapelide" flag? Are there old versions of MSVC which we could break by unconditionally setting the heapelide flag?
And do the new versions (16.8), which do not need the /await flag, need the heapelide flag?
I'd appreciate if we made sure we do not break anything by setting this flag.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cannot find any documentation about this hidden version of the /await flag, all I know is that MSVC 2019 does not complain about it.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we maybe, instead of checking for CMAKE_CL_64 (which is discouraged, according to CMake documentation), check for the existence of this flag using check_cxx_compiler_flag? That way we'd make sure nothing bad happens if some version of MSVC does not know it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't know CMAKE_CL_64 was discouraged...
Yes it look like the better way to go

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hum, however the /await:heapelide is also ok for 32bit target.
Do you know why have been added for 64bit targets only ?

Copy link
Author

@Garcia6l20 Garcia6l20 Oct 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when using the heapelide version on 32bit, auto reset event test also runs into SEGFAULT.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, sorry it's ok, I was targetting wrong processor

@Garcia6l20
Copy link
Author

I just tested an fixed install target:

  • change FindCppCoroCoroutine.cmake -> FindCoroutine.cmake
  • windows doesn't know what is the '.' path

Sylvain Garcia and others added 3 commits October 23, 2020 08:28
- tests-main library changed to STATIC
 - MSVC dlls would require to do extra import/export effort
@andreasbuhr
Copy link
Owner

Thanks a lot for your effort, we tested with MSVC 2019 Preview 16.8.5, works great!

@andreasbuhr andreasbuhr merged commit 07bb85e into andreasbuhr:add_cmake_support Oct 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants