system STL is not useful and should be removed

[tangobravo]

Description

CppReference suggests <cmath> should overload std::abs with versions taking (and returning) various floating-point types:
https://en.cppreference.com/w/cpp/numeric/math/fabs

With the system STL, this doesn't happen.

The latest NDK default compiler does generate a warning for this [though the suggested fix in the note clearly doesn't work for this STL]:

warning: using integer absolute value function 'abs' when argument is of floating point type [-Wabsolute-value]
note: use function 'std::abs' instead
note: include the header <cmath> or explicitly provide a declaration for 'std::abs'

Minimal test project:
https://github.com/tangobravo/ndk-abs-test

Commenting out the following block in app/build.gradle to show it working in with the default STL implementations:

        externalNativeBuild {
            cmake {
                arguments "-DANDROID_STL=system"
            }
        }

There are probably other cmath functions that should have overloads, but it was the abs() one that I was testing at the time when I noticed it. I realise the gnustl and stlport options are being removed, but I believe the system one is still supported as a reasonable "no STL" option (and remains the default with ndkBuild I think) so thought it worthwhile to file a bug.

Environment Details

OS X 10.12.6, Android Studio 3.0.1 [probably not important for this bug]
Using NDK bundle from Android Studio: 17.1.4828580

[enh]

since we don't seem to have any plans to remove the "system STL", i'll take a look through and try to make sure all the <c.*> headers are up to date.

[DanAlbert]

I don't think this is written down anywhere, and it might be useful for people trying to figure out what to do in their apps, so here's basically my view on the NDK's system STL:

If I had my way I'd just remove the system "STL". It's really only there because it's always been there and I'd break a bunch of builds by removing it. If people actually want C++ support they should use libc++.

but I believe the system one is still supported as a reasonable "no STL" option

The best way to do that is actually "none".

and remains the default with ndkBuild I think

It does... and I can never settle on whether I should change this. Since the system STL is almost the same as "none", it feels strange to me to switch ndk-build's default behavior from none-ish to libc++.

The main reason I think that's change is too invasive is that we have to either pick the default of libc++_static, which is probably a harmful default since using the static STL requires rather careful use, or libc++ shared, which means that your app is now bundling and STL that may or may not use, increasing APK size compared to when it was using the system STL.

When we some day reach the point where 21 is the minimum supported API level in the NDK (I don't suspect this day will come for quite some time), then using libc++ won't mean that your app must include libandroid_support, and we could probably work out something with --as-needed to make sure we only include the STL in your APK if it actually was used.

[tangobravo]

Is c++_headeronly a potential option? Just wouldn't do any linking - so presumably would give linker errors if the user actually needs to choose between c++_shared and c++_static?

I wasn't aware of none as an option - might be worth adding a note somewhere on https://developer.android.com/ndk/guides/cpp-support near Table 1?

I'd suggest switching to none by default for ndk-build - would still mean c libraries can build fine with the default, but would stop essentially any c++ code from building until the user explicitly chooses c++_shared or c++_static. I don't expect there are many true c++ codebases that system would be sufficient for anyway.

[DanAlbert]

Is c++_headeronly a potential option? Just wouldn't do any linking - so presumably would give linker errors if the user actually needs to choose between c++_shared and c++_static?

I'm always hesitant to add something like this because since "the header only part of the STL" isn't something that libc++ makes any attempt to support, so there are no guarantees about what is and what isn't header-only in any given release. It would be entirely possible for the header-only part of libc++ to be the entire library sans key functions in one release and nothing at all the release after. Having such a configuration just feels like laying a bunch of migration landmines to me.

I wasn't aware of none as an option - might be worth adding a note somewhere on https://developer.android.com/ndk/guides/cpp-support near Table 1?

I forgot to document this when I added it. I'm not sure this is something anyone wants though. It got added because we use ndk-build to build the STL itself, and that's one of the few corner cases where this is needed.

CMake and ndk-build are both smart enough to not include/link the STL for a C code (CMake gets this right per-file; ndk-build will still give you the STL include dirs for a module with mixed C and C++ files), and if you're using C++ you might as well just use the STL since it's going to make your life a lot easier.

Anyone that disagrees with that assessment, please speak up :)

I'd suggest switching to none by default for ndk-build - would still mean c libraries can build fine with the default, but would stop essentially any c++ code from building until the user explicitly chooses c++_shared or c++_static. I don't expect there are many true c++ codebases that system would be sufficient for anyway.

Yeah... Especially given that ndk-build and CMake will do the right thing when you have a C only module maybe I should just get over my worries and switch the default to one of c++_static or c++_shared...

[pirama-arumuga-nainar]

I'm always hesitant to add something like this because since "the header only part of the STL" isn't something that libc++ makes any attempt to support, so there are no guarantees about what is and what isn't header-only in any given release.

This is not fixed even within a release. For instance, Clang my choose to inline (or not inline) functions based on optimization flags.

[enh]

https://android-review.googlesource.com/c/platform/ndk/+/718562 modernizes the "system" STL a little.

it's untested, and definitely missing #if __ANDROID_API__ checks. it also doesn't add any of the inline overloads.

the more i think about this, though, the more i think -- regardless of whether "none" or "libc++" becomes the default -- we should just remove "system". i'm highly skeptical anyone's using it in its current state. i'm also skeptical that anyone would use an "STL" that's just the <c.*> headers. and i don't think we're ever likely to commit to maintaining a header-only libc++, as various people have already said.

[tangobravo]

i'm also skeptical that anyone would use an "STL" that's just the <c.*> headers. and i don't think we're ever likely to commit to maintaining a header-only libc++, as various people have already said.

Makes sense. Outside of a std::abs test case I can see very few uses for it in the real world anyway.

the more i think about this, though, the more i think -- regardless of whether "none" or "libc++" becomes the default -- we should just remove "system".

Yup I'd be for that. Even for code not using the STL, it isn't really an option as the basic c++ headers provided are not standards compliant. Definitely seems like a poor default that may actually compile but then just have subtly broken behaviour. I've never actually used "system" on purpose, just stumbled onto this bug due to it being the default with ndk-build.

Given the "single runtime per app" caveat, it seems "c++_shared" is the sensible default, whilst users can opt-in to c++_static if they know they only have one .so using it and want to get the benefits of smaller binary size and potentially better optimisations.

The downside of the _shared approach is the need to do a system.loadLibrary("c++_shared"); before the user library, at least on some platform versions. Now API 14 is the minimum supported target, is that still necessary? I can't remember which version started resolving those dependencies. Related - is the recommendation to use ReLinker still valid with a minimum API of 14, or are the system.loadLibrary failures still an issue there?

[enh]

i tried to convince @DanAlbert yesterday but i'm not sure i succeeded. i'll try again today :-)

i think one specific concern was "we've always had one release with a deprecation notice, another release where the default is switched, and then a final release to actually remove the feature". and since as of today the docs still mention "system" but not "none", that ought to make r18 the deprecation release and r21 the removal release. i'll try to get the documentation fixed today, at least.

as for ReLinker, the "you need a reverse topological sort" bug was fixed in JB-MR2 --
https://android.googlesource.com/platform/bionic/+/master/android-changes-for-ndk-developers.md#changes-to-library-dependency-resolution -- and the "PackageManager doesn't handle atomic copies right" bug was fixed even later than that, i think. if i was shipping an Android app myself, i'd probably use ReLinker until i wasn't supporting anything older than M or N.

[DanAlbert]

Yeah, I'm convinced. I think it's okay for us to skip the "we're changing the default next release" past and just so that in r18. I'm considering just removing it straight away in beta 1 and seeing if anyone cares.

The decision we have to make is whether we use shared or static. As has been discussed, shared is a safer default, but it would be inconsistent with cmake. It would be consistent with standalone toolchain, but that's a much smaller set of users. Maybe we should change the cmake default too...

I think the libstdc++.so binary itself will remain in the sysroot for r18 since removing it may break other build systems if they haven't yet started passing -nostdlib++ since the clang driver will link it anyway. This won't be a problem with the clang we'll have for r19, so the problem will solve itself by then.

[DanAlbert]

The other thing we'll need to decide is whether we maintain the support for new/delete, exceptions, and RTTI when STL is none. I'm inclined to opt for no, especially since you won't have full support without <exception> and <typeinfo>. On the other hand, the support is already there. It breaks not infrequently though.

[rprichard]

Aside: The <typeinfo> header looks broken when STL is system. It forward-declares class std::type_info, but it defines class type_info in the global namespace rather than std. If I fix that, then std::type_info::name and std::type_info::operator== are undefined references. Those functions are defined as inline in libc++'s typeinfo header.

The typeinfo symbols for built-in types (e.g. int) are undefined with the system STL.

It looks like selecting the system STL with ndk-build links in libc++abi.a, so some RTTI stuff works, like using typeid on a user-declared struct type. e.g. With an STL of none, I see undefined references to the vtables for __cxxabiv1::__class_type_info and __cxxabiv1::__si_class_type_info.

[enh]

to be fair, the docs claim that RTTI doesn't work with "system", so we're under-promising and over-delivering :-)

[enh]

(also, we've updated https://developer.android.com/ndk/guides/cpp-support to talk about deprecation and explicitly mention "none". though it looks like i fucked up a link. will fix...)

[DanAlbert]

We've settled on libc++_shared as a new default for ndk-build, and I'm hoping to bring CMake in line with that (only thing that will prevent it is if that will cause issues for the gradle plugin, as I'm not sure how it decides whether to include the library in the APK or not).

Given that the docs claimed that the system STL didn't support RTTI/exceptions (technically it doesn't, but if you request it the build system will include libc++abi for you) and that the system's typeinfo header was broken, I've also chosen to drop that support along with the system STL rather than try to apply the same support for "none". If the user requests none then we ought to honor it completely (plus I'd need to add a no-really-i-mean-none "STL" for building libc++ itself if I did that).

The new default will be in NDK r18, and the system STL will be removed in r19.

Taking over this bug for this purpose since it contains all the useful discussion and we won't be fixing the originally reported issue anyway. Thanks for filing it and kicking off the discussion 👍

[rprichard]

I think switching cmake would break the gradle plugin:

https://android.googlesource.com/platform/tools/base/+/ead3937b864fb09daf2f743ed57f9b762d78565e/build-system/gradle-core/src/main/java/com/android/build/gradle/tasks/CmakeExternalNativeJsonGenerator.java#212

A new project defaults to empty cmake args:

        externalNativeBuild {
            cmake {
                cppFlags ""
            }
        }

The plugin doesn't hard-code a particular default STL, but instead it assumes the default is some static STL, and it only looks for a shared STL setting.

The equivalent code for ndk-build returns an empty container. I can't remember how the plugin packages the solib. I'll keep looking.

[enh]

(i've abandoned the partial cleanup of "system".)

[rprichard]

For ndk-build, gradle parses -n output. The entry point for the parser is the com.android.build.gradle.external.gnumake.NativeBuildConfigValueBuilder class's build method.

However, the generated JSON file doesn't list the STL solib, only the app/user's solibs.

By experimentation, it looks like Gradle packages any file matching *.so from one of the external build tool's output directories:

• /x/Test31CMake/app/build/intermediates/cmake/debug/obj/armeabi-v7a
• /x/Test31NdkBuild/app/build/intermediates/ndkBuild/debug/obj/local/armeabi-v7a

(This is obj/local/$ABI, not libs/$ABI...)

When I sync (Refresh Linked C++ Projects), Gradle removes solibs from this directory that it doesn't recognize. For ndk-build, it always removes STL solibs (probably this code). Building an ndk-build project copies the STL solib into obj/local, but building a CMake project doesn't, so Gradle uses getStlSharedObjectFiles to find the solib in the NDK installation.

I think we could change ndk-build to c++_shared, but changing CMake would break gradle.

[rprichard]

For CMake, it looks like Gradle copies the STL solib into app/build/intermediates/cmake/debug/obj/armeabi-v7a.

[DanAlbert]

I think the best option for us is to punt this out to r19 so we can give the gradle folks time to prepare for the change.

[DanAlbert]

This one still depends on having AGP always specify an STL. Triaging to r23 as a reminder to myself to see if we can make that change, but it'll then get punted a few releases past that to give a few versions of compatibility.

[DanAlbert]

Still would really like to do this, but haven't had the time to fix AGP.