Skip to content

[Clang][Darwin] Centralize framework search paths for headers & libraries. #118543

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[Clang][Darwin] Centralize framework search paths for headers & libraries. #118543

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion clang/include/clang/Basic/DarwinSDKInfo.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
//===--- DarwinSDKInfo.h - SDK Information parser for darwin ----*- C++ -*-===//
//===--- DarwinSDKInfo.h - SDK Information for darwin -----------*- C++ -*-===//
//
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information.
Expand Down Expand Up @@ -192,6 +192,17 @@ class DarwinSDKInfo {
Expected<std::optional<DarwinSDKInfo>>
parseDarwinSDKInfo(llvm::vfs::FileSystem &VFS, StringRef SDKRootPath);

/// Get the system platform prefix for the active target triple.
StringRef getSystemPrefix(const llvm::Triple &T);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel this header is used for parsing SDKSettings.json only. I don't feel like these functions are in the correct place.

If we can do the cleanup for where my other comment is, we can just limit these function to be local to Driver/Toolchains/Darwin.cpp. No other file needs to have this information.


using KnownSystemPaths = std::array<std::string, 2>;

/// Compute and get the common system search paths for header frontend and
/// library linker searching.
///
/// \param T The active target triple to determine platform specific paths.
KnownSystemPaths getCommonSystemPaths(llvm::Triple T);

} // end namespace clang

#endif // LLVM_CLANG_BASIC_DARWINSDKINFO_H
20 changes: 20 additions & 0 deletions clang/lib/Basic/DarwinSDKInfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,3 +150,23 @@ clang::parseDarwinSDKInfo(llvm::vfs::FileSystem &VFS, StringRef SDKRootPath) {
return llvm::make_error<llvm::StringError>("invalid SDKSettings.json",
llvm::inconvertibleErrorCode());
}

// For certain platforms/environments almost all resources (e.g., headers) are
// located in sub-directories, e.g., for DriverKit they live in
// <SYSROOT>/System/DriverKit/usr/include (instead of <SYSROOT>/usr/include).
StringRef clang::getSystemPrefix(const llvm::Triple &T) {
if (T.isDriverKit())
return "/System/DriverKit";
return "";
}

KnownSystemPaths clang::getCommonSystemPaths(llvm::Triple T) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like this function might be slightly misnamed, unless I don't properly understand its purpose. getCommonSystemPaths is a very general name, but in reality this seems to only return the framework paths. As currently named, this makes it seem as though we could potentially return header paths, library paths, framework paths, etc.

If this will only ever return framework paths, perhaps a name like getCommonSystemFrameworkPaths would be better?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, I was initially thinking any common paths between header & linker search which in practice is only frameworks but could be extended in the future. That feels unlikely though. I can apply your suggestion, assuming it's still relevant, after rebasing your patch.

KnownSystemPaths CommonSysPaths = {"/System/Library/Frameworks",
"/System/Library/SubFrameworks"};

const StringRef Prefix = getSystemPrefix(T);
for (std::string &SysPath : CommonSysPaths)
SysPath = (Prefix + SysPath).str();

return CommonSysPaths;
}
27 changes: 11 additions & 16 deletions clang/lib/Driver/ToolChains/Darwin.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
#include "Arch/ARM.h"
#include "CommonArgs.h"
#include "clang/Basic/AlignedAllocation.h"
#include "clang/Basic/DarwinSDKInfo.h"
#include "clang/Basic/ObjCRuntime.h"
#include "clang/Config/config.h"
#include "clang/Driver/Compilation.h"
Expand Down Expand Up @@ -564,8 +565,6 @@ static void renderRemarksOptions(const ArgList &Args, ArgStringList &CmdArgs,
}
}

static void AppendPlatformPrefix(SmallString<128> &Path, const llvm::Triple &T);

void darwin::Linker::ConstructJob(Compilation &C, const JobAction &JA,
const InputInfo &Output,
const InputInfoList &Inputs,
Expand Down Expand Up @@ -811,16 +810,22 @@ void darwin::Linker::ConstructJob(Compilation &C, const JobAction &JA,

if (NonStandardSearchPath) {
if (auto *Sysroot = Args.getLastArg(options::OPT_isysroot)) {
auto AddSearchPath = [&](StringRef Flag, StringRef SearchPath) {
auto AddSearchPath = [&](StringRef Flag, StringRef SearchPath,
bool HasPrefix = false) {
SmallString<128> P(Sysroot->getValue());
AppendPlatformPrefix(P, Triple);
if (!HasPrefix)
P.append(getSystemPrefix(Triple));
llvm::sys::path::append(P, SearchPath);
if (getToolChain().getVFS().exists(P)) {
CmdArgs.push_back(Args.MakeArgString(Flag + P));
}
};

AddSearchPath("-L", "/usr/lib");
AddSearchPath("-F", "/System/Library/Frameworks");
for (const StringRef Path : getCommonSystemPaths(Triple)) {
if (Path.contains("Framework"))
AddSearchPath("-F", Path, /*HasPrefix=*/true);
}
}
}
}
Expand Down Expand Up @@ -2463,16 +2468,6 @@ void Darwin::AddDeploymentTarget(DerivedArgList &Args) const {
}
}

// For certain platforms/environments almost all resources (e.g., headers) are
// located in sub-directories, e.g., for DriverKit they live in
// <SYSROOT>/System/DriverKit/usr/include (instead of <SYSROOT>/usr/include).
static void AppendPlatformPrefix(SmallString<128> &Path,
const llvm::Triple &T) {
if (T.isDriverKit()) {
llvm::sys::path::append(Path, "System", "DriverKit");
}
}

// Returns the effective sysroot from either -isysroot or --sysroot, plus the
// platform prefix (if any).
llvm::SmallString<128>
Expand All @@ -2484,7 +2479,7 @@ DarwinClang::GetEffectiveSysroot(const llvm::opt::ArgList &DriverArgs) const {
Path = getDriver().SysRoot;

if (hasEffectiveTriple()) {
AppendPlatformPrefix(Path, getEffectiveTriple());
Path.append(getSystemPrefix(getEffectiveTriple()));
}
return Path;
}
Expand Down
11 changes: 5 additions & 6 deletions clang/lib/Lex/InitHeaderSearch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
//
//===----------------------------------------------------------------------===//

#include "clang/Basic/DarwinSDKInfo.h"
#include "clang/Basic/FileManager.h"
#include "clang/Basic/LangOptions.h"
#include "clang/Config/config.h" // C_INCLUDE_DIRS
Expand Down Expand Up @@ -339,13 +340,11 @@ void InitHeaderSearch::AddDefaultIncludePaths(
if (triple.isOSDarwin()) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think we can just clean up this code block by moving all logics to driver?

Copy link
Member Author

@cyndyishida cyndyishida Dec 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like that was previously attempted by @ldionne in #75841

The problem was appending /Library/Frameworks to the sysroot. framework search paths were no longer being passed for header search. I'll try rebasing that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked into this a bit more. IMO the only way to move all this code to the driver is to either

  1. changing search path order between Framework search paths & other default search paths like -internal-isystem $(SDKROOT)/usr/include (since existing iframework puts the search path at the top instead of at the bottom like what InitHeaderSearch does)
    OR
  2. introducing some kind of search path argument to preserve back precedence for S/L/F e.g. a -internal-iframework

Both feel out of scope to me.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW I agree with your assessment @cyndyishida. I just did the exercise of picking up #75841 and we basically need to pass -iframework from the driver to the frontend, but if we limit ourselves to existing functionality in the driver that ends up putting the framework search paths before other system headers. I think that should be attempted as a separate patch that tries reviving #75841.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm trying to revive this as #120149

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);
for (const StringRef Path : getCommonSystemPaths(triple))
AddPath(Path, System, true);

if (!triple.isDriverKit())
AddPath("/Library/Frameworks", System, true);
}
}
return;
}
Expand Down
3 changes: 3 additions & 0 deletions clang/test/Driver/driverkit-path.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,11 @@ int main() { return 0; }
// LD64-OLD: "-isysroot" "[[PATH:[^"]*]]Inputs/DriverKit19.0.sdk"
// LD64-OLD: "-L[[PATH]]Inputs/DriverKit19.0.sdk/System/DriverKit/usr/lib"
// LD64-OLD: "-F[[PATH]]Inputs/DriverKit19.0.sdk/System/DriverKit/System/Library/Frameworks"
// LD64-OLD: "-F[[PATH]]Inputs/DriverKit19.0.sdk/System/DriverKit/System/Library/SubFrameworks"
// LD64-NEW: "-isysroot" "[[PATH:[^"]*]]Inputs/DriverKit19.0.sdk"
// LD64-NEW-NOT: "-L[[PATH]]Inputs/DriverKit19.0.sdk/System/DriverKit/usr/lib"
// LD64-NEW-NOT: "-F[[PATH]]Inputs/DriverKit19.0.sdk/System/DriverKit/System/Library/Frameworks"
// LD64-NEW-NOT: "-F[[PATH]]Inputs/DriverKit19.0.sdk/System/DriverKit/System/Library/SubFrameworks"


// RUN: %clang %s -target x86_64-apple-driverkit19.0 -isysroot %S/Inputs/DriverKit19.0.sdk -E -v -x c++ 2>&1 | FileCheck %s --check-prefix=INC
Expand All @@ -31,3 +33,4 @@ int main() { return 0; }
// INC: /lib{{(64)?}}/clang/{{[^/ ]+}}/include
// INC: [[PATH]]/System/DriverKit/usr/include
// INC: [[PATH]]/System/DriverKit/System/Library/Frameworks (framework directory)
// INC: [[PATH]]/System/DriverKit/System/Library/SubFrameworks (framework directory)
Loading