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.

Sign up for GitHub

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

Jump to bottom

fix detection of cxx11_std_atomic #3096

Merged
merged 3 commits into from Jan 18, 2018
Merged

fix detection of cxx11_std_atomic #3096

merged 3 commits into from Jan 18, 2018

Conversation

kleinhenz
Copy link
Contributor

Fixes #3031

  • add some instructions to cxx11_std_atomic.cpp to require __atomic_load_16 symbol
  • fix cmake code for testing for std::atomic support

The previous cmake code is broken in several ways. First, the REQUIRED option is forwarded to the first add_hpx_config_test invocation so it generates at fatal error if the test doesn't compile without the library. Second the CheckLibraryExists module is not included so the check_library_exists call fails. Third the second add_hpx_config_test invocation does nothing because HPX_WITH_CXX11_ATOMIC is already set. This fixes those issues and makes the test source a little more comprehensive.

@hkaiser hkaiser added this to the 1.1.0 milestone Jan 10, 2018
@hkaiser
Copy link
Member

hkaiser commented Jan 10, 2018

Thanks for providing this patch! Unfortunately, it breaks the Windows build (see for instance here: https://ci.appveyor.com/project/hkaiser/hpx/build/master-1623/job/f3bc6ergxleaw4y9).

@kleinhenz
Copy link
Contributor Author

I guess MSVC needs to be treated separately. It seems that in llvm it's assumed that atomics work without a library in MSVC so I'm not sure why the first test failed. I will look into it.

@hkaiser
Copy link
Member

hkaiser commented Jan 11, 2018

Yes, MSVC does not need a library for atomics.

@kleinhenz
Copy link
Contributor Author

ok I think I fixed the windows build.

hkaiser
hkaiser approved these changes Jan 17, 2018
View reviewed changes
Copy link
Member

@hkaiser hkaiser left a comment

Choose a reason for hiding this comment

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

LGTM, thanks a lot!

@msimberg msimberg merged commit 90aa7df into STEllAR-GROUP:master Jan 18, 2018
@msimberg
Copy link
Contributor

@kleinhenz Unfortunately this test now fails using clang (or probably libc++), see e.g. here: http://rostam.cct.lsu.edu/builders/hpx_clang_5_boost_1_65_centos_x86_64_debug/builds/161/steps/configure/logs/stdio, or here for an overview: http://rostam.cct.lsu.edu/grid.

Or is it in fact correctly complaining now...?

Hopefully we can catch these kind of things next time with pycicle.

@msimberg msimberg mentioned this pull request Jan 18, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hkaiser hkaiser hkaiser approved these changes

Assignees
No one assigned
Labels
category: CMake type: enhancement
Projects
None yet
Milestone
1.1.0
Development

Successfully merging this pull request may close these issues.

Errors linking libhpx.so due to missing references (master branch, commit 6679a8882)
3 participants
@kleinhenz @hkaiser @msimberg