[clang][Darwin] Remove legacy framework search path logic in the frontend #120149
+75
−69
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
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-driver 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. 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 #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 #75638 Full diff: https://github.com/llvm/llvm-project/pull/120149.diff 8 Files Affected:
diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td
index 88862ae9edb29d..8692d5d7eabe1c 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -8265,6 +8265,11 @@ def internal_externc_isystem : Separate<["-"], "internal-externc-isystem">,
"implicit extern \"C\" semantics; these are assumed to not be "
"user-provided and are used to model system and standard headers' "
"paths.">;
+def internal_iframework : Separate<["-"], "internal-iframework">,
+ MetaVarName<"<directory>">,
+ HelpText<"Add directory to the internal system framework include search path; these "
+ "are assumed to not be user-provided and are used to model system "
+ "and standard frameworks' paths.">;
} // let Visibility = [CC1Option]
diff --git a/clang/lib/Driver/Job.cpp b/clang/lib/Driver/Job.cpp
index ae2f1cd1f56c99..07d2d371c5626b 100644
--- a/clang/lib/Driver/Job.cpp
+++ b/clang/lib/Driver/Job.cpp
@@ -73,7 +73,7 @@ static bool skipArgs(const char *Flag, bool HaveCrashVFS, int &SkipNum,
.Cases("-internal-externc-isystem", "-iprefix", true)
.Cases("-iwithprefixbefore", "-isystem", "-iquote", true)
.Cases("-isysroot", "-I", "-F", "-resource-dir", true)
- .Cases("-iframework", "-include-pch", true)
+ .Cases("-internal-iframework", "-iframework", "-include-pch", true)
.Default(false);
if (IsInclude)
return !HaveCrashVFS;
diff --git a/clang/lib/Driver/ToolChains/Darwin.cpp b/clang/lib/Driver/ToolChains/Darwin.cpp
index cdb6d21a0148b6..76d6244daff920 100644
--- a/clang/lib/Driver/ToolChains/Darwin.cpp
+++ b/clang/lib/Driver/ToolChains/Darwin.cpp
@@ -800,9 +800,15 @@ 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>/System/Library/SubFrameworks
+ // -F<sysroot>/Library/Frameworks
{
bool NonStandardSearchPath = false;
const auto &Triple = getToolChain().getTriple();
@@ -813,18 +819,23 @@ 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", "/System/Library/SubFrameworks");
+ AddSearchPath("-F", "/Library/Frameworks");
}
}
}
@@ -2539,6 +2550,18 @@ void DarwinClang::AddClangSystemIncludeArgs(const llvm::opt::ArgList &DriverArgs
llvm::sys::path::append(P, "usr", "include");
addExternCSystemInclude(DriverArgs, CC1Args, P.str());
}
+
+ // Add default framework search paths
+ auto addFrameworkInclude = [&](auto ...Path) {
+ SmallString<128> P(Sysroot);
+ llvm::sys::path::append(P, Path...);
+
+ CC1Args.push_back("-internal-iframework");
+ CC1Args.push_back(DriverArgs.MakeArgString(P));
+ };
+ addFrameworkInclude("System", "Library", "Frameworks");
+ addFrameworkInclude("System", "Library", "SubFrameworks");
+ addFrameworkInclude("Library", "Frameworks");
}
bool DarwinClang::AddGnuCPlusPlusIncludePaths(const llvm::opt::ArgList &DriverArgs,
diff --git a/clang/lib/Frontend/CompilerInvocation.cpp b/clang/lib/Frontend/CompilerInvocation.cpp
index 23906d5c06d380..4713fea5a26378 100644
--- a/clang/lib/Frontend/CompilerInvocation.cpp
+++ b/clang/lib/Frontend/CompilerInvocation.cpp
@@ -3413,11 +3413,14 @@ static bool ParseHeaderSearchArgs(HeaderSearchOptions &Opts, ArgList &Args,
// Add the internal paths from a driver that detects standard include paths.
for (const auto *A :
- Args.filtered(OPT_internal_isystem, OPT_internal_externc_isystem)) {
+ Args.filtered(OPT_internal_isystem, OPT_internal_externc_isystem, OPT_internal_iframework)) {
frontend::IncludeDirGroup Group = frontend::System;
+ bool IsFramework = false;
if (A->getOption().matches(OPT_internal_externc_isystem))
Group = frontend::ExternCSystem;
- Opts.AddPath(A->getValue(), Group, false, true);
+ if (A->getOption().matches(OPT_internal_iframework))
+ IsFramework = true;
+ Opts.AddPath(A->getValue(), Group, IsFramework, true);
}
// Add the path prefixes which are implicitly treated as being system headers.
diff --git a/clang/lib/Lex/InitHeaderSearch.cpp b/clang/lib/Lex/InitHeaderSearch.cpp
index ea02f5dfb62644..76bc23a9054960 100644
--- a/clang/lib/Lex/InitHeaderSearch.cpp
+++ b/clang/lib/Lex/InitHeaderSearch.cpp
@@ -320,6 +320,9 @@ bool InitHeaderSearch::ShouldAddDefaultIncludePaths(
break;
}
+ if (triple.isOSDarwin())
+ return false;
+
return true; // Everything else uses AddDefaultIncludePaths().
}
@@ -334,22 +337,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("/System/Library/SubFrameworks", 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/darwin-framework-search-paths.c b/clang/test/Driver/darwin-framework-search-paths.c
new file mode 100644
index 00000000000000..4bfe269f8d903d
--- /dev/null
+++ b/clang/test/Driver/darwin-framework-search-paths.c
@@ -0,0 +1,26 @@
+// UNSUPPORTED: system-windows
+// Windows is unsupported because we use the Unix path separator `/` in the test.
+
+// Add default directories before running clang to check default
+// search paths.
+// RUN: rm -rf %t && mkdir -p %t
+// RUN: cp -R %S/Inputs/MacOSX15.1.sdk %t/
+// RUN: mkdir -p %t/MacOSX15.1.sdk/System/Library/Frameworks
+// RUN: mkdir -p %t/MacOSX15.1.sdk/System/Library/SubFrameworks
+// RUN: mkdir -p %t/MacOSX15.1.sdk/usr/include
+
+// RUN: %clang -xc %s -target arm64-apple-darwin13.0 -isysroot %t/MacOSX15.1.sdk -E -v 2>&1 | FileCheck --check-prefix=CHECK-C %s
+//
+// CHECK-C: -isysroot [[PATH:[^ ]*/MacOSX15.1.sdk]]
+// CHECK-C: #include <...> search starts here:
+// CHECK-C: [[PATH]]/usr/include
+// CHECK-C: [[PATH]]/System/Library/Frameworks (framework directory)
+// CHECK-C: [[PATH]]/System/Library/SubFrameworks (framework directory)
+
+// RUN: %clang -xc++ %s -target arm64-apple-darwin13.0 -isysroot %t/MacOSX15.1.sdk -E -v 2>&1 | FileCheck --check-prefix=CHECK-CXX %s
+//
+// CHECK-CXX: -isysroot [[PATH:[^ ]*/MacOSX15.1.sdk]]
+// CHECK-CXX: #include <...> search starts here:
+// CHECK-CXX: [[PATH]]/usr/include
+// CHECK-CXX: [[PATH]]/System/Library/Frameworks (framework directory)
+// CHECK-CXX: [[PATH]]/System/Library/SubFrameworks (framework directory)
diff --git a/clang/test/Driver/darwin-subframeworks.c b/clang/test/Driver/darwin-subframeworks.c
deleted file mode 100644
index 1a7a095c642922..00000000000000
--- a/clang/test/Driver/darwin-subframeworks.c
+++ /dev/null
@@ -1,18 +0,0 @@
-// UNSUPPORTED: system-windows
-// Windows is unsupported because we use the Unix path separator `\`.
-
-// Add default directories before running clang to check default
-// search paths.
-// RUN: rm -rf %t && mkdir -p %t
-// RUN: cp -R %S/Inputs/MacOSX15.1.sdk %t/
-// RUN: mkdir -p %t/MacOSX15.1.sdk/System/Library/Frameworks
-// RUN: mkdir -p %t/MacOSX15.1.sdk/System/Library/SubFrameworks
-// RUN: mkdir -p %t/MacOSX15.1.sdk/usr/include
-
-// RUN: %clang %s -target arm64-apple-darwin13.0 -isysroot %t/MacOSX15.1.sdk -E -v 2>&1 | FileCheck %s
-
-// CHECK: -isysroot [[PATH:[^ ]*/MacOSX15.1.sdk]]
-// CHECK: #include <...> search starts here:
-// CHECK: [[PATH]]/usr/include
-// CHECK: [[PATH]]/System/Library/Frameworks (framework directory)
-// CHECK: [[PATH]]/System/Library/SubFrameworks (framework directory)
diff --git a/clang/test/Preprocessor/cuda-macos-includes.cu b/clang/test/Preprocessor/cuda-macos-includes.cu
index 6ef94b0e453520..dbc991dd610882 100644
--- a/clang/test/Preprocessor/cuda-macos-includes.cu
+++ b/clang/test/Preprocessor/cuda-macos-includes.cu
@@ -1,13 +1,6 @@
-// RUN: %clang -cc1 -fcuda-is-device -isysroot /var/empty \
-// RUN: -triple nvptx-nvidia-cuda -aux-triple i386-apple-macosx \
-// RUN: -E -fcuda-is-device -v -o /dev/null -x cuda %s 2>&1 | FileCheck %s
-
-// RUN: %clang -cc1 -isysroot /var/empty \
-// RUN: -triple i386-apple-macosx -aux-triple nvptx-nvidia-cuda \
-// RUN: -E -fcuda-is-device -v -o /dev/null -x cuda %s 2>&1 | FileCheck %s
-
// Check that when we do CUDA host and device compiles on MacOS, we check for
// includes in /System/Library/Frameworks and /Library/Frameworks.
-// CHECK-DAG: ignoring nonexistent directory "/var/empty/System/Library/Frameworks"
-// CHECK-DAG: ignoring nonexistent directory "/var/empty/Library/Frameworks"
+// RUN: %clang -isysroot /var/empty -target unknown-nvidia-cuda -v -fsyntax-only -x cuda %s -### 2>&1 | FileCheck %s
+// CHECK-DAG: "-internal-iframework" "/var/empty/System/Library/Frameworks"
+// CHECK-DAG: "-internal-iframework" "/var/empty/Library/Frameworks"
|
|
@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. 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 #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 #75638 Full diff: https://github.com/llvm/llvm-project/pull/120149.diff 8 Files Affected:
diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td
index 88862ae9edb29d..8692d5d7eabe1c 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -8265,6 +8265,11 @@ def internal_externc_isystem : Separate<["-"], "internal-externc-isystem">,
"implicit extern \"C\" semantics; these are assumed to not be "
"user-provided and are used to model system and standard headers' "
"paths.">;
+def internal_iframework : Separate<["-"], "internal-iframework">,
+ MetaVarName<"<directory>">,
+ HelpText<"Add directory to the internal system framework include search path; these "
+ "are assumed to not be user-provided and are used to model system "
+ "and standard frameworks' paths.">;
} // let Visibility = [CC1Option]
diff --git a/clang/lib/Driver/Job.cpp b/clang/lib/Driver/Job.cpp
index ae2f1cd1f56c99..07d2d371c5626b 100644
--- a/clang/lib/Driver/Job.cpp
+++ b/clang/lib/Driver/Job.cpp
@@ -73,7 +73,7 @@ static bool skipArgs(const char *Flag, bool HaveCrashVFS, int &SkipNum,
.Cases("-internal-externc-isystem", "-iprefix", true)
.Cases("-iwithprefixbefore", "-isystem", "-iquote", true)
.Cases("-isysroot", "-I", "-F", "-resource-dir", true)
- .Cases("-iframework", "-include-pch", true)
+ .Cases("-internal-iframework", "-iframework", "-include-pch", true)
.Default(false);
if (IsInclude)
return !HaveCrashVFS;
diff --git a/clang/lib/Driver/ToolChains/Darwin.cpp b/clang/lib/Driver/ToolChains/Darwin.cpp
index cdb6d21a0148b6..76d6244daff920 100644
--- a/clang/lib/Driver/ToolChains/Darwin.cpp
+++ b/clang/lib/Driver/ToolChains/Darwin.cpp
@@ -800,9 +800,15 @@ 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>/System/Library/SubFrameworks
+ // -F<sysroot>/Library/Frameworks
{
bool NonStandardSearchPath = false;
const auto &Triple = getToolChain().getTriple();
@@ -813,18 +819,23 @@ 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", "/System/Library/SubFrameworks");
+ AddSearchPath("-F", "/Library/Frameworks");
}
}
}
@@ -2539,6 +2550,18 @@ void DarwinClang::AddClangSystemIncludeArgs(const llvm::opt::ArgList &DriverArgs
llvm::sys::path::append(P, "usr", "include");
addExternCSystemInclude(DriverArgs, CC1Args, P.str());
}
+
+ // Add default framework search paths
+ auto addFrameworkInclude = [&](auto ...Path) {
+ SmallString<128> P(Sysroot);
+ llvm::sys::path::append(P, Path...);
+
+ CC1Args.push_back("-internal-iframework");
+ CC1Args.push_back(DriverArgs.MakeArgString(P));
+ };
+ addFrameworkInclude("System", "Library", "Frameworks");
+ addFrameworkInclude("System", "Library", "SubFrameworks");
+ addFrameworkInclude("Library", "Frameworks");
}
bool DarwinClang::AddGnuCPlusPlusIncludePaths(const llvm::opt::ArgList &DriverArgs,
diff --git a/clang/lib/Frontend/CompilerInvocation.cpp b/clang/lib/Frontend/CompilerInvocation.cpp
index 23906d5c06d380..4713fea5a26378 100644
--- a/clang/lib/Frontend/CompilerInvocation.cpp
+++ b/clang/lib/Frontend/CompilerInvocation.cpp
@@ -3413,11 +3413,14 @@ static bool ParseHeaderSearchArgs(HeaderSearchOptions &Opts, ArgList &Args,
// Add the internal paths from a driver that detects standard include paths.
for (const auto *A :
- Args.filtered(OPT_internal_isystem, OPT_internal_externc_isystem)) {
+ Args.filtered(OPT_internal_isystem, OPT_internal_externc_isystem, OPT_internal_iframework)) {
frontend::IncludeDirGroup Group = frontend::System;
+ bool IsFramework = false;
if (A->getOption().matches(OPT_internal_externc_isystem))
Group = frontend::ExternCSystem;
- Opts.AddPath(A->getValue(), Group, false, true);
+ if (A->getOption().matches(OPT_internal_iframework))
+ IsFramework = true;
+ Opts.AddPath(A->getValue(), Group, IsFramework, true);
}
// Add the path prefixes which are implicitly treated as being system headers.
diff --git a/clang/lib/Lex/InitHeaderSearch.cpp b/clang/lib/Lex/InitHeaderSearch.cpp
index ea02f5dfb62644..76bc23a9054960 100644
--- a/clang/lib/Lex/InitHeaderSearch.cpp
+++ b/clang/lib/Lex/InitHeaderSearch.cpp
@@ -320,6 +320,9 @@ bool InitHeaderSearch::ShouldAddDefaultIncludePaths(
break;
}
+ if (triple.isOSDarwin())
+ return false;
+
return true; // Everything else uses AddDefaultIncludePaths().
}
@@ -334,22 +337,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("/System/Library/SubFrameworks", 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/darwin-framework-search-paths.c b/clang/test/Driver/darwin-framework-search-paths.c
new file mode 100644
index 00000000000000..4bfe269f8d903d
--- /dev/null
+++ b/clang/test/Driver/darwin-framework-search-paths.c
@@ -0,0 +1,26 @@
+// UNSUPPORTED: system-windows
+// Windows is unsupported because we use the Unix path separator `/` in the test.
+
+// Add default directories before running clang to check default
+// search paths.
+// RUN: rm -rf %t && mkdir -p %t
+// RUN: cp -R %S/Inputs/MacOSX15.1.sdk %t/
+// RUN: mkdir -p %t/MacOSX15.1.sdk/System/Library/Frameworks
+// RUN: mkdir -p %t/MacOSX15.1.sdk/System/Library/SubFrameworks
+// RUN: mkdir -p %t/MacOSX15.1.sdk/usr/include
+
+// RUN: %clang -xc %s -target arm64-apple-darwin13.0 -isysroot %t/MacOSX15.1.sdk -E -v 2>&1 | FileCheck --check-prefix=CHECK-C %s
+//
+// CHECK-C: -isysroot [[PATH:[^ ]*/MacOSX15.1.sdk]]
+// CHECK-C: #include <...> search starts here:
+// CHECK-C: [[PATH]]/usr/include
+// CHECK-C: [[PATH]]/System/Library/Frameworks (framework directory)
+// CHECK-C: [[PATH]]/System/Library/SubFrameworks (framework directory)
+
+// RUN: %clang -xc++ %s -target arm64-apple-darwin13.0 -isysroot %t/MacOSX15.1.sdk -E -v 2>&1 | FileCheck --check-prefix=CHECK-CXX %s
+//
+// CHECK-CXX: -isysroot [[PATH:[^ ]*/MacOSX15.1.sdk]]
+// CHECK-CXX: #include <...> search starts here:
+// CHECK-CXX: [[PATH]]/usr/include
+// CHECK-CXX: [[PATH]]/System/Library/Frameworks (framework directory)
+// CHECK-CXX: [[PATH]]/System/Library/SubFrameworks (framework directory)
diff --git a/clang/test/Driver/darwin-subframeworks.c b/clang/test/Driver/darwin-subframeworks.c
deleted file mode 100644
index 1a7a095c642922..00000000000000
--- a/clang/test/Driver/darwin-subframeworks.c
+++ /dev/null
@@ -1,18 +0,0 @@
-// UNSUPPORTED: system-windows
-// Windows is unsupported because we use the Unix path separator `\`.
-
-// Add default directories before running clang to check default
-// search paths.
-// RUN: rm -rf %t && mkdir -p %t
-// RUN: cp -R %S/Inputs/MacOSX15.1.sdk %t/
-// RUN: mkdir -p %t/MacOSX15.1.sdk/System/Library/Frameworks
-// RUN: mkdir -p %t/MacOSX15.1.sdk/System/Library/SubFrameworks
-// RUN: mkdir -p %t/MacOSX15.1.sdk/usr/include
-
-// RUN: %clang %s -target arm64-apple-darwin13.0 -isysroot %t/MacOSX15.1.sdk -E -v 2>&1 | FileCheck %s
-
-// CHECK: -isysroot [[PATH:[^ ]*/MacOSX15.1.sdk]]
-// CHECK: #include <...> search starts here:
-// CHECK: [[PATH]]/usr/include
-// CHECK: [[PATH]]/System/Library/Frameworks (framework directory)
-// CHECK: [[PATH]]/System/Library/SubFrameworks (framework directory)
diff --git a/clang/test/Preprocessor/cuda-macos-includes.cu b/clang/test/Preprocessor/cuda-macos-includes.cu
index 6ef94b0e453520..dbc991dd610882 100644
--- a/clang/test/Preprocessor/cuda-macos-includes.cu
+++ b/clang/test/Preprocessor/cuda-macos-includes.cu
@@ -1,13 +1,6 @@
-// RUN: %clang -cc1 -fcuda-is-device -isysroot /var/empty \
-// RUN: -triple nvptx-nvidia-cuda -aux-triple i386-apple-macosx \
-// RUN: -E -fcuda-is-device -v -o /dev/null -x cuda %s 2>&1 | FileCheck %s
-
-// RUN: %clang -cc1 -isysroot /var/empty \
-// RUN: -triple i386-apple-macosx -aux-triple nvptx-nvidia-cuda \
-// RUN: -E -fcuda-is-device -v -o /dev/null -x cuda %s 2>&1 | FileCheck %s
-
// Check that when we do CUDA host and device compiles on MacOS, we check for
// includes in /System/Library/Frameworks and /Library/Frameworks.
-// CHECK-DAG: ignoring nonexistent directory "/var/empty/System/Library/Frameworks"
-// CHECK-DAG: ignoring nonexistent directory "/var/empty/Library/Frameworks"
+// RUN: %clang -isysroot /var/empty -target unknown-nvidia-cuda -v -fsyntax-only -x cuda %s -### 2>&1 | FileCheck %s
+// CHECK-DAG: "-internal-iframework" "/var/empty/System/Library/Frameworks"
+// CHECK-DAG: "-internal-iframework" "/var/empty/Library/Frameworks"
|
This was referenced Dec 16, 2024
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
ldionne
commented
Dec 16, 2024
ian-twilightcoder
approved these changes
Dec 17, 2024
cyndyishida
reviewed
Dec 17, 2024
cachemeifyoucan
approved these changes
Dec 18, 2024
| @@ -8265,6 +8265,11 @@ def internal_externc_isystem : Separate<["-"], "internal-externc-isystem">, | |||
| "implicit extern \"C\" semantics; these are assumed to not be " | |||
| "user-provided and are used to model system and standard headers' " | |||
| "paths.">; | |||
| def internal_iframework : Separate<["-"], "internal-iframework">, | |||
There was a problem hiding this comment.
Should we call this internal-externc-iframework since it more closely corresponds to internal-externc-isystem than internal-isystem? Especially its position in the search ordering system -> internal-system -> internal-externc-system
There was a problem hiding this comment.
Is it really closer to internal-externc-isystem than internal-isystem though? My understanding is that all of these flags are simply taken (by CC1) in the order in which they appear in the command-line. Is that not what's happening?
There was a problem hiding this comment.
No, they get bucketed. e.g. internal-externc-isystem always comes after -isystem no matter which order the flags are on the command line.
There was a problem hiding this comment.
I see. But there is no notion of "extern C" for framework includes, so IMO it makes more sense to make -internal-iframework "equivalent" to -internal-isystem than to -internal-externc-isystem, would you agree?
More generally, I guess I am confused about the desired grouping for framework includes. Do we want them to be IncludeDirGroup::System, IncludeDirGroup::ExternCSystem or something else? IIUC, -iframework is already part of IncludeDirGroup::System. If that's correct, then why is it not sufficient to pass these system framework paths using -iframework directly in the first place (i.e. why do we even need to introduce -internal-iframework in the first place)?
There was a problem hiding this comment.
(As a test to make sure I'm not crazy, I just committed e624fd8 which removes -internal-iframework - I expect that won't work as intended)
There was a problem hiding this comment.
Sorry for the naive question, but where/how is that order determined?
There was a problem hiding this comment.
That one I don't know, I've only empirically observed by taking a look at -v output.
There was a problem hiding this comment.
I think it's handled here? https://github.com/llvm/llvm-project/blob/main/clang/lib/Frontend/CompilerInvocation.cpp#L3327
There was a problem hiding this comment.
But it's not as straightforward as the paths being added in the particular order, the group matters too. But only sometimes - After goes last, but ExternCSystem and System get mixed in together.
There was a problem hiding this comment.
So I guess maybe it doesn't matter and internal-iframework is fine, we just need ParseHeaderSearchArgs to put it where we want?
|
@ldionne Ping. |
…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
force-pushed
the
review/clang-move-search-paths-to-driver
branch
from
21a85df to
2f01df2
Compare
cyndyishida
approved these changes
Jan 7, 2025
…uld swear this totally didn't work previously
|
Ping. |
|
The next release is a few days away from branching. Is this ready or does this still need more work? |
I think we need to restore |
|
Unfortunately I don't have bandwidth to take this through the finish line. I just spoke with @ian-twilightcoder who said he'd try to pick it up soon (thanks a lot!). |
I'm not a maintainer so I can't update this PR. I ended up needing to tweak most of it, especially where it conflicted with #120507. New PR is #138234 |
|
@ldionne Feel free to reopen, but I will close that to avoid duplicates. |
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.
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 #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 #75638