Unified Headers: pthread_barrier_t defined when __ANDROID_API__ < 24

[chris-araman]

Description

When compiling libuv with -D__ANDROID_API__=16, I found that these macros and types were defined:

• PTHREAD_BARRIER_SERIAL_THREAD
• pthread_barrier_t
• pthread_barrierattr_t

I would expect that since pthread_barrier_init and friends are excluded when __ANDROID_API__ < 24, that these types would be excluded as well. They were not available on

libuv checks for a definition of PTHREAD_BARRIER_SERIAL_THREAD and for OS=android when deciding whether or not to include its own pthread_barrier* implementation. I can patch libuv to always use its own implementation, but I can't easily patch the NDK to exclude its conflicting definition of pthread_barrier_t.

Environment Details

Not all of these will be relevant to every bug, but please provide as much
information as you can.

• NDK Version: 14.0.3770861
• Build system: CMake ExternalProject_Add invoking configure/make with environment variables set for Unified Headers
• Host OS: macOS 10.12.3
• Compiler: GCC and Clang (tried both)
• ABI: armeabi
• STL: libc++
• NDK API level: 16
• Device API level: 16

[DanAlbert]

I would expect that since pthread_barrier_init and friends are excluded when ANDROID_API < 24, that these types would be excluded as well.

The problem is that a common use case for the NDK is to target a lower API level (since your target NDK API level is the minimum supported version for your app) and conditionally access newer APIs with dlopen/dlsym. In this case, you need those types and constants to be defined even on the lower API level. Because of that, we can't alter this behavior without causing a much larger set of problems.

libuv checks for a definition of PTHREAD_BARRIER_SERIAL_THREAD and for OS=android when deciding whether or not to include its own pthread_barrier* implementation.

libuv needs to check for the availability of the function, not the availability of the constant. Some configure step needs to be checking this. Alternatively libuv could be patched to expose its own implementation based on __ANDROID_API__.

[DanAlbert]

Discussed this with the rest of the bionic folks today and they all suspect I've overestimating the number of people that rely on this kind of behavior. Reopening for discussion and summoning some other unified headers early adopters and folks that have been involved in earlier discussions: @alexcohn @DoDoENT @hrydgard

We don't have much insight into how important the use case of using dlsym to conditionally access APIs that are beyond your APP_PLATFORM is. I'm under the impression that this is fairly critical, but @enh raised the good point that it may not be all that useful for things inside libc.

We have some options here:

1. Keep things as is. Types and constants are exposed regardless of APP_PLATFORM and only functions will be hidden. This keeps the "dlsym future things" use case working as best as possible, but breaks the case where people are assuming a constant's presence implies the availability of a related function.

2. Reverse this approach. Types and constants won't be visible unless the functions making use of them are also available. This means that if you ever want to use one of these functions conditionally via dlsym, you'll have to copy/paste the types/constants into your source to do so. The advantage is that it doesn't break projects that are using #ifdef to check if a function is available based on the availability of its related constants.

   Note that we're only discussing userspace things here. We won't be adding guards to the kernel headers since those can be used even if bionic doesn't have the function. See ev-compile-cpp-source regression in r14 beta 2 #302 for more discussion on this.

3. Offer both and make it configurable. Something like -DANDROID_EXPOSE_ALL_TYPES or similar. At first I sort of prefer this since it keeps everyone happy, but it won't actually help for the case where you both need to dlsym new APIs and also need to work with a project that configures based on the constants rather than the functions...

4. ?

Thoughts?

[chris-araman]

FWIW, I haven't seen the dlsym approach in the libraries we use. Functionality is determined at compile-time using availability of the symbols on the minimum supported Android release, in our case __ANDROID_API__=16. It seems the authors favor a compile-time decision resulting in similar behavior on later OS releases, rather than a run-time decision resulting in potentially divergent behavior, which could complicate validation.

libuv, sqlite and others could continue to make changes to support compiling for the Android NDK. However, if the point of the Unified Headers effort is to reduce the need for such changes, then options 3 or 2 have to receive priority over 1.

A platform in such broad use cannot easily exclude authors/users who want improved/divergent behavior on later Android releases. Perhaps 2 could be implemented first, with the eye toward a streamlined migration to Unified Headers, with 3 in a subsequent NDK release for additional flexibility.

[hrydgard]

I don't really have a strong opinion regarding option 1 vs option 2, I tend to only use what exists on api=9. I don't like option 3 because it's adding yet another setting that needs to be tested both ways which will take time that the NDK team could use for more important things.

I've mostly had issues with STL incompatibilities between gnustl_static and clang, which will be solved by switching to libc++ in a couple of NDK versions. Apart from that one strerror_r thing which will be fixed in NDK 14r2, my app builds fine with unified headers as they are. I don't use dlsym except for conditionally loading Vulkan.

[DoDoENT]

In our SDK, we generally use only what is offered by API 9, except for OpenGL ES 3.1, from which we only use headers (which are available from API 21). Since those headers offer pointers to functions which we can inspect at runtime, there is no need to manually use dlsym (although this actually done somewhere "behind the scenes").

We also plan to use Vulkan for some of our use cases, however we still do not have any experience with it. Using it will require dlsym to conditionally load Vulkan.

We also made some experiments with using OpenCL for accelerating some of our core algorithms and since it is not part of android API, we heavily used dlsym for accessing its functions (this worked fine until Android 7, where such attempts are deliberately disabled :@ ).

So, my opinion would be to stick with option 1 or give a choice as with option 3 - those people that require usage of both dlsym and using extern project can pre-build the extern project with -DANDROID_EXPOSE_ALL_TYPES (or without it, whichever suits best) and build their project with different option of that flag and then link them together.

[alexcohn]

TL;NR: I vote for the second option: hide the defines and constants together with APIs.

I haven't yet seen a situation where dlsym for newer platform was necessary.

Similar condition is quite popular on the Java side of Android, but the way it works there is by using minSdkVersion lower than compileSdkVersion, and keeping the newer-platform-dependent code isolated by if (android.os.Build.VERSION.SDK_INT>=XXX) {}. Once, Dalvik was less generous, but still, there was a way to avoid reflection for system API (the Java equivalent of dlopen/dlsym) for Cupcake.

On the Native side, I have never came across extended libc API that provides important additional functionality as compared to lower platforms. Having atof() defined as inline in stdlib.h prior to SDK 21 does not break anything; the only burden it creates is that mixing components built with different APP_PLATFORM is hard.

[alexcohn]

PS OpenCL is a very different beast. AFAIK, presence of OpenCL is not determined by the platform level, but depends on the chipset; the library may have different names, and expose different capabilities. Will there be some wrapper for it in the shiny post-Nougat future?

[hrydgard]

If Vulkan is available, spinning that up and using it to dispatch compute shaders seems like a better (and better-supported) option than going OpenCL. (But that's a bit off topic here).

[enh]

other than pthread_barrier_t, pthread_barrierattr_t, and pthread_spinlock_t, there aren't many other examples of this. those there are, are things like <ifaddrs.h> where the whole header was introduced late, all the functions and types in it appear at once, but the types aren't guarded.

since the case of "the header exists, but is effectively empty" is also weird, i'm leaning towards leaving those alone until/unless we get an actual motivating use case.

https://android-review.googlesource.com/355466 hides the definitions of the types until API level 24 (to match the functions), but unfortunately that breaks the versioner tool. it spews messages like

    /huge-ssd/aosp-arm64/bionic/tools/versioner/current/pthread.h:183:30: error: unknown type name 'pthread_barrierattr_t'; did you mean 'pthread_mutexattr_t'?
    int pthread_barrierattr_init(pthread_barrierattr_t* _Nonnull attr) __INTRODUCED_IN(24);
                                 ^~~~~~~~~~~~~~~~~~~~~
                                 pthread_mutexattr_t

and then dumps core:

    tools/versioner/preupload.sh: line 8: 25844 Segmentation fault      (core dumped) versioner -r arm -r arm64

[jmgao]

Yeah, you'll need to add guards to the functions as well.

[DanAlbert]

Thanks a lot for the feedback! Sounds like mostly votes for 2.

@DoDoENT, if I'm understanding you correctly, you're only interested in being able to dlsym from actual APIs like GL and Vulkan? You don't care about POSIX stuff in bionic? Assuming that's the case, I think we're going to go with option 2 within bionic and keep with 1 for actual platform APIs like GL, Vulkan, and frameworks stuff (you won't need to manually use the android-21 GL headers any more, they'll be up to date even for android-9).

[DoDoENT]

@DoDoENT, if I'm understanding you correctly, you're only interested in being able to dlsym from actual APIs like GL and Vulkan?

Yes.

You don't care about POSIX stuff in bionic?

No.

Assuming that's the case, I think we're going to go with option 2 within bionic and keep with 1 for actual platform APIs like GL, Vulkan, and frameworks stuff (you won't need to manually use the android-21 GL headers any more, they'll be up to date even for android-9).

👍

In short terms, I am more interested in having fixes for issues #318, #313 and #21. #318 is really a blocker for us at the moment and #313 is quite critical (some of our developers need to run android studio under linux virtual machine on their windows system just to get LTO working).

[DanAlbert]

Just a heads up: this is going to be in r15 but won't be in beta 1. We've got a bunch of new code in the headers (better FORTIFY support) that requires a Clang newer than the one we have in the NDK right now and the new Clang has some issues with arm5, so we're not taking that update just yet. We should have another update really soon, but it won't be in time for beta 1. Apologies for the inconvenience!