[Driver] Teach Barmetal toolchain about GCC installation #121829
Conversation
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! |
| 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. |
quic-garvgupt
force-pushed
the
users/quic-garvgupt/compile_line
branch
from
03b91a8 to
a4a50a2
Compare
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
quic-garvgupt
force-pushed
the
users/quic-garvgupt/compile_line
branch
from
a4a50a2 to
35d581f
Compare
quic-garvgupt
force-pushed
the
users/quic-garvgupt/compile_line
branch
from
35d581f to
9bb7823
Compare
|
Hi @petrhosek, I've addressed all your comments. Please review the changes and approve the PR if everything looks good. |
| getLibraryPaths().push_back(std::string(Dir)); | ||
| : Generic_ELF(D, Triple, Args) { | ||
| GCCInstallation.init(Triple, Args); | ||
| SysRoot = computeSysRoot(); |
There was a problem hiding this comment.
I'd make this a local variable.
| SysRoot = computeSysRoot(); | |
| std::string SysRoot = computeSysRoot(); |
| @@ -79,7 +87,6 @@ class LLVM_LIBRARY_VISIBILITY BareMetal : public ToolChain { | |||
| OrderedMultilibs getOrderedMultilibs() const; | |||
|
|
|||
| std::string SysRoot; | |||
There was a problem hiding this comment.
I'd remove this class member to avoid confusion whether you're referring to a local variable or a class member in methods.
| std::string SysRoot; |
There was a problem hiding this comment.
Removing this private variable was making the code messy because there have been updates to this variable in some member functions and the updated value is then further used in other functions. Removing this variable would need to pass this as an argument to all those functions which would make the code less readable.
To avoid the confusion, all local variables for sysroot are named as SysRootDir and any references to this variable as SysRoot only
quic-garvgupt
force-pushed
the
users/quic-garvgupt/compile_line
branch
from
9bb7823 to
e07a4cd
Compare
quic-garvgupt
changed the title
|
Hi @petrhosek, I've addressed all your comments. Please review the changes and approve the PR if everything looks good. |
quic-garvgupt
force-pushed
the
users/quic-garvgupt/compile_line
branch
from
e07a4cd to
1d0db96
Compare
|
Gentle Ping again! |
There was a problem hiding this comment.
I'm looking at this patch in isolation, so its possible that the comments here are addressed elasewhere.
It looks like useful functionality to add to the bare metal driver.
It also looks like there is some scope for some documentation, for users and for toolchain developers. Users will need to know about what options to set to point at their GCC installation (or place adjacent). Non-GNU toolchain developers will need to avoid <triple>/lib/crt0.o. If multilibs doesn't work for all toolchains yet, would be good to add that as a limitation.
That could be done with a follow up patch though.
| @@ -110,20 +110,81 @@ static std::string computeBaseSysRoot(const Driver &D, bool IncludeTriple) { | |||
| return std::string(SysRootDir); | |||
| } | |||
|
|
|||
| static bool hasGCCToolChainAlongSideClang(const Driver &D) { | |||
There was a problem hiding this comment.
IIUC this is when we have something like clang copied into a GNU installation bin directory or another directory at the same level. So that bin\clang\..\<target-triple>\lib\crt0.o
One minor concern is that almost any toolchain will have a crt0.o although in the current bare-metal driver they are most likely in clang-runtimes so won't clash with the location.
I'm wondering if there's a common file that is really GCC specific so we can avoid inferring a GNU toolchain by mistake.
We could decide that this is unlikely, or make it a requirment that non GNU based bare-metal toolchains just avoid this. If that's the case could we document this? I'll leave a separate comment surrounding documentation.
There was a problem hiding this comment.
If understand correctly, this should cover the case when we're using newlib (and potentially libgcc and libstdc++) together with Clang (but not using clang-runtimes). @quic-garvgupt can you clarify this? If that's the case, then I think this function could use a better name. In either case, I agree with @smithp35 that this logic is fragile since crt0.o is commonly used by most C libraries.
There was a problem hiding this comment.
IIUC this is when we have something like clang copied into a GNU installation bin
This logic was again borrowed from RISCVToolChain which forms the sysroot from the target triple prefixed directory same as that of clang when gcc-toolchain is not explicitly specified. Although, the logic might seems shaky, but as already pointed out that for bare-metal driver, crt0.o file will be in clang-runtimes so it won't clash. Given that it is bit peculiar, I agree that it does warrants some documentation somewhere.
I'm wondering if there's a common file that is really GCC specific so we can avoid inferring a GNU toolchain by mistake.
I am not aware if there is any common file that is really GCC specific. Pls do let me know if there is such a file or any other feedback/comments on how this can be handled better in general.
If understand correctly, this should cover the case when we're using newlib (and potentially libgcc and
computeSysRoot is the function which differentiate when we are using sysroot from GCCInstallation or clang-runtime. The function hasGCCToolChainAlongSideClang is called inside the computeSysRoot function which forms sysroot from the target triple prefixed directory same as that of clang. However, this will only be done when there is indeed a GCCinstallation detected alongside clang. The logic of detection is borrowed from RISCVToolChain Object.
Apologies for the late reply.
There was a problem hiding this comment.
I am not aware if there is any common file that is really GCC specific. Pls do let me know if there is such a file or any other feedback/comments on how this can be handled better in general.
I don't think there's a single common file that is unique to GCC.
Stepping back a bit, there's a bit of the logic/use-case in computeSysRoot that I don't understand. When we find a GCC ToolChain alongside clang, I don't see any logic to validate it. There is code later on in BareMetal::BareMetal to use the GNU multilib detection only if the GCC installation is valid.
Is the intention here to find a valid GCC installation equivalent to the -gcc-install-dir or --gcc-toolchain, or something else? For example this could be an equivalent to lib/clang-runtimes although in that case it wouldn't necessarily be a GCC installation.
If the intention is to find a valid installation then can we borrow some of the logic from the GCCInstallation?
clang/test/Driver/arm-gnutools.c
Outdated
| @@ -0,0 +1,12 @@ | |||
| // check that gnu assembler is invoked with arm baremetal as well | |||
|
|
|||
| // RUN: %clang --target=armv6m-none-eabi --gcc-toolchain=%S/Inputs/basic_riscv32_tree -fno-integrated-as %s -### -c \ | |||
There was a problem hiding this comment.
I'm a bit confused about this test. It is called arm-gnutools.c, but has both arm and aarch64 triples, and uses a basic_riscv32 tree.
Is it just that we're using basic_riscv32_tree to tell the driver that it is in GNU "mode" so look for <triple>-as
Could be worth a comment if that's the case. There's a possibility that the driver could actually look for the exectuable in the GCC toolchain.
There was a problem hiding this comment.
Thanks for catching this!. I fixed this in some previous version directly on this branch through which I push however didn't do it on my development branch where I do all my experiments. Therefore while making further changes on this PR I still ended up pushing the incorrect version. In the current version, I am using the correct gcc-toolchain path and armv6m and aarch64 tests are in different files.
quic-garvgupt
force-pushed
the
users/quic-garvgupt/compile_line
branch
from
1d0db96 to
0565206
Compare
Thanks for providing feedback @smithp35. I can add documentation related to the points you mentioned. Pls let me know if adding them in the BareMetal.cpp file would be ok or should I consider adding it in some other file. |
I think developer documentation in BareMetal.cpp is fine. User documentation is probably best in the clang/docs, for example updating https://github.com/llvm/llvm-project/blob/main/clang/docs/Toolchain.rst |
| @@ -110,20 +110,81 @@ static std::string computeBaseSysRoot(const Driver &D, bool IncludeTriple) { | |||
| return std::string(SysRootDir); | |||
| } | |||
|
|
|||
| static bool hasGCCToolChainAlongSideClang(const Driver &D) { | |||
There was a problem hiding this comment.
I am not aware if there is any common file that is really GCC specific. Pls do let me know if there is such a file or any other feedback/comments on how this can be handled better in general.
I don't think there's a single common file that is unique to GCC.
Stepping back a bit, there's a bit of the logic/use-case in computeSysRoot that I don't understand. When we find a GCC ToolChain alongside clang, I don't see any logic to validate it. There is code later on in BareMetal::BareMetal to use the GNU multilib detection only if the GCC installation is valid.
Is the intention here to find a valid GCC installation equivalent to the -gcc-install-dir or --gcc-toolchain, or something else? For example this could be an equivalent to lib/clang-runtimes although in that case it wouldn't necessarily be a GCC installation.
If the intention is to find a valid installation then can we borrow some of the logic from the GCCInstallation?
| .str()); | ||
| PPaths.push_back((GCCInstallation.getParentLibPath() + "/../bin").str()); | ||
| getFilePaths().push_back(SysRoot + "/lib"); | ||
| } else { |
There was a problem hiding this comment.
IIUC we hit this point when in BareMetal::computeSysRoot() when the GCCInstallation isn't valid, but there is a GCC Toolchain adjacent
if (GCCInstallation.isValid()) {
...
}
else if (hasGCCToolChainAlongSideClang(D)) {
// 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, D.Dir, "..", D.getTargetTriple());
}
The logic in this block is the same as if clang-runtimes with multilib.yaml were used. Is this intentional?
There was a problem hiding this comment.
Stepping back a bit, there's a bit of the logic/use-case in computeSysRoot that I don't understand. When we find a GCC ToolChain alongside clang, I don't see any logic to validate it.
There is no code to validate it in the constructor. However, it's use was needed in the linker job (see Linker::ConstructJob). This was done to preserve the behavior of RISCVToolChain Object. I am not sure why it was decided there that the mere presence of lib/crt0.o file alongside clang in target triple prefixed directory will indicate a GCC toolchain, however in RISCVToolchain object, toolchain found along clang isn't considered a valid GCCInstallation and the same has been implemented here to preserve that behavior.
One needs to look at hasGCCToolchain, computeSysRoot and constructor in RISCVToolChain.cpp for the same
Is the intention here to find a valid GCC installation equivalent to the -gcc-install-dir or --gcc-toolchain, or something else?
Yes it is trying to find a toolchain that is equivalent to --gcc-toolchain. The same is also mentioned in the comments for the function hasGCCToolChain in RISCVToolChain.cpp. You can also look at tests "clang/test/Driver/riscv64-toolchain-extra.c` and "clang/test/Driver/riscv32-toolchain-extra.c" to see how it works.
The logic in this block is the same as if clang-runtimes with multilib.yaml were used. Is this intentional?
I am not sure if I understood your question completely. But yes, the logic is a bit similar but the directory for sysroot will be different. In the case when GCCInstallation isn't valid but there is a GCC toolchain adjacent, then the sysroot would be bin/../<target-triple>/lib/crt0.o whereas with clang-runtimes with multilib.yaml, it will be bin/../lib/clang-runtimes/ (include triple is set to false, when multilib.yaml is found).
Apologies for the late reply. I was out for sometime in the last month and then I my commit access got revoked. Thanks for reviewing this patch and pls let me know if you have further questions/comments.
There was a problem hiding this comment.
OK I see that this is just using the ../lib/ rather than using the GCCInstallation.
quic-garvgupt
force-pushed
the
users/quic-garvgupt/compile_line
branch
from
0565206 to
8d6d2e0
Compare
llvmbot
added
the
clang:frontend
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
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. |
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.
| Triple.getArch() != llvm::Triple::aarch64_be) | ||
| return false; | ||
| // Users can specify their GCC toolchain using `-gcc-install-dir` or | ||
| // `--gcc-toolchain`. If no sysroot is explicitly provided, the driver will |
There was a problem hiding this comment.
I've got a mild preference for putting the sysroot part first. For example:
If no sysroot is provided the driver will first attempt to infer it from the values of `--gcc-install-dir` or `--gcc-toolchain`, which specify the location of a GCC toolchain.
If neither flag is used ...
| // lib/clang-runtimes above the driver. | ||
| SmallString<128> SysRootDir; | ||
| if (GCCInstallation.isValid()) { | ||
| StringRef LibDir = GCCInstallation.getParentLibPath(); |
There was a problem hiding this comment.
Could these be inlined? I'm not sure that the temporary names add much value, and it looks closer to the code in the else if branch.
llvm::sys::path::append(SysRootDir, GCCInstallation.getParentLibPath(), "..", GCCInstallation.getTriple().str());
|
|
||
| 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?
| return Triple.isPPC() && Triple.getOS() == llvm::Triple::UnknownOS && | ||
| Triple.getEnvironment() == llvm::Triple::EABI; | ||
| // GCC mutltilibs will only work for those targets that have their multlib | ||
| // structure encoded into GCCInstallation. Baremetal toolchain support ARM, |
There was a problem hiding this comment.
Typo support -> supports
I also suggest using "and of these only ... " rather than "and of them"
| 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.
| } 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?
| .str()); | ||
| PPaths.push_back((GCCInstallation.getParentLibPath() + "/../bin").str()); | ||
| getFilePaths().push_back(SysRoot + "/lib"); | ||
| } else { |
There was a problem hiding this comment.
OK I see that this is just using the ../lib/ rather than using the GCCInstallation.
This patch introduces the baretmetal toolchain object about GCC Installation.
Currently, if
--gcc-installationot--gcc-install-diroptions are passed oncommandline, 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