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

[libc++] Enable C++ stdatomic.h for all C++ versions #95498

Merged
merged 2 commits into from
Sep 19, 2024
Merged

[libc++] Enable C++ stdatomic.h for all C++ versions #95498

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ac50370
[libc++] Enable C++ stdatomic.h for all C++ versions
rprichard Jun 11, 2024
7b24c13
Add a space between >> so generic-cxx03 test passes
rprichard Jun 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions libcxx/include/atomic
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -589,10 +589,6 @@ template <class T>

#include <__config>

#if _LIBCPP_STD_VER < 23 && defined(_LIBCPP_STDATOMIC_H)
# error <atomic> is incompatible with <stdatomic.h> before C++23. Please compile with -std=c++23.
Copy link
Member

Choose a reason for hiding this comment

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

Why do we claim that <atomic> is incompatible with <stdatomic.h> before C++23? Can you refresh my memory? In other words, what changed in C++23 that made the header compatible?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

C++ versions before C++23 don't specify a <stdatomic.h>, so including <stdatomic.h> in a C++ file typically finds <stdatomic.h> from the C library (or maybe from Clang's resource directory). That header typically implements <stdatomic.h> from C11, which typically implements type-generic operations using macros. For example, atomic_is_lock_free is a macro that works with any _Atomic-qualified object:

From clang/lib/Headers/stdatomic.h:

#define atomic_is_lock_free(obj) __c11_atomic_is_lock_free(sizeof(*(obj)))

If <stdatomic.h> is included first, then this macro will break the parsing of std::atomic_is_lock_free when <atomic> is included later (e.g. libcxx/include/__atomic/atomic.h):

template <class _Tp>
_LIBCPP_HIDE_FROM_ABI bool atomic_is_lock_free(const volatile atomic<_Tp>* __o) _NOEXCEPT {
  return __o->is_lock_free();
}

It can be possible to use the C11 <stdatomic.h> in a C++ file, as long as <atomic> is included first (or not at all), and only if the C++ compiler and <stdatomic.h> are compatible. For example:

  • GCC can't include its C11 <stdatomic.h> in C++ mode because GCC only provides the _Atomic qualifier in C mode, not C++.
  • Clang can include its C11 <stdatomic.h> in C++ mode.

Other STL headers that use the C11 <stdatomic.h> identifiers can also be an issue. For example, including <memory> after <stdatomic.h> also hits the incompatibility:

$ cat test.cpp
#include <stdatomic.h>
#include <memory>

$ clang++-19 -D_LIBCPP_REMOVE_TRANSITIVE_INCLUDES test.cpp -c -stdlib=libc++ -std=c++23
# works fine -- libc++ provides the C++23 stdatomic.h

$ clang++-19 -D_LIBCPP_REMOVE_TRANSITIVE_INCLUDES test.cpp -c -stdlib=libc++ -std=c++20
In file included from test.cpp:2:
In file included from /usr/lib/llvm-19/bin/../include/c++/v1/memory:937:
/usr/lib/llvm-19/bin/../include/c++/v1/__memory/shared_ptr.h:1587:35: error: expected expression
 1587 | inline _LIBCPP_HIDE_FROM_ABI bool atomic_is_lock_free(const shared_ptr<_Tp>*) {
      |                                   ^
/usr/lib/llvm-19/lib/clang/19/include/stdatomic.h:87:73: note: expanded from macro 'atomic_is_lock_free'
   87 | #define atomic_is_lock_free(obj) __c11_atomic_is_lock_free(sizeof(*(obj)))
      |                                                                         ^

Including <memory> defines std::atomic_* function overloads for std::shared_ptr (until C++26, when they are removed). https://en.cppreference.com/w/cpp/memory/shared_ptr/atomic

#endif

#include <__atomic/aliases.h>
#include <__atomic/atomic.h>
#include <__atomic/atomic_base.h>
Expand Down
14 changes: 7 additions & 7 deletions libcxx/include/stdatomic.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ using std::atomic_signal_fence // see below
# pragma GCC system_header
#endif

#if defined(__cplusplus) && _LIBCPP_STD_VER >= 23
#if defined(__cplusplus)
Copy link
Member

Choose a reason for hiding this comment

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

Since this header from libc++ is supposed to only be included in C++ mode, we should always take this branch and the #else should be unnecessary. Do you agree? The #else was previously useful because it handled the pre-C++23 behavior, but it wouldn't be useful anymore with this patch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Many of the libc++ C headers also have defined(__cplusplus). (For example, stdlib.h.) It has mattered in the past for Android's Soong build system because IIRC it adds libc++ headers to the include path even when compiling a C source file.

Copy link
Member

Choose a reason for hiding this comment

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

Nevermind, I was under the wrong impression that stdatomic.h was only provided in C++. It's also a C header, so of course it would need this #if defined(__cplusplus).


# include <atomic>
# include <version>
Expand Down Expand Up @@ -154,10 +154,14 @@ using std::atomic_long _LIBCPP_USING_IF_EXISTS;
using std::atomic_ulong _LIBCPP_USING_IF_EXISTS;
using std::atomic_llong _LIBCPP_USING_IF_EXISTS;
using std::atomic_ullong _LIBCPP_USING_IF_EXISTS;
# ifndef _LIBCPP_HAS_NO_CHAR8_T
Copy link
Member

Choose a reason for hiding this comment

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

The using-if-exists attribute should handle this, is there a reason why you're adding this #if?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like GCC doesn't have the __using_if_exists__ attribute, so the generic-gcc-cxx11 test failed until I added the #if.

Copy link
Member

Choose a reason for hiding this comment

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

Right. char8_t is not supported before C++20 so this starts failing when we don't have the attribute. Ok, I think that's acceptable.

using std::atomic_char8_t _LIBCPP_USING_IF_EXISTS;
# endif
using std::atomic_char16_t _LIBCPP_USING_IF_EXISTS;
using std::atomic_char32_t _LIBCPP_USING_IF_EXISTS;
# ifndef _LIBCPP_HAS_NO_WIDE_CHARACTERS
using std::atomic_wchar_t _LIBCPP_USING_IF_EXISTS;
# endif

using std::atomic_int8_t _LIBCPP_USING_IF_EXISTS;
using std::atomic_uint8_t _LIBCPP_USING_IF_EXISTS;
Expand Down Expand Up @@ -220,16 +224,12 @@ using std::atomic_store_explicit _LIBCPP_USING_IF_EXISTS;
using std::atomic_signal_fence _LIBCPP_USING_IF_EXISTS;
using std::atomic_thread_fence _LIBCPP_USING_IF_EXISTS;

#elif defined(_LIBCPP_COMPILER_CLANG_BASED)
#else

// Before C++23, we include the next <stdatomic.h> on the path to avoid hijacking
// the header. We do this because Clang has historically shipped a <stdatomic.h>
// header that would be available in all Standard modes, and we don't want to
// break that use case.
# if __has_include_next(<stdatomic.h>)
# include_next <stdatomic.h>
# endif

#endif // defined(__cplusplus) && _LIBCPP_STD_VER >= 23
#endif // defined(__cplusplus)

#endif // _LIBCPP_STDATOMIC_H
22 changes: 22 additions & 0 deletions libcxx/test/libcxx/atomics/atomics.syn/compatible_with_stdatomic.compile.pass.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
//===----------------------------------------------------------------------===//
//
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
//
//===----------------------------------------------------------------------===//

// UNSUPPORTED: no-threads

// This test verifies that <stdatomic.h> redirects to <atomic>. As an extension,
// libc++ enables this redirection even before C++23.

// Ordinarily, <stdatomic.h> can be included after <atomic>, but including it
// first doesn't work because its macros break <atomic>. Verify that
// <stdatomic.h> can be included first.
#include <stdatomic.h>
#include <atomic>

#include <type_traits>

static_assert(std::is_same<atomic_int, std::atomic<int> >::value, "");
22 changes: 0 additions & 22 deletions libcxx/test/libcxx/atomics/atomics.syn/incompatible_with_stdatomic.verify.cpp
View file
Open in desktop

This file was deleted.

23 changes: 0 additions & 23 deletions libcxx/test/libcxx/atomics/stdatomic.h.syn/dont_hijack_header.compile.pass.cpp
View file
Open in desktop

This file was deleted.

Loading