[Driver] Add support for GCC installation detection in Baremetal toolchain #121829
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 @llvm/pr-subscribers-clang Author: Garvit Gupta (quic-garvgupt) ChangesThis patch introduces the baretmetal toolchain object about GCC Installation. Additionally, the restriction to always use integrated assembler is removed This patch currently adds and modifies arm related test only. The riscv specific This is the first PR in the series of 3 PRs for merging and extending Baremetal
The above division will also ensure that riscv and arm specific tests are not RFC: https://discourse.llvm.org/t/merging-riscvtoolchain-and-baremetal-toolchains/75524 Change-Id: Ibaeb569cf7e2cee03c022aa9ecd1abe29d5c30d4 Patch is 29.34 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/121829.diff 31 Files Affected:
diff --git a/clang/lib/Driver/ToolChains/BareMetal.cpp b/clang/lib/Driver/ToolChains/BareMetal.cpp
index eecaaa9a42930d..7b0f2bc2fd3895 100644
--- a/clang/lib/Driver/ToolChains/BareMetal.cpp
+++ b/clang/lib/Driver/ToolChains/BareMetal.cpp
@@ -97,7 +97,8 @@ static bool findRISCVMultilibs(const Driver &D,
return false;
}
-static std::string computeBaseSysRoot(const Driver &D, bool IncludeTriple) {
+static std::string computeInstalledToolchainSysRoot(const Driver &D,
+ bool IncludeTriple) {
if (!D.SysRoot.empty())
return D.SysRoot;
@@ -110,20 +111,93 @@ static std::string computeBaseSysRoot(const Driver &D, bool IncludeTriple) {
return std::string(SysRootDir);
}
+// GCC sysroot here means form sysroot from either --gcc-install-dir, or from
+// --gcc-toolchain or if the toolchain is installed alongside clang in
+// bin/../<TargetTriple> directory if it is not explicitly specified on the
+// command line through `--sysroot` option. libc here will be newlib.
+std::string BareMetal::computeGCCSysRoot() const {
+ if (!getDriver().SysRoot.empty())
+ return getDriver().SysRoot;
+
+ SmallString<128> SysRootDir;
+ if (GCCInstallation.isValid()) {
+ StringRef LibDir = GCCInstallation.getParentLibPath();
+ StringRef TripleStr = GCCInstallation.getTriple().str();
+ llvm::sys::path::append(SysRootDir, LibDir, "..", TripleStr);
+ } else {
+ // Use the triple as provided to the driver. Unlike the parsed triple
+ // this has not been normalized to always contain every field.
+ llvm::sys::path::append(SysRootDir, getDriver().Dir, "..",
+ getDriver().getTargetTriple());
+ }
+
+ if (!llvm::sys::fs::exists(SysRootDir))
+ return std::string();
+
+ return std::string(SysRootDir);
+}
+
+std::string BareMetal::computeSysRoot() const {
+ if (!SysRoot.empty())
+ return SysRoot;
+
+ std::string SysRoot = getDriver().SysRoot;
+ if (!SysRoot.empty() && llvm::sys::fs::exists(SysRoot))
+ return SysRoot;
+
+ // Verify the GCC installation from -gcc-install-dir, --gcc-toolchain, or
+ // alongside clang. If valid, form the sysroot. Otherwise, check
+ // lib/clang-runtimes above the driver.
+ SysRoot = computeGCCSysRoot();
+ if (!SysRoot.empty())
+ return SysRoot;
+
+ SysRoot =
+ computeInstalledToolchainSysRoot(getDriver(), /*IncludeTriple*/ true);
+
+ return SysRoot;
+}
+
+static void addMultilibsFilePaths(const Driver &D, const MultilibSet &Multilibs,
+ const Multilib &Multilib,
+ StringRef InstallPath,
+ ToolChain::path_list &Paths) {
+ if (const auto &PathsCallback = Multilibs.filePathsCallback())
+ for (const auto &Path : PathsCallback(Multilib))
+ addPathIfExists(D, InstallPath + Path, Paths);
+}
+
BareMetal::BareMetal(const Driver &D, const llvm::Triple &Triple,
const ArgList &Args)
- : ToolChain(D, Triple, Args),
- SysRoot(computeBaseSysRoot(D, /*IncludeTriple=*/true)) {
- getProgramPaths().push_back(getDriver().Dir);
-
- findMultilibs(D, Triple, Args);
- SmallString<128> SysRoot(computeSysRoot());
- if (!SysRoot.empty()) {
- for (const Multilib &M : getOrderedMultilibs()) {
- SmallString<128> Dir(SysRoot);
- llvm::sys::path::append(Dir, M.osSuffix(), "lib");
- getFilePaths().push_back(std::string(Dir));
- getLibraryPaths().push_back(std::string(Dir));
+ : Generic_ELF(D, Triple, Args) {
+ GCCInstallation.init(Triple, Args);
+ SysRoot = computeSysRoot();
+ if (GCCInstallation.isValid()) {
+ Multilibs = GCCInstallation.getMultilibs();
+ SelectedMultilibs.assign({GCCInstallation.getMultilib()});
+ path_list &Paths = getFilePaths();
+ // Add toolchain/multilib specific file paths.
+ addMultilibsFilePaths(D, Multilibs, SelectedMultilibs.back(),
+ GCCInstallation.getInstallPath(), Paths);
+ getFilePaths().push_back(GCCInstallation.getInstallPath().str());
+ ToolChain::path_list &PPaths = getProgramPaths();
+ // Multilib cross-compiler GCC installations put ld in a triple-prefixed
+ // directory off of the parent of the GCC installation.
+ PPaths.push_back(Twine(GCCInstallation.getParentLibPath() + "/../" +
+ GCCInstallation.getTriple().str() + "/bin")
+ .str());
+ PPaths.push_back((GCCInstallation.getParentLibPath() + "/../bin").str());
+ getFilePaths().push_back(computeSysRoot() + "/lib");
+ } else {
+ getProgramPaths().push_back(getDriver().Dir);
+ findMultilibs(D, Triple, Args);
+ if (!SysRoot.empty()) {
+ for (const Multilib &M : getOrderedMultilibs()) {
+ SmallString<128> Dir(SysRoot);
+ llvm::sys::path::append(Dir, M.osSuffix(), "lib");
+ getFilePaths().push_back(std::string(Dir));
+ getLibraryPaths().push_back(std::string(Dir));
+ }
}
}
}
@@ -215,7 +289,7 @@ getMultilibConfigPath(const Driver &D, const llvm::Triple &Triple,
return {};
}
} else {
- MultilibPath = computeBaseSysRoot(D, /*IncludeTriple=*/false);
+ MultilibPath = computeInstalledToolchainSysRoot(D, /*IncludeTriple=*/false);
llvm::sys::path::append(MultilibPath, MultilibFilename);
}
return MultilibPath;
@@ -233,7 +307,7 @@ void BareMetal::findMultilibs(const Driver &D, const llvm::Triple &Triple,
if (D.getVFS().exists(*MultilibPath)) {
// If multilib.yaml is found, update sysroot so it doesn't use a target
// specific suffix
- SysRoot = computeBaseSysRoot(D, /*IncludeTriple=*/false);
+ SysRoot = computeInstalledToolchainSysRoot(D, /*IncludeTriple=*/false);
findMultilibsFromYAML(*this, D, *MultilibPath, Args, Result);
SelectedMultilibs = Result.SelectedMultilibs;
Multilibs = Result.Multilibs;
@@ -258,8 +332,6 @@ Tool *BareMetal::buildStaticLibTool() const {
return new tools::baremetal::StaticLibTool(*this);
}
-std::string BareMetal::computeSysRoot() const { return SysRoot; }
-
BareMetal::OrderedMultilibs BareMetal::getOrderedMultilibs() const {
// Get multilibs in reverse order because they're ordered most-specific last.
if (!SelectedMultilibs.empty())
@@ -304,6 +376,19 @@ void BareMetal::addClangTargetOptions(const ArgList &DriverArgs,
CC1Args.push_back("-nostdsysteminc");
}
+void BareMetal::addLibStdCxxIncludePaths(
+ const llvm::opt::ArgList &DriverArgs,
+ llvm::opt::ArgStringList &CC1Args) const {
+ if (!GCCInstallation.isValid())
+ return;
+ const GCCVersion &Version = GCCInstallation.getVersion();
+ StringRef TripleStr = GCCInstallation.getTriple().str();
+ const Multilib &Multilib = GCCInstallation.getMultilib();
+ addLibStdCXXIncludePaths(computeSysRoot() + "/include/c++/" + Version.Text,
+ TripleStr, Multilib.includeSuffix(), DriverArgs,
+ CC1Args);
+}
+
void BareMetal::AddClangCXXStdlibIncludeArgs(const ArgList &DriverArgs,
ArgStringList &CC1Args) const {
if (DriverArgs.hasArg(options::OPT_nostdinc, options::OPT_nostdlibinc,
@@ -341,7 +426,7 @@ void BareMetal::AddClangCXXStdlibIncludeArgs(const ArgList &DriverArgs,
break;
}
case ToolChain::CST_Libstdcxx:
- // We only support libc++ toolchain installation.
+ addLibStdCxxIncludePaths(DriverArgs, CC1Args);
break;
}
diff --git a/clang/lib/Driver/ToolChains/BareMetal.h b/clang/lib/Driver/ToolChains/BareMetal.h
index 483b5efab5e6e2..de3b1b267c8e7c 100644
--- a/clang/lib/Driver/ToolChains/BareMetal.h
+++ b/clang/lib/Driver/ToolChains/BareMetal.h
@@ -9,6 +9,7 @@
#ifndef LLVM_CLANG_LIB_DRIVER_TOOLCHAINS_BAREMETAL_H
#define LLVM_CLANG_LIB_DRIVER_TOOLCHAINS_BAREMETAL_H
+#include "ToolChains/Gnu.h"
#include "clang/Driver/Tool.h"
#include "clang/Driver/ToolChain.h"
@@ -19,7 +20,7 @@ namespace driver {
namespace toolchains {
-class LLVM_LIBRARY_VISIBILITY BareMetal : public ToolChain {
+class LLVM_LIBRARY_VISIBILITY BareMetal : public Generic_ELF {
public:
BareMetal(const Driver &D, const llvm::Triple &Triple,
const llvm::opt::ArgList &Args);
@@ -35,7 +36,6 @@ class LLVM_LIBRARY_VISIBILITY BareMetal : public ToolChain {
Tool *buildStaticLibTool() const override;
public:
- bool useIntegratedAs() const override { return true; }
bool isBareMetal() const override { return true; }
bool isCrossCompiling() const override { return true; }
bool HasNativeLLVMSupport() const override { return true; }
@@ -67,6 +67,9 @@ class LLVM_LIBRARY_VISIBILITY BareMetal : public ToolChain {
void AddClangCXXStdlibIncludeArgs(
const llvm::opt::ArgList &DriverArgs,
llvm::opt::ArgStringList &CC1Args) const override;
+ void
+ addLibStdCxxIncludePaths(const llvm::opt::ArgList &DriverArgs,
+ llvm::opt::ArgStringList &CC1Args) const override;
std::string computeSysRoot() const override;
SanitizerMask getSupportedSanitizers() const override;
@@ -76,6 +79,7 @@ class LLVM_LIBRARY_VISIBILITY BareMetal : public ToolChain {
OrderedMultilibs getOrderedMultilibs() const;
std::string SysRoot;
+ std::string computeGCCSysRoot() const;
};
} // namespace toolchains
diff --git a/clang/test/Driver/Inputs/basic_aarch64_gcc_tree/aarch64-none-elf/bin/ld b/clang/test/Driver/Inputs/basic_aarch64_gcc_tree/aarch64-none-elf/bin/ld
new file mode 100644
index 00000000000000..b23e55619b2ff0
--- /dev/null
+++ b/clang/test/Driver/Inputs/basic_aarch64_gcc_tree/aarch64-none-elf/bin/ld
@@ -0,0 +1 @@
+#!/bin/true
diff --git a/clang/test/Driver/Inputs/basic_aarch64_gcc_tree/aarch64-none-elf/include/c++/8.2.1/.keep b/clang/test/Driver/Inputs/basic_aarch64_gcc_tree/aarch64-none-elf/include/c++/8.2.1/.keep
new file mode 100644
index 00000000000000..e69de29bb2d1d6
diff --git a/clang/test/Driver/Inputs/basic_aarch64_gcc_tree/aarch64-none-elf/lib/.keep b/clang/test/Driver/Inputs/basic_aarch64_gcc_tree/aarch64-none-elf/lib/.keep
new file mode 100644
index 00000000000000..e69de29bb2d1d6
diff --git a/clang/test/Driver/Inputs/basic_aarch64_gcc_tree/aarch64-none-elf/lib/crt0.o b/clang/test/Driver/Inputs/basic_aarch64_gcc_tree/aarch64-none-elf/lib/crt0.o
new file mode 100644
index 00000000000000..e69de29bb2d1d6
diff --git a/clang/test/Driver/Inputs/basic_aarch64_gcc_tree/lib/gcc/aarch64-none-elf/8.2.1/crtbegin.o b/clang/test/Driver/Inputs/basic_aarch64_gcc_tree/lib/gcc/aarch64-none-elf/8.2.1/crtbegin.o
new file mode 100644
index 00000000000000..e69de29bb2d1d6
diff --git a/clang/test/Driver/Inputs/basic_aarch64_gcc_tree/lib/gcc/aarch64-none-elf/8.2.1/crtend.o b/clang/test/Driver/Inputs/basic_aarch64_gcc_tree/lib/gcc/aarch64-none-elf/8.2.1/crtend.o
new file mode 100644
index 00000000000000..e69de29bb2d1d6
diff --git a/clang/test/Driver/Inputs/basic_aarch64_nogcc_tree/aarch64-none-elf/bin/ld b/clang/test/Driver/Inputs/basic_aarch64_nogcc_tree/aarch64-none-elf/bin/ld
new file mode 100644
index 00000000000000..b23e55619b2ff0
--- /dev/null
+++ b/clang/test/Driver/Inputs/basic_aarch64_nogcc_tree/aarch64-none-elf/bin/ld
@@ -0,0 +1 @@
+#!/bin/true
diff --git a/clang/test/Driver/Inputs/basic_aarch64_nogcc_tree/aarch64-none-elf/lib/.keep b/clang/test/Driver/Inputs/basic_aarch64_nogcc_tree/aarch64-none-elf/lib/.keep
new file mode 100644
index 00000000000000..e69de29bb2d1d6
diff --git a/clang/test/Driver/Inputs/basic_aarch64_nogcc_tree/aarch64-none-elf/lib/crt0.o b/clang/test/Driver/Inputs/basic_aarch64_nogcc_tree/aarch64-none-elf/lib/crt0.o
new file mode 100644
index 00000000000000..e69de29bb2d1d6
diff --git a/clang/test/Driver/Inputs/basic_aarch64_nogcc_tree/aarch64-none-elf/lib/crtbegin.o b/clang/test/Driver/Inputs/basic_aarch64_nogcc_tree/aarch64-none-elf/lib/crtbegin.o
new file mode 100644
index 00000000000000..e69de29bb2d1d6
diff --git a/clang/test/Driver/Inputs/basic_aarch64_nogcc_tree/aarch64-none-elf/lib/crtend.o b/clang/test/Driver/Inputs/basic_aarch64_nogcc_tree/aarch64-none-elf/lib/crtend.o
new file mode 100644
index 00000000000000..e69de29bb2d1d6
diff --git a/clang/test/Driver/Inputs/basic_arm_gcc_tree/armv6m-none-eabi/bin/ld b/clang/test/Driver/Inputs/basic_arm_gcc_tree/armv6m-none-eabi/bin/ld
new file mode 100644
index 00000000000000..b23e55619b2ff0
--- /dev/null
+++ b/clang/test/Driver/Inputs/basic_arm_gcc_tree/armv6m-none-eabi/bin/ld
@@ -0,0 +1 @@
+#!/bin/true
diff --git a/clang/test/Driver/Inputs/basic_arm_gcc_tree/armv6m-none-eabi/include/c++/8.2.1/.keep b/clang/test/Driver/Inputs/basic_arm_gcc_tree/armv6m-none-eabi/include/c++/8.2.1/.keep
new file mode 100644
index 00000000000000..e69de29bb2d1d6
diff --git a/clang/test/Driver/Inputs/basic_arm_gcc_tree/armv6m-none-eabi/lib/.keep b/clang/test/Driver/Inputs/basic_arm_gcc_tree/armv6m-none-eabi/lib/.keep
new file mode 100644
index 00000000000000..e69de29bb2d1d6
diff --git a/clang/test/Driver/Inputs/basic_arm_gcc_tree/armv6m-none-eabi/lib/crt0.o b/clang/test/Driver/Inputs/basic_arm_gcc_tree/armv6m-none-eabi/lib/crt0.o
new file mode 100644
index 00000000000000..e69de29bb2d1d6
diff --git a/clang/test/Driver/Inputs/basic_arm_gcc_tree/armv6m-none-eabi/lib/libstdc++.a b/clang/test/Driver/Inputs/basic_arm_gcc_tree/armv6m-none-eabi/lib/libstdc++.a
new file mode 100644
index 00000000000000..e69de29bb2d1d6
diff --git a/clang/test/Driver/Inputs/basic_arm_gcc_tree/lib/gcc/armv6m-none-eabi/8.2.1/crtbegin.o b/clang/test/Driver/Inputs/basic_arm_gcc_tree/lib/gcc/armv6m-none-eabi/8.2.1/crtbegin.o
new file mode 100644
index 00000000000000..e69de29bb2d1d6
diff --git a/clang/test/Driver/Inputs/basic_arm_gcc_tree/lib/gcc/armv6m-none-eabi/8.2.1/crtend.o b/clang/test/Driver/Inputs/basic_arm_gcc_tree/lib/gcc/armv6m-none-eabi/8.2.1/crtend.o
new file mode 100644
index 00000000000000..e69de29bb2d1d6
diff --git a/clang/test/Driver/Inputs/basic_arm_nogcc_tree/armv6m-none-eabi/bin/ld b/clang/test/Driver/Inputs/basic_arm_nogcc_tree/armv6m-none-eabi/bin/ld
new file mode 100644
index 00000000000000..b23e55619b2ff0
--- /dev/null
+++ b/clang/test/Driver/Inputs/basic_arm_nogcc_tree/armv6m-none-eabi/bin/ld
@@ -0,0 +1 @@
+#!/bin/true
diff --git a/clang/test/Driver/Inputs/basic_arm_nogcc_tree/armv6m-none-eabi/lib/.keep b/clang/test/Driver/Inputs/basic_arm_nogcc_tree/armv6m-none-eabi/lib/.keep
new file mode 100644
index 00000000000000..e69de29bb2d1d6
diff --git a/clang/test/Driver/Inputs/basic_arm_nogcc_tree/armv6m-none-eabi/lib/crt0.o b/clang/test/Driver/Inputs/basic_arm_nogcc_tree/armv6m-none-eabi/lib/crt0.o
new file mode 100644
index 00000000000000..e69de29bb2d1d6
diff --git a/clang/test/Driver/Inputs/basic_arm_nogcc_tree/armv6m-none-eabi/lib/crtbegin.o b/clang/test/Driver/Inputs/basic_arm_nogcc_tree/armv6m-none-eabi/lib/crtbegin.o
new file mode 100644
index 00000000000000..e69de29bb2d1d6
diff --git a/clang/test/Driver/Inputs/basic_arm_nogcc_tree/armv6m-none-eabi/lib/crtend.o b/clang/test/Driver/Inputs/basic_arm_nogcc_tree/armv6m-none-eabi/lib/crtend.o
new file mode 100644
index 00000000000000..e69de29bb2d1d6
diff --git a/clang/test/Driver/aarch64-toolchain-extra.c b/clang/test/Driver/aarch64-toolchain-extra.c
new file mode 100644
index 00000000000000..c4e05fd5d10436
--- /dev/null
+++ b/clang/test/Driver/aarch64-toolchain-extra.c
@@ -0,0 +1,28 @@
+// A basic clang -cc1 command-line, and simple environment check.
+
+// The tests here are similar to those in aarch64-toolchain.c, however
+// these tests need to create symlinks to test directory trees in order to
+// set up the environment and therefore shell support is required.
+// REQUIRES: shell
+// UNSUPPORTED: system-windows
+
+// If there is no GCC install detected then the driver searches for executables
+// and runtime starting from the directory tree above the driver itself.
+// The test below checks that the driver correctly finds the linker and
+// runtime if and only if they exist.
+//
+// RUN: rm -rf %t
+// RUN: mkdir -p %t/aarch64-nogcc/bin
+// RUN: ln -s %clang %t/aarch64-nogcc/bin/clang
+// RUN: ln -s %S/Inputs/basic_aarch64_nogcc_tree/aarch64-none-elf %t/aarch64-nogcc/aarch64-none-elf
+// RUN: %t/aarch64-nogcc/bin/clang %s -### -no-canonical-prefixes \
+// RUN: --gcc-toolchain=%t/aarch64-nogcc/invalid \
+// RUN: --target=aarch64-none-elf --rtlib=libgcc -fuse-ld=ld 2>&1 \
+// RUN: | FileCheck -check-prefix=C-ARM-BAREMETAL-NOGCC %s
+
+// RUN: %t/aarch64-nogcc/bin/clang %s -### -no-canonical-prefixes \
+// RUN: --sysroot=%t/aarch64-nogcc/bin/../aarch64-none-elf \
+// RUN: --target=aarch64-none-elf --rtlib=libgcc -fuse-ld=ld 2>&1 \
+// RUN: | FileCheck -check-prefix=C-ARM-BAREMETAL-NOGCC %s
+
+// C-ARM-BAREMETAL-NOGCC: "-internal-isystem" "{{.*}}/aarch64-nogcc/bin/../aarch64-none-elf/include"
\ No newline at end of file
diff --git a/clang/test/Driver/aarch64-toolchain.c b/clang/test/Driver/aarch64-toolchain.c
new file mode 100644
index 00000000000000..b850e0fe8954e2
--- /dev/null
+++ b/clang/test/Driver/aarch64-toolchain.c
@@ -0,0 +1,61 @@
+// UNSUPPORTED: system-windows
+
+// RUN: %clang -### %s -fuse-ld= \
+// RUN: --target=aarch64-none-elf --rtlib=libgcc \
+// RUN: --gcc-toolchain=%S/Inputs/basic_aarch64_gcc_tree \
+// RUN: --sysroot=%S/Inputs/basic_aarch64_gcc_tree/aarch64-none-elf 2>&1 \
+// RUN: | FileCheck -check-prefix=C-AARCH64-BAREMETAL %s
+
+// C-AARCH64-BAREMETAL: "-cc1" "-triple" "aarch64-unknown-none-elf"
+// C-AARCH64-BAREMETAL: "-isysroot" "{{.*}}Inputs/basic_aarch64_gcc_tree/aarch64-none-elf"
+// C-AARCH64-BAREMETAL: "-internal-isystem" "{{.*}}Inputs/basic_aarch64_gcc_tree/aarch64-none-elf/include"
+
+// RUN: %clang -### %s -fuse-ld= \
+// RUN: --target=aarch64-none-elf --rtlib=libgcc \
+// RUN: --gcc-toolchain=%S/Inputs/basic_aarch64_gcc_tree \
+// RUN: --sysroot= 2>&1 \
+// RUN: | FileCheck -check-prefix=C-AARCH64-BAREMETAL-NOSYSROOT %s
+
+// C-AARCH64-BAREMETAL-NOSYSROOT: "-cc1" "-triple" "aarch64-unknown-none-elf"
+// C-AARCH64-BAREMETAL-NOSYSROOT: "-internal-isystem" "{{.*}}/Inputs/basic_aarch64_gcc_tree/lib/gcc/aarch64-none-elf/8.2.1/../../../../aarch64-none-elf/include"
+
+// RUN: %clangxx -### %s -fuse-ld= \
+// RUN: --target=aarch64-none-elf -stdlib=libstdc++ --rtlib=libgcc \
+// RUN: --gcc-toolchain=%S/Inputs/basic_aarch64_gcc_tree \
+// RUN: --sysroot=%S/Inputs/basic_aarch64_gcc_tree/aarch64-none-elf 2>&1 \
+// RUN: | FileCheck -check-prefix=CXX-AARCH64-BAREMETAL %s
+
+// CXX-AARCH64-BAREMETAL: "-internal-isystem" "{{.*}}/Inputs/basic_aarch64_gcc_tree/aarch64-none-elf/include/c++/8.2.1/aarch64-none-elf"
+// CXX-AARCH64-BAREMETAL: "-internal-isystem" "{{.*}}/Inputs/basic_aarch64_gcc_tree/aarch64-none-elf/include/c++/8.2.1/backward"
+// CXX-AARCH64-BAREMETAL: "-internal-isystem" "{{.*}}/Inputs/basic_aarch64_gcc_tree/aarch64-none-elf/include/c++/8.2.1"
+// CXX-AARCH64-BAREMETAL: "-internal-isystem" "{{.*}}/Inputs/basic_aarch64_gcc_tree/aarch64-none-elf/include"
+
+// RUN: %clangxx -### %s -fuse-ld= \
+// RUN: --target=aarch64-none-elf -stdlib=libstdc++ --rtlib=libgcc \
+// RUN: --gcc-toolchain=%S/Inputs/basic_aarch64_gcc_tree \
+// RUN: --sysroot= 2>&1 \
+// RUN: | FileCheck -check-prefix=CXX-AARCH64-BAREMETAL-NOSYSROOT %s
+
+// CXX-AARCH64-BAREMETAL-NOSYSROOT: "-internal-isystem" "{{.*}}/Inputs/basic_aarch64_gcc_tree/lib/gcc/aarch64-none-elf/8.2.1/../../../../aarch64-none-elf/include/c++/8.2.1/aarch64-none-elf"
+// CXX-AARCH64-BAREMETAL-NOSYSROOT: "-internal-isystem" "{{.*}}/Inputs/basic_aarch64_gcc_tree/lib/gcc/aarch64-none-elf/8.2.1/../../../../aarch64-none-elf/include/c++/8.2.1/backward"
+// CXX-AARCH64-BAREMETAL-NOSYSROOT: "-internal-isystem" "{{.*}}/Inputs/basic_aarch64_gcc_tree/lib/gcc/aarch64-none-elf/8.2.1/../../../../aarch64-none-elf/include/c++/8.2.1"
+// CXX-AARCH64-BAREMETAL-NOSYSROOT: "-internal-isystem" "{{.*}}/Inputs/basic_aarch64_gcc_tree/lib/gcc/aarch64-none-elf/8.2.1/../../../../aarch64-none-elf/include"
+
+// RUN: %clangxx -### %s -fuse-ld= \
+// RUN: --target=aarch64-none-elf -stdlib=libc++ --rtlib=libgcc \
+// RUN: --gcc-toolchain=%S/Inputs/basic_aarch64_gcc_tree \
+// RUN: --sysroot=%S/Inputs/basic_aarch64_gcc_tree/aarch64-none-elf 2>&1 \
+// RUN: | FileCheck -check-prefix=CXX-AARCH64-BAREMETAL-LIBCXX %s
+
+// CXX-AARCH64-BAREMETAL-LIBCXX: "-isysroot" "{{.*}}Inputs/basic_aarch64_gcc_tree/aarch64-none-elf"
+// CXX-AARCH64-BAREMETAL-LIBCXX: "-internal-isystem" "{{.*}}/Inputs/basic_aarch64_gcc_tree/aarch64-none-elf/include/c++/v1"
+// CXX-AARCH64-BAREMETAL-LIBCXX: "-inte...
[truncated]
|
quic-garvgupt
force-pushed
the
users/quic-garvgupt/compile_line
branch
2 times, most recently
from
02d70a2 to
03b91a8
Compare
quic-garvgupt
changed the title
|
Gentle Ping! |
MaskRay
reviewed
Jan 14, 2025
| getDriver().getTargetTriple()); | ||
| } | ||
|
|
||
| if (!llvm::sys::fs::exists(SysRootDir)) |
There was a problem hiding this comment.
The exists test from RISCVToolChain is not a good idea. The sysroot should not be affected whether it is existent or not.
There was a problem hiding this comment.
I understand that. However, to preserve the behavior of both the BareMetal and RISCVToolchain objects, I need to maintain this condition. If this returns empty, only then does the control transfer to compute the sysroot, as it was done previously in the BareMetal toolchain. (See line 152 under BareMetal::computeSysRoot() function)
There was a problem hiding this comment.
There is a similarly unusual condition OPT_gcc_toolchain in RISCVToolchain introduced in 2020. It's unfortunate that I only noticed in 2023 https://reviews.llvm.org/D91442#inline-1551065
I don't have a horse in the RISCVToolchain race but as a driver maintainer I want to ensure new logic follows the best practice. I am concerned with the peculiarity.
This divergence is ok if eventually the condition will be removed. I do want the users of RISCVToolchain to be aware that this is a quirk that they should not depend on.
There was a problem hiding this comment.
I agree that the condition can be removed if it is actually not needed. Though that has to be done as a part of new patch because for now, this patch aims to preserve the behavior as it was before.
There was a problem hiding this comment.
Thanks. If there is commitment to remove the quirks, this looks good to me!
There was a problem hiding this comment.
I believe the underlying issue is that BareMetal and RISCVToolchain original authors made different choices when it comes to the default sysroot location:
- BareMetal uses the
bin/../lib/clang-runtimes/<TargetTriple>directory. - RISCVToolchain uses the
bin/../<TargetTriple>directory.
Unless we deprecate one (or both) of these and force everyone to switch to a common one, I don't think we can avoid the directory check.
I'd be in favor of unifying on a single location sooner rather than later even if it's going to require a migration for some users. This would be probably best discussed in an RFC though.
|
Gentle Ping again! |
|
Hi @petrhosek, following up on our last RISC-V embedded sync-up, can you please review this patch? Thanks |
|
It's been a few weeks since this patch was last reviewed. If everything looks good, could someone please provide an LGTM? I'd like to merge this patch soon. |
petrhosek
reviewed
Feb 26, 2025
petrhosek
reviewed
Feb 27, 2025
petrhosek
reviewed
Feb 27, 2025
petrhosek
reviewed
Feb 27, 2025
petrhosek
reviewed
Feb 27, 2025
quic-garvgupt
force-pushed
the
users/quic-garvgupt/compile_line
branch
from
03b91a8 to
a4a50a2
Compare
llvmbot
added
the
clang:frontend
quic-garvgupt
force-pushed
the
users/quic-garvgupt/compile_line
branch
from
8d6d2e0 to
63eaf99
Compare
|
Hi @smithp35, can you pls review the latest changes and approve this PR if everything looks fine? |
I'll take a look this week. You'll probably need an approval from PetrHosek too. I'm mostly coming at this from an angle of will it break anything in the existing bare-metal driver. Probably needs some input from the RISCV side. |
smithp35
reviewed
Jun 5, 2025
There was a problem hiding this comment.
I've done another review pass. Mostly some small stylistic comments.
I'm finding it difficult to follow the code with the various different code-paths with similar names. I've made some suggestions on renaming. We may also be able to add some more comments to make it clearer.
|
|
||
| return Triple.getEnvironmentName() == "elf"; | ||
| if (llvm::sys::fs::exists(SysRootDir)) |
There was a problem hiding this comment.
Would it be better to test if SysRootDir is non-empty before going to the filesystem?
| const ArgList &Args) | ||
| : Generic_ELF(D, Triple, Args) { | ||
| GCCInstallation.init(Triple, Args); | ||
| SysRoot = computeSysRoot(); |
There was a problem hiding this comment.
Looking at the code I think we've got:
Driver::SysRoot ; The value of --sysroot
BareMetal::SysRoot ; A cached computation of SysRoot used by computeSysRoot
computeSysRoot; Either GCCInstallation, ../lib/{triple} or ../lib/clang-runtimes/{triple}. Used to override default of Driver::SysRoot.
computeBaseSysRoot ; The sysroot../lib/clang-runtimes/{triple}
It is possible we may need to cache the SysRoot here. Assuming computeSysRoot is idempotent it can be done later.
I think it would also be useful to rename computeBaseSysRoot to something like computeClangRuntimesSysRoot.
There was a problem hiding this comment.
It is possible we may need to cache the SysRoot here. Assuming computeSysRoot is idempotent it can be done later.
Can you expand a bit on where do we need to cache the sysroot. I couldn't understood this comment clearly.
There was a problem hiding this comment.
I was referring to the code path that starts at the constructor BareMetal::BareMetal takes the else branch for GCCInstallation.isValid() and calls findMultilibs()
If there is a multilib.yaml file found then the BareMetal::SysRoot that was computed in the constructor is updated by
// If multilib.yaml is found, update sysroot so it doesn't use a target
// specific suffix
SysRoot = computeClangRuntimesSysRoot(D, /*IncludeTriple=*/false);
Subsequent calls to computeSysRoot() will return that updated value.
So we need to cache the SysRoot in BareMetal::SysRoot. From following through the code it looks like that calculation of the updated SysRoot doesn't call computeSysRoot() so it looks like it is possible to make SysRoot a local variable here (ideally changing its name). This will lose some performance as it looks like computeSysRoot() is called several times later in addLibStdCxxIncludePaths() and AddClangCXXStdlibIncludeArgs() and there's no other place that assigns to SysRoot but findMultiLibs() , but it could be done.
Hopefully that explains it. In summary we can make SysRoot a local variable here with no loss in functionality but IIUC we'll lose some performance, which is likely negligble.
| } else { | ||
| getProgramPaths().push_back(getDriver().Dir); | ||
| findMultilibs(D, Triple, Args); | ||
| const SmallString<128> SysRootDir(computeSysRoot()); |
There was a problem hiding this comment.
Assuming it is still the ToolChain::SysRoot that is set on line 208 can you still use it rather than recompute. All computeSysRoot will do is return ToolChain::SysRoot as it has been set either on line 208 or by findMultilibs?
There was a problem hiding this comment.
Fixed!. I didn't do it previously to minimize code changes.
quic-garvgupt
force-pushed
the
users/quic-garvgupt/compile_line
branch
from
63eaf99 to
0ffc48d
Compare
I've made few more changes in addition to suggestions provided above to improve code readability and facilitate the review process:
|
quic-garvgupt
force-pushed
the
users/quic-garvgupt/compile_line
branch
from
0ffc48d to
c4d61f3
Compare
|
Hi @petrhosek, while petrsmith is reviewing this PR, if you don’t have any further suggestions, comments, or feedback, could you please provide an LGTM? That would help us proceed with merging the PR promptly once Peter approves it. Thanks so much! |
quic-garvgupt
changed the title
smithp35
approved these changes
Jun 10, 2025
| const ArgList &Args) | ||
| : Generic_ELF(D, Triple, Args) { | ||
| GCCInstallation.init(Triple, Args); | ||
| SysRoot = computeSysRoot(); |
There was a problem hiding this comment.
I was referring to the code path that starts at the constructor BareMetal::BareMetal takes the else branch for GCCInstallation.isValid() and calls findMultilibs()
If there is a multilib.yaml file found then the BareMetal::SysRoot that was computed in the constructor is updated by
// If multilib.yaml is found, update sysroot so it doesn't use a target
// specific suffix
SysRoot = computeClangRuntimesSysRoot(D, /*IncludeTriple=*/false);
Subsequent calls to computeSysRoot() will return that updated value.
So we need to cache the SysRoot in BareMetal::SysRoot. From following through the code it looks like that calculation of the updated SysRoot doesn't call computeSysRoot() so it looks like it is possible to make SysRoot a local variable here (ideally changing its name). This will lose some performance as it looks like computeSysRoot() is called several times later in addLibStdCxxIncludePaths() and AddClangCXXStdlibIncludeArgs() and there's no other place that assigns to SysRoot but findMultiLibs() , but it could be done.
Hopefully that explains it. In summary we can make SysRoot a local variable here with no loss in functionality but IIUC we'll lose some performance, which is likely negligble.
On the files changed in #121831 specifically I don't have a strong opinion. I can see the merit in making changes specific to RISCV, at least initially; although I think it would be likely fine to do those for all toolchains when the GCC installation is valid. If you mean all the PRs in the stack, that might take me a few days to work through them. |
smithp35
requested changes
Jun 11, 2025
quic-garvgupt
force-pushed
the
users/quic-garvgupt/compile_line
branch
from
c4d61f3 to
f19e2e0
Compare
smithp35
approved these changes
Jun 13, 2025
petrhosek
approved these changes
Jun 17, 2025
This patch introduces the baretmetal toolchain object about GCC Installation. Currently, if `--gcc-installation` ot `--gcc-install-dir` options are passed on commandline, then sysroot will be formed from there if paths will be valid. Otherwise, it will be fallback to as it already existed in the Baremetal toolchaibn object. Moreover, support for adding include paths for libstd C++ library is added as well. Additionally, the restriction to always use integrated assembler is removed because with valid gcc installation, gnu assembler can be invoked as well. This patch currently adds and modifies arm related test only. The riscv specific test will be added in the last PR when driver code related to calling of RISCVToolchain object will be removed. Currently in this PR, there is no way to test riscv target. RFC: https://discourse.llvm.org/t/merging-riscvtoolchain-and-baremetal-toolchains/75524 Change-Id: Ibaeb569cf7e2cee03c022aa9ecd1abe29d5c30d4
quic-garvgupt
force-pushed
the
users/quic-garvgupt/compile_line
branch
from
f19e2e0 to
99664ba
Compare
|
Rebased the stack on the tip of the branch. Will start merging the stack as soon as CI checks gets completed. Thanks again petrhosek and petersmith for reviewing the changes. |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/11/builds/17495 Here is the relevant piece of the build log for the reference |
|
This is consistently breaking the Fuchsia builders as seen in the above buildbot. Accordingly, I'm issuing a rollback. This will take just a bit of time; if you have a trivial fix forward, let know, and I'll pause. |
mysterymath
added a commit
that referenced
this pull request
Jun 17, 2025
#144603) …Baremetal toolchain (#121829)" This reverts the following stack of commits, due to them breaking the Fuchsia toolchain and corresponding LLVM buildbot. Revert "[Driver] Fix Arm/AArch64 Link Argument tests (#144582)" This reverts commit a79186c. Revert "[Driver] Add option to force undefined symbols during linking in BareMetal toolchain object. (#132807)" This reverts commit 9cb7545. Revert "[Driver] Fix link order of BareMetal toolchain object (#132806)" This reverts commit 31523de. Revert "[Driver] Add support for crtbegin.o, crtend.o and libgloss lib to BareMetal toolchain object (#121830)" This reverts commit ec230aa. Revert "[Driver] Add support for GCC installation detection in Baremetal toolchain (#121829)" This reverts commit eb31c42.
mysterymath
added a commit
to mysterymath/llvm-project
that referenced
this pull request
Jun 17, 2025
…Baremetal toolchain (llvm#121829)" This reverts the following stack of commits, due to them breaking the Fuchsia toolchain and corresponding LLVM buildbot. Revert "[Driver] Fix Arm/AArch64 Link Argument tests (llvm#144582)" This reverts commit a79186c. Revert "[Driver] Add option to force undefined symbols during linking in BareMetal toolchain object. (llvm#132807)" This reverts commit 9cb7545. Revert "[Driver] Fix link order of BareMetal toolchain object (llvm#132806)" This reverts commit 31523de. Revert "[Driver] Add support for crtbegin.o, crtend.o and libgloss lib to BareMetal toolchain object (llvm#121830)" This reverts commit ec230aa. Revert "[Driver] Add support for GCC installation detection in Baremetal toolchain (llvm#121829)" This reverts commit eb31c42.
|
Do the fuchsia builders actually produce output on a failure? We seem to just get exit code 1, but no indication of what filecheck had problems with. |
They do; here's a link! Let me know if you have an problems with the Web UI; I can paste things here if so. |
|
Nice, I can navigate that UI. Looks like a similar thing that I fixed-forward, but this time with a different unwind library, but clang isn't very happy with mixing unwind libraries anyway. I think I'll leave this to @quic-garvgupt to fix, using the info from your link. |
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 patch introduces enhancements to the Baremetal toolchain to support GCC toolchain detection.
Additionally, the restriction to always use the integrated assembler has been removed. With a valid GCC installation, the GNU assembler can now be used as well.
This patch currently updates and adds tests for the ARM target only. RISC-V-specific tests will be introduced in a later patch, once the RISCVToolChain is fully merged into the Baremetal toolchain. At this stage, there is no way to test the RISC-V target within this PR.
RFC: https://discourse.llvm.org/t/merging-riscvtoolchain-and-baremetal-toolchains/75524