[RISCV] Add support for computing sysroot for riscv32-unknown-elf

[lewis-revill]

Summary

Extends r338385 to allow the driver to compute the sysroot when an explicit path is not provided. This allows the linker to find C runtime files and the correct include directory for header files.

[lewis-revill]

Modified test which assumed the path to C++ includes was determined without use of the sysroot path.

[xbolva00]

Nico Weber (cfe-commits)

"I'm getting this warning from the mac linker after this commit:

/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/libtool: warning same member name (libclangDriver.RISCV.o) in output file used for input files: obj/clang/lib/Driver/ToolChains/Arch/libclangDriver.RISCV.o and: obj/clang/lib/Driver/ToolChains/libclangDriver.RISCV.o (due to use of basename, truncation, blank padding or duplicate input files)

Could we rename the file to fix that warning?"

[lewis-revill]

@xbolva00 In my opinion this is an issue for another revision. Personally I would choose to rename/move the other RISCV driver file to something along the lines of RISCVLinux, however I think it is best for @asb to decide since he is the code owner for the RISC-V backend.

[simoncook]

I've tested this, regression tests involving linking now mostly pass as crt0 can now be found, but it seems that system headers are still being pulled in causing a couple of tests to fail. In particular using limits.h is causing build failures for me.

Compiling with -v, I see for riscv32-unknown-elf-clang

#include "..." search starts here:
#include <...> search starts here:
 /data/jenkins/workspace/riscv32-llvm-gcc-branches/install/bin/../lib/gcc/riscv32-unknown-elf/8.2.0/../../../../riscv32-unknown-elf/include
 /usr/local/include
 /data/jenkins/workspace/riscv32-llvm-gcc-branches/install/lib/clang/8.0.0/include
 /usr/include
End of search list.

whereas for riscv32-unknown-elf-gcc:

#include "..." search starts here:
#include <...> search starts here:
 /data/jenkins/workspace/riscv32-llvm-gcc-branches/install/lib/gcc/riscv32-unknown-elf/8.2.0/include
 /data/jenkins/workspace/riscv32-llvm-gcc-branches/install/lib/gcc/riscv32-unknown-elf/8.2.0/include-fixed
 /data/jenkins/workspace/riscv32-llvm-gcc-branches/install/lib/gcc/riscv32-unknown-elf/8.2.0/../../../../riscv32-unknown-elf/include
End of search list.

Can you see why system headers are still being pulled in unless --sysroot is specified?

[lewis-revill]

Fixed this issue by adding -nostdsysteminc to the Clang target options, preventing the frontend from generating additional include paths.

[simoncook]

My tests now look better, there are a couple of failures, but this seems to be a bug in newlib, rather than with clang/this patch (the bug was masked before as we would have been pulling in system headers). So this patch looks good to me.

@asb Does this look good to you/are you able to commit this?

[asb]

Thanks for the patch and sorry for the delay. Once someone else marked it accepted it moves to a separate list in Phabricator - obviously I need to check the 'waiting on authors' list better.

As I see it are two changes here:

1. Calculating libcxx include paths based on specified sysroot
2. Computing the sysroot when an explicit path isn't provided

Could you please add a test that covers 2) above?

[lewis-revill]

Added tests for the case where --sysroot is not provided.

[asb]

Looks good to me, thanks!

[lewis-revill]

Rebased and ensured that -fuse-init-array is always used (as intended in D50043), since addClangTargetOptions is overidden for this patch.

[lewis-revill]

@asb can we get this committed?

[kristina]

Going to land it as it's approved by code owner.

[kristina]

Closed by rC341655.

[kristina]

Seems to be causing a test failure for someone (per comment in rL341655), reopened it for now (fails when sysroot is set via DEFAULT_SYSROOT in CMake flags).

[rogfer01]

Thanks for reopening this @kristina.

I suggest passing --sysroot= to make sure we see the expected behaviour when the sysroot is actually empty.

Note that this would not really test the scenario where DEFAULT_SYSROOT is empty and no --sysroot appears in the command line. I'm not sure if we really want to test that case (but if we do, I think we will have to move that case into a test of its own and add a feature in lit.cfg.py that describes that clang does not have any built-in default sysroot).

Thoughts?

[kristina]

In D50246#1229068, @rogfer01 wrote:

Thanks for reopening this @kristina.

I suggest passing --sysroot= to make sure we see the expected behaviour when the sysroot is actually empty.

Note that this would not really test the scenario where DEFAULT_SYSROOT is empty and no --sysroot appears in the command line. I'm not sure if we really want to test that case (but if we do, I think we will have to move that case into a test of its own and add a feature in lit.cfg.py that describes that clang does not have any built-in default sysroot).

Thoughts?

Seems like a fairly niche case, you can request changes to this revision but there's been no regressions otherwise, buildbots have been handling this fine.

I think this may be something that would belong in a separate diff for the test suite, not for this particular diff. You can always just submit a diff for the test suite if you think it should have that option and if it would be useful as a general thing.

[rogfer01]

Hi @kristina .

Sure, I didn't mean to do that broader change here. Apologies if it read that way.

Would it be acceptable to add an empty --sysroot= to the test? I can post the change for review in another diff.

Thanks a lot.

[kristina]

In D50246#1229152, @rogfer01 wrote:

Hi @kristina .

Sure, I didn't mean to do that broader change here. Apologies if it read that way.

Would it be acceptable to add an empty --sysroot= to the test? I can post the change for review in another diff.

Thanks a lot.

Yes, you can submit another diff for this specific test, just use "Update Diff" and add yours on top which should open it up for re-review or use "Commandeer Revision" (and then submit another diff since it was previously closed). Up to your judgement.

[lebedev.ri]

In D50246#1229177, @kristina wrote:

In D50246#1229152, @rogfer01 wrote:

Hi @kristina .

Sure, I didn't mean to do that broader change here. Apologies if it read that way.

Would it be acceptable to add an empty --sysroot= to the test? I can post the change for review in another diff.

Thanks a lot.

Yes, you can submit another diff for this specific test, just use "Update Diff" and add yours on top which should open it up for re-review or use "Commandeer Revision" (and then submit another diff since it was previously closed). Up to your judgement.

Please don't perform necromancy on already committed and closed differentials (unless the commit was reverted, of course).
Do open new differentials.

[kristina]

In D50246#1229191, @lebedev.ri wrote:

In D50246#1229177, @kristina wrote:

In D50246#1229152, @rogfer01 wrote:

Hi @kristina .

Sure, I didn't mean to do that broader change here. Apologies if it read that way.

Would it be acceptable to add an empty --sysroot= to the test? I can post the change for review in another diff.

Thanks a lot.

Yes, you can submit another diff for this specific test, just use "Update Diff" and add yours on top which should open it up for re-review or use "Commandeer Revision" (and then submit another diff since it was previously closed). Up to your judgement.

Please don't perform necromancy on already committed and closed differentials (unless the commit was reverted, of course).
Do open new differentials.

Fair enough, sorry I suggested that.

lib/Driver/ToolChains/RISCV.h

 namespace toolchains {
 
 class LLVM_LIBRARY_VISIBILITY RISCVToolChain : public Generic_ELF {
 public:
   RISCVToolChain(const Driver &D, const llvm::Triple &Triple,
                  const llvm::opt::ArgList &Args);
 
   bool IsIntegratedAssemblerDefault() const override { return true; }
+  void addClangTargetOptions(const llvm::opt::ArgList &DriverArgs,
+                             llvm::opt::ArgStringList &CC1Args,
+                             Action::OffloadKind) const override;
   void
   AddClangSystemIncludeArgs(const llvm::opt::ArgList &DriverArgs,
                             llvm::opt::ArgStringList &CC1Args) const override;
   void
   addLibStdCxxIncludePaths(const llvm::opt::ArgList &DriverArgs,
                            llvm::opt::ArgStringList &CC1Args) const override;
 
 protected:
   Tool *buildLinker() const override;
+
+private:
+  std::string computeSysRoot() const;
 };
 
 } // end namespace toolchains
 
 namespace tools {
 namespace RISCV {
 class LLVM_LIBRARY_VISIBILITY Linker : public GnuTool {
 public:

lib/Driver/ToolChains/RISCV.cpp

 //===--- RISCV.cpp - RISCV ToolChain Implementations ------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "RISCV.h"
 #include "CommonArgs.h"
 #include "InputInfo.h"
 #include "clang/Driver/Compilation.h"
 #include "clang/Driver/Options.h"
 #include "llvm/Option/ArgList.h"
+#include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/raw_ostream.h"
 
 using namespace clang::driver;
 using namespace clang::driver::toolchains;
 using namespace clang::driver::tools;
 using namespace clang;
 using namespace llvm::opt;
 
 /// RISCV Toolchain
 RISCVToolChain::RISCVToolChain(const Driver &D, const llvm::Triple &Triple,
                                const ArgList &Args)
     : Generic_ELF(D, Triple, Args) {
   GCCInstallation.init(Triple, Args);
-  getFilePaths().push_back(D.SysRoot + "/lib");
+  getFilePaths().push_back(computeSysRoot() + "/lib");
   if (GCCInstallation.isValid()) {
     getFilePaths().push_back(GCCInstallation.getInstallPath().str());
     getProgramPaths().push_back(
         (GCCInstallation.getParentLibPath() + "/../bin").str());
   }
 }
 
 Tool *RISCVToolChain::buildLinker() const {
   return new tools::RISCV::Linker(*this);
 }
 
+void RISCVToolChain::addClangTargetOptions(
+    const llvm::opt::ArgList &DriverArgs,
+    llvm::opt::ArgStringList &CC1Args,
+    Action::OffloadKind) const {
+  CC1Args.push_back("-nostdsysteminc");
+  CC1Args.push_back("-fuse-init-array");
+}
+
 void RISCVToolChain::AddClangSystemIncludeArgs(const ArgList &DriverArgs,
                                                ArgStringList &CC1Args) const {
   if (DriverArgs.hasArg(options::OPT_nostdinc))
     return;
 
   if (!DriverArgs.hasArg(options::OPT_nostdlibinc)) {
-    SmallString<128> Dir(getDriver().SysRoot);
+    SmallString<128> Dir(computeSysRoot());
     llvm::sys::path::append(Dir, "include");
     addSystemInclude(DriverArgs, CC1Args, Dir.str());
   }
 }
 
 void RISCVToolChain::addLibStdCxxIncludePaths(
     const llvm::opt::ArgList &DriverArgs,
     llvm::opt::ArgStringList &CC1Args) const {
-  StringRef LibDir = GCCInstallation.getParentLibPath();
   const GCCVersion &Version = GCCInstallation.getVersion();
   StringRef TripleStr = GCCInstallation.getTriple().str();
   const Multilib &Multilib = GCCInstallation.getMultilib();
-  addLibStdCXXIncludePaths(
-      LibDir.str() + "/../" + TripleStr.str() + "/include/c++/" + Version.Text,
+  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();
+
+  StringRef LibDir = GCCInstallation.getParentLibPath();
+  StringRef TripleStr = GCCInstallation.getTriple().str();
+  std::string SysRootDir = LibDir.str() + "/../" + TripleStr.str();
+
+  if (!llvm::sys::fs::exists(SysRootDir))
+    return std::string();
+
+  return SysRootDir;
+}
+
 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();
   const Driver &D = ToolChain.getDriver();
   ArgStringList CmdArgs;

test/Driver/riscv32-toolchain.c

 // C-RV32-BAREMETAL-ILP32: "--sysroot={{.*}}/Inputs/basic_riscv32_tree/riscv32-unknown-elf"
 // C-RV32-BAREMETAL-ILP32: "{{.*}}/Inputs/basic_riscv32_tree/riscv32-unknown-elf/lib{{/|\\\\}}crt0.o"
 // C-RV32-BAREMETAL-ILP32: "{{.*}}/Inputs/basic_riscv32_tree/lib/gcc/riscv32-unknown-elf/8.0.1{{/|\\\\}}crtbegin.o"
 // C-RV32-BAREMETAL-ILP32: "-L{{.*}}/Inputs/basic_riscv32_tree/riscv32-unknown-elf/lib"
 // C-RV32-BAREMETAL-ILP32: "-L{{.*}}/Inputs/basic_riscv32_tree/lib/gcc/riscv32-unknown-elf/8.0.1"
 // C-RV32-BAREMETAL-ILP32: "--start-group" "-lc" "-lgloss" "--end-group" "-lgcc"
 // C-RV32-BAREMETAL-ILP32: "{{.*}}/Inputs/basic_riscv32_tree/lib/gcc/riscv32-unknown-elf/8.0.1{{/|\\\\}}crtend.o"
 
+// RUN: %clang %s -### -no-canonical-prefixes \
+// RUN:   -target riscv32-unknown-elf \
+// RUN:   --gcc-toolchain=%S/Inputs/basic_riscv32_tree 2>&1 \
+// RUN:   | FileCheck -check-prefix=C-RV32-BAREMETAL-NOSYSROOT-ILP32 %s
+
+// C-RV32-BAREMETAL-NOSYSROOT-ILP32: "-fuse-init-array"
+// C-RV32-BAREMETAL-NOSYSROOT-ILP32: "{{.*}}Inputs/basic_riscv32_tree/lib/gcc/riscv32-unknown-elf/8.0.1/../../../../bin{{/|\\\\}}riscv32-unknown-elf-ld"
+// C-RV32-BAREMETAL-NOSYSROOT-ILP32: "{{.*}}/Inputs/basic_riscv32_tree/lib/gcc/riscv32-unknown-elf/8.0.1/../../../../riscv32-unknown-elf/lib{{/|\\\\}}crt0.o"
+// C-RV32-BAREMETAL-NOSYSROOT-ILP32: "{{.*}}/Inputs/basic_riscv32_tree/lib/gcc/riscv32-unknown-elf/8.0.1{{/|\\\\}}crtbegin.o"
+// C-RV32-BAREMETAL-NOSYSROOT-ILP32: "-L{{.*}}/Inputs/basic_riscv32_tree/lib/gcc/riscv32-unknown-elf/8.0.1/../../../../riscv32-unknown-elf{{/|\\\\}}lib"
+// C-RV32-BAREMETAL-NOSYSROOT-ILP32: "-L{{.*}}/Inputs/basic_riscv32_tree/lib/gcc/riscv32-unknown-elf/8.0.1"
+// C-RV32-BAREMETAL-NOSYSROOT-ILP32: "--start-group" "-lc" "-lgloss" "--end-group" "-lgcc"
+// C-RV32-BAREMETAL-NOSYSROOT-ILP32: "{{.*}}/Inputs/basic_riscv32_tree/lib/gcc/riscv32-unknown-elf/8.0.1{{/|\\\\}}crtend.o"
+
 // RUN: %clangxx %s -### -no-canonical-prefixes \
 // RUN:   -target riscv32-unknown-elf -stdlib=libstdc++ \
 // RUN:   --gcc-toolchain=%S/Inputs/basic_riscv32_tree \
 // RUN:   --sysroot=%S/Inputs/basic_riscv32_tree/riscv32-unknown-elf 2>&1 \
 // RUN:   | FileCheck -check-prefix=CXX-RV32-BAREMETAL-ILP32 %s
 
 // CXX-RV32-BAREMETAL-ILP32: "-fuse-init-array"
-// CXX-RV32-BAREMETAL-ILP32: "-internal-isystem" "{{.*}}Inputs/basic_riscv32_tree/lib/gcc/riscv32-unknown-elf/8.0.1/../../../../riscv32-unknown-elf/include/c++{{/|\\\\}}8.0.1"
+// CXX-RV32-BAREMETAL-ILP32: "-internal-isystem" "{{.*}}Inputs/basic_riscv32_tree/riscv32-unknown-elf/include/c++{{/|\\\\}}8.0.1"
 // CXX-RV32-BAREMETAL-ILP32: "{{.*}}Inputs/basic_riscv32_tree/lib/gcc/riscv32-unknown-elf/8.0.1/../../../../bin{{/|\\\\}}riscv32-unknown-elf-ld"
 // CXX-RV32-BAREMETAL-ILP32: "--sysroot={{.*}}/Inputs/basic_riscv32_tree/riscv32-unknown-elf"
 // CXX-RV32-BAREMETAL-ILP32: "{{.*}}/Inputs/basic_riscv32_tree/riscv32-unknown-elf/lib{{/|\\\\}}crt0.o"
 // CXX-RV32-BAREMETAL-ILP32: "{{.*}}/Inputs/basic_riscv32_tree/lib/gcc/riscv32-unknown-elf/8.0.1{{/|\\\\}}crtbegin.o"
 // CXX-RV32-BAREMETAL-ILP32: "-L{{.*}}/Inputs/basic_riscv32_tree/riscv32-unknown-elf/lib"
 // CXX-RV32-BAREMETAL-ILP32: "-L{{.*}}/Inputs/basic_riscv32_tree/lib/gcc/riscv32-unknown-elf/8.0.1"
 // CXX-RV32-BAREMETAL-ILP32: "-lstdc++" "--start-group" "-lc" "-lgloss" "--end-group" "-lgcc"
 // CXX-RV32-BAREMETAL-ILP32: "{{.*}}/Inputs/basic_riscv32_tree/lib/gcc/riscv32-unknown-elf/8.0.1{{/|\\\\}}crtend.o"
 
+// RUN: %clangxx %s -### -no-canonical-prefixes \
+// RUN:   -target riscv32-unknown-elf -stdlib=libstdc++ \
+// RUN:   --gcc-toolchain=%S/Inputs/basic_riscv32_tree 2>&1 \
+// RUN:   | FileCheck -check-prefix=CXX-RV32-BAREMETAL-NOSYSROOT-ILP32 %s
+
+// CXX-RV32-BAREMETAL-NOSYSROOT-ILP32: "-fuse-init-array"
+// CXX-RV32-BAREMETAL-NOSYSROOT-ILP32: "-internal-isystem" "{{.*}}Inputs/basic_riscv32_tree/lib/gcc/riscv32-unknown-elf/8.0.1/../../../../riscv32-unknown-elf/include/c++{{/|\\\\}}8.0.1"
+// CXX-RV32-BAREMETAL-NOSYSROOT-ILP32: "{{.*}}Inputs/basic_riscv32_tree/lib/gcc/riscv32-unknown-elf/8.0.1/../../../../bin{{/|\\\\}}riscv32-unknown-elf-ld"
+// CXX-RV32-BAREMETAL-NOSYSROOT-ILP32: "{{.*}}/Inputs/basic_riscv32_tree/lib/gcc/riscv32-unknown-elf/8.0.1/../../../../riscv32-unknown-elf/lib{{/|\\\\}}crt0.o"
+// CXX-RV32-BAREMETAL-NOSYSROOT-ILP32: "{{.*}}/Inputs/basic_riscv32_tree/lib/gcc/riscv32-unknown-elf/8.0.1{{/|\\\\}}crtbegin.o"
+// CXX-RV32-BAREMETAL-NOSYSROOT-ILP32: "-L{{.*}}/Inputs/basic_riscv32_tree/lib/gcc/riscv32-unknown-elf/8.0.1/../../../../riscv32-unknown-elf/lib"
+// CXX-RV32-BAREMETAL-NOSYSROOT-ILP32: "-L{{.*}}/Inputs/basic_riscv32_tree/lib/gcc/riscv32-unknown-elf/8.0.1"
+// CXX-RV32-BAREMETAL-NOSYSROOT-ILP32: "-lstdc++" "--start-group" "-lc" "-lgloss" "--end-group" "-lgcc"
+// CXX-RV32-BAREMETAL-NOSYSROOT-ILP32: "{{.*}}/Inputs/basic_riscv32_tree/lib/gcc/riscv32-unknown-elf/8.0.1{{/|\\\\}}crtend.o"
+
 // RUN: %clang %s -### -no-canonical-prefixes -fuse-ld=ld \
 // RUN:   -target riscv32-linux-unknown-elf \
 // RUN:   --gcc-toolchain=%S/Inputs/multilib_riscv_linux_sdk \
 // RUN:   --sysroot=%S/Inputs/multilib_riscv_linux_sdk/sysroot 2>&1 \
 // RUN:   | FileCheck -check-prefix=C-RV32-LINUX-MULTI-ILP32 %s
 
 // C-RV32-LINUX-MULTI-ILP32: "-fuse-init-array"
 // C-RV32-LINUX-MULTI-ILP32: "{{.*}}/Inputs/multilib_riscv_linux_sdk/lib/gcc/riscv64-unknown-linux-gnu/7.2.0/../../../../riscv64-unknown-linux-gnu/bin{{/|\\\\}}ld"