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

update atomics.h to be more fully C11/C++11 compliant #42152

Merged
merged 2 commits into from
Sep 15, 2021
Merged

Conversation

vtjnash
Copy link
Member

@vtjnash vtjnash commented Sep 7, 2021

Fixes #42098

@yuyichao
Copy link
Contributor

yuyichao commented Sep 7, 2021

I couldn’t find anything in the standard saying that the pointer cast to atomic type is defined, or that the atomic typed are the same in c and c++ which was why I did not want to use them. Otoh, c++20 has https://en.cppreference.com/w/cpp/atomic/atomic_ref which should be well defined to do what we want here. I don’t know if there’s something like that added/proposed for c though…

@vtjnash
Copy link
Member Author

vtjnash commented Sep 8, 2021

This is finally specified in C++23:

_Atomic(T) is identical to std::atomic while both are well-formed

This came out of http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2020/p0943r6.html, and the existing bugs you mentioned are slowly being addressed in GCC: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65146. Though those corner cases are relevant for common struct objects, but not for the primitive types we usually annotate here.

OTOH, atomic_ref<T> is not compatible with C11, as it does have different alignment expectations. We currently have the static analyzer disabled for these checks, since we weren't correctly annotating our fields to ensure the expected alignment.

@vtjnash vtjnash force-pushed the jn/atomic11 branch 5 times, most recently from 3e7f980 to b900387 Compare September 10, 2021 04:32
@KristofferC
Copy link
Member

This needs a manual backport against #42255.

@KristofferC
Copy link
Member

Just point out that this has still not been backported since someone needs to do it manually (@barche, if you are interested in this, perhaps you can help out?)

@barche
Copy link
Contributor

barche commented Oct 23, 2021

OK, how does this work in practice? Make a separate PR to merge into the release-1.7 branch?

@vchuravy
Copy link
Member

againt backports-release-1.7 yeah

barche pushed a commit to barche/julia that referenced this pull request Oct 27, 2021
barche pushed a commit to barche/julia that referenced this pull request Oct 28, 2021
KristofferC pushed a commit that referenced this pull request Oct 31, 2021

Co-authored-by: Jameson Nash <vtjnash@gmail.com>
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Feb 22, 2022
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Mar 8, 2022
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.

Julia 1.7.0beta4 atomics.h errors embedding Julia in C++ Windows 10 MSVC 2019 v142
5 participants