Page MenuHomePhabricator

Driver: Don't look for libc++ headers in the install directory on Android.
ClosedPublic

Authored by pcc on Dec 6 2019, 4:02 PM.

Details

Summary

The NDK uses a separate set of libc++ headers in the sysroot. Any headers
in the installation directory are not going to work on Android, not least
because they use a different name for the inline namespace (std::1 instead
of std::
ndk1).

This effectively makes it impossible to produce a single toolchain that is
capable of targeting both Android and another platform that expects libc++
headers to be installed in the installation directory, such as Mac.

In order to allow this scenario to work, stop looking for headers in the
install directory on Android.

Diff Detail

Event Timeline

pcc created this revision.Dec 6 2019, 4:02 PM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a subscriber: ldionne. · View Herald Transcript

Build result: pass - 60573 tests passed, 0 failed and 726 were skipped.

Log files: console-log.txt, CMakeCache.txt

danalbert requested changes to this revision.Dec 6 2019, 4:49 PM
danalbert added inline comments.
clang/test/Driver/android-no-installed-libcxx.cpp
9

This looks like it should be failing. The NDK headers are installed to <NDK>/sysroot/usr/include/c++/v1. Presumably it's passing because the trivial sysroot in the test directory is not complete enough?

Will need a more specific CHECK-NOT here, and also a CHECK to make sure the correct directory is being searched.

This revision now requires changes to proceed.Dec 6 2019, 4:49 PM
pcc marked an inline comment as done.Dec 6 2019, 4:59 PM
pcc added inline comments.
clang/test/Driver/android-no-installed-libcxx.cpp
9

The %t/include/c++/v1 path here represents the install directory, not the sysroot. Note that the test isn't passing a -sysroot flag.

We already have a test that the correct directory is being searched, see test/Driver/android-ndk-standalone.cpp.

danalbert accepted this revision.Dec 6 2019, 5:05 PM
danalbert added inline comments.
clang/test/Driver/android-no-installed-libcxx.cpp
9

Ah, I'd forgotten that the tests required an explicit --sysroot flag. That's not required for a typical NDK clang call, but it is here. Never mind then, this LGTM.

This revision is now accepted and ready to land.Dec 6 2019, 5:05 PM
This revision was automatically updated to reflect the committed changes.

Can we revert this? It doesn't seem to work on my Fedora 31 box:

FAIL: Clang :: Driver/android-no-installed-libcxx.cpp (9249 of 59761)
******************** TEST 'Clang :: Driver/android-no-installed-libcxx.cpp' FAILED ********************
Script:
--
: 'RUN: at line 4';   mkdir -p /tmp/_update_lc/r/tools/clang/test/Driver/Output/android-no-installed-libcxx.cpp.tmp/bin
: 'RUN: at line 5';   mkdir -p /tmp/_update_lc/r/tools/clang/test/Driver/Output/android-no-installed-libcxx.cpp.tmp/include/c++/v1
: 'RUN: at line 6';   /tmp/_update_lc/r/bin/clang -target aarch64-linux-android -ccc-install-dir /tmp/_update_lc/r/tools/clang/test/Driver/Output/android-no-installed-libcxx.cpp.tmp/bin    -stdlib=libc++ -fsyntax-only /home/dave/s/lp/clang/test/Driver/android-no-installed-libcxx.cpp -### 2>&1 | /tmp/_update_lc/r/bin/FileCheck /home/dave/s/lp/clang/test/Driver/android-no-installed-libcxx.cpp
--
Exit Code: 1

Command Output (stderr):
--
/home/dave/s/lp/clang/test/Driver/android-no-installed-libcxx.cpp:8:15: error: CHECK-NOT: excluded string found in input
// CHECK-NOT: "-internal-isystem" "{{.*}}v1"
              ^
<stdin>:5:653: note: found here
 "/tmp/_update_lc/r/bin/clang-10" "-cc1" "-triple" "aarch64-unknown-linux-android" "-fsyntax-only" "-disable-free" "-disable-llvm-verifier" "-discard-value-names" "-main-file-name" "android-no-installed-libcxx.cpp" "-mrelocation-model" "pic" "-pic-level" "2" "-pic-is-pie" "-mthread-model" "posix" "-mframe-pointer=all" "-fno-rounding-math" "-masm-verbose" "-mconstructor-aliases" "-fuse-init-array" "-target-cpu" "generic" "-target-feature" "+neon" "-target-abi" "aapcs" "-mllvm" "-aarch64-fix-cortex-a53-835769=1" "-fallow-half-arguments-and-returns" "-dwarf-column-info" "-debugger-tuning=gdb" "-resource-dir" "/tmp/_update_lc/r/lib64/clang/10.0.0" "-internal-isystem" "/usr/include/c++/v1" "-internal-isystem" "/usr/local/include" "-internal-isystem" "/tmp/_update_lc/r/lib64/clang/10.0.0/include" "-internal-externc-isystem" "/include" "-internal-externc-isystem" "/usr/include" "-fdeprecated-macro" "-fdebug-compilation-dir" "/tmp/_update_lc/r/tools/clang/test/Driver" "-ferror-limit" "19" "-fmessage-length" "0" "-fno-signed-char" "-fgnuc-version=4.2.1" "-fobjc-runtime=gcc" "-fcxx-exceptions" "-fexceptions" "-fdiagnostics-show-option" "-x" "c++" "/home/dave/s/lp/clang/test/Driver/android-no-installed-libcxx.cpp"

Sorry, I had to revert this for breaking things. Please ping me when you have a new patch.

pcc reopened this revision.Dec 9 2019, 9:59 AM

Relanding with a fix for the problem found by @davezarzycki. The test needed to be adjusted to set --sysroot, otherwise it will fail in the case where a directory named /usr/include/c++/v1 or /usr/local/include/c++/v1 exists.

This revision is now accepted and ready to land.Dec 9 2019, 9:59 AM
This revision was automatically updated to reflect the committed changes.