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

Fix c.inc _Atomic define for C++ #1231

Merged
merged 1 commit into from
Jun 18, 2024
Merged

Fix c.inc _Atomic define for C++ #1231

merged 1 commit into from
Jun 18, 2024

Conversation

mrdomino
Copy link
Collaborator

No description provided.

c.inc (AFAICT erroneously) defined _Atomic(t) as `volatile t *`, when it
should have just said `volatile t`, when __STDC_VERSION__ was too small.
This happens when we’re compiling C++, but in C++11, _Atomic is a define
supplied by the STL rather than a keyword supplied by the compiler. Wait
though, it gets better: in C++11, _Atomic hooks you into the morass that
is stdatomic.h, and ultimately refers everything back to std::atomic<T>.

The gory, horrifying details are in libcxx's __atomic/cxx_atomic_impl.h.
The tldr is that for our purposes it’s fine to just say volatile and use
the normal libc/intrin/atomic.h functions.
@github-actions github-actions bot added the libc label Jun 18, 2024
@mrdomino
Copy link
Collaborator Author

mrdomino commented Jun 18, 2024

PR description (can be drag-select copied and pasted in github's UI)

c.inc (AFAICT erroneously) defined _Atomic(t) as `volatile t *`, when it should have just said `volatile t`, when __STDC_VERSION__ was too small. This happens when we’re compiling C++, but in C++11, _Atomic is a define supplied by the STL rather than a keyword supplied by the compiler. Wait though, it gets better: in C++11, _Atomic hooks you into the morass that is stdatomic.h, and ultimately refers everything back to std::atomic<T>.

The gory, horrifying details are in libcxx’s __atomic/cxx_atomic_impl.h. The tldr is that for our purposes it’s fine to just say volatile and use the normal libc/intrin/atomic.h functions.

@mrdomino
Copy link
Collaborator Author

#1229 compiles with this change, and not without it.

@jart jart merged commit 6a2904a into jart:master Jun 18, 2024
6 checks passed
@mrdomino mrdomino deleted the fix-c-atomic branch June 18, 2024 04:11
mrdomino added a commit that referenced this pull request Jun 18, 2024
c.inc (AFAICT erroneously) defined _Atomic(t) as `volatile t *`, when it
should have just said `volatile t`, when __STDC_VERSION__ was too small.
This happens when we’re compiling C++, but in C++11, _Atomic is a define
supplied by the STL rather than a keyword supplied by the compiler. Wait
though, it gets better: in C++11, _Atomic hooks you into the morass that
is stdatomic.h, and ultimately refers everything back to std::atomic<T>.

The gory, horrifying details are in libcxx's __atomic/cxx_atomic_impl.h.
The tldr is that for our purposes it’s fine to just say volatile and use
the normal libc/intrin/atomic.h functions.
@mrdomino
Copy link
Collaborator Author

From cppreference on std::atomic:

Implementations are recommended to ensure that the representation of _Atomic(T) in C is same as that of std::atomic<T> in C++ for every possible type T. The mechanisms used to ensure atomicity and memory ordering should be compatible.

This gives us hope that it may be legitimate to go down this road…

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants