ndk
ndk copied to clipboard
android
system STL is not useful and should be removed
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
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.
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.
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.
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...
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.
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.
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?
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.
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.
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.
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.
to be fair, the docs claim that RTTI doesn't work with "system", so we're under-promising and over-delivering :-)
(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...)
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 :+1:
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.
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.
For CMake, it looks like Gradle copies the STL solib into app/build/intermediates/cmake/debug/obj/armeabi-v7a.
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.
Makes sense. Outside of a std::abs test case I can see very few uses for it in the real world anyway.
I got problems compiling newset boost 1.67 with for older ndk 16, as in boost std::abs is used not ::abs and in older api abs is inline, while boost used cmath , as in result failed linking stage.
So for 3rd-party libraries that might execute in an environment with multiple static STLs, its better to work with c++_shared, correct?
EDIT: my question was answered in the linked issue below (#813)
Fixing gradle for this is something we probably need to do ourselves, and I haven't found the time yet, so moving to r20.
Please do not remove system STL, isn't removing from default good enough? I would want to write programs in C++ but do not need any fancy stuffs (RTTI, STL etc.)
For some context, I'm migrating a pure C code base to C++ simply for language features like function overload, OOP, templates, lambdas, auto, and many other more. But I don't want to pull in the whole libc++ just for that since I want to optimize the result binary size. Without system, which means no new and delete, it makes OOP impossible and as a result making C++ extremely limited.
Basically, with system, I can at have the same environment as C++ code in AOSP. That is just what I needed.
Actualy it dosnt matter what libc++ You use as long you link staticaly to it, if your code will not use any functionality from stl it may be not linked in your shared lib at all.
@arturbac I would think so too, but in my case the exact same code with APP_STL set as libc++ still resulted in significant larger binary compared with system
Basically, with system, I can at have the same environment as C++ code in AOSP.
AOSP uses libc++.
@arturbac I would think so to, but in my case the exact same code with APP_STL set as libc++ still resulted in significant larger binary compared with system
Then this is the bug we should be fixing. We shouldn't bog down this bug with it, but if you have any information on why that happened please open another bug. libandroid_support will be responsible for some increase, but it's quite small.
@topjohnwu so the difference must come from somthing you use from stl or "stl_abi", like exception hadling code, thread_local code etc. Best will be figuring out what it is used from stl to compile -fvisibility=defeault to get public symbols in lib or -ggdb to get debug info symbols then using objdump to dump symbols linked, with grep to much std, _ndk1
I don't really feel all that strongly about keeping support for libstdc++ around. The one part I would like to drop is the support for the system STL with exception/RTTI support. Since that configuration also requires libandroid_support (for, iirc, exactly KitKat), I don't think there's any value in keeping it since it's equivalent to libc++_static if you don't use any part of the STL.
So I've spent some time understanding the history of the NDK vs platform bionic mess and finally have some understanding about what libandroid_support is doing. I've tried building with c++_shared and use objdump to see the symbols, and as expected there is no STL used in any place. So I'm pretty sure that the additional size is indeed contributed by libandroid_support as mentioned by @DanAlbert. Correct me if I'm wrong, but is statically linking libandroid_support effectively pulling in a small portion of re-implemented libc into the binary?
BTW, I'm not building libraries for applications, but building binaries that are expected to be used as system components. I selected using NDK over AOSP because I want to keep compiling simple rather than using the complicated AOSP building system.
The binary will be installed to sometimes extreme environments so size does matter, every byte counts. When I was using pure C, the result binary size is 90.7 KB. I just finished migrating to C++ without any STL/exception/RTTI, compiling with system results with a binary sized 93.8 KB, which is totally reasonable. Without any changes other than switching to c++_static, the result binary grew to a whopping 197.7 KB!
I'm fine with using old NDKs if libstdc++.so is completely removed in a future release, and I know I'm not the normal NDK user: a 100 KB size increase for an APK won't actually make a dent. But what I'm curious about is that does leaving the system option available while dropping the exception/RTTI mess make things difficult to support on your side?
@topjohnwu , if you can increase your minimum Android API level to at least 21 (APP_PLATFORM:=android-21 in ndk-build) - then libandroid_support will not be linked in, IIRC.
