-
Notifications
You must be signed in to change notification settings - Fork 12.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Darwin][Driver][clang] Prioritise command line args over DEFAULT_SYSROOT
#115993
Conversation
@llvm/pr-subscribers-clang-driver @llvm/pr-subscribers-clang Author: Carlo Cabrera (carlocab) ChangesOn Darwin, This is wrong, because it leads to using headers that are of different We can see this from:
We can fix this by reversing the order in which the Downstream bug report: Homebrew/homebrew-core#197277 Co-authored-by: Bo Anderson <[email protected]> Full diff: https://github.com/llvm/llvm-project/pull/115993.diff 2 Files Affected:
diff --git a/clang/lib/Driver/ToolChains/Darwin.cpp b/clang/lib/Driver/ToolChains/Darwin.cpp
index 87380869f6fdab..8bb239d8d3f9df 100644
--- a/clang/lib/Driver/ToolChains/Darwin.cpp
+++ b/clang/lib/Driver/ToolChains/Darwin.cpp
@@ -428,15 +428,14 @@ void darwin::Linker::AddLinkArgs(Compilation &C, const ArgList &Args,
Args.AddAllArgs(CmdArgs, options::OPT_sub__library);
Args.AddAllArgs(CmdArgs, options::OPT_sub__umbrella);
- // Give --sysroot= preference, over the Apple specific behavior to also use
- // --isysroot as the syslibroot.
- StringRef sysroot = C.getSysRoot();
- if (sysroot != "") {
- CmdArgs.push_back("-syslibroot");
- CmdArgs.push_back(C.getArgs().MakeArgString(sysroot));
- } else if (const Arg *A = Args.getLastArg(options::OPT_isysroot)) {
+ // Prioritise -isysroot to ensure that the libraries we link to are taken
+ // from the same SDK that our headers come from.
+ if (const Arg *A = Args.getLastArg(options::OPT_isysroot)) {
CmdArgs.push_back("-syslibroot");
CmdArgs.push_back(A->getValue());
+ } else if (StringRef sysroot = C.getSysRoot(); sysroot != "") {
+ CmdArgs.push_back("-syslibroot");
+ CmdArgs.push_back(C.getArgs().MakeArgString(sysroot));
}
Args.AddLastArg(CmdArgs, options::OPT_twolevel__namespace);
diff --git a/clang/test/Driver/sysroot.c b/clang/test/Driver/sysroot.c
index 85da2499090af1..1e85760409a486 100644
--- a/clang/test/Driver/sysroot.c
+++ b/clang/test/Driver/sysroot.c
@@ -11,9 +11,9 @@
// RUN: FileCheck --check-prefix=CHECK-APPLE-ISYSROOT < %t2 %s
// CHECK-APPLE-ISYSROOT: "-arch" "i386"{{.*}} "-syslibroot" "{{[^"]*}}/FOO"
-// Check that honor --sysroot= over -isysroot, for Apple Darwin.
+// Check that honor -isysroot over --sysroot=, for Apple Darwin.
// RUN: touch %t3.o
// RUN: %clang -target i386-apple-darwin10 \
// RUN: -isysroot /FOO --sysroot=/BAR -### %t3.o 2> %t3
// RUN: FileCheck --check-prefix=CHECK-APPLE-SYSROOT < %t3 %s
-// CHECK-APPLE-SYSROOT: "-arch" "i386"{{.*}} "-syslibroot" "{{[^"]*}}/BAR"
+// CHECK-APPLE-SYSROOT: "-arch" "i386"{{.*}} "-syslibroot" "{{[^"]*}}/FOO"
|
Ping? |
This is the intended/expected behavior. On every other platform, Except, on Darwin platforms, for some reason that's probably lost to history, the However, notably, even on Darwin, if you pass both flags, you still get the intended behavior of |
What's the use-case for this? Because for most users, you almost certainly don't want to mix headers and libraries of different versions, and keeping the existing behavior is essentially a big footgun when trying to build a useful toolchain on Darwin. At Homebrew, we used to configure the toolchain with It seems to me that getting rid of a footgun that can impact many users (our toolchain is installed a few hundred thousand times a year, see also the downstream bug report I initially linked) might be worth breaking what currently seems to be a niche use-case of using headers and libraries of different versions. If you really wanted to do this, you can set Of course, I'm open to alternatives that don't involve breaking anyone. (I'm similarly also open to being persuaded that mixing headers and libraries of different versions is actually not as niche as I make it out to be.) Footnotes
|
(I am not familiar with macOS and I'll defer the decision to Apple folks. You probably want a different way to handle default sysroot in homebrew. |
The expected flow on Apple platforms is to only pass an |
Hm, I had a thought which might meet the needs here, and not add additional confusion: we could have the Darwin SDKROOT environment variable override both |
I think the correct solution for Homebrew is to never use |
Independently of the rationale for the current behaviour, I would be extremely concerned about potential breakage from this behavior change (more due to hyrum's law that intentional choices) |
The main concern is it happens when setting Ultimately most downstream users don't really expect the need to use |
I don't think it's deprecated in the sense we have plans to drop support for it. The problem here seems more like when those values conflict what should be expected? Today it seems expected that |
I do agree with this and this does make sense. The problem has mostly come from two expectations:
However those two points are incompatible. And This probably crosses into #38193, but to paint the wider picture since I agree the problem here is more complicated than a prioritisation problem, it's generally expected from various build scripts that something like this: #include <stdio.h>
int main() {
printf("Hello World!");
return 0;
} will just work out of the box without needing extra flags or to pipe it through other commands. It works out of the box on Linux and likely most other platforms but does not on LLVM Clang for macOS (but does work with Apple
|
That is flang document, which is not endorsed by platform vendor in anyway. You can use DEFAULT_SYSROOT for local builds (or package manager builds for local host only) but never for products that are built for distribution on macOS. There is no standard location for macOS SDK, and it can be anywhere you want. You have to specify All Darwin platforms use a different model from your standard linux distribution and all SDKs are backwards compatible to older OS versions, which allows you to build software for older OS on latest version. The compilation model is closer to a cross-compilation in linux world. The fundamental problem you hit is that you pass a secret |
To be clear: the build I talked about there required the CLT where the SDK location is standardised, so this isn't entirely true for the use case I had. But yes a general LLVM distribution that worked with Xcode.app-only installs would require something more like The major-fixed
This is mostly true but there are notable exceptions such as:
And we have seen these break real world applications, even high profile ones like
This doesn't really change anything here though. Distributing
Migrating to config files does give us the option to use But yes: I agree avoiding |
Ah, I see. I think Maybe instead, the logic in the patch should be setting
I don't think DEFAULT_SYSROOT is a common setting, and if it is set, users are probably expect to overwrite using |
That proposal makes sense to me. |
d35918e
to
e257a79
Compare
-isysroot
over --sysroot
consistentlyDEFAULT_SYSROOT
Thanks, pushed a change that I think should implement this. Haven't gotten around to testing it yet, I will this afternoon. |
Maybe this has already been answered, but just in case. You get conflicting SDKs when:
In each of the four possible scenarios above (two possibilities in each bullet point), you end up with
This would help too, actually. I'll look into this. Thanks.
I'm looking at this in Homebrew/homebrew-core#200047. However, based on this comment, it doesn't quite solve the downstream bug report entirely. But that's something I'll have to look into. Us switching to using |
Current patch breaks test:
I think that just a pure hack in the test case, we can just update the test. @cyndyishida what do you think about the current approach? |
We could probably do something like diff --git a/clang/lib/Driver/ToolChains/Darwin.cpp b/clang/lib/Driver/ToolChains/Darwin.cpp
index cdb6d21a0148..f3769ffcb4ae 100644
--- a/clang/lib/Driver/ToolChains/Darwin.cpp
+++ b/clang/lib/Driver/ToolChains/Darwin.cpp
@@ -432,7 +432,7 @@ void darwin::Linker::AddLinkArgs(Compilation &C, const ArgList &Args,
// --isysroot as the syslibroot.
// We check `OPT__sysroot_EQ` directly instead of `getSysRoot` to make sure we
// prioritise command line arguments over configuration of `DEFAULT_SYSROOT`.
- if (const Arg *A = Args.getLastArg(options::OPT__sysroot_EQ)) {
+ if (const Arg *A = Args.getLastArg(options::OPT__sysroot_EQ); A && strcmp(A->getValue(), "")) {
CmdArgs.push_back("-syslibroot");
CmdArgs.push_back(A->getValue());
} else if (const Arg *A = Args.getLastArg(options::OPT_isysroot)) { to retain the current behaviour and avoid breaking the test. Not sure it's worth going to the trouble though. |
SGTM too. I don't see wide usage of it on my end. It looks like that part of test itself was added to workaround a toolchain build configuration issue, where Changing the test itself (to e.g. locally set that env var) is a good regression test that should be added for this pr anyway. |
…YSROOT` If a toolchain is configured with `DEFAULT_SYSROOT`, then this could result in an unintended value for `-syslibroot` being passed to the linker if the user manually sets `-isysroot` or `SDKROOT`. Let's fix this by prioritising command line flags when determining `-syslibroot` before checking `getSysRoot`. Downstream bug report: Homebrew/homebrew-core#197277 Co-authored-by: Bo Anderson <[email protected]>
e257a79
to
b072008
Compare
Dropped the
Sorry, which env var are you referring to? |
I'm suggesting an explicit test case that checks that |
llvm-project/clang/CMakeLists.txt Lines 208 to 209 in aac000a
llvm-project/clang/include/clang/Config/config.h.cmake Lines 48 to 49 in aac000a
I don't think |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
The PR description (first comment), which will be used as the default commit message, should be fixed. |
Thanks for the reminder, fixed. I plan to do a few more local tests with |
Tested locally; seems to be working as intended. |
If a toolchain is configured with
DEFAULT_SYSROOT
, then this couldresult in an unintended value for
-syslibroot
being passed to thelinker if the user manually sets
-isysroot
orSDKROOT
.Let's fix this by prioritising command line flags when determining
-syslibroot
before checkinggetSysRoot
.Downstream bug report: Homebrew/homebrew-core#197277
Co-authored-by: Bo Anderson [email protected]