[Support] add vfs support for ExpandResponseFiles

[lh123]

Summary

add vfs support for ExpandResponseFiles.

[merge_guards_bot]

Build result: pass - 60330 tests passed, 0 failed and 732 were skipped.

Log files: console-log.txt, CMakeCache.txt

[jhenderson]

Are there any instances where we DON'T want to get the real file system? If not, could the *llvm::vfs::getRealFileSystem() call be put inside cl::ExpandResponseFiles?

llvm/include/llvm/Support/CommandLine.h

32

Can llvm::vfs::FileSystem be forward declared and this moved into the .cpp?

[lh123]

In D70769#1761394, @jhenderson wrote:

Are there any instances where we DON'T want to get the real file system? If not, could the *llvm::vfs::getRealFileSystem() call be put inside cl::ExpandResponseFiles?

This patch is part of D70222.
This is for using InMemoryFileSystem in unit tests.

[jhenderson]

In D70769#1761428, @lh123 wrote:

In D70769#1761394, @jhenderson wrote:

Are there any instances where we DON'T want to get the real file system? If not, could the *llvm::vfs::getRealFileSystem() call be put inside cl::ExpandResponseFiles?

This patch is part of D70222.
This is for using InMemoryFileSystem in unit tests.

Okay, that makes sense. Perhaps we should introduce a new overload of ExpandResponseFiles that allows specifying a different file system then, and having the current version call into that one, specifying the getRealFileSystem call? I'm not overly keen on having yet another apparently boiler-plate piece of functionality required in every single call of ExpandResponseFiles.

[sammccall]

In D70769#1761494, @jhenderson wrote:

In D70769#1761428, @lh123 wrote:

In D70769#1761394, @jhenderson wrote:

Are there any instances where we DON'T want to get the real file system? If not, could the *llvm::vfs::getRealFileSystem() call be put inside cl::ExpandResponseFiles?

This patch is part of D70222.
This is for using InMemoryFileSystem in unit tests.

Okay, that makes sense. Perhaps we should introduce a new overload of ExpandResponseFiles that allows specifying a different file system then, and having the current version call into that one, specifying the getRealFileSystem call? I'm not overly keen on having yet another apparently boiler-plate piece of functionality required in every single call of ExpandResponseFiles.

It's not just unit-tests, in D70222 we will ultimately use FSes other than getRealFilesystem() in clangd.
Having non-VFS-clean versions of functions around that silently use the real FS is a bit of a scourge for users that need to be VFS-clean, honestly. Parsing argv is exactly the sort of place where FS access isn't obvious and exposing a function that doesn't accept a VFS encourages callers to miss this aspect. At the same time this is mostly called from a bunch of drivers that (presumably) don't need VFS support. Maybe allowing nullptr = real FS, or a default argument, would be an OK compromise?

[jhenderson]

In D70769#1761517, @sammccall wrote:

It's not just unit-tests, in D70222 we will ultimately use FSes other than getRealFilesystem() in clangd.

I see, thank you.

Having non-VFS-clean versions of functions around that silently use the real FS is a bit of a scourge for users that need to be VFS-clean, honestly. Parsing argv is exactly the sort of place where FS access isn't obvious and exposing a function that doesn't accept a VFS encourages callers to miss this aspect. At the same time this is mostly called from a bunch of drivers that (presumably) don't need VFS support. Maybe allowing nullptr = real FS, or a default argument, would be an OK compromise?

A default argument would be my preference there. It looks like we have some already.

llvm/include/llvm/Support/CommandLine.h

1963

Does VFS make sense here, given that it could be a real filesystem?

[lh123]

Address comments

[lh123]

address comments and delete unused code.

[merge_guards_bot]

Build result: fail - 60341 tests passed, 3 failed and 732 were skipped.

failed: Clang.CodeGen/catch-implicit-integer-sign-changes-incdec.c
failed: Clang.CodeGen/catch-implicit-signed-integer-truncations-incdec.c
failed: libc++.std/thread/futures/futures_async/async.pass.cpp

Log files: console-log.txt, CMakeCache.txt

[lh123]

address comments.

[merge_guards_bot]

Build result: FAILURE -
Log files: console-log.txt, CMakeCache.txt

[jhenderson]

Aside from one nit, this looks okay to me. I'd like someone else to look at it though too. I'm not at all familiar with the file system stuff to be confident to say everything there is good.

llvm/lib/Support/CommandLine.cpp

1152

Has this been clang-formatted?

[kadircet]

llvm/include/llvm/Support/CommandLine.h

47

now that we are also pulling the the function, I think it is OK to include the header instead.

llvm/lib/Support/CommandLine.cpp

1080

you can move this to the beginning of the function, to exit immediately, without checking for anything else.

1081

it is sad that, ExpandResponseFile returns a bool, but that's a battle for another day.

unfortunately, it is not enough return false in this case you need to consume the error with llvm::consumeError before exiting the scope.

1149–1162

there are FileSystem::status and Status::equivalent functions to implement llvm::sys::fs:equivalent behaviour

[lh123]

address comments.

[lh123]

llvm/include/llvm/Support/CommandLine.h

47

Yes, we need to include VirtualFileSystem.h, IntrusiveRefCntPtr needs to know the complete definition of FileSystem.

[merge_guards_bot]

Build result: pass - 60338 tests passed, 0 failed and 732 were skipped.

Log files: console-log.txt, CMakeCache.txt

[kadircet]

llvm/lib/Support/CommandLine.cpp

1053

this comment has moved into a irrelevant line and wasn't addressed, so re-stating it here:

it is sad that, ExpandResponseFile returns a bool, but that's a battle for another day.

unfortunately, it is not enough return false in this case you need to consume the error with llvm::consumeError before exiting the scope.

1152

again you need to consumeErrors before returning. I would first get LHS, consume and bail out if it was an error, then do the same for RHS and only after that return equivalent.

[lh123]

address comments.

[lh123]

address comments.

[merge_guards_bot]

Build result: FAILURE -
Log files: console-log.txt, CMakeCache.txt

[lh123]

Sorry, upload the wrong patch.

[jhenderson]

llvm/lib/Support/CommandLine.cpp

1054

I'd ultimately like to see the public interface return llvm::Expected/llvm::Error so that a potentially useful error with information isn't just thrown away. However, I agree that that's probably a separate change. On the other hand, it should be simple to return it here, since this is only used locally, for consumption in ExpandResponseFiles below. Could you make that change, please?

1152

Could you add a TODO comment here and at the other consumeError() call to say that the error should be propagated up the stack, please?

[lh123]

address comments.

[merge_guards_bot]

Build result: FAILURE -
Log files: console-log.txt, CMakeCache.txt

[lh123]

address comments.

[merge_guards_bot]

Build result: FAILURE -
Log files: console-log.txt, CMakeCache.txt

[lh123]

fixes no matching constructor for initialization of llvm::StringError.

[kadircet]

LGTM, please make sure you've clang-formatted the changes though

llvm/lib/Support/CommandLine.cpp

1054

nit: no need for braces

1059

nit: no need for braces

1070

nit: you can make use of llvm::createStringError instead

1178

clang-format ?

[jhenderson]

llvm/lib/Support/CommandLine.cpp

1071

Not sure, but you might want to use illegal_byte_sequence here instead of llvm::inconvertibleErrorCode().

1178

clang-format please

[lh123]

address comments.

[jhenderson]

llvm/lib/Support/CommandLine.cpp

1071

std::make_error_code(std::errc::illegal_byte_sequence) -> errc::illegal_byte_sequence

[lh123]

address comments.

[jhenderson]

llvm/lib/Support/CommandLine.cpp

1071

LLVM has its own errc error_code set. Please use that, i.e. delete the std:: (note that I didn't add std:: in my previous comment).

[lh123]

llvm/lib/Support/CommandLine.cpp

1071

I think we should use std::errc::illegal_byte_sequence, because it's sigature is:

template <typename... Ts>
inline Error createStringError(std::errc EC, char const *Fmt, const Ts &... Vals)

[jhenderson]

llvm/lib/Support/CommandLine.cpp

1071

I'm not sure if that's a mistake in the function interface or not, and it looks like our usage is very inconsistent, but the comments in Errc.h imply that we really should use llvm::errc values and not std::errc, or there may be problems.

[lh123]

llvm/lib/Support/CommandLine.cpp

1071

I think llvm::errc and std::errc are both correct, llvm::errc eventually implicitly converts to std::error_code.

[lh123]

fix typo:
"Could not convert UTF16 To UTF8" -> "Could not convert UTF16 to UTF8"

[kadircet]

@jhenderson I am planning to commit this if the discussion around std::errc vs llvm::errc is resolved, I don't have any preference towards one or the other both seems to get the work done.

[jhenderson]

In D70769#1763703, @kadircet wrote:

@jhenderson I am planning to commit this if the discussion around std::errc vs llvm::errc is resolved, I don't have any preference towards one or the other both seems to get the work done.

I'm not going to block this being committed. I do think llvm::errc exists for a reason, and those reasons are documented in the associated file, giving me the impression we should use it and not std::errc. However, I don't know how applicable those reasons really are nowadays - I'm certainly no authority on them.

llvm/include/llvm/Support/CommandLine.h

 #include "llvm/ADT/SmallPtrSet.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/Twine.h"
 #include "llvm/ADT/iterator_range.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/ManagedStatic.h"
+#include "llvm/Support/VirtualFileSystem.h"
 #include "llvm/Support/raw_ostream.h"
 #include <cassert>
 #include <climits>
 #include <cstddef>
 #include <functional>
 #include <initializer_list>
 #include <string>
 #include <type_traits>
 #include <vector>
 
 namespace llvm {
 
 class StringSaver;
 class raw_ostream;
 
 /// cl Namespace - This namespace contains all of the command line option
 /// processing machinery.  It is intentionally a short name to make qualified
 /// usage concise.
 namespace cl {
 
 //===----------------------------------------------------------------------===//
 // ParseCommandLineOptions - Command line option processing entry point.
 //

 /// 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] FS File system used for all file access when running the tool.
 /// \return true if all @files were expanded successfully or there were none.
-bool ExpandResponseFiles(StringSaver &Saver, TokenizerCallback Tokenizer,
-                         SmallVectorImpl<const char *> &Argv,
-                         bool MarkEOLs = false, bool RelativeNames = false);
+bool ExpandResponseFiles(
+    StringSaver &Saver, TokenizerCallback Tokenizer,
+    SmallVectorImpl<const char *> &Argv, bool MarkEOLs = false,
+    bool RelativeNames = false,
+    llvm::vfs::FileSystem &FS = *llvm::vfs::getRealFileSystem());
 
 /// Mark all options not part of this category as cl::ReallyHidden.
 ///
 /// \param Category the category of options to keep displaying
 ///
 /// Some tools (like clang-format) like to be able to hide all options that are
 /// not specific to the tool. This function allows a tool to specify a single
 /// option category to display in the -help output.

llvm/lib/Support/CommandLine.cpp

 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/Triple.h"
 #include "llvm/ADT/Twine.h"
 #include "llvm/Config/config.h"
 #include "llvm/Support/ConvertUTF.h"
 #include "llvm/Support/Debug.h"
+#include "llvm/Support/Error.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Host.h"
 #include "llvm/Support/ManagedStatic.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Process.h"
 #include "llvm/Support/StringSaver.h"
+#include "llvm/Support/VirtualFileSystem.h"
 #include "llvm/Support/raw_ostream.h"
 #include <cstdlib>
 #include <map>
 using namespace llvm;
 using namespace cl;
 
 #define DEBUG_TYPE "commandline"
 

 }
 
 // It is called byte order marker but the UTF-8 BOM is actually not affected
 // by the host system's endianness.
 static bool hasUTF8ByteOrderMark(ArrayRef<char> S) {
   return (S.size() >= 3 && S[0] == '\xef' && S[1] == '\xbb' && S[2] == '\xbf');
 }
 
-static bool ExpandResponseFile(StringRef FName, StringSaver &Saver,
+static llvm::Error ExpandResponseFile(StringRef FName, StringSaver &Saver,
                                TokenizerCallback Tokenizer,
                                SmallVectorImpl<const char *> &NewArgv,
-                               bool MarkEOLs, bool RelativeNames) {
-  ErrorOr<std::unique_ptr<MemoryBuffer>> MemBufOrErr =
-      MemoryBuffer::getFile(FName);
-  if (!MemBufOrErr)
-    return false;
+                               bool MarkEOLs, bool RelativeNames,
+                               llvm::vfs::FileSystem &FS) {
+  llvm::ErrorOr<std::string> CurrDirOrErr = FS.getCurrentWorkingDirectory();
+  if (!CurrDirOrErr) {
+    return llvm::errorCodeToError(CurrDirOrErr.getError());
+  }
+  llvm::ErrorOr<std::unique_ptr<MemoryBuffer>> MemBufOrErr =
+      FS.getBufferForFile(FName);
+  if (!MemBufOrErr) {
+    return llvm::errorCodeToError(MemBufOrErr.getError());
+  }
   MemoryBuffer &MemBuf = *MemBufOrErr.get();
   StringRef Str(MemBuf.getBufferStart(), MemBuf.getBufferSize());
 
   // If we have a UTF-16 byte order mark, convert to UTF-8 for parsing.
   ArrayRef<char> BufRef(MemBuf.getBufferStart(), MemBuf.getBufferEnd());
   std::string UTF8Buf;
   if (hasUTF16ByteOrderMark(BufRef)) {
     if (!convertUTF16ToUTF8String(BufRef, UTF8Buf))
-      return false;
+      return llvm::make_error<llvm::StringError>(
+          "Could not convert UTF16 To UTF8", llvm::inconvertibleErrorCode());
     Str = StringRef(UTF8Buf);
   }
   // If we see UTF-8 BOM sequence at the beginning of a file, we shall remove
   // these bytes before parsing.
   // Reference: http://en.wikipedia.org/wiki/UTF-8#Byte_order_mark
   else if (hasUTF8ByteOrderMark(BufRef))
     Str = StringRef(BufRef.data() + 3, BufRef.size() - 3);
 
   // Tokenize the contents into NewArgv.
   Tokenizer(Str, Saver, NewArgv, MarkEOLs);
 
   // If names of nested response files should be resolved relative to including
   // file, replace the included response file names with their full paths
   // obtained by required resolution.
   if (RelativeNames)
     for (unsigned I = 0; I < NewArgv.size(); ++I)
       if (NewArgv[I]) {
         StringRef Arg = NewArgv[I];
         if (Arg.front() == '@') {
           StringRef FileName = Arg.drop_front();
           if (llvm::sys::path::is_relative(FileName)) {
             SmallString<128> ResponseFile;
             ResponseFile.append(1, '@');
             if (llvm::sys::path::is_relative(FName)) {
-              SmallString<128> curr_dir;
-              llvm::sys::fs::current_path(curr_dir);
-              ResponseFile.append(curr_dir.str());
+              ResponseFile.append(CurrDirOrErr.get());
             }
             llvm::sys::path::append(
                 ResponseFile, llvm::sys::path::parent_path(FName), FileName);
             NewArgv[I] = Saver.save(ResponseFile.c_str()).data();
           }
         }
       }
 
-  return true;
+  return Error::success();
 }
 
 /// Expand response files on a command line recursively using the given
 /// StringSaver and tokenization strategy.
 bool cl::ExpandResponseFiles(StringSaver &Saver, TokenizerCallback Tokenizer,
-                             SmallVectorImpl<const char *> &Argv,
-                             bool MarkEOLs, bool RelativeNames) {
+                             SmallVectorImpl<const char *> &Argv, bool MarkEOLs,
+                             bool RelativeNames, llvm::vfs::FileSystem &FS) {
   bool AllExpanded = true;
   struct ResponseFileRecord {
     const char *File;
     size_t End;
   };
 
   // To detect recursive response files, we maintain a stack of files and the
   // position of the last argument in the file. This position is updated
for (unsigned I = 0; I != Argv.size();) {
     }
 
     if (Arg[0] != '@') {
       ++I;
       continue;
     }
 
     const char *FName = Arg + 1;
-    auto IsEquivalent = [FName](const ResponseFileRecord &RFile) {
-      return sys::fs::equivalent(RFile.File, FName);
+    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()));
+        return false;
+      }
+      llvm::ErrorOr<llvm::vfs::Status> RHS = FS.status(RFile.File);
+      if (!RHS) {
+        // TODO: The error should be propagated up the stack.
+        llvm::consumeError(llvm::errorCodeToError(RHS.getError()));
+        return false;
+      }
+      return LHS->equivalent(*RHS);
     };
 
     // Check for recursive response files.
     if (std::any_of(FileStack.begin() + 1, FileStack.end(), IsEquivalent)) {
       // This file is recursive, so we leave it in the argument stream and
       // move on.
       AllExpanded = false;
       ++I;
       continue;
     }
 
     // Replace this response file argument with the tokenization of its
     // contents.  Nested response files are expanded in subsequent iterations.
     SmallVector<const char *, 0> ExpandedArgv;
-    if (!ExpandResponseFile(FName, Saver, Tokenizer, ExpandedArgv, MarkEOLs,
-                            RelativeNames)) {
+
+    if (llvm::Error Err = ExpandResponseFile(FName, Saver, Tokenizer, ExpandedArgv, MarkEOLs,
+                            RelativeNames, FS)) {
       // We couldn't read this file, so we leave it in the argument stream and
       // move on.
+      // TODO: The error should be propagated up the stack.
+      llvm::consumeError(std::move(Err));
       AllExpanded = false;
       ++I;
       continue;
     }
 
     for (ResponseFileRecord &Record : FileStack) {
       // Increase the end of all active records by the number of newly expanded
       // arguments, minus the response file itself.
bool cl::ExpandResponseFiles(StringSaver &Saver, TokenizerCallback Tokenizer,
   // don't have a chance to pop the stack when encountering recursive files at
   // the end of the stream, so seeing that doesn't indicate a bug.
   assert(FileStack.size() > 0 && Argv.size() == FileStack.back().End);
   return AllExpanded;
 }
 
 bool cl::readConfigFile(StringRef CfgFile, StringSaver &Saver,
                         SmallVectorImpl<const char *> &Argv) {
-  if (!ExpandResponseFile(CfgFile, Saver, cl::tokenizeConfigFile, Argv,
-                          /*MarkEOLs*/ false, /*RelativeNames*/ true))
+  if (llvm::Error Err =
+          ExpandResponseFile(CfgFile, Saver, cl::tokenizeConfigFile, Argv,
+                             /*MarkEOLs*/ false, /*RelativeNames*/ true,
+                             *llvm::vfs::getRealFileSystem())) {
+    // 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);
 }
 
 /// ParseEnvironmentOptions - An alternative entry point to the
 /// CommandLine library, which allows you to read the program's name
 /// from the caller (as PROGNAME) and its command-line arguments from
 /// an environment variable (whose name is given in ENVVAR).