[clang][Darwin] Remove legacy framework search path logic in the frontend #75841
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…tend This removes a long standing piece of technical debt. Most other platforms have moved all their header search path logic to the driver, but Darwin still had some logic for setting framework search paths present in the frontend. This patch moves that logic to the driver alongside existing logic that already handles part of these search paths. This is intended to be a pure refactor without any functional change visible to users, since the search paths before and after should be the same, and in the same order. The change in the tests is necessary because we would previously add the DriverKit framework search path in the frontend regardless of whether we actually need to, which we now handle correctly because the driver checks for ld64-605.1+. Fixes llvm#75638
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-driver @llvm/pr-subscribers-clang Author: Louis Dionne (ldionne) ChangesThis removes a long standing piece of technical debt. Most other platforms have moved all their header search path logic to the driver, but Darwin still had some logic for setting framework search paths present in the frontend. This patch moves that logic to the driver alongside existing logic that already handles part of these search paths. This is intended to be a pure refactor without any functional change visible to users, since the search paths before and after should be the same, and in the same order. The change in the tests is necessary because we would previously add the DriverKit framework search path in the frontend regardless of whether we actually need to, which we now handle correctly because the driver checks for ld64-605.1+. Fixes #75638 Full diff: https://github.com/llvm/llvm-project/pull/75841.diff 3 Files Affected:
diff --git a/clang/lib/Driver/ToolChains/Darwin.cpp b/clang/lib/Driver/ToolChains/Darwin.cpp
index 65846cace461e3..f76a42d2d8e7e3 100644
--- a/clang/lib/Driver/ToolChains/Darwin.cpp
+++ b/clang/lib/Driver/ToolChains/Darwin.cpp
@@ -758,9 +758,14 @@ void darwin::Linker::ConstructJob(Compilation &C, const JobAction &JA,
}
}
- // Add non-standard, platform-specific search paths, e.g., for DriverKit:
- // -L<sysroot>/System/DriverKit/usr/lib
- // -F<sysroot>/System/DriverKit/System/Library/Framework
+ // Add framework include paths and library search paths.
+ // There are two flavors:
+ // 1. The "non-standard" paths, e.g. for DriverKit:
+ // -L<sysroot>/System/DriverKit/usr/lib
+ // -F<sysroot>/System/DriverKit/System/Library/Frameworks
+ // 2. The "standard" paths, e.g. for macOS and iOS:
+ // -F<sysroot>/System/Library/Frameworks
+ // -F<sysroot>/Library/Frameworks
{
bool NonStandardSearchPath = false;
const auto &Triple = getToolChain().getTriple();
@@ -771,18 +776,22 @@ void darwin::Linker::ConstructJob(Compilation &C, const JobAction &JA,
(Version.getMajor() == 605 && Version.getMinor().value_or(0) < 1);
}
- if (NonStandardSearchPath) {
- if (auto *Sysroot = Args.getLastArg(options::OPT_isysroot)) {
- auto AddSearchPath = [&](StringRef Flag, StringRef SearchPath) {
- SmallString<128> P(Sysroot->getValue());
- AppendPlatformPrefix(P, Triple);
- llvm::sys::path::append(P, SearchPath);
- if (getToolChain().getVFS().exists(P)) {
- CmdArgs.push_back(Args.MakeArgString(Flag + P));
- }
- };
+ if (auto *Sysroot = Args.getLastArg(options::OPT_isysroot)) {
+ auto AddSearchPath = [&](StringRef Flag, StringRef SearchPath) {
+ SmallString<128> P(Sysroot->getValue());
+ AppendPlatformPrefix(P, Triple);
+ llvm::sys::path::append(P, SearchPath);
+ if (getToolChain().getVFS().exists(P)) {
+ CmdArgs.push_back(Args.MakeArgString(Flag + P));
+ }
+ };
+
+ if (NonStandardSearchPath) {
AddSearchPath("-L", "/usr/lib");
AddSearchPath("-F", "/System/Library/Frameworks");
+ } else if (!Triple.isDriverKit()) {
+ AddSearchPath("-F", "/System/Library/Frameworks");
+ AddSearchPath("-F", "/Library/Frameworks");
}
}
}
diff --git a/clang/lib/Lex/InitHeaderSearch.cpp b/clang/lib/Lex/InitHeaderSearch.cpp
index 2218db15013d92..1350fa5f01a578 100644
--- a/clang/lib/Lex/InitHeaderSearch.cpp
+++ b/clang/lib/Lex/InitHeaderSearch.cpp
@@ -324,6 +324,9 @@ bool InitHeaderSearch::ShouldAddDefaultIncludePaths(
break;
}
+ if (triple.isOSDarwin())
+ return false;
+
return true; // Everything else uses AddDefaultIncludePaths().
}
@@ -338,21 +341,6 @@ void InitHeaderSearch::AddDefaultIncludePaths(
if (!ShouldAddDefaultIncludePaths(triple))
return;
- // NOTE: some additional header search logic is handled in the driver for
- // Darwin.
- if (triple.isOSDarwin()) {
- if (HSOpts.UseStandardSystemIncludes) {
- // Add the default framework include paths on Darwin.
- if (triple.isDriverKit()) {
- AddPath("/System/DriverKit/System/Library/Frameworks", System, true);
- } else {
- AddPath("/System/Library/Frameworks", System, true);
- AddPath("/Library/Frameworks", System, true);
- }
- }
- return;
- }
-
if (Lang.CPlusPlus && !Lang.AsmPreprocessor &&
HSOpts.UseStandardCXXIncludes && HSOpts.UseStandardSystemIncludes) {
if (HSOpts.UseLibcxx) {
diff --git a/clang/test/Driver/driverkit-path.c b/clang/test/Driver/driverkit-path.c
index 9699b9c01f4e81..43e5aa40fc6f31 100644
--- a/clang/test/Driver/driverkit-path.c
+++ b/clang/test/Driver/driverkit-path.c
@@ -31,4 +31,3 @@ int main() { return 0; }
// INC: [[PATH]]/System/DriverKit/usr/local/include
// INC: /lib{{(64)?}}/clang/{{[^/ ]+}}/include
// INC: [[PATH]]/System/DriverKit/usr/include
-// INC: [[PATH]]/System/DriverKit/System/Library/Frameworks (framework directory)
|
ldionne
commented
Dec 18, 2023
| // -F<sysroot>/System/DriverKit/System/Library/Frameworks | ||
| // 2. The "standard" paths, e.g. for macOS and iOS: | ||
| // -F<sysroot>/System/Library/Frameworks | ||
| // -F<sysroot>/Library/Frameworks | ||
| { | ||
| bool NonStandardSearchPath = false; |
There was a problem hiding this comment.
@yln Note that I think this whole block could almost go away now. I think we always do the right thing in the linker on Apple platforms, so I don't think the driver needs any special logic for -F and -L anymore, but I could be wrong. I also wanted to keep this change minimal.
There was a problem hiding this comment.
Do you mean that ld64-605.1 is old enough / has "aged out" by now so we can drop this special case?
There was a problem hiding this comment.
Yes, that is what I suspect.
There was a problem hiding this comment.
Got it, that would be another good cleanup! I recommend doing this as a separate PR.
|
@jlebar The CI seems to be failing due to a test you were the last to touch: Long story short, that test was checking for the diagnostic output from the Frontend complaining that we can't find This shouldn't be a problem if the normal linker is being used when linking object files produced for cuda targets, since the linker on Apple platforms nowadays knows about these search paths, and Clang doesn't really need to specify them explicitly. If that's the case, the test could simply be removed. But I don't know much about cuda, so I'd rather ask in case that's not the case. |
…inker on Apple platforms already handles these paths
yln
approved these changes
Dec 18, 2023
| // -F<sysroot>/System/DriverKit/System/Library/Frameworks | ||
| // 2. The "standard" paths, e.g. for macOS and iOS: | ||
| // -F<sysroot>/System/Library/Frameworks | ||
| // -F<sysroot>/Library/Frameworks | ||
| { | ||
| bool NonStandardSearchPath = false; |
There was a problem hiding this comment.
Got it, that would be another good cleanup! I recommend doing this as a separate PR.
|
Update? |
|
I think this can be merged. I would have liked to have @jlebar 's input on interactions with Cuda, but I think this is probably good enough. |
|
Thank you for the effort. |
|
This appears to have broken the LSan build which is now failing with the following error: |
|
same in our stage 2 build if this is intended to be NFC but isn't, then we should probably revert |
|
I've reverted this, hopefully the command line in the failing invocation I gave is enough to repro the regression, otherwise lmk what else I can do to help repro |
|
@aeubanks thanks for the revert! |
|
@aeubanks @petrhosek Can you folks share what was the top level invocations you did to get those errors? I am having trouble understanding the problem just from the pasted errors. |
|
is the clang invocation in the logs I posted sufficient? |
|
No, I would like to reproduce locally (and there are too many things specific to the setup in the clang command line for me to) |
|
Ping. I don't know how to fix the issue, I need some help. |
|
sorry, I keep missing notifications, will find a repro |
|
Here's what I did to repro:
#include <CoreFoundation/CoreFoundation.h>Build clang: I get the following error with this patch: System information: |
|
If we pass without this patch: with this patch: So it looks like this patch causes Clang to think that the directory |
|
Able to look at this further? |
|
Ping. |
|
Any chance of reviving this? |
|
@ldionne Ping. |
ldionne
added a commit
to ldionne/llvm-project
that referenced
this pull request
Dec 16, 2024
…tend This removes a long standing piece of technical debt. Most other platforms have moved all their header search path logic to the driver, but Darwin still had some logic for setting framework search paths present in the frontend. This patch moves that logic to the driver alongside existing logic that already handles part of these search paths. To achieve parity with the previous search path order, this patch introduces the -internal-iframework flag which is used to pass system framework paths from the driver to the frontend. These paths are handled specially in that they are added after normal framework search paths, which preserves the old frontend behavior for system frameworks. This patch is a re-application of llvm#75841 which was reverted in d34901f because it broke framework search paths. In fact, the original patch was only adding framework search paths to the linker job, but was not adding search paths for finding headers. That issue is resolved in this version of the patch, with added tests. Fixes llvm#75638
ldionne
requested review from
cachemeifyoucan,
cyndyishida and
jhuber6
and removed request for
cachemeifyoucan,
cyndyishida and
jhuber6
|
Sorry for the spurious request for review here, this is the wrong PR. |
ldionne
added a commit
to ldionne/llvm-project
that referenced
this pull request
Jan 7, 2025
…tend This removes a long standing piece of technical debt. Most other platforms have moved all their header search path logic to the driver, but Darwin still had some logic for setting framework search paths present in the frontend. This patch moves that logic to the driver alongside existing logic that already handles part of these search paths. To achieve parity with the previous search path order, this patch introduces the -internal-iframework flag which is used to pass system framework paths from the driver to the frontend. These paths are handled specially in that they are added after normal framework search paths, which preserves the old frontend behavior for system frameworks. This patch is a re-application of llvm#75841 which was reverted in d34901f because it broke framework search paths. In fact, the original patch was only adding framework search paths to the linker job, but was not adding search paths for finding headers. That issue is resolved in this version of the patch, with added tests. Fixes llvm#75638
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This removes a long standing piece of technical debt. Most other platforms have moved all their header search path logic to the driver, but Darwin still had some logic for setting framework search paths present in the frontend. This patch moves that logic to the driver alongside existing logic that already handles part of these search paths.
This is intended to be a pure refactor without any functional change visible to users, since the search paths before and after should be the same, and in the same order. The change in the tests is necessary because we would previously add the DriverKit framework search path in the frontend regardless of whether we actually need to, which we now handle correctly because the driver checks for ld64-605.1+.
Fixes #75638