[Clang] Search computed sysroot for libc++ header paths

[rprichard]

Summary

The Android NDK's clang driver is used with an Android -target setting,
and the driver automatically finds the Android sysroot at a path
relative to the driver. The sysroot has the libc++ headers in it.

Remove Hurd::computeSysRoot as it is equivalent to the new
ToolChain::computeSysRoot method.

Fixes PR46213.

[rprichard]

Rerun after installing clang-format

[rprichard]

Reverse clang-format's change.

[rprichard]

FWIW, computeSysRoot is overridden by these toolchains:

• Linux
• MSP430ToolChain
• MipsLLVMToolChain
• RISCVToolChain

I think this change restores the pre D69758 behavior for Linux and MipsLLVMToolChain. It might be nice to have a review from someone familiar with the MSP430 and RISC-V toolchains.

[srhines]

Thanks, Ryan, for diagnosing and addressing this bug.

clang/lib/Driver/ToolChains/Linux.h

45

Can drop virtual here.

[rprichard]

Remove unnecessary virtual keyword.

[srhines]

Thanks, Ryan!

[ldionne]

While this doesn't look wrong to me -- and the correctness of this depends entirely on where vendors decide to put their headers so it's hard for me to verify -- I'm wondering why not all toolchains use this mechanism. We seem to be adding an abstraction that's used only by some toolchains, but not all. I think it would be great to have a single canonical way of representing the system root.

Commenting out of curiosity, don't let this block you.

[danalbert]

In D81622#2094409, @ldionne wrote:

While this doesn't look wrong to me -- and the correctness of this depends entirely on where vendors decide to put their headers so it's hard for me to verify -- I'm wondering why not all toolchains use this mechanism. We seem to be adding an abstraction that's used only by some toolchains, but not all. I think it would be great to have a single canonical way of representing the system root.

Yeah, I agree that the inconsistencies between vendors in the driver are pretty odd. I think essentially the way we got into this situation is that it's not always clear what the right way to do things is, and if it's not even clear that there even is an existing mechanism we can easily end up creating our own per vendor :(

I'd be interested in seeing if there are things we can do to try to unify the behavior in this area. I suspect it could be difficult given that it necessarily involves all of the different vendors, but maybe we could come up with one recommended way of doing all this and any of us that don't fit that model can make the change at a pace convenient for us?

Commenting out of curiosity, don't let this block you.

Thanks for making that clear :) We're going to go ahead and submit since this does fix a regression but I'm open to making changes here if (hopefully when) we find some cleanup that can be done.

[MaskRay]

Hi, you can drop Reviewers: Subscribers: Tags: and the text Summary: from the git commit with the following script:

arcfilter () {
        arc amend
        git log -1 --pretty=%B | awk '/Reviewers:|Subscribers:/{p=1} /Reviewed By:|Differential Revision:/{p=0} !p && !/^Summary:$/ {sub(/^Summary: /,"");print}' | git commit --amend --date=now -F -
}

Reviewed By: is considered important by some people. Please keep the tag. (I have updated my script to use --date=now (setting author date to committer date))

https://reviews.llvm.org/D80978 contains a git pre-push hook to automate this.

[rprichard]

Ok, I'll run that command in the future, and I'll watch D80978 for updates.

clang/include/clang/Driver/ToolChain.h

public:
 
   /// hasBlocksRuntime - Given that the user is compiling with
   /// -fblocks, does this tool chain guarantee the existence of a
   /// blocks runtime?
   ///
   /// FIXME: this really belongs on some sort of DeploymentTarget abstraction
   virtual bool hasBlocksRuntime() const { return true; }
 
+  /// Return the sysroot, possibly searching for a default sysroot using
+  /// target-specific logic.
+  virtual std::string computeSysRoot() const;
+
   /// Add the clang cc1 arguments for system include paths.
   ///
   /// This routine is responsible for adding the necessary cc1 arguments to
   /// include headers from standard system header directories.
   virtual void
   AddClangSystemIncludeArgs(const llvm::opt::ArgList &DriverArgs,
                             llvm::opt::ArgStringList &CC1Args) const;
 

clang/lib/Driver/ToolChain.cpp

std::string ToolChain::ComputeLLVMTriple(const ArgList &Args,
   }
 }
 
 std::string ToolChain::ComputeEffectiveClangTriple(const ArgList &Args,
                                                    types::ID InputType) const {
   return ComputeLLVMTriple(Args, InputType);
 }
 
+std::string ToolChain::computeSysRoot() const {
+  return D.SysRoot;
+}
+
 void ToolChain::AddClangSystemIncludeArgs(const ArgList &DriverArgs,
                                           ArgStringList &CC1Args) const {
   // Each toolchain should provide the appropriate include flags.
 }
 
 void ToolChain::addClangTargetOptions(
     const ArgList &DriverArgs, ArgStringList &CC1Args,
     Action::OffloadKind DeviceOffloadKind) const {}

clang/lib/Driver/ToolChains/Gnu.cpp

for (llvm::vfs::directory_iterator LI = vfs.dir_begin(base, EC), LE;
     }
   }
   return MaxVersion ? (base + "/" + MaxVersionString).str() : "";
 }
 
 void
 Generic_GCC::addLibCxxIncludePaths(const llvm::opt::ArgList &DriverArgs,
                                    llvm::opt::ArgStringList &CC1Args) const {
-  const std::string& SysRoot = getDriver().SysRoot;
   auto AddIncludePath = [&](std::string Path) {
     std::string IncludePath = DetectLibcxxIncludePath(getVFS(), Path);
     if (IncludePath.empty() || !getVFS().exists(IncludePath))
       return false;
     addSystemInclude(DriverArgs, CC1Args, IncludePath);
     return true;
   };
   // Android never uses the libc++ headers installed alongside the toolchain,
   // which are generally incompatible with the NDK libraries anyway.
   if (!getTriple().isAndroid())
     if (AddIncludePath(getDriver().Dir + "/../include/c++"))
       return;
   // If this is a development, non-installed, clang, libcxx will
   // not be found at ../include/c++ but it likely to be found at
   // one of the following two locations:
+  std::string SysRoot = computeSysRoot();
   if (AddIncludePath(SysRoot + "/usr/local/include/c++"))
     return;
   if (AddIncludePath(SysRoot + "/usr/include/c++"))
     return;
 }
 
 /// Helper to add the variant paths of a libstdc++ installation.
 bool Generic_GCC::addLibStdCXXIncludePaths(

clang/lib/Driver/ToolChains/Hurd.h

Hurd(const Driver &D, const llvm::Triple &Triple,
        const llvm::opt::ArgList &Args);
 
   bool HasNativeLLVMSupport() const override;
 
   void
   AddClangSystemIncludeArgs(const llvm::opt::ArgList &DriverArgs,
                             llvm::opt::ArgStringList &CC1Args) const override;
 
-  std::string computeSysRoot() const;
-
   std::string getDynamicLinker(const llvm::opt::ArgList &Args) const override;
 
   void addExtraOpts(llvm::opt::ArgStringList &CmdArgs) const override;
 
   std::vector<std::string> ExtraOpts;
 
 protected:
   Tool *buildAssembler() const override;

clang/lib/Driver/ToolChains/Hurd.cpp

 bool Hurd::HasNativeLLVMSupport() const { return true; }
 
 Tool *Hurd::buildLinker() const { return new tools::gnutools::Linker(*this); }
 
 Tool *Hurd::buildAssembler() const {
   return new tools::gnutools::Assembler(*this);
 }
 
-std::string Hurd::computeSysRoot() const {
-  if (!getDriver().SysRoot.empty())
-    return getDriver().SysRoot;
-
-  return std::string();
-}
-
 std::string Hurd::getDynamicLinker(const ArgList &Args) const {
   if (getArch() == llvm::Triple::x86)
     return "/lib/ld.so";
 
   llvm_unreachable("unsupported architecture");
 }
 
 void Hurd::AddClangSystemIncludeArgs(const ArgList &DriverArgs,

clang/lib/Driver/ToolChains/Linux.h

void AddIAMCUIncludeArgs(const llvm::opt::ArgList &DriverArgs,
                            llvm::opt::ArgStringList &CC1Args) const override;
   CXXStdlibType GetDefaultCXXStdlibType() const override;
   bool isPIEDefault() const override;
   bool isNoExecStackDefault() const override;
   bool IsMathErrnoDefault() const override;
   SanitizerMask getSupportedSanitizers() const override;
   void addProfileRTLibs(const llvm::opt::ArgList &Args,
                         llvm::opt::ArgStringList &CmdArgs) const override;
-  virtual std::string computeSysRoot() const;
+  std::string computeSysRoot() const override;
 
   std::string getDynamicLinker(const llvm::opt::ArgList &Args) const override;
 
   void addExtraOpts(llvm::opt::ArgStringList &CmdArgs) const override;
 
   std::vector<std::string> ExtraOpts;
 
   llvm::DenormalMode getDefaultDenormalModeForType(

clang/lib/Driver/ToolChains/MSP430.h

public:
   bool isPICDefault() const override { return false; }
   bool isPIEDefault() const override { return false; }
   bool isPICDefaultForced() const override { return true; }
 
 protected:
   Tool *buildLinker() const override;
 
 private:
-  std::string computeSysRoot() const;
+  std::string computeSysRoot() const override;
 };
 
 } // end namespace toolchains
 
 namespace tools {
 namespace msp430 {
 
 class LLVM_LIBRARY_VISIBILITY Linker : public GnuTool {

clang/lib/Driver/ToolChains/RISCVToolchain.h

public:
   void
   addLibStdCxxIncludePaths(const llvm::opt::ArgList &DriverArgs,
                            llvm::opt::ArgStringList &CC1Args) const override;
 
 protected:
   Tool *buildLinker() const override;
 
 private:
-  std::string computeSysRoot() const;
+  std::string computeSysRoot() const override;
 };
 
 } // end namespace toolchains
 
 namespace tools {
 namespace RISCV {
 class LLVM_LIBRARY_VISIBILITY Linker : public GnuTool {
 public: