[PATCH] D88984: Prefer libc++ headers in the -isysroot over the toolchain
ldionne abandoned this revision. ldionne added a comment. Herald added a project: clang-tools-extra. Revisiting this, I don't think we need to move forward with this. Let's just carry our downstream patch until we don't need it anymore (which should be soon ish), and leave upstream Clang as it is (I think the current behavior makes the most sense). I'm abandoning this - I'll reopen if someone thinks that having Clang and AppleClang behave differently for a little while is harmful, but I don't think it is. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D88984/new/ https://reviews.llvm.org/D88984 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D88984: Prefer libc++ headers in the -isysroot over the toolchain
teemperor added a comment. From the LLDB side we can just change our test setup to explicitly add an include the libc++ in our build tree (and have -nostdinc to avoid the libc++ in the SDK). So it wouldn't be a problem for us. I can make a review for that once this gets accepted. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D88984/new/ https://reviews.llvm.org/D88984 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D88984: Prefer libc++ headers in the -isysroot over the toolchain
dexonsmith requested changes to this revision. dexonsmith added a comment. This revision now requires changes to proceed. @rsmith , @teemperor , @ldionne , thoughts on the above? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D88984/new/ https://reviews.llvm.org/D88984 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D88984: Prefer libc++ headers in the -isysroot over the toolchain
dexonsmith added reviewers: rsmith, teemperor. dexonsmith added a comment. Hmm, that's right, to support user-provided libc++ headers it's simpler if we prefer the toolchain / next-to-clang. The reason we need to invert it is transitional. The status quo is that we have no headers in the SDKs (either internally or publicly). We want to move the headers from the toolchain to the SDK. For [reasons], in some internal contexts we'll have headers both places for a small number of years, where the SDK headers should be preferred if both exist. Some ideas: - Add a command-line option to decide whether to check the SDK or next-to-clang first. Maybe we can restrict it to a `-cc1` option. - Make the default behaviour configurable, and potentially use a different default in the Apple CMake cache. WDYT? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D88984/new/ https://reviews.llvm.org/D88984 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D88984: Prefer libc++ headers in the -isysroot over the toolchain
teemperor added a comment. On macOS, LLDB's tests pass `-isysroot /path/to/SDK` to Clang but expect that Clang always prefers the the libc++ headers that are in the Clang build directory. So if we change this behaviour then we should have a way to keep the old behaviour around. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D88984/new/ https://reviews.llvm.org/D88984 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D88984: Prefer libc++ headers in the -isysroot over the toolchain
rsmith added a comment. This seems like it would break the opposite situation: a user places a custom clang and the corresponding libc++ somewhere (perhaps in `$HOME`, or perhaps they run Clang from the build tree). Right now we use the custom clang and its corresponding libc++, but with this change, I would expect we'd instead pick up the SDK version of libc++. Is that what we want? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D88984/new/ https://reviews.llvm.org/D88984 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D88984: Prefer libc++ headers in the -isysroot over the toolchain
ldionne updated this revision to Diff 296772.
ldionne added a comment.
Split the patch in two
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D88984/new/
https://reviews.llvm.org/D88984
Files:
clang-tools-extra/test/clang-tidy/infrastructure/Inputs/empty-sysroot/.gitkeep
clang-tools-extra/test/clang-tidy/infrastructure/Inputs/mock-libcxx/bin/clang
clang-tools-extra/test/clang-tidy/infrastructure/Inputs/mock-libcxx/include/c++/v1/mock_vector
clang-tools-extra/test/clang-tidy/infrastructure/Inputs/mock-toolchain-root/bin/clang
clang-tools-extra/test/clang-tidy/infrastructure/Inputs/mock-toolchain-root/include/c++/v1/mock_vector
clang-tools-extra/test/clang-tidy/infrastructure/clang-tidy-mac-libcxx.cpp
clang/lib/Driver/ToolChains/Darwin.cpp
clang/test/Driver/darwin-header-search-libcxx.cpp
clang/test/Tooling/Inputs/empty-sysroot/.gitkeep
clang/test/Tooling/Inputs/mock-libcxx/bin/clang
clang/test/Tooling/Inputs/mock-libcxx/include/c++/v1/mock_vector
clang/test/Tooling/Inputs/mock-toolchain-root/bin/clang
clang/test/Tooling/Inputs/mock-toolchain-root/include/c++/v1/mock_vector
clang/test/Tooling/clang-check-mac-libcxx-abspath.cpp
clang/test/Tooling/clang-check-mac-libcxx-fixed-compilation-db.cpp
clang/test/Tooling/clang-check-mac-libcxx-relpath.cpp
Index: clang/test/Tooling/clang-check-mac-libcxx-relpath.cpp
===
--- clang/test/Tooling/clang-check-mac-libcxx-relpath.cpp
+++ clang/test/Tooling/clang-check-mac-libcxx-relpath.cpp
@@ -1,15 +1,20 @@
-// Clang on MacOS can find libc++ living beside the installed compiler.
-// This test makes sure our libTooling-based tools emulate this properly.
+// Clang on MacOS can find libc++ living beside the installed compiler if there
+// are no headers in the sysroot. This test makes sure our libTooling-based tools
+// emulate this properly.
//
// RUN: rm -rf %t
// RUN: mkdir %t
//
-// Install the mock libc++ (simulates the libc++ directory structure).
-// RUN: cp -r %S/Inputs/mock-libcxx %t/
+// Install a mock toolchain root that contains a simulation of libc++.
+// RUN: cp -r %S/Inputs/mock-toolchain-root %t/mock-toolchain-root
//
-// Pretend clang is installed beside the mock library that we provided.
-// RUN: echo '[{"directory":"%t","command":"mock-libcxx/bin/clang++ -stdlib=libc++ -target x86_64-apple-darwin -c test.cpp","file":"test.cpp"}]' | sed -e 's/\\/\//g' > %t/compile_commands.json
+// Install a mock sysroot that doesn't contain anything, to make sure we
+// prefer the libc++ headers in the toolchain.
+// RUN: cp -r %S/Inputs/empty-sysroot %t/empty-sysroot
+//
+// RUN: echo '[{"directory":"%t","command":"mock-toolchain-root/bin/clang -isysroot %t/empty-sysroot -stdlib=libc++ -target x86_64-apple-darwin -c test.cpp","file":"test.cpp"}]' | sed -e 's/\\/\//g' > %t/compile_commands.json
// RUN: cp "%s" "%t/test.cpp"
+//
// clang-check will produce an error code if the mock library is not found.
// RUN: clang-check -p "%t" "%t/test.cpp"
Index: clang/test/Tooling/clang-check-mac-libcxx-fixed-compilation-db.cpp
===
--- clang/test/Tooling/clang-check-mac-libcxx-fixed-compilation-db.cpp
+++ clang/test/Tooling/clang-check-mac-libcxx-fixed-compilation-db.cpp
@@ -1,18 +1,23 @@
-// Clang on MacOS can find libc++ living beside the installed compiler.
-// This test makes sure our libTooling-based tools emulate this properly with
-// fixed compilation database.
+// Clang on MacOS can find libc++ living beside the installed compiler if there
+// are no headers in the sysroot. This test makes sure our libTooling-based tools
+// emulate this properly with a fixed compilation database.
//
// RUN: rm -rf %t
// RUN: mkdir %t
//
-// Install the mock libc++ (simulates the libc++ directory structure).
-// RUN: cp -r %S/Inputs/mock-libcxx %t/
+// Install a mock toolchain root that contains a simulation of libc++.
+// RUN: cp -r %S/Inputs/mock-toolchain-root %t/mock-toolchain-root
//
-// RUN: cp clang-check %t/mock-libcxx/bin/
+// Install a mock sysroot that doesn't contain anything, to make sure we
+// prefer the libc++ headers in the toolchain.
+// RUN: cp -r %S/Inputs/empty-sysroot %t/empty-sysroot
+//
+// RUN: cp clang-check %t/mock-toolchain-root/bin/
// RUN: cp %s %t/test.cpp
-// RUN: "%t/mock-libcxx/bin/clang-check" -p %t %t/test.cpp -- \
+// RUN: "%t/mock-toolchain-root/bin/clang-check" -p %t %t/test.cpp -- \
// RUN: -stdlib=libc++ -target x86_64-apple-darwin \
-// RUN: -ccc-install-dir %t/mock-libcxx/bin
+// RUN: -isysroot %t/empty-sysroot \
+// RUN: -ccc-install-dir %t/mock-toolchain-root/bin
//
// ^ -ccc-install-dir passed to unbreak tests on *BSD where
// getMainExecutable() relies on real argv[0] being passed
Index: clang/test/Tooling/clang-check-mac-libcxx-abspath.cpp
[PATCH] D88984: Prefer libc++ headers in the -isysroot over the toolchain
dexonsmith added a comment. Note that I don't see any issues besides splitting the patch and dealing with the clang-format linter problems. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D88984/new/ https://reviews.llvm.org/D88984 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D88984: Prefer libc++ headers in the -isysroot over the toolchain
dexonsmith requested changes to this revision. dexonsmith added a comment. This revision now requires changes to proceed. > Furthermore, instead of always adding both search paths, we find > the first search path that exists and only add that one. Huh, I'm surprised this wasn't already the behaviour. This seems like a good patch to split out and commit first, since in the (somewhat unlikely) case this causes a problem for someone it would be good for them to be able to bisect this separately the other part. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D88984/new/ https://reviews.llvm.org/D88984 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D88984: Prefer libc++ headers in the -isysroot over the toolchain
ldionne created this revision.
ldionne added reviewers: arphaman, dexonsmith.
Herald added subscribers: cfe-commits, jkorous.
Herald added a project: clang.
ldionne requested review of this revision.
Currently, Clang looks for libc++ headers alongside the installation
directory of Clang, and it also adds a search path for headers in the
-isysroot. This patch roughly reverses the order so that headers are
searched for in the following order:
1. /usr/include/c++/v1
2. /bin/../include/c++/v1
The benefit of doing this is that a user-installed libc++ can be picked
up when providing a custom sysroot, which doesn't work properly right
now. Furthermore, instead of always adding both search paths, we find
the first search path that exists and only add that one. Otherwise,
include_next is broken because there could be two sets of libc++ headers
on the search paths.
rdar://48638125
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D88984
Files:
clang-tools-extra/test/clang-tidy/infrastructure/Inputs/empty-sysroot/.gitkeep
clang-tools-extra/test/clang-tidy/infrastructure/Inputs/mock-libcxx/bin/clang
clang-tools-extra/test/clang-tidy/infrastructure/Inputs/mock-libcxx/include/c++/v1/mock_vector
clang-tools-extra/test/clang-tidy/infrastructure/Inputs/mock-toolchain-root/bin/clang
clang-tools-extra/test/clang-tidy/infrastructure/Inputs/mock-toolchain-root/include/c++/v1/mock_vector
clang-tools-extra/test/clang-tidy/infrastructure/clang-tidy-mac-libcxx.cpp
clang/lib/Driver/ToolChains/Darwin.cpp
clang/test/Driver/Inputs/basic_darwin_sdk_usr_cxx_v1/usr/include/c++/v1/.keep
clang/test/Driver/Inputs/basic_darwin_sdk_usr_cxx_v1/usr/lib/.keep
clang/test/Driver/darwin-header-search-libcxx.cpp
clang/test/Tooling/Inputs/empty-sysroot/.gitkeep
clang/test/Tooling/Inputs/mock-libcxx/bin/clang
clang/test/Tooling/Inputs/mock-libcxx/include/c++/v1/mock_vector
clang/test/Tooling/Inputs/mock-toolchain-root/bin/clang
clang/test/Tooling/Inputs/mock-toolchain-root/include/c++/v1/mock_vector
clang/test/Tooling/clang-check-mac-libcxx-abspath.cpp
clang/test/Tooling/clang-check-mac-libcxx-fixed-compilation-db.cpp
clang/test/Tooling/clang-check-mac-libcxx-relpath.cpp
Index: clang/test/Tooling/clang-check-mac-libcxx-relpath.cpp
===
--- clang/test/Tooling/clang-check-mac-libcxx-relpath.cpp
+++ clang/test/Tooling/clang-check-mac-libcxx-relpath.cpp
@@ -1,15 +1,20 @@
-// Clang on MacOS can find libc++ living beside the installed compiler.
-// This test makes sure our libTooling-based tools emulate this properly.
+// Clang on MacOS can find libc++ living beside the installed compiler if there
+// are no headers in the sysroot. This test makes sure our libTooling-based tools
+// emulate this properly.
//
// RUN: rm -rf %t
// RUN: mkdir %t
//
-// Install the mock libc++ (simulates the libc++ directory structure).
-// RUN: cp -r %S/Inputs/mock-libcxx %t/
+// Install a mock toolchain root that contains a simulation of libc++.
+// RUN: cp -r %S/Inputs/mock-toolchain-root %t/mock-toolchain-root
//
-// Pretend clang is installed beside the mock library that we provided.
-// RUN: echo '[{"directory":"%t","command":"mock-libcxx/bin/clang++ -stdlib=libc++ -target x86_64-apple-darwin -c test.cpp","file":"test.cpp"}]' | sed -e 's/\\/\//g' > %t/compile_commands.json
+// Install a mock sysroot that doesn't contain anything, to make sure we
+// prefer the libc++ headers in the toolchain.
+// RUN: cp -r %S/Inputs/empty-sysroot %t/empty-sysroot
+//
+// RUN: echo '[{"directory":"%t","command":"mock-toolchain-root/bin/clang -isysroot %t/empty-sysroot -stdlib=libc++ -target x86_64-apple-darwin -c test.cpp","file":"test.cpp"}]' | sed -e 's/\\/\//g' > %t/compile_commands.json
// RUN: cp "%s" "%t/test.cpp"
+//
// clang-check will produce an error code if the mock library is not found.
// RUN: clang-check -p "%t" "%t/test.cpp"
Index: clang/test/Tooling/clang-check-mac-libcxx-fixed-compilation-db.cpp
===
--- clang/test/Tooling/clang-check-mac-libcxx-fixed-compilation-db.cpp
+++ clang/test/Tooling/clang-check-mac-libcxx-fixed-compilation-db.cpp
@@ -1,18 +1,23 @@
-// Clang on MacOS can find libc++ living beside the installed compiler.
-// This test makes sure our libTooling-based tools emulate this properly with
-// fixed compilation database.
+// Clang on MacOS can find libc++ living beside the installed compiler if there
+// are no headers in the sysroot. This test makes sure our libTooling-based tools
+// emulate this properly with a fixed compilation database.
//
// RUN: rm -rf %t
// RUN: mkdir %t
//
-// Install the mock libc++ (simulates the libc++ directory structure).
-// RUN: cp -r %S/Inputs/mock-libcxx %t/
+// Install a mock toolchain root that contains a simulation of libc++.
+// RUN: cp -r %S/Inputs/mock-toolchain-root %t/mock-toolchain-root
//
-// RUN: cp clang-check
