[clang] c1728a4 - Revert "Handle errors in expansion of response files"

Serge Pavlov via cfe-commits cfe-commits at lists.llvm.org
Sat Oct 29 12:04:01 PDT 2022


Author: Serge Pavlov
Date: 2022-10-30T02:03:12+07:00
New Revision: c1728a40aae31abc0a5d4d07f6f6a6773d803f2c

URL: https://github.com/llvm/llvm-project/commit/c1728a40aae31abc0a5d4d07f6f6a6773d803f2c
DIFF: https://github.com/llvm/llvm-project/commit/c1728a40aae31abc0a5d4d07f6f6a6773d803f2c.diff

LOG: Revert "Handle errors in expansion of response files"

This reverts commit 17eb198de934eced784e16ec15e020a574ba07e1.
Reverted for investigation, because ClangDriverTests failed on some builders.

Added: 
    

Modified: 
    clang/include/clang/Basic/DiagnosticDriverKinds.td
    clang/lib/Driver/Driver.cpp
    clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp
    clang/test/Driver/config-file-errs.c
    clang/tools/driver/driver.cpp
    clang/unittests/Driver/ToolChainTest.cpp
    flang/tools/flang-driver/driver.cpp
    llvm/include/llvm/Support/CommandLine.h
    llvm/lib/Support/CommandLine.cpp
    llvm/unittests/Support/CommandLineTest.cpp

Removed: 
    


################################################################################
diff  --git a/clang/include/clang/Basic/DiagnosticDriverKinds.td b/clang/include/clang/Basic/DiagnosticDriverKinds.td
index e477d93ba067f..b09e124206213 100644
--- a/clang/include/clang/Basic/DiagnosticDriverKinds.td
+++ b/clang/include/clang/Basic/DiagnosticDriverKinds.td
@@ -214,14 +214,14 @@ def err_drv_malformed_sanitizer_coverage_ignorelist : Error<
   "malformed sanitizer coverage ignorelist: '%0'">;
 def err_drv_duplicate_config : Error<
   "no more than one option '--config' is allowed">;
-def err_drv_cannot_open_config_file : Error<
-  "configuration file '%0' cannot be opened: %1">;
+def err_drv_config_file_not_exist : Error<
+  "configuration file '%0' does not exist">;
 def err_drv_config_file_not_found : Error<
   "configuration file '%0' cannot be found">;
 def note_drv_config_file_searched_in : Note<
   "was searched for in the directory: %0">;
 def err_drv_cannot_read_config_file : Error<
-  "cannot read configuration file '%0': %1">;
+  "cannot read configuration file '%0'">;
 def err_drv_nested_config_file: Error<
   "option '--config' is not allowed inside configuration file">;
 def err_drv_arg_requires_bitcode_input: Error<

diff  --git a/clang/lib/Driver/Driver.cpp b/clang/lib/Driver/Driver.cpp
index 80e6ec76d16f7..26729f1d03ea8 100644
--- a/clang/lib/Driver/Driver.cpp
+++ b/clang/lib/Driver/Driver.cpp
@@ -940,24 +940,10 @@ static void appendOneArg(InputArgList &Args, const Arg *Opt,
 
 bool Driver::readConfigFile(StringRef FileName,
                             llvm::cl::ExpansionContext &ExpCtx) {
-  // Try opening the given file.
-  auto Status = getVFS().status(FileName);
-  if (!Status) {
-    Diag(diag::err_drv_cannot_open_config_file)
-        << FileName << Status.getError().message();
-    return true;
-  }
-  if (Status->getType() != llvm::sys::fs::file_type::regular_file) {
-    Diag(diag::err_drv_cannot_open_config_file)
-        << FileName << "not a regular file";
-    return true;
-  }
-
   // Try reading the given file.
   SmallVector<const char *, 32> NewCfgArgs;
-  if (llvm::Error Err = ExpCtx.readConfigFile(FileName, NewCfgArgs)) {
-    Diag(diag::err_drv_cannot_read_config_file)
-        << FileName << toString(std::move(Err));
+  if (!ExpCtx.readConfigFile(FileName, NewCfgArgs)) {
+    Diag(diag::err_drv_cannot_read_config_file) << FileName;
     return true;
   }
 
@@ -1039,9 +1025,12 @@ bool Driver::loadConfigFiles() {
       if (llvm::sys::path::has_parent_path(CfgFileName)) {
         CfgFilePath.assign(CfgFileName);
         if (llvm::sys::path::is_relative(CfgFilePath)) {
-          if (getVFS().makeAbsolute(CfgFilePath)) {
-            Diag(diag::err_drv_cannot_open_config_file)
-                << CfgFilePath << "cannot get absolute path";
+          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;
           }
         }

diff  --git a/clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp b/clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp
index 88d20ba3957d2..c4b3abc1a0a45 100644
--- a/clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp
+++ b/clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp
@@ -61,11 +61,9 @@ class ExpandResponseFilesDatabase : public CompilationDatabase {
         continue;
       llvm::BumpPtrAllocator Alloc;
       llvm::cl::ExpansionContext ECtx(Alloc, Tokenizer);
-      llvm::Error Err = ECtx.setVFS(FS.get())
-                            .setCurrentDir(Cmd.Directory)
-                            .expandResponseFiles(Argv);
-      if (Err)
-        llvm::errs() << Err;
+      ECtx.setVFS(FS.get())
+          .setCurrentDir(Cmd.Directory)
+          .expandResponseFiles(Argv);
       // Don't assign directly, Argv aliases CommandLine.
       std::vector<std::string> ExpandedArgv(Argv.begin(), Argv.end());
       Cmd.CommandLine = std::move(ExpandedArgv);

diff  --git a/clang/test/Driver/config-file-errs.c b/clang/test/Driver/config-file-errs.c
index 85b3443a7e530..497e6cb4a5c2b 100644
--- a/clang/test/Driver/config-file-errs.c
+++ b/clang/test/Driver/config-file-errs.c
@@ -13,7 +13,7 @@
 //--- Argument of '--config' must be existing file, if it is specified by path.
 //
 // RUN: not %clang --config somewhere/nonexistent-config-file 2>&1 | FileCheck %s -check-prefix CHECK-NONEXISTENT
-// CHECK-NONEXISTENT: configuration file '{{.*}}somewhere{{.}}nonexistent-config-file' cannot be opened: {{[Nn]}}o such file or directory
+// CHECK-NONEXISTENT: configuration file '{{.*}}somewhere/nonexistent-config-file' does not exist
 
 
 //--- All '--config' arguments must be existing files.

diff  --git a/clang/tools/driver/driver.cpp b/clang/tools/driver/driver.cpp
index 2cc3b48609cb3..11eba44fcf22e 100644
--- a/clang/tools/driver/driver.cpp
+++ b/clang/tools/driver/driver.cpp
@@ -309,10 +309,7 @@ static int ExecuteCC1Tool(SmallVectorImpl<const char *> &ArgV) {
 
   llvm::BumpPtrAllocator A;
   llvm::cl::ExpansionContext ECtx(A, llvm::cl::TokenizeGNUCommandLine);
-  if (llvm::Error Err = ECtx.expandResponseFiles(ArgV)) {
-    llvm::errs() << toString(std::move(Err)) << '\n';
-    return 1;
-  }
+  ECtx.expandResponseFiles(ArgV);
   StringRef Tool = ArgV[1];
   void *GetExecutablePathVP = (void *)(intptr_t)GetExecutablePath;
   if (Tool == "-cc1")
@@ -376,11 +373,7 @@ int clang_main(int Argc, char **Argv) {
   if (MarkEOLs && Args.size() > 1 && StringRef(Args[1]).startswith("-cc1"))
     MarkEOLs = false;
   llvm::cl::ExpansionContext ECtx(A, Tokenizer);
-  ECtx.setMarkEOLs(MarkEOLs);
-  if (llvm::Error Err = ECtx.expandResponseFiles(Args)) {
-    llvm::errs() << Err << '\n';
-    return 1;
-  }
+  ECtx.setMarkEOLs(MarkEOLs).expandResponseFiles(Args);
 
   // Handle -cc1 integrated tools, even if -cc1 was expanded from a response
   // file.

diff  --git a/clang/unittests/Driver/ToolChainTest.cpp b/clang/unittests/Driver/ToolChainTest.cpp
index 04c9bb13161bf..10b20a91aee27 100644
--- a/clang/unittests/Driver/ToolChainTest.cpp
+++ b/clang/unittests/Driver/ToolChainTest.cpp
@@ -450,204 +450,4 @@ TEST(ToolChainTest, ConfigFileSearch) {
   }
 }
 
-struct FileSystemWithError : public llvm::vfs::FileSystem {
-  llvm::ErrorOr<llvm::vfs::Status> status(const Twine &Path) override {
-    return std::make_error_code(std::errc::no_such_file_or_directory);
-  }
-  llvm::ErrorOr<std::unique_ptr<llvm::vfs::File>>
-  openFileForRead(const Twine &Path) override {
-    return std::make_error_code(std::errc::permission_denied);
-  }
-  llvm::vfs::directory_iterator dir_begin(const Twine &Dir,
-                                          std::error_code &EC) override {
-    return llvm::vfs::directory_iterator();
-  }
-  std::error_code setCurrentWorkingDirectory(const Twine &Path) override {
-    return std::make_error_code(std::errc::permission_denied);
-  }
-  llvm::ErrorOr<std::string> getCurrentWorkingDirectory() const override {
-    return std::make_error_code(std::errc::permission_denied);
-  }
-};
-
-TEST(ToolChainTest, ConfigFileError) {
-  IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts = new DiagnosticOptions();
-  IntrusiveRefCntPtr<DiagnosticIDs> DiagID(new DiagnosticIDs());
-  std::unique_ptr<SimpleDiagnosticConsumer> DiagConsumer(
-      new SimpleDiagnosticConsumer());
-  DiagnosticsEngine Diags(DiagID, &*DiagOpts, DiagConsumer.get(), false);
-  IntrusiveRefCntPtr<llvm::vfs::FileSystem> FS(new FileSystemWithError);
-
-  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", "--no-default-config",
-                                  "--config", "./root.cfg", "--version"}));
-  ASSERT_TRUE(C);
-  ASSERT_TRUE(C->containsError());
-  EXPECT_EQ(1U, Diags.getNumErrors());
-  EXPECT_STREQ("configuration file './root.cfg' cannot be opened: cannot get "
-               "absolute path",
-               DiagConsumer->Errors[0].c_str());
-}
-
-TEST(ToolChainTest, BadConfigFile) {
-  IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts = new DiagnosticOptions();
-  IntrusiveRefCntPtr<DiagnosticIDs> DiagID(new DiagnosticIDs());
-  std::unique_ptr<SimpleDiagnosticConsumer> DiagConsumer(
-      new SimpleDiagnosticConsumer());
-  DiagnosticsEngine Diags(DiagID, &*DiagOpts, DiagConsumer.get(), false);
-  IntrusiveRefCntPtr<llvm::vfs::InMemoryFileSystem> FS(
-      new llvm::vfs::InMemoryFileSystem);
-
-#ifdef _WIN32
-  const char *TestRoot = "C:\\";
-#define FILENAME "C:/opt/root.cfg"
-#define DIRNAME "C:/opt"
-#else
-  const char *TestRoot = "/";
-#define FILENAME "/opt/root.cfg"
-#define DIRNAME "/opt"
-#endif
-  // UTF-16 string must be aligned on 2-byte boundary. Strings and char arrays
-  // do not provide necessary alignment, so copy constant string into properly
-  // allocated memory in heap.
-  llvm::BumpPtrAllocator Alloc;
-  char *StrBuff = (char *)Alloc.Allocate(6, 2);
-  std::memcpy(StrBuff, "\xFF\xFE\x00\xD8\x00\x00", 6);
-  StringRef BadUTF(StrBuff, 6);
-  FS->setCurrentWorkingDirectory(TestRoot);
-  FS->addFile("/opt/root.cfg", 0, llvm::MemoryBuffer::getMemBuffer(BadUTF));
-  FS->addFile("/home/user/test.cfg", 0,
-              llvm::MemoryBuffer::getMemBuffer("@file.rsp"));
-
-  {
-    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", "/opt/root.cfg", "--version"}));
-    ASSERT_TRUE(C);
-    ASSERT_TRUE(C->containsError());
-    EXPECT_EQ(1U, DiagConsumer->Errors.size());
-    EXPECT_STREQ("cannot read configuration file '" FILENAME
-                 "': Could not convert UTF16 to UTF8",
-                 DiagConsumer->Errors[0].c_str());
-  }
-  DiagConsumer->clear();
-  {
-    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", "/opt", "--version"}));
-    ASSERT_TRUE(C);
-    ASSERT_TRUE(C->containsError());
-    EXPECT_EQ(1U, DiagConsumer->Errors.size());
-    EXPECT_STREQ("configuration file '" DIRNAME
-                 "' cannot be opened: not a regular file",
-                 DiagConsumer->Errors[0].c_str());
-  }
-  DiagConsumer->clear();
-  {
-    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",
-         "--config-system-dir=", "--config-user-dir=", "--version"}));
-    ASSERT_TRUE(C);
-    ASSERT_TRUE(C->containsError());
-    EXPECT_EQ(1U, DiagConsumer->Errors.size());
-    EXPECT_STREQ("configuration file 'root' cannot be found",
-                 DiagConsumer->Errors[0].c_str());
-  }
-
-#undef FILENAME
-#undef DIRNAME
-}
-
-TEST(ToolChainTest, ConfigInexistentInclude) {
-  IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts = new DiagnosticOptions();
-  IntrusiveRefCntPtr<DiagnosticIDs> DiagID(new DiagnosticIDs());
-  std::unique_ptr<SimpleDiagnosticConsumer> DiagConsumer(
-      new SimpleDiagnosticConsumer());
-  DiagnosticsEngine Diags(DiagID, &*DiagOpts, DiagConsumer.get(), false);
-  IntrusiveRefCntPtr<llvm::vfs::InMemoryFileSystem> FS(
-      new llvm::vfs::InMemoryFileSystem);
-
-#ifdef _WIN32
-  const char *TestRoot = "C:\\";
-#define USERCONFIG "C:\\home\\user\\test.cfg"
-#define UNEXISTENT "C:\\home\\user\\file.rsp"
-#else
-  const char *TestRoot = "/";
-#define USERCONFIG "/home/user/test.cfg"
-#define UNEXISTENT "/home/user/file.rsp"
-#endif
-  FS->setCurrentWorkingDirectory(TestRoot);
-  FS->addFile("/home/user/test.cfg", 0,
-              llvm::MemoryBuffer::getMemBuffer("@file.rsp"));
-
-  {
-    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", "test.cfg",
-         "--config-system-dir=", "--config-user-dir=/home/user", "--version"}));
-    ASSERT_TRUE(C);
-    ASSERT_TRUE(C->containsError());
-    EXPECT_EQ(1U, DiagConsumer->Errors.size());
-    EXPECT_STREQ("cannot read configuration file '" USERCONFIG
-                 "': cannot not open file '" UNEXISTENT "'",
-                 DiagConsumer->Errors[0].c_str());
-  }
-
-#undef USERCONFIG
-#undef UNEXISTENT
-}
-
-TEST(ToolChainTest, ConfigRecursiveInclude) {
-  IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts = new DiagnosticOptions();
-  IntrusiveRefCntPtr<DiagnosticIDs> DiagID(new DiagnosticIDs());
-  std::unique_ptr<SimpleDiagnosticConsumer> DiagConsumer(
-      new SimpleDiagnosticConsumer());
-  DiagnosticsEngine Diags(DiagID, &*DiagOpts, DiagConsumer.get(), false);
-  IntrusiveRefCntPtr<llvm::vfs::InMemoryFileSystem> FS(
-      new llvm::vfs::InMemoryFileSystem);
-
-#ifdef _WIN32
-  const char *TestRoot = "C:\\";
-#define USERCONFIG "C:\\home\\user\\test.cfg"
-#define INCLUDED1 "C:\\home\\user\\file1.cfg"
-#else
-  const char *TestRoot = "/";
-#define USERCONFIG "/home/user/test.cfg"
-#define INCLUDED1 "/home/user/file1.cfg"
-#endif
-  FS->setCurrentWorkingDirectory(TestRoot);
-  FS->addFile("/home/user/test.cfg", 0,
-              llvm::MemoryBuffer::getMemBuffer("@file1.cfg"));
-  FS->addFile("/home/user/file1.cfg", 0,
-              llvm::MemoryBuffer::getMemBuffer("@file2.cfg"));
-  FS->addFile("/home/user/file2.cfg", 0,
-              llvm::MemoryBuffer::getMemBuffer("@file3.cfg"));
-  FS->addFile("/home/user/file3.cfg", 0,
-              llvm::MemoryBuffer::getMemBuffer("@file1.cfg"));
-
-  {
-    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", "test.cfg",
-         "--config-system-dir=", "--config-user-dir=/home/user", "--version"}));
-    ASSERT_TRUE(C);
-    ASSERT_TRUE(C->containsError());
-    EXPECT_EQ(1U, DiagConsumer->Errors.size());
-    EXPECT_STREQ("cannot read configuration file '" USERCONFIG
-                 "': recursive expansion of: '" INCLUDED1 "'",
-                 DiagConsumer->Errors[0].c_str());
-  }
-
-#undef USERCONFIG
-#undef INCLUDED1
-}
-
 } // end anonymous namespace.

diff  --git a/flang/tools/flang-driver/driver.cpp b/flang/tools/flang-driver/driver.cpp
index 28a8db2584b5c..3ab6fc6205011 100644
--- a/flang/tools/flang-driver/driver.cpp
+++ b/flang/tools/flang-driver/driver.cpp
@@ -77,9 +77,7 @@ static void ExpandResponseFiles(
   // We're defaulting to the GNU syntax, since we don't have a CL mode.
   llvm::cl::TokenizerCallback tokenizer = &llvm::cl::TokenizeGNUCommandLine;
   llvm::cl::ExpansionContext ExpCtx(saver.getAllocator(), tokenizer);
-  if (llvm::Error Err = ExpCtx.expandResponseFiles(args)) {
-    llvm::errs() << toString(std::move(Err)) << '\n';
-  }
+  ExpCtx.expandResponseFiles(args);
 }
 
 int main(int argc, const char **argv) {

diff  --git a/llvm/include/llvm/Support/CommandLine.h b/llvm/include/llvm/Support/CommandLine.h
index e68addcaded09..f30d844dc3dd3 100644
--- a/llvm/include/llvm/Support/CommandLine.h
+++ b/llvm/include/llvm/Support/CommandLine.h
@@ -2206,10 +2206,10 @@ class ExpansionContext {
   /// 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.
-  Error readConfigFile(StringRef CfgFile, SmallVectorImpl<const char *> &Argv);
+  bool readConfigFile(StringRef CfgFile, SmallVectorImpl<const char *> &Argv);
 
   /// Expands constructs "@file" in the provided array of arguments recursively.
-  Error expandResponseFiles(SmallVectorImpl<const char *> &Argv);
+  bool expandResponseFiles(SmallVectorImpl<const char *> &Argv);
 };
 
 /// A convenience helper which concatenates the options specified by the
@@ -2221,8 +2221,11 @@ bool expandResponseFiles(int Argc, const char *const *Argv, const char *EnvVar,
 
 /// A convenience helper which supports the typical use case of expansion
 /// function call.
-bool ExpandResponseFiles(StringSaver &Saver, TokenizerCallback Tokenizer,
-                         SmallVectorImpl<const char *> &Argv);
+inline bool ExpandResponseFiles(StringSaver &Saver, TokenizerCallback Tokenizer,
+                                SmallVectorImpl<const char *> &Argv) {
+  ExpansionContext ECtx(Saver.getAllocator(), Tokenizer);
+  return ECtx.expandResponseFiles(Argv);
+}
 
 /// A convenience helper which concatenates the options specified by the
 /// environment variable EnvVar and command line options, then expands response

diff  --git a/llvm/lib/Support/CommandLine.cpp b/llvm/lib/Support/CommandLine.cpp
index 136b813b1f6c8..3986c9103754d 100644
--- a/llvm/lib/Support/CommandLine.cpp
+++ b/llvm/lib/Support/CommandLine.cpp
@@ -1153,14 +1153,14 @@ static void ExpandBasePaths(StringRef BasePath, StringSaver &Saver,
 }
 
 // FName must be an absolute path.
-Error ExpansionContext::expandResponseFile(
-    StringRef FName, SmallVectorImpl<const char *> &NewArgv) {
+llvm::Error
+ExpansionContext::expandResponseFile(StringRef FName,
+                                     SmallVectorImpl<const char *> &NewArgv) {
   assert(sys::path::is_absolute(FName));
   llvm::ErrorOr<std::unique_ptr<MemoryBuffer>> MemBufOrErr =
       FS->getBufferForFile(FName);
   if (!MemBufOrErr)
-    return llvm::createStringError(
-        MemBufOrErr.getError(), Twine("cannot not open file '") + FName + "'");
+    return llvm::errorCodeToError(MemBufOrErr.getError());
   MemoryBuffer &MemBuf = *MemBufOrErr.get();
   StringRef Str(MemBuf.getBufferStart(), MemBuf.getBufferSize());
 
@@ -1182,30 +1182,29 @@ Error ExpansionContext::expandResponseFile(
   // Tokenize the contents into NewArgv.
   Tokenizer(Str, Saver, NewArgv, MarkEOLs);
 
-  // Expanded file content may require additional transformations, like using
-  // absolute paths instead of relative in '@file' constructs or expanding
-  // macros.
-  if (!RelativeNames && !InConfigFile)
+  if (!RelativeNames)
     return Error::success();
-
-  StringRef BasePath = llvm::sys::path::parent_path(FName);
-  for (auto I = NewArgv.begin(), E = NewArgv.end(); I != E; ++I) {
-    const char *&Arg = *I;
-    if (Arg == nullptr)
+  llvm::StringRef BasePath = llvm::sys::path::parent_path(FName);
+  // 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.
+  for (auto &Arg : NewArgv) {
+    if (!Arg)
       continue;
 
     // Substitute <CFGDIR> with the file's base path.
     if (InConfigFile)
       ExpandBasePaths(BasePath, Saver, Arg);
 
-    // Get expanded file name.
-    StringRef FileName(Arg);
-    if (!FileName.consume_front("@"))
+    // Skip non-rsp file arguments.
+    if (Arg[0] != '@')
       continue;
+
+    StringRef FileName(Arg + 1);
+    // Skip if non-relative.
     if (!llvm::sys::path::is_relative(FileName))
       continue;
 
-    // Update expansion construct.
     SmallString<128> ResponseFile;
     ResponseFile.push_back('@');
     ResponseFile.append(BasePath);
@@ -1217,8 +1216,9 @@ Error ExpansionContext::expandResponseFile(
 
 /// Expand response files on a command line recursively using the given
 /// StringSaver and tokenization strategy.
-Error ExpansionContext::expandResponseFiles(
+bool ExpansionContext::expandResponseFiles(
     SmallVectorImpl<const char *> &Argv) {
+  bool AllExpanded = true;
   struct ResponseFileRecord {
     std::string File;
     size_t End;
@@ -1262,8 +1262,9 @@ Error ExpansionContext::expandResponseFiles(
         if (auto CWD = FS->getCurrentWorkingDirectory()) {
           CurrDir = *CWD;
         } else {
-          return make_error<StringError>(
-              CWD.getError(), Twine("cannot get absolute path for: ") + FName);
+          // TODO: The error should be propagated up the stack.
+          llvm::consumeError(llvm::errorCodeToError(CWD.getError()));
+          return false;
         }
       } else {
         CurrDir = CurrentDir;
@@ -1271,51 +1272,43 @@ Error ExpansionContext::expandResponseFiles(
       llvm::sys::path::append(CurrDir, FName);
       FName = CurrDir.c_str();
     }
-    auto IsEquivalent =
-        [FName, this](const ResponseFileRecord &RFile) -> ErrorOr<bool> {
-      ErrorOr<llvm::vfs::Status> LHS = FS->status(FName);
-      if (!LHS)
-        return LHS.getError();
-      ErrorOr<llvm::vfs::Status> RHS = FS->status(RFile.File);
-      if (!RHS)
-        return RHS.getError();
+    auto IsEquivalent = [FName, this](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.
-    for (const auto &F : drop_begin(FileStack)) {
-      if (ErrorOr<bool> R = IsEquivalent(F)) {
-        if (R.get())
-          return make_error<StringError>(
-              Twine("recursive expansion of: '") + F.File + "'", R.getError());
-      } else {
-        return make_error<StringError>(Twine("cannot open file: ") + F.File,
-                                       R.getError());
-      }
+    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;
     }
 
     // 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 (!InConfigFile) {
-      // If the specified file does not exist, leave '@file' unexpanded, as
-      // libiberty does.
-      ErrorOr<llvm::vfs::Status> Res = FS->status(FName);
-      if (!Res) {
-        std::error_code EC = Res.getError();
-        if (EC == llvm::errc::no_such_file_or_directory) {
-          ++I;
-          continue;
-        }
-      } else {
-        if (!Res->exists()) {
-          ++I;
-          continue;
-        }
-      }
+    if (llvm::Error Err = expandResponseFile(FName, ExpandedArgv)) {
+      // 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;
     }
-    if (Error Err = expandResponseFile(FName, ExpandedArgv))
-      return Err;
 
     for (ResponseFileRecord &Record : FileStack) {
       // Increase the end of all active records by the number of newly expanded
@@ -1334,7 +1327,7 @@ Error ExpansionContext::expandResponseFiles(
   // 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 Error::success();
+  return AllExpanded;
 }
 
 bool cl::expandResponseFiles(int Argc, const char *const *Argv,
@@ -1351,21 +1344,7 @@ bool cl::expandResponseFiles(int Argc, const char *const *Argv,
   // Command line options can override the environment variable.
   NewArgv.append(Argv + 1, Argv + Argc);
   ExpansionContext ECtx(Saver.getAllocator(), Tokenize);
-  if (Error Err = ECtx.expandResponseFiles(NewArgv)) {
-    errs() << toString(std::move(Err)) << '\n';
-    return false;
-  }
-  return true;
-}
-
-bool cl::ExpandResponseFiles(StringSaver &Saver, TokenizerCallback Tokenizer,
-                             SmallVectorImpl<const char *> &Argv) {
-  ExpansionContext ECtx(Saver.getAllocator(), Tokenizer);
-  if (Error Err = ECtx.expandResponseFiles(Argv)) {
-    errs() << toString(std::move(Err)) << '\n';
-    return false;
-  }
-  return true;
+  return ECtx.expandResponseFiles(NewArgv);
 }
 
 ExpansionContext::ExpansionContext(BumpPtrAllocator &A, TokenizerCallback T)
@@ -1408,20 +1387,22 @@ bool ExpansionContext::findConfigFile(StringRef FileName,
   return false;
 }
 
-Error ExpansionContext::readConfigFile(StringRef CfgFile,
-                                       SmallVectorImpl<const char *> &Argv) {
+bool ExpansionContext::readConfigFile(StringRef CfgFile,
+                                      SmallVectorImpl<const char *> &Argv) {
   SmallString<128> AbsPath;
   if (sys::path::is_relative(CfgFile)) {
     AbsPath.assign(CfgFile);
     if (std::error_code EC = FS->makeAbsolute(AbsPath))
-      return make_error<StringError>(
-          EC, Twine("cannot get absolute path for " + CfgFile));
+      return false;
     CfgFile = AbsPath.str();
   }
   InConfigFile = true;
   RelativeNames = true;
-  if (Error Err = expandResponseFile(CfgFile, Argv))
-    return Err;
+  if (llvm::Error Err = expandResponseFile(CfgFile, Argv)) {
+    // TODO: The error should be propagated up the stack.
+    llvm::consumeError(std::move(Err));
+    return false;
+  }
   return expandResponseFiles(Argv);
 }
 
@@ -1477,28 +1458,25 @@ bool CommandLineParser::ParseCommandLineOptions(int argc,
                                                 bool LongOptionsUseDoubleDash) {
   assert(hasOptions() && "No options specified!");
 
-  ProgramOverview = Overview;
-  bool IgnoreErrors = Errs;
-  if (!Errs)
-    Errs = &errs();
-  bool ErrorParsing = false;
-
   // Expand response files.
   SmallVector<const char *, 20> newArgv(argv, argv + argc);
   BumpPtrAllocator A;
   ExpansionContext ECtx(A, Triple(sys::getProcessTriple()).isOSWindows()
                                ? cl::TokenizeWindowsCommandLine
                                : cl::TokenizeGNUCommandLine);
-  if (Error Err = ECtx.expandResponseFiles(newArgv)) {
-    *Errs << toString(std::move(Err)) << '\n';
-    return false;
-  }
+  ECtx.expandResponseFiles(newArgv);
   argv = &newArgv[0];
   argc = static_cast<int>(newArgv.size());
 
   // Copy the program name into ProgName, making sure not to overflow it.
   ProgramName = std::string(sys::path::filename(StringRef(argv[0])));
 
+  ProgramOverview = Overview;
+  bool IgnoreErrors = Errs;
+  if (!Errs)
+    Errs = &errs();
+  bool ErrorParsing = false;
+
   // Check out the positional arguments to collect information about them.
   unsigned NumPositionalRequired = 0;
 

diff  --git a/llvm/unittests/Support/CommandLineTest.cpp b/llvm/unittests/Support/CommandLineTest.cpp
index 26e82d13e799c..dd6e12223dacb 100644
--- a/llvm/unittests/Support/CommandLineTest.cpp
+++ b/llvm/unittests/Support/CommandLineTest.cpp
@@ -871,7 +871,7 @@ TEST(CommandLineTest, ResponseFiles) {
   llvm::BumpPtrAllocator A;
   llvm::cl::ExpansionContext ECtx(A, llvm::cl::TokenizeGNUCommandLine);
   ECtx.setVFS(&FS).setCurrentDir(TestRoot).setRelativeNames(true);
-  ASSERT_FALSE((bool)ECtx.expandResponseFiles(Argv));
+  ASSERT_TRUE(ECtx.expandResponseFiles(Argv));
   EXPECT_THAT(Argv, testing::Pointwise(
                         StringEquality(),
                         {"test/test", "-flag_1", "-option_1", "-option_2",
@@ -933,14 +933,7 @@ TEST(CommandLineTest, RecursiveResponseFiles) {
 #endif
   llvm::cl::ExpansionContext ECtx(A, Tokenizer);
   ECtx.setVFS(&FS).setCurrentDir(TestRoot);
-  llvm::Error Err = ECtx.expandResponseFiles(Argv);
-  ASSERT_TRUE((bool)Err);
-  SmallString<128> FilePath = SelfFilePath;
-  std::error_code EC = FS.makeAbsolute(FilePath);
-  ASSERT_FALSE((bool)EC);
-  std::string ExpectedMessage =
-      std::string("recursive expansion of: '") + std::string(FilePath) + "'";
-  ASSERT_TRUE(toString(std::move(Err)) == ExpectedMessage);
+  ASSERT_FALSE(ECtx.expandResponseFiles(Argv));
 
   EXPECT_THAT(Argv,
               testing::Pointwise(StringEquality(),
@@ -978,7 +971,7 @@ TEST(CommandLineTest, ResponseFilesAtArguments) {
   BumpPtrAllocator A;
   llvm::cl::ExpansionContext ECtx(A, cl::TokenizeGNUCommandLine);
   ECtx.setVFS(&FS).setCurrentDir(TestRoot);
-  ASSERT_FALSE((bool)ECtx.expandResponseFiles(Argv));
+  ASSERT_FALSE(ECtx.expandResponseFiles(Argv));
 
   // ASSERT instead of EXPECT to prevent potential out-of-bounds access.
   ASSERT_EQ(Argv.size(), 1 + NON_RSP_AT_ARGS + 2);
@@ -1012,7 +1005,7 @@ TEST(CommandLineTest, ResponseFileRelativePath) {
   BumpPtrAllocator A;
   llvm::cl::ExpansionContext ECtx(A, cl::TokenizeGNUCommandLine);
   ECtx.setVFS(&FS).setCurrentDir(TestRoot).setRelativeNames(true);
-  ASSERT_FALSE((bool)ECtx.expandResponseFiles(Argv));
+  ASSERT_TRUE(ECtx.expandResponseFiles(Argv));
   EXPECT_THAT(Argv,
               testing::Pointwise(StringEquality(), {"test/test", "-flag"}));
 }
@@ -1032,7 +1025,7 @@ TEST(CommandLineTest, ResponseFileEOLs) {
   llvm::cl::ExpansionContext ECtx(A, cl::TokenizeWindowsCommandLine);
   ECtx.setVFS(&FS).setCurrentDir(TestRoot).setMarkEOLs(true).setRelativeNames(
       true);
-  ASSERT_FALSE((bool)ECtx.expandResponseFiles(Argv));
+  ASSERT_TRUE(ECtx.expandResponseFiles(Argv));
   const char *Expected[] = {"clang", "-Xclang", "-Wno-whatever", nullptr,
                             "input.cpp"};
   ASSERT_EQ(std::size(Expected), Argv.size());
@@ -1045,30 +1038,6 @@ TEST(CommandLineTest, ResponseFileEOLs) {
   }
 }
 
-TEST(CommandLineTest, BadResponseFile) {
-  BumpPtrAllocator A;
-  StringSaver Saver(A);
-  TempDir ADir("dir", /*Unique*/ true);
-  SmallString<128> AFilePath = ADir.path();
-  llvm::sys::path::append(AFilePath, "file.rsp");
-  std::string AFileExp = std::string("@") + std::string(AFilePath.str());
-  SmallVector<const char *, 2> Argv = {"clang", AFileExp.c_str()};
-
-  bool Res = cl::ExpandResponseFiles(Saver, cl::TokenizeGNUCommandLine, Argv);
-  ASSERT_TRUE(Res);
-  ASSERT_EQ(2U, Argv.size());
-  ASSERT_STREQ(Argv[0], "clang");
-  ASSERT_STREQ(Argv[1], AFileExp.c_str());
-
-  std::string ADirExp = std::string("@") + std::string(ADir.path());
-  Argv = {"clang", ADirExp.c_str()};
-  Res = cl::ExpandResponseFiles(Saver, cl::TokenizeGNUCommandLine, Argv);
-  ASSERT_FALSE(Res);
-  ASSERT_EQ(2U, Argv.size());
-  ASSERT_STREQ(Argv[0], "clang");
-  ASSERT_STREQ(Argv[1], ADirExp.c_str());
-}
-
 TEST(CommandLineTest, SetDefaultValue) {
   cl::ResetCommandLineParser();
 
@@ -1176,9 +1145,9 @@ TEST(CommandLineTest, ReadConfigFile) {
 
   llvm::BumpPtrAllocator A;
   llvm::cl::ExpansionContext ECtx(A, cl::tokenizeConfigFile);
-  llvm::Error Result = ECtx.readConfigFile(ConfigFile.path(), Argv);
+  bool Result = ECtx.readConfigFile(ConfigFile.path(), Argv);
 
-  EXPECT_FALSE((bool)Result);
+  EXPECT_TRUE(Result);
   EXPECT_EQ(Argv.size(), 13U);
   EXPECT_STREQ(Argv[0], "-option_1");
   EXPECT_STREQ(Argv[1],


        


More information about the cfe-commits mailing list