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

[BUG] Error when including stdatomic.h inside extern "C" (r21 regression) #1177

Closed
triplef opened this issue Jan 28, 2020 · 5 comments
Closed
Assignees

Comments

@triplef
Copy link

triplef commented Jan 28, 2020

Description

Including "stdatomic.h" inside an extern "C" block in a C++ file causes the following compile error with NDK r21, while building the same code with r20 works fine:

In file included from test.cpp:2:
In file included from /Users/me/Library/Android/sdk/ndk/21.0.6113669/toolchains/llvm/prebuilt/darwin-x86_64/lib64/clang/9.0.8/include/stdatomic.h:45:
In file included from /Users/me/Library/Android/sdk/ndk/21.0.6113669/toolchains/llvm/prebuilt/darwin-x86_64/sysroot/usr/include/c++/v1/atomic:552:
/Users/me/Library/Android/sdk/ndk/21.0.6113669/toolchains/llvm/prebuilt/darwin-x86_64/sysroot/usr/include/c++/v1/type_traits:426:1: error: templates must have C++ linkage
template <class _T1, class _T2> struct _LIBCPP_TEMPLATE_VIS pair;
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
test.cpp:1:1: note: extern "C" language linkage specification begins here
extern "C" {
^

test.cpp:

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

Compiled using:

$HOME/Library/Android/sdk/ndk/21.0.6113669/toolchains/llvm/prebuilt/darwin-x86_64/bin/clang++ --target=armv7-none-linux-androideabi21 --gcc-toolchain=$HOME/Library/Android/sdk/ndk/21.0.6113669/toolchains/llvm/prebuilt/darwin-x86_64 --sysroot=$HOME/Library/Android/sdk/ndk/21.0.6113669/toolchains/llvm/prebuilt/darwin-x86_64/sysroot -DANDROID -march=armv7-a -mthumb -fPIC -std=gnu++11 -o test.cpp.o -c test.cpp

Using NDK 20.1.5948944 in the above compile command works without issues.

I think the issue was introduced with change 1086558 in bionic:

diff --git a/libc/include/stdatomic.h b/libc/include/stdatomic.h
index 3c61334..b7dac4a 100644
--- a/libc/include/stdatomic.h
+++ b/libc/include/stdatomic.h
@@ -32,7 +32,7 @@
 
 #include <sys/cdefs.h>
 
-#if defined(__cplusplus) && __cplusplus >= 201103L && defined(_USING_LIBCXX)
+#if defined(__cplusplus) && __cplusplus >= 201103L && __has_include(<atomic>)
 # if __has_feature(cxx_atomic)
 #  define _STDATOMIC_HAVE_ATOMIC
 # endif

As far as I can see, _USING_LIBCXX was previously undefined in NDK r20, causing stdatomic.h to use its C implementation instead of including C++ . Note that defined(__cplusplus) will still be true inside the extern "C" block.

Environment Details

  • NDK Version: 21.0.6113669
  • Build system: manually running clang++
  • Host OS: macOS
  • ABI: any
  • NDK API level: any
@DanAlbert
Copy link
Member

Will be reverting the patch that caused the break in r21b.

Similar warning as in #1178 (comment) though, I think this will be broken when the committee adopts the proposal discussed in that bug. The only way to avoid this bug (I think) is to avoid the <atomic> header entirely when in an extern "C" context, and the preprocessor has no way of knowing if it is or not.

In general, it's best to avoid having any #include inside extern "C".

@jmgao
Copy link
Contributor

jmgao commented Feb 20, 2020

Maybe we could #include <atomic> inside of an extern "C++" block?

@DanAlbert
Copy link
Member

Can you nest those? That would probably work.

@DanAlbert
Copy link
Member

I did get an authoritative answer on this from multiple members of the C++ standard committee, and using #include within extern "C" is disallowed by the standard.

http://eel.is/c++draft/using.headers#3

A translation unit shall include a header only outside of any declaration or definition and, in the case of a module unit, only in its global-module-fragment, and shall include the header or import the corresponding header unit lexically before the first reference in that translation unit to any of the entities declared in that header. No diagnostic is required.

An extern "C" block is a declaration, so it's disallowed.

Given that, I don't think we should be fixing this. I was in favor when we thought this behavior was unspecified, but given the prohibition in the spec I don't think there's any clear benefit to reverting to the old behavior in r21b if we're just going to let it break in r22 anyway. @enh-google, wdyt?

@enh-google
Copy link
Collaborator

yeah, makes sense to me --- this seems like the direction the world is going wrt <atomic> and <stdatomic.h>.

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

No branches or pull requests

4 participants