[BUG] Compilation error using _Atomic type in C++ (r21 regression)

[triplef]

Description

Using the C-style _Atomic type in a C++ file fails to compile with NDK r21, but compiles fine with r20 and on other platforms.

test.cpp:

#include <stdatomic.h>

void test() {
    _Atomic int p;
    atomic_load_explicit(&p, memory_order_relaxed);
}

Error:

test.cpp:5:2: error: no matching function for call to 'atomic_load_explicit'
        atomic_load_explicit(&p, memory_order_relaxed);
        ^~~~~~~~~~~~~~~~~~~~
/Users/me/Library/Android/sdk/ndk/21.0.6113669/toolchains/llvm/prebuilt/darwin-x86_64/sysroot/usr/include/c++/v1/atomic:1816:1: note: candidate template ignored: could not match 'atomic<type-parameter-0-0>' against '_Atomic(int) *'
atomic_load_explicit(const volatile atomic<_Tp>* __o, memory_order __m) _NOEXCEPT
^
/Users/me/Library/Android/sdk/ndk/21.0.6113669/toolchains/llvm/prebuilt/darwin-x86_64/sysroot/usr/include/c++/v1/atomic:1825:1: note: candidate template ignored: could not match 'atomic<type-parameter-0-0>' against '_Atomic(int) *'
atomic_load_explicit(const atomic<_Tp>* __o, memory_order __m) _NOEXCEPT
^

I think the issue is caused by the same change in bionic as #1177.

This e.g. causes the swift-corelibs-libdispatch project to fail to compile with r21 due to its use of atomic types in atomic.h, which is used in C++.

Environment Details

• NDK Version: 21.0.6113669
• Build system: manually running clang++
• Host OS: macOS
• ABI: any
• NDK API level: any

[alexcohn]

Note that you code does not compile on NDK r20:

test.cpp:5:5: error: address argument to atomic operation must be a pointer to _Atomic type ('_Atomic(int) **' invalid)
      atomic_load_explicit(&p, memory_order_relaxed);
      ^                    ~~

That's a simple copy/paste typo, line 4 should be

    _Atomic int p;

[triplef]

Ah yes, thank you. I’ve fixed the code above (I had initially been using a void pointer...).

[alexcohn]

The easy fix is to use <bits/stdatomics.h>. Instead of modifying the code, you can put …/lib64/clang/9.0.8/include/bits early on your isystem list. See #1178 (comment)!

For me, it's enough to add to build.gradle:

cppFlags "-isystem $(NDK_ROOT)\\toolchains\\llvm\\prebuilt\\windows-x86_64\\lib64\\clang\\9.0.8\\include\\bits"

[triplef]

Yes, including <bits/stdatomics.h> works fine, and it’s what I did to work around this issue and #1177.

However I’d rather not have to change the code or hardcode llvm paths in the build script, so it would be great to see this fixed in a future NDK release. 👍

[DanAlbert]

I would strongly recommend against adding the bits directory to your include path. This might work to fix stdatomic.h for this release, but it could quite easily give you the wrong header in other cases.

[alexcohn]

@DanAlbert, you are right.

I believe that the basic idea of using <atomic> inside <stdatomic.h> is flawed. People who include the latter expect it to work C11-style, not as a wrapper around STL.

[DanAlbert]

We'll be fixing the regression in r21b. Thanks for being clear up front that this was a regression from r20. That makes triage easy :)

I believe that the basic idea of using inside <stdatomic.h> is flawed. People who include the latter expect it to work C11-style, not as a wrapper around STL.

tl;dr wrapping <atomic> in <stdatomic.h> is intended to get you C11 style

C++ atomics were designed to be compatible with C atomics, but the standard specified behavior is currently broken. You can't have <stdatomic.h> and <atomic> in the same translation unit, let alone source file. C's <stdatomic.h> defines as macros the same names that are in <atomic> as template functions. When you reach <atomic>, the preprocessor performs macro replacement of the names in the function declarations, and the whole thing falls apart.

The expectation is that http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p0943r4.html will be the fix for this. This proposal describes the behavior we've had in AOSP for the past ~6 years. Using <stdatomic.h> in C++ will actually get you <atomic>, but with names in the global namespace. That's compatible with C11, so the same headers will work with either C11 or C++23 (libc++ will probably backport the behavior as well).

Note that the behavior you're describing has precedent in C++ already. <complex.h> is defined to #include <ccomplex> (into the global namespace) which is defined to #include <complex>.

[triplef]

That’s great news @DanAlbert! Can we expect r21b to also fix #1177?

[alexcohn]

When you reach <atomic>, the preprocessor performs macro replacement of the names in the function declarations, and the whole thing falls apart.

If I am not missing something, I see something exactly opposite:

#include <atomic>
extern "C" {
#include <stdatomic.h>
}

passes compilation.

[DanAlbert]

Other way around.

#include <stdatomic.h>
#include <atomic>

...
[arm64-v8a] Compile++      : foo <= foo.cpp
In file included from jni/foo.cpp:2:
/work/src/android-ndk-r20/sources/cxx-stl/llvm-libc++/include/atomic:1166:49: error: expected ')'
atomic_is_lock_free(const volatile atomic<_Tp>* __o) _NOEXCEPT
                                                ^
/work/src/android-ndk-r20/sources/cxx-stl/llvm-libc++/include/atomic:1166:1: note: to match this '('
atomic_is_lock_free(const volatile atomic<_Tp>* __o) _NOEXCEPT
^
/work/src/android-ndk-r20/toolchains/llvm/prebuilt/linux-x86_64/lib64/clang/8.0.7/include/stdatomic.h:250:68: note: expanded from macro 'atomic_is_lock_free'
#define atomic_is_lock_free(obj) __c11_atomic_is_lock_free(sizeof(*(obj)))
                                                                   ^
In file included from jni/foo.cpp:2:
/work/src/android-ndk-r20/sources/cxx-stl/llvm-libc++/include/atomic:1166:1: error: expected expression
atomic_is_lock_free(const volatile atomic<_Tp>* __o) _NOEXCEPT
^
...

[DanAlbert]

@triplef Yep. Sorry, been bad at triage this week. Fixed that now.

[alexcohn]

Other way around.

This fails with r20, but passes with r21.

[smeenai]

Something worth noting is that libdispatch has its own macro wrappers around _Atomic (e.g. https://github.com/apple/swift-corelibs-libdispatch/blob/c992dacf3ca114806e6ac9ffc9113b19255be9fe/src/shims/atomic.h#L46), so you'll want to confirm that whatever solution is used for r21b works with that case too.

[DanAlbert]

This fails with r20, but passes with r21

Sorry, forgot I hadn't replied to this.

Yes, r21 has what was intended to be a fix for this problem. Unfortunately the fix broke other things, so apparently we didn't fix it quite right :)

[DanAlbert]

Spoke with hboehm about this and #1177, and I think our best bet is to revert r21 to match the r20 behavior for the time being. This means that the problems I mentioned above (can't have stdatomic.h and atomic in the same TU unless you're careful about ordering) will return, but they've been present in the NDK for several years and afaik no one noticed, so there's no rush to fix them. Keeping r21 with that behavior saves us from making a change now that we would potentially need to break compatibility with as the proposal continues to evolve. If we have to break it, I'd rather break it once than twice.

Fair warning though: the current proposal and its general direction is toward not supporting the
_Atomic type qualifier (distinct from the _Atomic(T) type specifier, which has a less certain future) in C++. In other words, it's very likely that your code will be rebroken when the standard adopts this fix.

[DanAlbert]

@pirama-arumuga-nainar looks like me reverting https://android-review.googlesource.com/c/platform/bionic/+/1086558 in the NDK sysroot is not going to cut it since that header is also copied into the Clang builtin headers (lib64/clang/...). We'll need to also revert that in r21's clang branch. How does the toolchain build get that header?

[pirama-arumuga-nainar]

It probably gets copied during build.py. We can revert that change and regenerate prebuilts.

…

On Wed, Feb 19, 2020, 5:15 PM Dan Albert ***@***.***> wrote: @pirama-arumuga-nainar <https://github.com/pirama-arumuga-nainar> looks like me reverting https://android-review.googlesource.com/c/platform/bionic/+/1086558 in the NDK sysroot is not going to cut it since that header is also copied into the Clang builtin headers (lib64/clang/...). We'll need to also revert that in r21's clang branch. How does the toolchain build get that header? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1178?email_source=notifications&email_token=AEZPQ6EMNQTHK54LPFKK2PTRDXKTXA5CNFSM4KMRMC32YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEMKLKVA#issuecomment-588559700>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AEZPQ6GFU3WDAKNH5O4Q26DRDXKTXANCNFSM4KMRMC3Q> .

[stephenhines]

We're going to have to create a compiler release branch for r21 in order to make progress here.

[DanAlbert]

Also true for #1184 (sorry, forgot to assign the milestone on a few bugs).

[DanAlbert]

Fix is in build 6352462 on https://ci.android.com/builds/branches/aosp-ndk-release-r21/grid?