[android] Update headers that were split in NDK 23 #568
Conversation
NDK 23 split stdatomic.h up and that causes problems when invoked from C++ files, so use the new bits/stdatomic.h instead as a workaround, as seen in the test for aosp-mirror/platform_bionic@76e2b15.
|
@drodriguez, would you review? swiftlang/swift#38441 will need this, once we switch to NDK 23. |
|
Have you tried... One should not access the |
|
No, I'm not familiar with these linkage issues when mixing C headers with C++, will try your way instead and let you know. |
|
Just tried it, doesn't work either. If you look at the error trace from my link above, the problem is that This problem remains with your proposed fix, whereas mine makes sure only the C declarations are called, even if it calls an internal header to do so. If you have another fix in mind, let me know. |
|
@drexin, any input on this? Maybe you've dealt with these mixed linkage issues before, I haven't. |
|
I think this also applies to previous NDK versions. We’ve been using the following to work around these for a while: #if defined(ANDROID) && __has_include(<bits/stdatomic.h>)
#include <bits/stdatomic.h>
#else
#include <stdatomic.h>
#endifSee also the following related discussions in the NDK project for more background: |
|
Thanks for all the links. According to Dan Albert last year, what libdispatch does in "I did get an authoritative answer on this from multiple members of the C++ standard committee, and using http://eel.is/c++draft/using.headers#3
An @ktopley-apple, is there a better change we can make to |
|
@buttaface - I'm facing this issue in NDK r22, and can confirm that including Hence, wanted to ask: Any particular reason for enabling this fix only for |
|
@shrukul, the NDK issues @triplef linked above note that the Bionic header change that triggered this was reverted in later versions of NDK 21, ie the last version 21e doesn't have it. While NDK 22 did, it wasn't an LTS NDK and has now been obsoleted by NDK 23, so I chose the first LTS NDK that made this change, 23. Anyway, hopefully we can get this fixed properly in @etcwilde, any input? |
I'm not familiar with android development, so I don't have an opinion on a version, so long as it work. |
|
@etcwilde, I was more hoping for an opinion on this comment, which has nothing to do with Android, but is about how one of this project's headers is doing something non-standard, that happens to break for Android. If we fix it there, we might fix this issue without reference to Android or its versions. I'm unfamiliar with these arcane C/C++ issues, so I was hoping you might have some input on that. |
Ah, sorry, I misunderstood. The answer is a little bit round-a-bout. So as per the C++11 spec (7.6.1.2, Table 14), the C++11 standard library must vend an The part I don't know is how standards compliant the Android development kits are, or which version of developer kit contains which version(s) of the standard. This proposal, which targets C++23, actually comments on this issue. Unfortunately, right now the only portable way is to include e.g. #ifdef __cplusplus
#include <atomic>
using std::atomic_int;
using std::memory_order;
using std::memory_order_acquire;
...
#else /* not __cplusplus */
#include <stdatomic.h>
#endif /* __cplusplus */ |
|
@etcwilde, I was more hoping to hear about the issue causing this, ie that I just noticed another Any ideas for other changes to |
|
Could you share what the "other errors" were with switching linkage to c++? |
I don't think that would work either. Let me state the problem as briefly as possible: Prior to NDK 22, However, once NDK 22 enabled those C++ declarations, it broke compiling C++ files in this codebase, because of the incorrect usage of I'd rather fix the underlying problem in
Some other C++ definition issues, I'll rerun that and paste the error later this weekend. |
|
Changing the linkage to C++ before including the header as a workaround for android seems reasonable to accommodate the non-conforming behaviour on android. I'd say that once we have the errors we can likely understand the situation better. |
I don't think there was anything to "conform" to for |
|
I had tried applying the following patch instead to avoid these linkage issues: I got a bunch of template matching errors: It looks like switching to C++ |
|
Btw, the Android CI has been broken for the last month with this issue, because the LLVM rebranch last month switched from linking Android against libgcc to compiler-rt, swiftlang/llvm-project@a478b0a, so Daniel switched the Android NDK on the CI to the latest version 23 that uses such a recent clang too, but has a sysroot that also made this Since this pull only affects Android and is a workaround for another header that does not conform to the C++ standard in this codebase, I think we should just get this in as a workaround for Android, since nobody has come up with another solution that works. |
|
@swift-ci please test |
|
@shahmishal can we merge this one to unblock the Android CI before the Thanksgiving break? Thanks! |
NDK 23 split
stdatomic.hup and that causes problems when invoked from C++ files, so use the newbits/stdatomic.hinstead as a workaround, as seen in the test for aosp-mirror/platform_bionic@76e2b15.Building libdispatch for Android with the new NDK causes linkage issues in the headers, so I used this same workaround that can be seen in the C++ test for that Bionic commit that split the header up and libdispatch builds for Android again.
I don't know if there's a better way to fix these linkage issues, so let me know if there is.