[llvm] Add overload to propagate errors up to caller from ExpandResponseFiles

[oontvoo]

use some optional params

[dblaikie]

llvm/lib/Support/CommandLine.cpp

1194–1210

This "!FirstError / FirstError = " idiom seems like one that'd be nice to avoid. I guess it's mostly necessary because FirstError starts with success, and assigning it a new value immediately would cause an assertion about the error not being tested before being replaced.

Rather than using that idiom - perhaps the function could early return instead? This lambda could return Expected<bool>? Oh, nope, because it's used in any_of. /might/ be worth expanding that out/not using any_of to simplify the error handling in some ways I think.

1222–1224

Can this return immediately with th eerror, rather than setting the error and continuing to do more work?

1259–1263

Looks like this'll result in an assert if the function returns a non-success error, it probably needs a consumeError (or whatever it's called) in the failure case before returning false.

[oontvoo]

llvm/lib/Support/CommandLine.cpp

1194–1210

Right, I'd like to keep the first error (because usually it's the more useful than subsequent ones).

How about this - we can have it queue up all the errors and return them? (Rather than just the first one?)

1222–1224

No, because we should clean up Argv and ExpandedArgv (code below) to ensure consistency.

[dblaikie]

llvm/lib/Support/CommandLine.cpp

1194–1210

Hmm, think maybe there's some confusion - could we keep the first error and stop there, returning the error immediately, rather than tracking it through the function?

1222–1224

hmm - consistency with what? We could say that if this function produces an Error the argv should not be used - that the caller should error out immediately and not try to parse anything else from argv.

1259–1263

^ this is still pending/unaddressed

[oontvoo]

llvm/lib/Support/CommandLine.cpp

1222–1224

fair enough!

1259–1263

done (for real this time)

[oontvoo]

addressed review comments

[oontvoo]

rebase

[dblaikie]

What's your motivation in making this change? Do you need this for some new or out of tree caller? Would it be possible to avoid adding this as an overload and migrate existing callers to the Error return value, rather than the bool value immediately, if there aren't too many of them? (or as follow-up commits if it's too much to do in one patch?)

llvm/lib/Support/CommandLine.cpp

1222–1224

This (changing the way argv is left on failure) only works if we actually fix the callers to do that, though - if they aren't already. Which it seems they aren't. (eg: I see one call in cl::ExpandResponseFiles that says "TODO: The error should be propagated up the stack" and another similar one in cl::ReadConfigFile.

I think it'd probably be worth doing that cleanup, if you're up for it - though it does expand the scope. If not doing that work, then yes, we probably need to preserve the argv state as it was being preserved before (at least do any tests fail/care about this? If not I might be willing to say we could break the behavior since people aren't relying on it).

1259–1263

Is this error path tested? (I would've thought if it was tested, this would've been crashing/highlighted the problem earlier - so that seems to hint that maybe this isn't tested, and should be?)

[oontvoo]

updated diff

[oontvoo]

In D110905#3043671, @dblaikie wrote:

What's your motivation in making this change? Do you need this for some new or out of tree caller? Would it be possible to avoid adding this as an overload and migrate existing callers to the Error return value, rather than the bool value immediately, if there aren't too many of them? (or as follow-up commits if it's too much to do in one patch?)

The motivation was to have the error on response file not found (or not readable for whatever reasons) propagated up to caller (lld-macho in my case) so that it's clearer what the problem is. (Previously, if I ran ld64.lld @filename_with_typo.txt, it'd complain about missing arguments, whereas the real problem was the response filename being mistyped).

llvm/lib/Support/CommandLine.cpp

1222–1224

I've updated the the diff to address your original concern (most of it) wrt to the "!FirstError / FirstError = " idiom.

Ultimately, I guess I prefer to keep the current behaviour rather than overhauling it and its callers (clang seems to specifically rely on this and has a bunch of tests that failed when I tried to change this).

[oontvoo]

llvm/lib/Support/CommandLine.cpp

1259–1263

it's likely not tested given the build was all green earlier.

[oontvoo]

Withdrawing this . With some deliberation, I've realised this patch is actually doing the wrong thing . Specifically, the expansion here happens too early that it shouldn't know yet if it would really be an error if @foo is not a file ... it could have meant to be some variable(, eg -rpath @loader_path)

The fix I'm hoping to make can be done in lld-macho . (it also should have had a test that fails because of this patch).

Thanks for entertaining this!

[dblaikie]

In D110905#3046352, @oontvoo wrote:

Withdrawing this . With some deliberation, I've realised this patch is actually doing the wrong thing . Specifically, the expansion here happens too early that it shouldn't know yet if it would really be an error if @foo is not a file ... it could have meant to be some variable(, eg -rpath @loader_path)

The fix I'm hoping to make can be done in lld-macho . (it also should have had a test that fails because of this patch).

Thanks for entertaining this!

OK, thanks for the discussion!

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>

 /// \param [in] CurrentDir Path used to resolve relative rsp files. If set to
 /// None, process' cwd 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,
                          llvm::Optional<llvm::StringRef> CurrentDir,
                          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] 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.
+/// \return success if all @files were expanded successfully or there were none.
+llvm::Error ExpandResponseFilesWithError(
+    StringSaver &Saver, TokenizerCallback Tokenizer,
+    SmallVectorImpl<const char *> &Argv, bool MarkEOLs = false,
+    bool RelativeNames = false,
+    llvm::Optional<llvm::StringRef> CurrentDir = llvm::None,
+    llvm::vfs::FileSystem &FS = *llvm::vfs::getRealFileSystem());
 
 /// An overload of ExpandResponseFiles() that uses
 /// llvm::vfs::getRealFileSystem().
 bool ExpandResponseFiles(
     StringSaver &Saver, TokenizerCallback Tokenizer,
     SmallVectorImpl<const char *> &Argv, bool MarkEOLs = false,
     bool RelativeNames = false,
     llvm::Optional<llvm::StringRef> CurrentDir = llvm::None);

llvm/lib/Support/CommandLine.cpp

for (auto &Arg : NewArgv) {
     llvm::sys::path::append(ResponseFile, FileName);
     Arg = Saver.save(ResponseFile.c_str()).data();
   }
   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,
-                             llvm::Optional<llvm::StringRef> CurrentDir,
-                             llvm::vfs::FileSystem &FS) {
-  bool AllExpanded = true;
+llvm::Error cl::ExpandResponseFilesWithError(
+    StringSaver &Saver, TokenizerCallback Tokenizer,
+    SmallVectorImpl<const char *> &Argv, bool MarkEOLs, bool RelativeNames,
+    llvm::Optional<llvm::StringRef> CurrentDir, llvm::vfs::FileSystem &FS) {
   struct ResponseFileRecord {
     std::string 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
   // dynamically as we recursively expand files.
   SmallVector<ResponseFileRecord, 3> FileStack;
 
   // Push a dummy entry that represents the initial command line, removing
   // the need to check for an empty list.
   FileStack.push_back({"", Argv.size()});
 
+  llvm::Error FirstErr = llvm::Error::success();
   // Don't cache Argv.size() because it can change.
   for (unsigned I = 0; I != Argv.size();) {
     while (I == FileStack.back().End) {
       // Passing the end of a file's argument list, so we can remove it from the
       // stack.
       FileStack.pop_back();
     }
 
for (unsigned I = 0; I != Argv.size();) {
     if (llvm::sys::path::is_relative(FName)) {
       if (!CurrentDir)
         llvm::sys::fs::current_path(CurrDir);
       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()));
-        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);
-    };
 
+    SmallVector<const char *, 0> ExpandedArgv;
+    auto DoExpandResponseFile = [&]() -> llvm::Error {
       // Check for recursive response files.
-    if (any_of(drop_begin(FileStack), IsEquivalent)) {
-      // This file is recursive, so we leave it in the argument stream and
-      // move on.
-      AllExpanded = false;
-      ++I;
-      continue;
+      llvm::ErrorOr<llvm::vfs::Status> LHS = FS.status(FName);
+      if (!LHS)
+        return llvm::createStringError(LHS.getError(),
+                                       Twine("cannot read response file: ") +
+                                           Twine(FName));
+
+      for (const auto &RFile : drop_begin(FileStack)) {
+        llvm::ErrorOr<llvm::vfs::Status> RHS = FS.status(RFile.File);
+        if (!RHS)
+          return llvm::createStringError(RHS.getError(),
+                                         Twine("cannot to read file: ") +
+                                             Twine(RFile.File));
+
+        if (LHS->equivalent(*RHS))
+          return createStringError(
+              std::make_error_code(std::errc::invalid_argument),
+              Twine("cannot expand recursive response file: ") + Twine(FName));
       }
 
       // 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 we fail to read this file, then give up and return an error.
       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;
+              ExpandResponseFile(FName, Saver, Tokenizer, ExpandedArgv,
+                                 MarkEOLs, RelativeNames, FS))
+        return std::move(Err);
+      return llvm::Error::success();
+    };
+
+    if (llvm::Error Err = DoExpandResponseFile()) {
+      // Leave this on the argument stream and move on.
       ++I;
+      if (!FirstErr)
+        FirstErr = std::move(Err);
+      else
+        consumeError(std::move(Err));
       continue;
     }
-
     for (ResponseFileRecord &Record : FileStack) {
       // Increase the end of all active records by the number of newly expanded
       // arguments, minus the response file itself.
       Record.End += ExpandedArgv.size() - 1;
     }
 
     FileStack.push_back({FName, I + ExpandedArgv.size()});
     Argv.erase(Argv.begin() + I);
     Argv.insert(Argv.begin() + I, ExpandedArgv.begin(), ExpandedArgv.end());
   }
 
   // If successful, the top of the file stack will mark the end of the Argv
   // stream. A failure here indicates a bug in the stack popping logic above.
   // Note that FileStack may have more than one element at this point because we
   // 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;
+  return std::move(FirstErr);
 }
 
+/// 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,
+                             llvm::Optional<llvm::StringRef> CurrentDir,
+                             llvm::vfs::FileSystem &FS) {
+  if (Error err = ExpandResponseFilesWithError(
+          Saver, std::move(Tokenizer), Argv, MarkEOLs, RelativeNames,
+          std::move(CurrentDir), *vfs::getRealFileSystem())) {
+    consumeError(std::move(err));
+    return false;
+  }
+  return true;
+}
 bool cl::ExpandResponseFiles(StringSaver &Saver, TokenizerCallback Tokenizer,
                              SmallVectorImpl<const char *> &Argv, bool MarkEOLs,
                              bool RelativeNames,
                              llvm::Optional<StringRef> CurrentDir) {
   return ExpandResponseFiles(Saver, std::move(Tokenizer), Argv, MarkEOLs,
                              RelativeNames, std::move(CurrentDir),
                              *vfs::getRealFileSystem());
 }