[RISCV] Improve sysroot computation if no GCC install detected

[edward-jones]

Summary

If a GCC installed is not detected, the driver would default to the root of the filesystem. This is not ideal when this doesn't match the install directory of the toolchain and can cause undesireable behavior such as picking up system libraries or the system linker when cross-compiling.

[edward-jones]

Before falling back to returning the empty string this first looks for a directory with the triple name up one level from the driver.

This uses the Triple string in the path, however this Triple has been normalized so even if the user specifies "clang --target=riscv32-unknown-elf", it will use "riscv32-unknown-unknown-elf" when looking for the sysroot. I am wondering whether it would be sensible to do a more "intelligent" search for the sysroot here by iteratively stripping "unknown"s from the triple.

[edward-jones]

I'm reworking this at the moment and adding some tests. Standby!

[edward-jones]

Rebased and added tests

I've made this use the Triple from the driver rather than the parsed LLVM triple, this means the Triple doesn't get normalized which seems like more desirable behavior.

I've added to the riscv{32,64}-toolchain.c test files, however the added tests cannot be run without a shell so I've had to disable those tests on Windows. If necessary I can split these new tests out into separate files.

I realize that there don't appear to be any tests for the case where no GCC install is found and no sysroot is provided to the driver. At the moment this will result in a generic linker command using the system linker, such as:

/usr/bin/ld crt0.o crtbegin.o ... -lgcc crtend.o

Is this the desired behaviour? And if so should a test be added for this too?

[luismarques]

This is indeed an issue that would be nice to fix, I've often been annoyed by clang just defaulting to the root when some misconfiguration occurs. I have to wonder though, this patch only changes the clang RISC-V toolchain driver, but the problem isn't specific to RISC-V. Couldn't this tweak be generalized and made to apply to multiple/all target drivers?

[lenary]

In D68391#1694622, @edward-jones wrote:

Rebased and added tests

I've made this use the Triple from the driver rather than the parsed LLVM triple, this means the Triple doesn't get normalized which seems like more desirable behavior.

Yeah, using the user-provided triple seems the correct choice, as the user will expect it to match the directory name. Would be useful to add a comment saying something along the same lines where you do that.

I've added to the riscv{32,64}-toolchain.c test files, however the added tests cannot be run without a shell so I've had to disable those tests on Windows. If necessary I can split these new tests out into separate files.

I think splitting the tests might be sensible, so that we can run as many as possible on platforms without a shell/windows.

I realize that there don't appear to be any tests for the case where no GCC install is found and no sysroot is provided to the driver. At the moment this will result in a generic linker command using the system linker, such as:

/usr/bin/ld crt0.o crtbegin.o ... -lgcc crtend.o

Is this the desired behaviour? And if so should a test be added for this too?

I think defaulting to root if no sysroot/gcc install is found is better than erroring. In all likelihood a compile/link task will fail anyway because it cannot link the results together. In the case where the compile/link does work, then there's no issue. It would be useful to have a test for this.

In D68391#1707652, @luismarques wrote:

This is indeed an issue that would be nice to fix, I've often been annoyed by clang just defaulting to the root when some misconfiguration occurs. I have to wonder though, this patch only changes the clang RISC-V toolchain driver, but the problem isn't specific to RISC-V. Couldn't this tweak be generalized and made to apply to multiple/all target drivers?

I think that for baremetal risc-v, we can change this default without asking the wider community. For other targets, someone should email cfe-dev first with the proposal. I agree it makes sense, but I also imagine it could break a lot of builds unexpectedly.

[lenary]

LGTM.

[edward-jones]

Rebased, added a comment to explain that this is using the user provided triple instead of the canonical one, and split out the tests requiring a shell into separate files (clang/test/Driver/riscv{32,64}-toolchain-extra.c)

[lenary]

Thanks for the rebase. It looks good, let's get it committed!

[thakis]

The test seems to fail on Windows, could you take a look and revert if it takes a while to fix?

[thakis]

Sorry, meant to also paste a link to a failing build: http://lab.llvm.org:8011/builders/clang-x64-windows-msvc/builds/11994/steps/stage%201%20check/logs/stdio

[thakis]

nvm, rnk just got to it in https://github.com/llvm/llvm-project/commit/f37b5c800e150ad915c4e0571edd2c92c0160d89

clang/lib/Driver/ToolChains/RISCVToolchain.cpp

RISCVToolChain::RISCVToolChain(const Driver &D, const llvm::Triple &Triple,
                                const ArgList &Args)
     : Generic_ELF(D, Triple, Args) {
   GCCInstallation.init(Triple, Args);
   getFilePaths().push_back(computeSysRoot() + "/lib");
   if (GCCInstallation.isValid()) {
     getFilePaths().push_back(GCCInstallation.getInstallPath().str());
     getProgramPaths().push_back(
         (GCCInstallation.getParentLibPath() + "/../bin").str());
+  } else {
+    getProgramPaths().push_back(D.Dir);
   }
 }
 
 Tool *RISCVToolChain::buildLinker() const {
   return new tools::RISCV::Linker(*this);
 }
 
 void RISCVToolChain::addClangTargetOptions(
void RISCVToolChain::addLibStdCxxIncludePaths(
   addLibStdCXXIncludePaths(computeSysRoot() + "/include/c++/" + Version.Text,
       "", TripleStr, "", "", Multilib.includeSuffix(), DriverArgs, CC1Args);
 }
 
 std::string RISCVToolChain::computeSysRoot() const {
   if (!getDriver().SysRoot.empty())
     return getDriver().SysRoot;
 
-  if (!GCCInstallation.isValid())
-    return std::string();
-
+  SmallString<128> SysRootDir;
+  if (GCCInstallation.isValid()) {
     StringRef LibDir = GCCInstallation.getParentLibPath();
     StringRef TripleStr = GCCInstallation.getTriple().str();
-  std::string SysRootDir = LibDir.str() + "/../" + TripleStr.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 SysRootDir;
+  return SysRootDir.str();
 }
 
 void RISCV::Linker::ConstructJob(Compilation &C, const JobAction &JA,
                                  const InputInfo &Output,
                                  const InputInfoList &Inputs,
                                  const ArgList &Args,
                                  const char *LinkingOutput) const {
   const ToolChain &ToolChain = getToolChain();

clang/test/Driver/Inputs/basic_riscv32_nogcc_tree/bin/riscv32-unknown-elf-ld

+#!/bin/true

clang/test/Driver/Inputs/basic_riscv64_nogcc_tree/bin/riscv64-unknown-elf-ld

+#!/bin/true

clang/test/Driver/riscv32-toolchain-extra.c

+// A basic clang -cc1 command-line, and simple environment check.
+
+// The tests here are similar to those in riscv32-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: mkdir -p %T/testroot-riscv32-baremetal-nogcc/bin
+// RUN: [ ! -s %T/testroot-riscv32-baremetal-nogcc/bin/clang ] || rm %T/testroot-riscv32-baremetal-nogcc/bin/clang
+// RUN: [ ! -s %T/testroot-riscv32-baremetal-nogcc/bin/riscv32-unknown-elf-ld ] || rm %T/testroot-riscv32-baremetal-nogcc/bin/riscv32-unknown-elf-ld
+// RUN: [ ! -s %T/testroot-riscv32-baremetal-nogcc/riscv32-unknown-elf ] || rm %T/testroot-riscv32-baremetal-nogcc/riscv32-unknown-elf
+// RUN: ln -s %clang %T/testroot-riscv32-baremetal-nogcc/bin/clang
+// RUN: ln -s %S/Inputs/basic_riscv32_nogcc_tree/bin/riscv32-unknown-elf-ld %T/testroot-riscv32-baremetal-nogcc/bin/riscv32-unknown-elf-ld
+// RUN: ln -s %S/Inputs/basic_riscv32_nogcc_tree/riscv32-unknown-elf %T/testroot-riscv32-baremetal-nogcc/riscv32-unknown-elf
+// RUN: %T/testroot-riscv32-baremetal-nogcc/bin/clang %s -### -no-canonical-prefixes \
+// RUN:    -target riscv32-unknown-elf 2>&1 \
+// RUN:    | FileCheck -check-prefix=C-RV32-BAREMETAL-ILP32-NOGCC %s
+
+// C-RV32-BAREMETAL-ILP32-NOGCC: InstalledDir: [[DRIVERDIR:.*]]
+// C-RV32-BAREMETAL-ILP32-NOGCC: "-fuse-init-array"
+// C-RV32-BAREMETAL-ILP32-NOGCC: "-internal-isystem" "[[DRIVERDIR]]/../riscv32-unknown-elf/include"
+// C-RV32-BAREMETAL-ILP32-NOGCC: "[[DRIVERDIR]]/riscv32-unknown-elf-ld"
+// C-RV32-BAREMETAL-ILP32-NOGCC: "[[DRIVERDIR]]/../riscv32-unknown-elf/lib/crt0.o"
+// C-RV32-BAREMETAL-ILP32-NOGCC: "[[DRIVERDIR]]/../riscv32-unknown-elf/lib/crtbegin.o"
+// C-RV32-BAREMETAL-ILP32-NOGCC: "-L[[DRIVERDIR]]/../riscv32-unknown-elf/lib"
+// C-RV32-BAREMETAL-ILP32-NOGCC: "--start-group" "-lc" "-lgloss" "--end-group" "-lgcc"
+// C-RV32-BAREMETAL-ILP32-NOGCC: "[[DRIVERDIR]]/../riscv32-unknown-elf/lib/crtend.o"

clang/test/Driver/riscv64-toolchain-extra.c

+// A basic clang -cc1 command-line, and simple environment check.
+
+// The tests here are similar to those in riscv64-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: mkdir -p %T/testroot-riscv64-baremetal-nogcc/bin
+// RUN: [ ! -s %T/testroot-riscv64-baremetal-nogcc/bin/clang ] || rm %T/testroot-riscv64-baremetal-nogcc/bin/clang
+// RUN: [ ! -s %T/testroot-riscv64-baremetal-nogcc/bin/riscv64-unknown-elf-ld ] || rm %T/testroot-riscv64-baremetal-nogcc/bin/riscv64-unknown-elf-ld
+// RUN: [ ! -s %T/testroot-riscv64-baremetal-nogcc/riscv64-unknown-elf ] || rm %T/testroot-riscv64-baremetal-nogcc/riscv64-unknown-elf
+// RUN: ln -s %clang %T/testroot-riscv64-baremetal-nogcc/bin/clang
+// RUN: ln -s %S/Inputs/basic_riscv64_nogcc_tree/bin/riscv64-unknown-elf-ld %T/testroot-riscv64-baremetal-nogcc/bin/riscv64-unknown-elf-ld
+// RUN: ln -s %S/Inputs/basic_riscv64_nogcc_tree/riscv64-unknown-elf %T/testroot-riscv64-baremetal-nogcc/riscv64-unknown-elf
+// RUN: %T/testroot-riscv64-baremetal-nogcc/bin/clang %s -### -no-canonical-prefixes \
+// RUN:    -target riscv64-unknown-elf 2>&1 \
+// RUN:    | FileCheck -check-prefix=C-RV64-BAREMETAL-LP64-NOGCC %s
+
+// C-RV64-BAREMETAL-LP64-NOGCC: InstalledDir: [[DRIVERDIR:.*]]
+// C-RV64-BAREMETAL-LP64-NOGCC: "-fuse-init-array"
+// C-RV64-BAREMETAL-LP64-NOGCC: "-internal-isystem" "[[DRIVERDIR]]/../riscv64-unknown-elf/include"
+// C-RV64-BAREMETAL-LP64-NOGCC: "[[DRIVERDIR]]/riscv64-unknown-elf-ld"
+// C-RV64-BAREMETAL-LP64-NOGCC: "[[DRIVERDIR]]/../riscv64-unknown-elf/lib/crt0.o"
+// C-RV64-BAREMETAL-LP64-NOGCC: "[[DRIVERDIR]]/../riscv64-unknown-elf/lib/crtbegin.o"
+// C-RV64-BAREMETAL-LP64-NOGCC: "-L[[DRIVERDIR]]/../riscv64-unknown-elf/lib"
+// C-RV64-BAREMETAL-LP64-NOGCC: "--start-group" "-lc" "-lgloss" "--end-group" "-lgcc"
+// C-RV64-BAREMETAL-LP64-NOGCC: "[[DRIVERDIR]]/../riscv64-unknown-elf/lib/crtend.o"