[Driver] Fix detection of libc++ with empty sysroot.

[sam-mccall]

b1e3cd1 dropped the leading slash here,
presumably unintentionally.

(I don't understand Driver well enough to know how/where this is supposed
to be tested, but it broke clangd on any project that uses the
system-installed -stdlib=libc++ on a standard debian install)

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-driver

Changes

b1e3cd1 dropped the leading slash here,
presumably unintentionally.

(I don't understand Driver well enough to know how/where this is supposed
to be tested, but it broke clangd on any project that uses the
system-installed -stdlib=libc++ on a standard debian install)

---

Full diff: https://github.com/llvm/llvm-project/pull/66947.diff

1 Files Affected:

• (modified) clang/lib/Driver/ToolChains/Gnu.cpp (+2-2)

diff --git a/clang/lib/Driver/ToolChains/Gnu.cpp b/clang/lib/Driver/ToolChains/Gnu.cpp
index e909549d20708fe..f2fd85021a777b4 100644
--- a/clang/lib/Driver/ToolChains/Gnu.cpp
+++ b/clang/lib/Driver/ToolChains/Gnu.cpp
@@ -3139,11 +3139,11 @@ Generic_GCC::addLibCxxIncludePaths(const llvm::opt::ArgList &DriverArgs,
   // not be found at ../include/c++ but it likely to be found at
   // one of the following two locations:
   SmallString<128> UsrLocalIncludeDir(SysRoot);
-  llvm::sys::path::append(UsrLocalIncludeDir, "usr", "local", "include");
+  llvm::sys::path::append(UsrLocalIncludeDir, "/usr", "local", "include");
   if (AddIncludePath(UsrLocalIncludeDir))
     return;
   SmallString<128> UsrIncludeDir(SysRoot);
-  llvm::sys::path::append(UsrIncludeDir, "usr", "include");
+  llvm::sys::path::append(UsrIncludeDir, "/usr", "include");
   if (AddIncludePath(UsrIncludeDir))
     return;
 }

[smeenai]

Sorry about the breakage.

The sys::path::append behavior here is surprising. I think it'd be better to change the computation of SysRoot in the function above (line 3102, which I can't comment because of GitHub, sigh) to default to llvm::sys::path::get_separator() if it's empty, to handle Windows slashes as well as be less confusing to read.

This is tricky to test because it requires no sysroot to be passed, but we might be able to do so with a VFS overlay. I'm okay adding the test afterwards though so we get the breakage fixed first, and I can also look into the test myself if you'd like.

[sam-mccall]

Thanks for the quick response!

The sys::path::append behavior here is surprising. I think it'd be better to change the computation of SysRoot in the function above (line 3102, which I can't comment because of GitHub, sigh) to default to llvm::sys::path::get_separator() if it's empty, to handle Windows slashes as well as be less confusing to read.

That sounds sensible to me in principle, but not trivial: presumably the overrides of Toolchain::getSysroot() should have a consistent contract. There are 6 overrides, where some explicitly return the empty string, and 25 callers some of which do things like computeSysRoot() + "/include/c++/" (RISCVToolchain).
I feel it's likely I'll break something on a platform I don't understand before getting back to a good state :-)

This is tricky to test because it requires no sysroot to be passed, but we might be able to do so with a VFS overlay. I'm okay adding the test afterwards though so we get the breakage fixed first, and I can also look into the test myself if you'd like.

I can try to add something to the unittest. Looks like in the current form I also need to change the lit test back to hard-coding / for this one separator.
(I'm out of time tonight but will pick it up tomorrow)

[smeenai]

Thanks for the quick response!

The sys::path::append behavior here is surprising. I think it'd be better to change the computation of SysRoot in the function above (line 3102, which I can't comment because of GitHub, sigh) to default to llvm::sys::path::get_separator() if it's empty, to handle Windows slashes as well as be less confusing to read.

That sounds sensible to me in principle, but not trivial: presumably the overrides of Toolchain::getSysroot() should have a consistent contract. There are 6 overrides, where some explicitly return the empty string, and 25 callers some of which do things like computeSysRoot() + "/include/c++/" (RISCVToolchain). I feel it's likely I'll break something on a platform I don't understand before getting back to a good state :-)

I just meant to change

llvm-project/clang/lib/Driver/ToolChains/Gnu.cpp

Line 3102 in 5eaa531

std::string SysRoot = computeSysRoot();

to something like (untested):

std::string SysRoot = computeSysRoot();
if (SysRoot.empty())
  SysRoot = llvm::sys::path::get_separator();

It's a local fix, but all the code here is pretty inconsistent anyway, so I don't feel too bad about it.

[sam-mccall]

I just meant to change [...] It's a local fix, but all the code here is pretty inconsistent anyway, so I don't feel too bad about it.

Oops, of course. Done & I worked out the testing.

[smeenai]

LGTM, thanks!

[vvereschaka]

@sam-mccall ,
your 7ca8c21 commit breaks Clang-Unit :: Driver/./ClangDriverTests.exe/ToolChainTest/VFSGnuLibcxxPathNoSysroot (Clang-Unit::67) test on the windows toolchain builders

• https://lab.llvm.org/buildbot/#/builders/119/builds/15230
• https://lab.llvm.org/buildbot/#/builders/60/builds/14031

The problem remains for more than 3 days already. Would you fix it as soon as it possible or revert your changes?

[sam-mccall]

Sorry, there were no notifications, I guess the bot was already red.

I'm not really sure what to do other than disable the test on these bots - I don't know why it would fail on win/arm (it seems to pass on win/x86), and I don't have access to such a machine.
This behavior isn't particularly relevant on windows but is critical on linux. This isn't a regression, there was just previously no testing of it.

[sam-mccall]

... and as soon as I said that, I found something: looks like these are maybe building with DEFAULT_SYSROOT=C:/buildbot/.arm-ubuntu, which breaks the no-sysroot behavior we're testing here.

I'll see if I can override with --sysroot=

edit: hopefully good after 5aa3338

[asl]

@sam-mccall As far as I can see, the bot was green before this change: https://lab.llvm.org/buildbot/#/builders/119/builds/15229

And your commit was the only one in the set of 19 commits that were build that was relevant to this testcase.

Have you not received email from the buildbot?

[vvereschaka]

edit: hopefully good after 5aa3338

the test has been passed locally on the builder with this commit. Thank you for the fix.