1510897 - Android build broken with clang 8

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Mike Hommey [:glandium]]

Clang trunk changed the default standard lib for C++ from libstdc++ to libc++ on Android, and the NDK headers we use don't actually like that very much. For example, this happens during configure:

INFO: checking for unistd.h... 
DEBUG: Creating `/tmp/conftest.E0R80U.cpp` with content:
DEBUG: | #include <unistd.h>
DEBUG: | int
DEBUG: | main(void)
DEBUG: | {
DEBUG: |
DEBUG: | ;
DEBUG: | return 0;
DEBUG: | }
DEBUG: Executing: `/builds/worker/workspace/build/src/clang/bin/clang++ --target=arm-linux-androideabi -isystem /builds/worker/workspace/build/src/android-ndk/sysroot/usr/include/arm-linux-androideabi -isystem /builds/worker/workspace/build/src/android-ndk/sysroot/usr/include -gcc-toolchain /builds/worker/workspace/build/src/android-ndk/toolchains/arm-linux-androideabi-4.9/prebuilt/linux-x86_64 -D__ANDROID_API__=16 -I/builds/worker/workspace/build/src/android-ndk/sources/cxx-stl/llvm-libc++/include -I/builds/worker/workspace/build/src/android-ndk/sources/android/support/include -I/builds/worker/workspace/build/src/android-ndk/sources/cxx-stl/llvm-libc++abi/include -c /tmp/conftest.E0R80U.cpp`
DEBUG: The command returned non-zero exit status 1.
DEBUG: Its error output was:
DEBUG: | In file included from /tmp/conftest.E0R80U.cpp:1:
DEBUG: | In file included from /builds/worker/workspace/build/src/android-ndk/sysroot/usr/include/unistd.h:34:
DEBUG: | In file included from /builds/worker/workspace/build/src/android-ndk/sysroot/usr/include/sys/types.h:39:
DEBUG: | /builds/worker/workspace/build/src/android-ndk/sysroot/usr/include/bits/pthread_types.h:38:3: error: unknown type name 'size_t'
DEBUG: | size_t stack_size;
DEBUG: | ^
DEBUG: | /builds/worker/workspace/build/src/android-ndk/sysroot/usr/include/bits/pthread_types.h:39:3: error: unknown type name 'size_t'
DEBUG: | size_t guard_size;
DEBUG: | ^

Which leads to HAVE_UNISTD_H and many other things not being defined, which leads to build failure for missing e.g. lseek in gzlib.c and statfs in nsLocalFileUnix.cpp.

The "obvious" way to fix this is to add -stdlib=libstdc++ to the C++ command lines on android, but the problem is that it's not quite so straightforward to find the right spot for this, because those tests are in python configure, and we barely handle compiler flags in python configure, and for the flags that we do handle for android, we do no distinction between C and C++, but -stdlib=libstdc++ causes a -Werror error when used on C compilation.

Comment 1

[Nathan Froyd [:froydnj]]

That seems weird that the switch of standard libraries would cause problems in a C header.  Isn't this a bug in Android's unistd.h file?  (I mean, we have to address it in our builds until the NDK is fixed, but still...)

Comment 2

[Mike Hommey [:glandium]]

(In reply to Nathan Froyd [:froydnj] from comment #1)

That seems weird that the switch of standard libraries would cause problems
in a C header.

Here's what happens:
including stddef.h includes /builds/worker/workspace/build/src/android-ndk/sources/cxx-stl/llvm-libc++/include/stddef.h, which does this:

#if defined(__need_ptrdiff_t) || defined(__need_size_t) || \
    defined(__need_wchar_t) || defined(__need_NULL) || defined(__need_wint_t)
// (...)
#include_next <stddef.h>
#elif !defined(_LIBCPP_STDDEF_H)
#define _LIBCPP_STDDEF_H
// (...)
#include_next <stddef.h>
#endif

And with -stdlib=libstdc++, the #include_next gets /builds/worker/workspace/build/src/clang/lib/clang/8.0.0/include/stddef.h, which defines size_t and others.

With -stdlib=libc++, the #include_next gets /builds/worker/workspace/build/src/clang/bin/../include/c++/v1/stddef.h, which... does the same thing as /builds/worker/workspace/build/src/android-ndk/sources/cxx-stl/llvm-libc++/include/stddef.h, but we got here from the second branch there, so _LIBCPP_STDDEF_H is defined, and the next stddef.h is not included.

So the problem we have is that we use different libc++ headers manually, while clang wants to use its own too.

It seems the solution here could be to replace the -I$NDK/sources/cxx-stl/llvm-libc++ with -stdlib=libc++, but I'm not sure how it would work out with older versions of clang.

Nathan, do you remember why you went with -I in bug 1164921?

Comment 3

[Mike Hommey [:glandium]]

but I'm not sure how it would work out with older versions of clang.

In fact, I'm not even sure it would work out fine with this version of clang...

Comment 4

[Mike Hommey [:glandium]]

Nathan, do you remember why you went with -I in bug 1164921?

Was it maybe because we were using clang from the NDK back then? I guess -stdlib=libc++ wouldn't work with that...

Comment 5

[Nathan Froyd [:froydnj]]

(In reply to Mike Hommey [:glandium] from comment #4)

Nathan, do you remember why you went with -I in bug 1164921?

Was it maybe because we were using clang from the NDK back then? I guess -stdlib=libc++ wouldn't work with that...

That seems like a much nicer explanation than the one I was going to give about me doing the expedient thing rather than the correct thing. -stdlib might not have been supported at that point, too.

Comment 6

[Mike Hommey [:glandium]]

Attachment: Bug 1510897 - Separate Android C++ flags from the other Android toolchain flags.

We shouldn't pass those flags when building C. It doesn't matter /too/
much currently, but will in a subsequent change, which will introduce
a C++-only flag in stlport_cppflags.

Comment 7

[Mike Hommey [:glandium]]

Attachment: Bug 1510897 - Add -stdlib=libstdc++ to the compiler flags on Android.

There is no concern that the Android NDK clang may not support it, as
the flag was added in clang 2.9. The flag is also not supported with
GCC, which is not ideal, but we already crossed that bridge at least
with -gcc-toolchain added in extra_toolchain_flags, which is not
supported by GCC either.

Comment 8

[Mike Hommey [:glandium]]

Attachment: Bug 1510897 - Disable C++2a warnings in widget/android/EventDispatcher.cpp.

Somehow this was left alone in bug 1509926, although the file was
mentioned there.

Comment 9

[Mike Hommey [:glandium]]

Attachment: Bug 1510897 - Don't build clang with LLVM_ENABLE_LIBCXX=ON.

As of clang 8, llvm-config doesn't return all flags clang was built
with, and omits some flags that do impact the libclang ABI,
-stdlib=libc++ being one of them (it might well be the only one).

Building clang with LLVM_ENABLE_LIBCXX=ON does build it with
-stdlib=libc++, but is unrelated to whether or not libc++ is built and
shipped with clang, which still happens without it.

So while versions older than clang 8 are not really affected, it doesn't
hurt to build clang without -stdlib=libc++ (especially when it
currently only applies to the clang used to cross build android with
PGO, not even the other android cross builds), in preparation for
switching to clang 8.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a5b86fb8a24a
Separate Android C++ flags from the other Android toolchain flags. r=nalexander
https://hg.mozilla.org/integration/autoland/rev/550fed2c6b07
Add -stdlib=libstdc++ to the compiler flags on Android. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/a508fc434362
Disable C++2a warnings in widget/android/EventDispatcher.cpp. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/656e3d5c59be
Don't build clang with LLVM_ENABLE_LIBCXX=ON. r=froydnj

Comment 11

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/a5b86fb8a24a
https://hg.mozilla.org/mozilla-central/rev/550fed2c6b07
https://hg.mozilla.org/mozilla-central/rev/a508fc434362
https://hg.mozilla.org/mozilla-central/rev/656e3d5c59be