Page MenuHomePhabricator

[Clang][Driver] Fix include paths for `--sysroot /` on Linux
ClosedPublic

Authored by egorzhdan on May 24 2022, 4:14 AM.

Details

Summary

Currently if --sysroot / is passed to the Clang driver, the include paths generated by the Clang driver will start with a double slash: //usr/include/....
If VFS is used to inject files into the include paths (for example, the Swift compiler does this), VFS will get confused and the injected files won't be visible.

This change makes sure that the include paths start with a single slash.

Fixes #28283.

Diff Detail

Event Timeline

egorzhdan created this revision.May 24 2022, 4:14 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 24 2022, 4:14 AM
egorzhdan requested review of this revision.May 24 2022, 4:14 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 24 2022, 4:14 AM
MaskRay added inline comments.May 25 2022, 3:08 PM
clang/include/clang/Driver/ToolChain.h
219

This can use llvm::sys::path::append

egorzhdan added inline comments.May 25 2022, 3:51 PM
clang/include/clang/Driver/ToolChain.h
219

The implementation of this already uses llvm::sys::path::append, do you mean replacing the usages of concat with llvm::sys::path::append?
That would produce quite a lot of boilerplate I think, since llvm::sys::path::append modifies the path in-place, so every call to concat would expand into 3 lines of code. Also it would be very easy to forget to pass Style::posix and cause incorrect behavior on Windows (something I've stumbled upon while making this patch).

MaskRay added inline comments.May 25 2022, 5:02 PM
clang/include/clang/Driver/ToolChain.h
219

Ah I missed that. The current concat impl looks good to me.

clang/lib/Driver/ToolChains/Linux.cpp
277–278

Can the trailing / in a path component be removed now? Ditto below.

clang/test/Driver/linux-header-search.cpp
99

add -SAME whenever applicable.

egorzhdan updated this revision to Diff 432256.May 26 2022, 5:50 AM
  • Remove unnecessary trailing /
  • Add -SAME in tests
egorzhdan marked 2 inline comments as done.May 26 2022, 5:51 AM
MaskRay accepted this revision.May 26 2022, 1:51 PM
MaskRay added inline comments.
clang/lib/Driver/ToolChain.cpp
945

I think the first argument of concat should be StringRef, then we can avoid changing the signature of getMultiarchTriple

clang/test/Driver/linux-header-search.cpp
76

I suspect this test does not add more coverage than linux-cross.cpp.

To test --sysroot with a trailing /, just modify the previous CHECK-BASIC-LIBCXXV2 by appending a /

88

Remove the otherwise empty //

It adds no value but makes { } in vim-style editors inconvenient.

This revision is now accepted and ready to land.May 26 2022, 1:51 PM
egorzhdan updated this revision to Diff 432539.May 27 2022, 6:31 AM
  • Change arg type from std::string to StringRef
  • Remove unnecessary //

• Remove unneeded test

egorzhdan marked 2 inline comments as done.May 27 2022, 6:34 AM
egorzhdan added inline comments.
clang/lib/Driver/ToolChain.cpp
945

You're right, thanks!

clang/test/Driver/linux-header-search.cpp
76

CHECK-BASIC-LIBCXXV2 is not quite the same: I'd like to test the libstdc++ detection logic (e.g. GCCInstallationDetector) which doesn't get invoked for libc++.

I removed the unnecessary test.

egorzhdan updated this revision to Diff 432568.May 27 2022, 8:06 AM
egorzhdan marked an inline comment as done.

Rebase & fix formatting