[Clang] Use virtual FS in processing config files

[sepavloff]

Summary

Clang has support of virtual file system for the purpose of testing, but
treatment of config files did not use it. This change enables VFS in it
as well.

[sammccall]

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.

llvm/lib/Support/CommandLine.cpp

1268

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)

1271

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

[sepavloff]

Fix documentation

[sepavloff]

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

llvm/lib/Support/CommandLine.cpp

1268

You are right, I fixed that documentation.

[sammccall]

In D132867#3770898, @sepavloff wrote:

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

I agree, the situation is overall better after this change.

probably nobody uses such combination, otherwise someone filed a bug or prepared a patch.

I can tell you from experience this doesn't always hold. It's unreasonable to expect users to know that the observed behavior is incorrect.

Strictly speaking this is incorrect behavior because file system object must be used for all operations on files, according to documentation.

I don't think this is a strong argument against documenting the change (is that what we're disagreeing about?)

• what documentation? (E.g. Driver's constructor is undocumented)
• actual behavior trumps documentation: a breaking change that brings behavior into line with docs is still breaking
• it's not true in any case that all clang IO goes through the VFS, as a silly example consider plugin loading with -Xclang -load.
• in practice many users who rely on these details work it out by trial and error

[sepavloff]

Add a release note

[sepavloff]

In D132867#3771004, @sammccall wrote:

Strictly speaking this is incorrect behavior because file system object must be used for all operations on files, according to documentation.

I don't think this is a strong argument against documenting the change (is that what we're disagreeing about?)

No problem to add documentation. I added a release note, if you mean that.

• in practice many users who rely on these details work it out by trial and error

Absolutely!

[sammccall]

Thanks!

clang/docs/ReleaseNotes.rst

87–88 ↗

(On Diff #458205)

Because this is changing behavior that people might be relying on, I think it's important to call out precisely what the change is. Suggested a tweak.

[sepavloff]

Thanks!

clang/lib/Driver/Driver.cpp

 /// \param[in]  Dirs Directories used for the search.
 /// \param[in]  FileName Name of the file to search for.
 /// \return True if file was found.
 ///
 /// Looks for file specified by FileName sequentially in directories specified
 /// by Dirs.
 ///
 static bool searchForFile(SmallVectorImpl<char> &FilePath,
-                          ArrayRef<StringRef> Dirs, StringRef FileName) {
+                          ArrayRef<StringRef> Dirs, StringRef FileName,
+                          llvm::vfs::FileSystem &FS) {
   SmallString<128> WPath;
   for (const StringRef &Dir : Dirs) {
     if (Dir.empty())
       continue;
     WPath.clear();
     llvm::sys::path::append(WPath, Dir, FileName);
     llvm::sys::path::native(WPath);
-    if (llvm::sys::fs::is_regular_file(WPath)) {
+    auto Status = FS.status(WPath);
+    if (Status && Status->getType() == llvm::sys::fs::file_type::regular_file) {
       FilePath = std::move(WPath);
       return true;
     }
   }
   return false;
 }
 
 bool Driver::readConfigFile(StringRef FileName) {
   // Try reading the given file.
   SmallVector<const char *, 32> NewCfgArgs;
-  if (!llvm::cl::readConfigFile(FileName, Saver, NewCfgArgs)) {
+  if (!llvm::cl::readConfigFile(FileName, Saver, NewCfgArgs, getVFS())) {
     Diag(diag::err_drv_cannot_read_config_file) << FileName;
     return true;
   }
 
   // Read options from config file.
   llvm::SmallString<128> CfgFileName(FileName);
   llvm::sys::path::native(CfgFileName);
   ConfigFile = std::string(CfgFileName);
bool Driver::loadConfigFile() {
   bool FileSpecifiedExplicitly = false;
 
   // Process options that change search path for config files.
   if (CLOptions) {
     if (CLOptions->hasArg(options::OPT_config_system_dir_EQ)) {
       SmallString<128> CfgDir;
       CfgDir.append(
           CLOptions->getLastArgValue(options::OPT_config_system_dir_EQ));
-      if (CfgDir.empty() || llvm::sys::fs::make_absolute(CfgDir))
+      if (CfgDir.empty() || getVFS().makeAbsolute(CfgDir))
         SystemConfigDir.clear();
       else
         SystemConfigDir = static_cast<std::string>(CfgDir);
     }
     if (CLOptions->hasArg(options::OPT_config_user_dir_EQ)) {
       SmallString<128> CfgDir;
       CfgDir.append(
           CLOptions->getLastArgValue(options::OPT_config_user_dir_EQ));
-      if (CfgDir.empty() || llvm::sys::fs::make_absolute(CfgDir))
+      if (CfgDir.empty() || getVFS().makeAbsolute(CfgDir))
         UserConfigDir.clear();
       else
         UserConfigDir = static_cast<std::string>(CfgDir);
     }
   }
 
   // First try to find config file specified in command line.
   if (CLOptions) {
     std::vector<std::string> ConfigFiles =
         CLOptions->getAllArgValues(options::OPT_config);
     if (ConfigFiles.size() > 1) {
       if (!llvm::all_equal(ConfigFiles)) {
         Diag(diag::err_drv_duplicate_config);
         return true;
       }
     }
 
     if (!ConfigFiles.empty()) {
       CfgFileName = ConfigFiles.front();
       assert(!CfgFileName.empty());
 
       // If argument contains directory separator, treat it as a path to
       // configuration file.
       if (llvm::sys::path::has_parent_path(CfgFileName)) {
-        SmallString<128> CfgFilePath;
-        if (llvm::sys::path::is_relative(CfgFileName))
-          llvm::sys::fs::current_path(CfgFilePath);
-        llvm::sys::path::append(CfgFilePath, CfgFileName);
-        if (!llvm::sys::fs::is_regular_file(CfgFilePath)) {
+        SmallString<128> CfgFilePath(CfgFileName);
+        if (llvm::sys::path::is_relative(CfgFilePath)) {
+          if (getVFS().makeAbsolute(CfgFilePath))
+            return true;
+          auto Status = getVFS().status(CfgFilePath);
+          if (!Status ||
+              Status->getType() != llvm::sys::fs::file_type::regular_file) {
             Diag(diag::err_drv_config_file_not_exist) << CfgFilePath;
             return true;
           }
+        }
         return readConfigFile(CfgFilePath);
       }
 
       FileSpecifiedExplicitly = true;
     }
   }
 
   // If config file is not specified explicitly, try to deduce configuration
bool Driver::loadConfigFile() {
   }
 
   // Prepare list of directories where config file is searched for.
   StringRef CfgFileSearchDirs[] = {UserConfigDir, SystemConfigDir, Dir};
 
   // Try to find config file. First try file with corrected architecture.
   llvm::SmallString<128> CfgFilePath;
   if (!FixedConfigFile.empty()) {
-    if (searchForFile(CfgFilePath, CfgFileSearchDirs, FixedConfigFile))
+    if (searchForFile(CfgFilePath, CfgFileSearchDirs, FixedConfigFile,
+                      getVFS()))
       return readConfigFile(CfgFilePath);
     // If 'x86_64-clang.cfg' was not found, try 'x86_64.cfg'.
     FixedConfigFile.resize(FixedArchPrefixLen);
     FixedConfigFile.append(".cfg");
-    if (searchForFile(CfgFilePath, CfgFileSearchDirs, FixedConfigFile))
+    if (searchForFile(CfgFilePath, CfgFileSearchDirs, FixedConfigFile,
+                      getVFS()))
       return readConfigFile(CfgFilePath);
   }
 
   // Then try original file name.
-  if (searchForFile(CfgFilePath, CfgFileSearchDirs, CfgFileName))
+  if (searchForFile(CfgFilePath, CfgFileSearchDirs, CfgFileName, getVFS()))
     return readConfigFile(CfgFilePath);
 
   // Finally try removing driver mode part: 'x86_64-clang.cfg' -> 'x86_64.cfg'.
   if (!ClangNameParts.ModeSuffix.empty() &&
       !ClangNameParts.TargetPrefix.empty()) {
     CfgFileName.assign(ClangNameParts.TargetPrefix);
     CfgFileName.append(".cfg");
-    if (searchForFile(CfgFilePath, CfgFileSearchDirs, CfgFileName))
+    if (searchForFile(CfgFilePath, CfgFileSearchDirs, CfgFileName, getVFS()))
       return readConfigFile(CfgFilePath);
   }
 
   // Report error but only if config file was specified explicitly, by option
   // --config. If it was deduced from executable name, it is not an error.
   if (FileSpecifiedExplicitly) {
     Diag(diag::err_drv_config_file_not_found) << CfgFileName;
     for (const StringRef &SearchDir : CfgFileSearchDirs)

clang/unittests/Driver/ToolChainTest.cpp

EXPECT_EQ("Found candidate GCC installation: "
               "Selected GCC installation: "
               "/opt/rh/devtoolset-12/root/usr/lib/gcc/x86_64-redhat-linux/11\n"
               "Candidate multilib: .;@m64\n"
               "Selected multilib: .;@m64\n",
               S);
   }
 }
 
+TEST(ToolChainTest, ConfigFileSearch) {
+  IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts = new DiagnosticOptions();
+  IntrusiveRefCntPtr<DiagnosticIDs> DiagID(new DiagnosticIDs());
+  struct TestDiagnosticConsumer : public DiagnosticConsumer {};
+  DiagnosticsEngine Diags(DiagID, &*DiagOpts, new TestDiagnosticConsumer);
+  IntrusiveRefCntPtr<llvm::vfs::InMemoryFileSystem> FS(
+      new llvm::vfs::InMemoryFileSystem);
+
+#ifdef _WIN32
+  const char *TestRoot = "C:\\";
+#else
+  const char *TestRoot = "/";
+#endif
+  FS->setCurrentWorkingDirectory(TestRoot);
+
+  FS->addFile(
+      "/opt/sdk/root.cfg", 0,
+      llvm::MemoryBuffer::getMemBuffer("--sysroot=/opt/sdk/platform0\n"));
+  FS->addFile(
+      "/home/test/sdk/root.cfg", 0,
+      llvm::MemoryBuffer::getMemBuffer("--sysroot=/opt/sdk/platform1\n"));
+  FS->addFile(
+      "/home/test/bin/root.cfg", 0,
+      llvm::MemoryBuffer::getMemBuffer("--sysroot=/opt/sdk/platform2\n"));
+
+  {
+    Driver TheDriver("/home/test/bin/clang", "arm-linux-gnueabi", Diags,
+                     "clang LLVM compiler", FS);
+    std::unique_ptr<Compilation> C(TheDriver.BuildCompilation(
+        {"/home/test/bin/clang", "--config", "root.cfg",
+         "--config-system-dir=/opt/sdk", "--config-user-dir=/home/test/sdk"}));
+    ASSERT_TRUE(C);
+    ASSERT_FALSE(C->containsError());
+    EXPECT_EQ("/opt/sdk/platform1", TheDriver.SysRoot);
+  }
+  {
+    Driver TheDriver("/home/test/bin/clang", "arm-linux-gnueabi", Diags,
+                     "clang LLVM compiler", FS);
+    std::unique_ptr<Compilation> C(TheDriver.BuildCompilation(
+        {"/home/test/bin/clang", "--config", "root.cfg",
+         "--config-system-dir=/opt/sdk", "--config-user-dir="}));
+    ASSERT_TRUE(C);
+    ASSERT_FALSE(C->containsError());
+    EXPECT_EQ("/opt/sdk/platform0", TheDriver.SysRoot);
+  }
+  {
+    Driver TheDriver("/home/test/bin/clang", "arm-linux-gnueabi", Diags,
+                     "clang LLVM compiler", FS);
+    std::unique_ptr<Compilation> C(TheDriver.BuildCompilation(
+        {"/home/test/bin/clang", "--config", "root.cfg",
+         "--config-system-dir=", "--config-user-dir="}));
+    ASSERT_TRUE(C);
+    ASSERT_FALSE(C->containsError());
+    EXPECT_EQ("/opt/sdk/platform2", TheDriver.SysRoot);
+  }
+}
+
 } // end anonymous namespace.

llvm/include/llvm/Support/CommandLine.h

 /// \return true if the file was successfully read.
 ///
 /// It reads content of the specified file, tokenizes it and expands "@file"
 /// commands resolving file names in them relative to the directory where
 /// CfgFilename resides. It also expands "<CFGDIR>" to the base path of the
 /// current config file.
 ///
 bool readConfigFile(StringRef CfgFileName, StringSaver &Saver,
-                    SmallVectorImpl<const char *> &Argv);
+                    SmallVectorImpl<const char *> &Argv,
+                    llvm::vfs::FileSystem &FS);
 
 /// Expand response files on a command line recursively using the given
 /// StringSaver and tokenization strategy.  Argv should contain the command line
 /// before expansion and will be modified in place. If requested, Argv will
 /// also be populated with nullptrs indicating where each response file line
 /// ends, which is useful for the "/link" argument that needs to consume all
 /// remaining arguments only until the next end of line, when in a response
 /// file.
 ///
 /// \param [in] Saver Delegates back to the caller for saving parsed strings.
 /// \param [in] Tokenizer Tokenization strategy. Typically Unix or Windows.
 /// \param [in,out] Argv Command line into which to expand response files.
 /// \param [in] MarkEOLs Mark end of lines and the end of the response file
 /// with nullptrs in the Argv vector.
 /// \param [in] RelativeNames true if names of nested response files must be
 /// resolved relative to including file.
 /// \param [in] ExpandBasePath If true, "<CFGDIR>" expands to the base path of
 /// the current response file.
 /// \param [in] FS File system used for all file access when running the tool.
 /// \param [in] CurrentDir Path used to resolve relative rsp files. If set to
-/// None, process' cwd is used instead.
+/// None, the file system current directory is used instead.
 /// \return true if all @files were expanded successfully or there were none.
 bool ExpandResponseFiles(StringSaver &Saver, TokenizerCallback Tokenizer,
                          SmallVectorImpl<const char *> &Argv, bool MarkEOLs,
                          bool RelativeNames, bool ExpandBasePath,
                          llvm::Optional<llvm::StringRef> CurrentDir,
                          llvm::vfs::FileSystem &FS);
 
 /// An overload of ExpandResponseFiles() that uses

llvm/lib/Support/CommandLine.cpp

if (Arg[0] != '@') {
       continue;
     }
 
     const char *FName = Arg + 1;
     // Note that CurrentDir is only used for top-level rsp files, the rest will
     // always have an absolute path deduced from the containing file.
     SmallString<128> CurrDir;
     if (llvm::sys::path::is_relative(FName)) {
-      if (!CurrentDir)
-        llvm::sys::fs::current_path(CurrDir);
-      else
+      if (!CurrentDir) {
+        if (auto CWD = FS.getCurrentWorkingDirectory()) {
+          CurrDir = *CWD;
+        } else {
+          // TODO: The error should be propagated up the stack.
+          llvm::consumeError(llvm::errorCodeToError(CWD.getError()));
+          return false;
+        }
+      } else {
         CurrDir = *CurrentDir;
+      }
       llvm::sys::path::append(CurrDir, FName);
       FName = CurrDir.c_str();
     }
     auto IsEquivalent = [FName, &FS](const ResponseFileRecord &RFile) {
       llvm::ErrorOr<llvm::vfs::Status> LHS = FS.status(FName);
       if (!LHS) {
         // TODO: The error should be propagated up the stack.
         llvm::consumeError(llvm::errorCodeToError(LHS.getError()));
if (llvm::Optional<std::string> EnvValue = sys::Process::GetEnv(EnvVar))
       Tokenize(*EnvValue, Saver, NewArgv, /*MarkEOLs=*/false);
 
   // Command line options can override the environment variable.
   NewArgv.append(Argv + 1, Argv + Argc);
   return ExpandResponseFiles(Saver, Tokenize, NewArgv);
 }
 
 bool cl::readConfigFile(StringRef CfgFile, StringSaver &Saver,
-                        SmallVectorImpl<const char *> &Argv) {
+                        SmallVectorImpl<const char *> &Argv,
+                        llvm::vfs::FileSystem &FS) {
   SmallString<128> AbsPath;
   if (sys::path::is_relative(CfgFile)) {
-    llvm::sys::fs::current_path(AbsPath);
-    llvm::sys::path::append(AbsPath, CfgFile);
+    AbsPath.assign(CfgFile);
+    if (std::error_code EC = FS.makeAbsolute(AbsPath))
+      return false;
     CfgFile = AbsPath.str();
   }
-  if (llvm::Error Err = ExpandResponseFile(
-          CfgFile, Saver, cl::tokenizeConfigFile, Argv,
-          /*MarkEOLs=*/false, /*RelativeNames=*/true, /*ExpandBasePath=*/true,
-          *llvm::vfs::getRealFileSystem())) {
+  if (llvm::Error Err =
+          ExpandResponseFile(CfgFile, Saver, cl::tokenizeConfigFile, Argv,
+                             /*MarkEOLs=*/false, /*RelativeNames=*/true,
+                             /*ExpandBasePath=*/true, FS)) {
     // TODO: The error should be propagated up the stack.
     llvm::consumeError(std::move(Err));
     return false;
   }
   return ExpandResponseFiles(Saver, cl::tokenizeConfigFile, Argv,
                              /*MarkEOLs=*/false, /*RelativeNames=*/true,
-                             /*ExpandBasePath=*/true, llvm::None);
+                             /*ExpandBasePath=*/true, llvm::None, FS);
 }
 
 static void initCommonOptions();
 bool cl::ParseCommandLineOptions(int argc, const char *const *argv,
                                  StringRef Overview, raw_ostream *Errs,
                                  const char *EnvVar,
                                  bool LongOptionsUseDoubleDash) {
   initCommonOptions();