[clang] [Driver] Fix detection of libc++ with empty sysroot. (PR #66947)

Sam McCall via cfe-commits cfe-commits at lists.llvm.org
Wed Sep 20 13:49:20 PDT 2023


sam-mccall wrote:

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)

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


More information about the cfe-commits mailing list