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

Mon Sep 5 03:28:35 PDT 2022

sammccall added a comment.

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).

So I think this should land, but it needs a release note as it's a non-obvious breaking change.
@MaskRay in case he has any thoughts about VFS use in driver in general.

================
Comment at: llvm/lib/Support/CommandLine.cpp:1268
+      if (!CurrentDir) {
+        if (auto CWD = FS.getCurrentWorkingDirectory()) {
+          CurrDir = *CWD;
----------------
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)

================
Comment at: llvm/lib/Support/CommandLine.cpp:1271
+        } else {
+          // TODO: The error should be propagated up the stack.
+          llvm::consumeError(llvm::errorCodeToError(CWD.getError()));
----------------
(This seems unidiomatic: consumeError(errorCodeToError(...)) is a no-op, we usually spell TODO as FIXME... but it matches the surrounding code, so best as-is I guess)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132867