[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

1148

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

1076

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

1077

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.

1145–1151

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.

1148

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

1061

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?

1148

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

1061

nit: no need for braces

1066

nit: no need for braces

1067

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

1168

clang-format ?

[jhenderson]

llvm/lib/Support/CommandLine.cpp

1068

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

1168

clang-format please

[lh123]

address comments.

[jhenderson]

llvm/lib/Support/CommandLine.cpp

1068

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

[lh123]

address comments.

[jhenderson]

llvm/lib/Support/CommandLine.cpp

1068

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

1068

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

1068

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

1068

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/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"
 

 // 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,
                                TokenizerCallback Tokenizer,
                                SmallVectorImpl<const char *> &NewArgv,
-                               bool MarkEOLs, bool RelativeNames) {
+                               bool MarkEOLs, bool RelativeNames,
+                               llvm::vfs::FileSystem &FS) {
+  llvm::ErrorOr<std::string> CurrDir = FS.getCurrentWorkingDirectory();
+  if (!CurrDir)
+    return false;
   ErrorOr<std::unique_ptr<MemoryBuffer>> MemBufOrErr =
-      MemoryBuffer::getFile(FName);
+      FS.getBufferForFile(FName);
   if (!MemBufOrErr)
     return false;
   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;
     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(CurrDir.get());
             }
             llvm::sys::path::append(
                 ResponseFile, llvm::sys::path::parent_path(FName), FileName);
             NewArgv[I] = Saver.save(ResponseFile.c_str()).data();
           }
         }
       }
 
   return true;
 }
 
 /// 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);
+      llvm::ErrorOr<llvm::vfs::Status> RHS = FS.status(RFile.File);
+      if (!LHS || !RHS) {
+        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)) {
+                            RelativeNames, FS)) {
       // We couldn't read this file, so we leave it in the argument stream and
       // move on.
       AllExpanded = false;
       ++I;
       continue;
     }
 
     for (ResponseFileRecord &Record : FileStack) {
       // Increase the end of all active records by the number of newly expanded
bool cl::ExpandResponseFiles(StringSaver &Saver, TokenizerCallback Tokenizer,
   // 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))
+                          /*MarkEOLs*/ false, /*RelativeNames*/ true,
+                          *llvm::vfs::getRealFileSystem()))
     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