[PATCH] D132867: [Clang] Use virtual FS in processing config files

Mon Sep 5 10:40:25 PDT 2022

sepavloff added a comment.

In D132867#3769791 <https://reviews.llvm.org/D132867#3769791>, @sammccall wrote:

> This seems like the right thing in principle, but it's still pretty scary:
>
> - this is going to break anyone who's using VFS != RealFS together with config files, as we no longer search the old locations
> - the breakage is likely to be quiet/subtle, especially if it's e.g. an obscure flag being set in an arch-specific config file
> - the fixes for "part of the toolchain is missing from VFS" tend to need code changes in tools and tend to be hard to test
>
> The thing the change has going for it is that I think config files and nontrivial VFS are both pretty rarely used, so maybe there are few people using them together.
> (Google uses both, but having looked internally I don't believe we ever actually combine them).

It seems that compatibility issue can appear if someone use VFS for all files but configuration one, which reside in real FS. It is inconvenient and hard to maintain, so probably nobody uses such combination, otherwise someone filed a bug or prepared a patch. Strictly speaking this is incorrect behavior because file system object must be used for all operations on files, according to documentation.

================
Comment at: llvm/lib/Support/CommandLine.cpp:1268
+      if (!CurrentDir) {
+        if (auto CWD = FS.getCurrentWorkingDirectory()) {
+          CurrDir = *CWD;
----------------
sammccall wrote:
> the old behavior was explicitly documented ("process' cwd is used instead", not FS's) so that documentation should be updated.
> 
> (I just talked to @kadircet about the patch that added that documentation, and we can't think of any reason that the old behavior is actually desirable)
You are right, I fixed that documentation.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132867/new/

https://reviews.llvm.org/D132867