-
Notifications
You must be signed in to change notification settings - Fork 15.3k
[Clang][Driver] Fix the missing Target-Triple-Level include path resolution in Baremetal Driver #165321
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
base: main
Are you sure you want to change the base?
Conversation
…lution in Baremetal Driver The current baremetal driver implementation does not have a way to derive the target-triple-level include path within the sysroot. This feature is especially useful in setups where header paths deviate from the default bare-metal assumptions. For example, when headers are shared across target triples, it becomes necessary to organize them under target-triple-specific directories to ensure correct resolution..
|
@llvm/pr-subscribers-clang Author: Simi Pallipurath (simpal01) ChangesThe current baremetal driver implementation does not have a way to derive the target-triple-level include path within the sysroot. This feature is especially useful in setups where header paths deviate from the default bare-metal assumptions. For example, when headers are shared across target triples, it becomes necessary to organise them under target-triple-specific directories to ensure correct resolution.. Full diff: https://github.com/llvm/llvm-project/pull/165321.diff 1 Files Affected:
diff --git a/clang/lib/Driver/ToolChains/BareMetal.cpp b/clang/lib/Driver/ToolChains/BareMetal.cpp
index 9b7f58c392885..d048f228f4572 100644
--- a/clang/lib/Driver/ToolChains/BareMetal.cpp
+++ b/clang/lib/Driver/ToolChains/BareMetal.cpp
@@ -408,6 +408,8 @@ void BareMetal::AddClangSystemIncludeArgs(const ArgList &DriverArgs,
if (DriverArgs.hasArg(options::OPT_nostdlibinc))
return;
+ const Driver &D = getDriver();
+
if (std::optional<std::string> Path = getStdlibIncludePath())
addSystemInclude(DriverArgs, CC1Args, *Path);
@@ -418,6 +420,13 @@ void BareMetal::AddClangSystemIncludeArgs(const ArgList &DriverArgs,
llvm::sys::path::append(Dir, M.includeSuffix());
llvm::sys::path::append(Dir, "include");
addSystemInclude(DriverArgs, CC1Args, Dir.str());
+
+ Dir = SysRootDir;
+ llvm::sys::path::append(Dir, getTripleString());
+ if (D.getVFS().exists(Dir)) {
+ llvm::sys::path::append(Dir, "include");
+ addSystemInclude(DriverArgs, CC1Args, Dir.str());
+ }
}
}
}
@@ -498,9 +507,13 @@ void BareMetal::AddClangCXXStdlibIncludeArgs(const ArgList &DriverArgs,
addSystemInclude(DriverArgs, CC1Args, TargetDir.str());
break;
}
- // Add generic path if nothing else succeeded so far.
+ // Add generic paths if nothing else succeeded so far.
llvm::sys::path::append(Dir, "include", "c++", "v1");
addSystemInclude(DriverArgs, CC1Args, Dir.str());
+ Dir = SysRootDir;
+ llvm::sys::path::append(Dir, Target, "include", "c++", "v1");
+ if (D.getVFS().exists(Dir))
+ addSystemInclude(DriverArgs, CC1Args, Dir.str());
break;
}
case ToolChain::CST_Libstdcxx: {
|
|
@llvm/pr-subscribers-clang-driver Author: Simi Pallipurath (simpal01) ChangesThe current baremetal driver implementation does not have a way to derive the target-triple-level include path within the sysroot. This feature is especially useful in setups where header paths deviate from the default bare-metal assumptions. For example, when headers are shared across target triples, it becomes necessary to organise them under target-triple-specific directories to ensure correct resolution.. Full diff: https://github.com/llvm/llvm-project/pull/165321.diff 1 Files Affected:
diff --git a/clang/lib/Driver/ToolChains/BareMetal.cpp b/clang/lib/Driver/ToolChains/BareMetal.cpp
index 9b7f58c392885..d048f228f4572 100644
--- a/clang/lib/Driver/ToolChains/BareMetal.cpp
+++ b/clang/lib/Driver/ToolChains/BareMetal.cpp
@@ -408,6 +408,8 @@ void BareMetal::AddClangSystemIncludeArgs(const ArgList &DriverArgs,
if (DriverArgs.hasArg(options::OPT_nostdlibinc))
return;
+ const Driver &D = getDriver();
+
if (std::optional<std::string> Path = getStdlibIncludePath())
addSystemInclude(DriverArgs, CC1Args, *Path);
@@ -418,6 +420,13 @@ void BareMetal::AddClangSystemIncludeArgs(const ArgList &DriverArgs,
llvm::sys::path::append(Dir, M.includeSuffix());
llvm::sys::path::append(Dir, "include");
addSystemInclude(DriverArgs, CC1Args, Dir.str());
+
+ Dir = SysRootDir;
+ llvm::sys::path::append(Dir, getTripleString());
+ if (D.getVFS().exists(Dir)) {
+ llvm::sys::path::append(Dir, "include");
+ addSystemInclude(DriverArgs, CC1Args, Dir.str());
+ }
}
}
}
@@ -498,9 +507,13 @@ void BareMetal::AddClangCXXStdlibIncludeArgs(const ArgList &DriverArgs,
addSystemInclude(DriverArgs, CC1Args, TargetDir.str());
break;
}
- // Add generic path if nothing else succeeded so far.
+ // Add generic paths if nothing else succeeded so far.
llvm::sys::path::append(Dir, "include", "c++", "v1");
addSystemInclude(DriverArgs, CC1Args, Dir.str());
+ Dir = SysRootDir;
+ llvm::sys::path::append(Dir, Target, "include", "c++", "v1");
+ if (D.getVFS().exists(Dir))
+ addSystemInclude(DriverArgs, CC1Args, Dir.str());
break;
}
case ToolChain::CST_Libstdcxx: {
|
|
@petrhosek Please review this patch. This is one of the solutions provided to support Target Triple–specific include paths in the bare-metal driver via hard coded path. and another solution is via multilib.yaml which is added here #146651. This is more favourable both within the team and based on the feedback received in the RFC https://discourse.llvm.org/t/rfc-support-target-triple-specific-include-paths-derived-from-sysroot-via-multilib-yaml-or-driver-logic/86925/6 |
| Dir = SysRootDir; | ||
| llvm::sys::path::append(Dir, Target, "include", "c++", "v1"); | ||
| if (D.getVFS().exists(Dir)) | ||
| addSystemInclude(DriverArgs, CC1Args, Dir.str()); |
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.
Since this logic is unrelated to multilibs, this should be done inside AddCXXIncludePath.
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.
AddCXXIncludePath(Path) is currently passed a path derived from a top-level include directory that isn’t rooted in the active --sysroot. What this PR is actually aiming for is to derive the target-triple-level include path within the sysroot itself. But here the sysroot computation happens after the AddCXXIncludePath(Path) call.
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.
I see, I still think we need to move this logic outside of the multilib loop to avoid adding the include directory multiple times. I think this entire loop might need to be restructured since I don't think it's actually correct, so it might be probably easiest for now to just move the new logic outside.
| Dir = SysRootDir; | ||
| llvm::sys::path::append(Dir, getTripleString()); | ||
| if (D.getVFS().exists(Dir)) { | ||
| llvm::sys::path::append(Dir, "include"); | ||
| addSystemInclude(DriverArgs, CC1Args, Dir.str()); |
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.
This is unrelated to multilibs so should be done outside of the loop.
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.
Done
|
Would it be possible to also add a test case in |
ed2e69d to
c664b02
Compare
…th resolution in Baremetal Driver
🐧 Linux x64 Test Results
|
In this patch, the target-triple–specific header path is added only if the corresponding directory exists on the host system. Since the LLVM test environment doesn’t include such a layout, it’s not possible to test this behaviour purely within clang/test/Driver using the in-tree setup. I did verify it locally using the Arm Toolchain for Embedded (ATFE), which provides exactly this header layout, and confirmed that the driver picks up the correct triple-specific include directory. Separately, I have an alternate patch that adds support for target-triple-specific include paths in the bare-metal driver via multilib.yaml configuration, #146651 . That approach is what our team prefers, and in that case I was able to include appropriate driver tests, as the include paths are conditionally added based on entries available in the multilib.yaml file. Would you like to take a look at that patch too? |
| llvm::sys::path::append(Dir, "include"); | ||
| addSystemInclude(DriverArgs, CC1Args, Dir.str()); | ||
| } | ||
| SmallString<128> Dir = SysRootDir; |
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.
Nit: I'd use copy constructor over assignment operator for consistency with the existing code.
| SmallString<128> Dir = SysRootDir; | |
| SmallString<128> Dir(SysRootDir); |
The way we typically handle that is by constructing the artificial directory layout in https://github.com/llvm/llvm-project/tree/main/clang/test/Driver/Inputs for testing, see for example https://github.com/llvm/llvm-project/tree/main/clang/test/Driver/Inputs/basic_baremetal_tree.
Sure, but I'd like to merge this change as well since we have customers who don't use |
The current baremetal driver implementation does not have a way to derive the target-triple-level include path within the sysroot.
This feature is especially useful in setups where header paths deviate from the default bare-metal assumptions. For example, when headers are shared across target triples, it becomes necessary to organise them under target-triple-specific directories to ensure correct resolution..