[Clang][Driver] Fix include paths for `--sysroot /` on OpenBSD/FreeBSD

[egorzhdan]

Summary

This is the same change as https://reviews.llvm.org/D126289, but applied for OpenBSD & FreeBSD.

[MaskRay]

The code is trivially correct and makes aesthetic improvement.

Regarding the new test: Linux tests organization isn't great. --sysroot tests both include and library search paths, so ideally we use one RUN line to test both.

The name of linux-header-search.cpp focus on just include search path. For FreeBSD/OpenBSD, if you want to start in a clean state, consider adding the RUN line to a generic test (if --sysroot is considered a core functionality) or a test with name implying "cross compilation" (that's why I created linux-cross.cpp).

[brad]

There is already the initial --sysroot test in test/Driver/openbsd.c. It would be preferable to modify what is there. For FreeBSD a test should be added to test/Driver/freebsd.c.

[3405691582]

LGTM. Tested change on OpenBSD resolves downstream issues, was not able to check FreeBSD.

[brad]

In D129654#3650412, @3405691582 wrote:

Tested change on OpenBSD resolves downstream issues.

What issues?

[3405691582]

In D129654#3650534, @brad wrote:

In D129654#3650412, @3405691582 wrote:

Tested change on OpenBSD resolves downstream issues.

What issues?

As mentioned, this replicates https://reviews.llvm.org/D126289 for OpenBSD which fixes #28283. That particular issue caused a build problem in Swift's use of VFS in which I verified this patch solves. I was trying to clarify the scope and mechanism in which I tested this change, sorry for the confusion.

[brad]

In D129654#3650590, @3405691582 wrote:

As mentioned, this replicates https://reviews.llvm.org/D126289 for OpenBSD which fixes #28283. That particular issue caused a build problem in Swift's use of VFS in which I verified this patch solves. I was trying to clarify the scope and mechanism in which I tested this change, sorry for the confusion.

Ah, that clarifies things. Thank you.

[brad]

Also "-stdlib=libc++" is the default on OpenBSD and FreeBSD so you can probably leave that out and I think you can remove "--gcc-toolchain="" ".

[egorzhdan]

• Add a test for OpenBSD
• Modify existing test file instead of adding a new file

[egorzhdan]

Thanks @brad @3405691582 @MaskRay for the review!

clang/lib/Driver/ToolChains/FreeBSD.cpp

 FreeBSD::FreeBSD(const Driver &D, const llvm::Triple &Triple,
                  const ArgList &Args)
     : Generic_ELF(D, Triple, Args) {
 
   // When targeting 32-bit platforms, look for '/usr/lib32/crt1.o' and fall
   // back to '/usr/lib' if it doesn't exist.
   if ((Triple.getArch() == llvm::Triple::x86 || Triple.isMIPS32() ||
        Triple.isPPC32()) &&
-      D.getVFS().exists(getDriver().SysRoot + "/usr/lib32/crt1.o"))
-    getFilePaths().push_back(getDriver().SysRoot + "/usr/lib32");
+      D.getVFS().exists(concat(getDriver().SysRoot, "/usr/lib32/crt1.o")))
+    getFilePaths().push_back(concat(getDriver().SysRoot, "/usr/lib32"));
   else
-    getFilePaths().push_back(getDriver().SysRoot + "/usr/lib");
+    getFilePaths().push_back(concat(getDriver().SysRoot, "/usr/lib"));
 }
 
 ToolChain::CXXStdlibType FreeBSD::GetDefaultCXXStdlibType() const {
   unsigned Major = getTriple().getOSMajorVersion();
   if (Major >= 10 || Major == 0)
     return ToolChain::CST_Libcxx;
   return ToolChain::CST_Libstdcxx;
 }
 
 unsigned FreeBSD::GetDefaultDwarfVersion() const {
   if (getTriple().getOSMajorVersion() < 12)
     return 2;
   return 4;
 }
 
 void FreeBSD::addLibCxxIncludePaths(const llvm::opt::ArgList &DriverArgs,
                                     llvm::opt::ArgStringList &CC1Args) const {
   addSystemInclude(DriverArgs, CC1Args,
-                   getDriver().SysRoot + "/usr/include/c++/v1");
+                   concat(getDriver().SysRoot, "/usr/include/c++/v1"));
 }
 
 void FreeBSD::addLibStdCxxIncludePaths(
     const llvm::opt::ArgList &DriverArgs,
     llvm::opt::ArgStringList &CC1Args) const {
-  addLibStdCXXIncludePaths(getDriver().SysRoot + "/usr/include/c++/4.2", "", "",
-                           DriverArgs, CC1Args);
+  addLibStdCXXIncludePaths(concat(getDriver().SysRoot, "/usr/include/c++/4.2"),
+                           "", "", DriverArgs, CC1Args);
 }
 
 void FreeBSD::AddCXXStdlibLibArgs(const ArgList &Args,
                                   ArgStringList &CmdArgs) const {
   CXXStdlibType Type = GetCXXStdlibType(Args);
   unsigned Major = getTriple().getOSMajorVersion();
   bool Profiling = Args.hasArg(options::OPT_pg) && Major != 0 && Major < 14;
 

clang/lib/Driver/ToolChains/OpenBSD.cpp

SanitizerMask OpenBSD::getSupportedSanitizers() const {
   return Res;
 }
 
 /// OpenBSD - OpenBSD tool chain which can call as(1) and ld(1) directly.
 
 OpenBSD::OpenBSD(const Driver &D, const llvm::Triple &Triple,
                  const ArgList &Args)
     : Generic_ELF(D, Triple, Args) {
-  getFilePaths().push_back(getDriver().SysRoot + "/usr/lib");
+  getFilePaths().push_back(concat(getDriver().SysRoot, "/usr/lib"));
 }
 
 void OpenBSD::AddClangSystemIncludeArgs(
     const llvm::opt::ArgList &DriverArgs,
     llvm::opt::ArgStringList &CC1Args) const {
   const Driver &D = getDriver();
 
   if (DriverArgs.hasArg(clang::driver::options::OPT_nostdinc))
if (CIncludeDirs != "") {
     for (StringRef dir : dirs) {
       StringRef Prefix =
           llvm::sys::path::is_absolute(dir) ? StringRef(D.SysRoot) : "";
       addExternCSystemInclude(DriverArgs, CC1Args, Prefix + dir);
     }
     return;
   }
 
-  addExternCSystemInclude(DriverArgs, CC1Args, D.SysRoot + "/usr/include");
+  addExternCSystemInclude(DriverArgs, CC1Args,
+                          concat(D.SysRoot, "/usr/include"));
 }
 
 void OpenBSD::addLibCxxIncludePaths(const llvm::opt::ArgList &DriverArgs,
                                     llvm::opt::ArgStringList &CC1Args) const {
   addSystemInclude(DriverArgs, CC1Args,
-                   getDriver().SysRoot + "/usr/include/c++/v1");
+                   concat(getDriver().SysRoot, "/usr/include/c++/v1"));
 }
 
 void OpenBSD::AddCXXStdlibLibArgs(const ArgList &Args,
                                   ArgStringList &CmdArgs) const {
   bool Profiling = Args.hasArg(options::OPT_pg);
 
   CmdArgs.push_back(Profiling ? "-lc++_p" : "-lc++");
   if (Args.hasArg(options::OPT_fexperimental_library))

clang/test/Driver/freebsd.cpp

 // RUN: %clangxx %s -### -pg -o %t.o -target amd64-unknown-freebsd10.0 -stdlib=platform 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-PG-TEN %s
 // RUN: %clangxx %s -### -pg -o %t.o -target amd64-unknown-freebsd9.2 -stdlib=platform 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-PG-NINE %s
 // CHECK-PG-DEFAULT: "-lc++" "-lm"
 // CHECK-PG-FOURTEEN: "-lc++" "-lm"
 // CHECK-PG-TEN: "-lc++_p" "-lm_p"
 // CHECK-PG-NINE: "-lstdc++_p" "-lm_p"
+
+// Test include paths with a sysroot.
+// RUN: %clangxx %s -### -fsyntax-only 2>&1 \
+// RUN:     --target=amd64-unknown-freebsd \
+// RUN:     --sysroot=%S/Inputs/basic_openbsd_libcxx_tree \
+// RUN:     -resource-dir=%S/Inputs/resource_dir \
+// RUN:   | FileCheck --check-prefix=CHECK-LIBCXX-SYSROOT %s
+// CHECK-LIBCXX-SYSROOT: "-cc1"
+// CHECK-LIBCXX-SYSROOT-SAME: "-isysroot" "[[SYSROOT:[^"]+]]"
+// CHECK-LIBCXX-SYSROOT-SAME: "-internal-isystem" "[[SYSROOT]]/usr/include/c++/v1"
+
+// Test include paths when the sysroot path ends with `/`.
+// RUN: %clangxx %s -### -fsyntax-only 2>&1 \
+// RUN:     --target=amd64-unknown-freebsd \
+// RUN:     --sysroot=%S/Inputs/basic_openbsd_libcxx_tree/ \
+// RUN:     -resource-dir=%S/Inputs/resource_dir \
+// RUN:   | FileCheck --check-prefix=CHECK-LIBCXX-SYSROOT-SLASH %s
+// CHECK-LIBCXX-SYSROOT-SLASH: "-cc1"
+// CHECK-LIBCXX-SYSROOT-SLASH-SAME: "-isysroot" "[[SYSROOT:[^"]+/]]"
+// CHECK-LIBCXX-SYSROOT-SLASH-SAME: "-internal-isystem" "[[SYSROOT]]usr/include/c++/v1"

clang/test/Driver/openbsd.cpp

 // RUN:   | FileCheck --check-prefix=CHECK-PG-CXX %s
 // RUN: %clangxx %s -### -pg -o %t.o -target i686-pc-openbsd 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-PG-CXX %s
 // RUN: %clangxx %s -### -pg -o %t.o -target aarch64-unknown-openbsd 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-PG-CXX %s
 // RUN: %clangxx %s -### -pg -o %t.o -target arm-unknown-openbsd 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-PG-CXX %s
 // CHECK-PG-CXX: "-lc++_p" "-lc++abi_p" "-lpthread_p" "-lm_p"
+
+// Test include paths with a sysroot.
+// RUN: %clangxx %s -### -fsyntax-only 2>&1 \
+// RUN:     --target=amd64-pc-openbsd \
+// RUN:     --sysroot=%S/Inputs/basic_openbsd_libcxx_tree \
+// RUN:     -resource-dir=%S/Inputs/resource_dir \
+// RUN:   | FileCheck --check-prefix=CHECK-LIBCXX-SYSROOT %s
+// CHECK-LIBCXX-SYSROOT: "-cc1"
+// CHECK-LIBCXX-SYSROOT-SAME: "-isysroot" "[[SYSROOT:[^"]+]]"
+// CHECK-LIBCXX-SYSROOT-SAME: "-internal-isystem" "[[SYSROOT]]/usr/include/c++/v1"
+
+// Test include paths when the sysroot path ends with `/`.
+// RUN: %clangxx %s -### -fsyntax-only 2>&1 \
+// RUN:     --target=amd64-pc-openbsd \
+// RUN:     --sysroot=%S/Inputs/basic_openbsd_libcxx_tree/ \
+// RUN:     -resource-dir=%S/Inputs/resource_dir \
+// RUN:   | FileCheck --check-prefix=CHECK-LIBCXX-SYSROOT-SLASH %s
+// CHECK-LIBCXX-SYSROOT-SLASH: "-cc1"
+// CHECK-LIBCXX-SYSROOT-SLASH-SAME: "-isysroot" "[[SYSROOT:[^"]+/]]"
+// CHECK-LIBCXX-SYSROOT-SLASH-SAME: "-internal-isystem" "[[SYSROOT]]usr/include/c++/v1"