diff --git a/clang/include/clang/Basic/DiagnosticDriverKinds.td b/clang/include/clang/Basic/DiagnosticDriverKinds.td --- a/clang/include/clang/Basic/DiagnosticDriverKinds.td +++ b/clang/include/clang/Basic/DiagnosticDriverKinds.td @@ -214,14 +214,14 @@ "malformed sanitizer coverage ignorelist: '%0'">; def err_drv_duplicate_config : Error< "no more than one option '--config' is allowed">; -def err_drv_config_file_not_exist : Error< - "configuration file '%0' does not exist">; +def err_drv_cannot_open_config_file : Error< + "configuration file '%0' cannot be opened: %1">; 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'">; + "cannot read configuration file '%0': %1">; 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 --- a/clang/lib/Driver/Driver.cpp +++ b/clang/lib/Driver/Driver.cpp @@ -942,8 +942,9 @@ llvm::cl::ExpansionContext &ExpCtx) { // Try reading the given file. SmallVector NewCfgArgs; - if (!ExpCtx.readConfigFile(FileName, NewCfgArgs)) { - Diag(diag::err_drv_cannot_read_config_file) << FileName; + if (llvm::Error Err = ExpCtx.readConfigFile(FileName, NewCfgArgs)) { + Diag(diag::err_drv_cannot_read_config_file) + << FileName << toString(std::move(Err)); return true; } @@ -1025,15 +1026,23 @@ if (llvm::sys::path::has_parent_path(CfgFileName)) { CfgFilePath.assign(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; + if (std::error_code EC = getVFS().makeAbsolute(CfgFilePath)) { + Diag(diag::err_drv_cannot_open_config_file) + << CfgFilePath << "cannot get absolute path"; return true; } } + auto Status = getVFS().status(CfgFilePath); + if (!Status) { + Diag(diag::err_drv_cannot_open_config_file) + << CfgFilePath << Status.getError().message(); + return true; + } + if (Status->getType() != llvm::sys::fs::file_type::regular_file) { + Diag(diag::err_drv_cannot_open_config_file) + << CfgFilePath << "not a regular file"; + return true; + } } else if (!ExpCtx.findConfigFile(CfgFileName, CfgFilePath)) { // Report an error that the config file could not be found. Diag(diag::err_drv_config_file_not_found) << CfgFileName; diff --git a/clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp b/clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp --- a/clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp +++ b/clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp @@ -61,9 +61,12 @@ continue; llvm::BumpPtrAllocator Alloc; llvm::cl::ExpansionContext ECtx(Alloc, Tokenizer); - ECtx.setVFS(FS.get()) - .setCurrentDir(Cmd.Directory) - .expandResponseFiles(Argv); + llvm::Error Err = ECtx.setVFS(FS.get()) + .setCurrentDir(Cmd.Directory) + .expandResponseFiles(Argv); + if (Err) { + llvm::errs() << Err; + } // Don't assign directly, Argv aliases CommandLine. std::vector 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 --- 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' does not exist +// CHECK-NONEXISTENT: configuration file '{{.*}}somewhere/nonexistent-config-file' cannot be opened: No such file or directory //--- All '--config' arguments must be existing files. diff --git a/clang/tools/driver/driver.cpp b/clang/tools/driver/driver.cpp --- a/clang/tools/driver/driver.cpp +++ b/clang/tools/driver/driver.cpp @@ -309,7 +309,10 @@ llvm::BumpPtrAllocator A; llvm::cl::ExpansionContext ECtx(A, llvm::cl::TokenizeGNUCommandLine); - ECtx.expandResponseFiles(ArgV); + if (llvm::Error Err = ECtx.expandResponseFiles(ArgV)) { + llvm::errs() << toString(std::move(Err)) << '\n'; + return 1; + } StringRef Tool = ArgV[1]; void *GetExecutablePathVP = (void *)(intptr_t)GetExecutablePath; if (Tool == "-cc1") @@ -373,7 +376,11 @@ if (MarkEOLs && Args.size() > 1 && StringRef(Args[1]).startswith("-cc1")) MarkEOLs = false; llvm::cl::ExpansionContext ECtx(A, Tokenizer); - ECtx.setMarkEOLs(MarkEOLs).expandResponseFiles(Args); + ECtx.setMarkEOLs(MarkEOLs); + if (llvm::Error Err = ECtx.expandResponseFiles(Args)) { + llvm::errs() << Err << '\n'; + return 1; + } // 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 --- a/clang/unittests/Driver/ToolChainTest.cpp +++ b/clang/unittests/Driver/ToolChainTest.cpp @@ -541,4 +541,199 @@ } } +struct FileSystemWithError : public llvm::vfs::FileSystem { + llvm::ErrorOr status(const Twine &Path) override { + return std::make_error_code(std::errc::no_such_file_or_directory); + } + llvm::ErrorOr> + 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 getCurrentWorkingDirectory() const override { + return std::make_error_code(std::errc::permission_denied); + } +}; + +TEST(ToolChainTest, ConfigFileError) { + IntrusiveRefCntPtr DiagOpts = new DiagnosticOptions(); + IntrusiveRefCntPtr DiagID(new DiagnosticIDs()); + std::unique_ptr DiagConsumer( + new SimpleDiagnosticConsumer()); + DiagnosticsEngine Diags(DiagID, &*DiagOpts, DiagConsumer.get(), false); + IntrusiveRefCntPtr FS(new FileSystemWithError); + + Driver TheDriver("/home/test/bin/clang", "arm-linux-gnueabi", Diags, + "clang LLVM compiler", FS); + std::unique_ptr 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 DiagOpts = new DiagnosticOptions(); + IntrusiveRefCntPtr DiagID(new DiagnosticIDs()); + std::unique_ptr DiagConsumer( + new SimpleDiagnosticConsumer()); + DiagnosticsEngine Diags(DiagID, &*DiagOpts, DiagConsumer.get(), false); + IntrusiveRefCntPtr 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 + FS->setCurrentWorkingDirectory(TestRoot); + FS->addFile("/opt/root.cfg", 0, + llvm::MemoryBuffer::getMemBuffer( + StringRef("\xFF\xFE\x00\xD8\x00\x00", 6))); + 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 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 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 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 DiagOpts = new DiagnosticOptions(); + IntrusiveRefCntPtr DiagID(new DiagnosticIDs()); + std::unique_ptr DiagConsumer( + new SimpleDiagnosticConsumer()); + DiagnosticsEngine Diags(DiagID, &*DiagOpts, DiagConsumer.get(), false); + IntrusiveRefCntPtr 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 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 DiagOpts = new DiagnosticOptions(); + IntrusiveRefCntPtr DiagID(new DiagnosticIDs()); + std::unique_ptr DiagConsumer( + new SimpleDiagnosticConsumer()); + DiagnosticsEngine Diags(DiagID, &*DiagOpts, DiagConsumer.get(), false); + IntrusiveRefCntPtr 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 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 --- a/flang/tools/flang-driver/driver.cpp +++ b/flang/tools/flang-driver/driver.cpp @@ -77,7 +77,9 @@ // 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); - ExpCtx.expandResponseFiles(args); + if (llvm::Error Err = ExpCtx.expandResponseFiles(args)) { + llvm::errs() << toString(std::move(Err)) << '\n'; + } } int main(int argc, const char **argv) { diff --git a/llvm/include/llvm/Support/CommandLine.h b/llvm/include/llvm/Support/CommandLine.h --- a/llvm/include/llvm/Support/CommandLine.h +++ b/llvm/include/llvm/Support/CommandLine.h @@ -2206,10 +2206,10 @@ /// commands resolving file names in them relative to the directory where /// CfgFilename resides. It also expands "" to the base path of the /// current config file. - bool readConfigFile(StringRef CfgFile, SmallVectorImpl &Argv); + Error readConfigFile(StringRef CfgFile, SmallVectorImpl &Argv); /// Expands constructs "@file" in the provided array of arguments recursively. - bool expandResponseFiles(SmallVectorImpl &Argv); + Error expandResponseFiles(SmallVectorImpl &Argv); }; /// A convenience helper which concatenates the options specified by the @@ -2221,11 +2221,8 @@ /// A convenience helper which supports the typical use case of expansion /// function call. -inline bool ExpandResponseFiles(StringSaver &Saver, TokenizerCallback Tokenizer, - SmallVectorImpl &Argv) { - ExpansionContext ECtx(Saver.getAllocator(), Tokenizer); - return ECtx.expandResponseFiles(Argv); -} +bool ExpandResponseFiles(StringSaver &Saver, TokenizerCallback Tokenizer, + SmallVectorImpl &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 --- a/llvm/lib/Support/CommandLine.cpp +++ b/llvm/lib/Support/CommandLine.cpp @@ -1153,14 +1153,14 @@ } // FName must be an absolute path. -llvm::Error -ExpansionContext::expandResponseFile(StringRef FName, - SmallVectorImpl &NewArgv) { +Error ExpansionContext::expandResponseFile( + StringRef FName, SmallVectorImpl &NewArgv) { assert(sys::path::is_absolute(FName)); llvm::ErrorOr> MemBufOrErr = FS->getBufferForFile(FName); if (!MemBufOrErr) - return llvm::errorCodeToError(MemBufOrErr.getError()); + return llvm::createStringError( + MemBufOrErr.getError(), Twine("cannot not open file '") + FName + "'"); MemoryBuffer &MemBuf = *MemBufOrErr.get(); StringRef Str(MemBuf.getBufferStart(), MemBuf.getBufferSize()); @@ -1182,29 +1182,34 @@ // Tokenize the contents into NewArgv. Tokenizer(Str, Saver, NewArgv, MarkEOLs); - if (!RelativeNames) + // Expanded file content may require additional transformations, like using + // absolute paths instead of relative in '@file' constructs or expanding + // macros. + if (!RelativeNames && !InConfigFile) return Error::success(); - 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) + + 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) continue; // Substitute with the file's base path. if (InConfigFile) ExpandBasePaths(BasePath, Saver, Arg); - // Skip non-rsp file arguments. - if (Arg[0] != '@') - continue; - - StringRef FileName(Arg + 1); - // Skip if non-relative. - if (!llvm::sys::path::is_relative(FileName)) + // Get expanded file name. + StringRef ArgStr(Arg); + StringRef FileName; + if (ArgStr[0] == '@') { + FileName = ArgStr.drop_front(1); + if (!llvm::sys::path::is_relative(FileName)) + continue; + } else { continue; + } + // Update expansion construct. SmallString<128> ResponseFile; ResponseFile.push_back('@'); ResponseFile.append(BasePath); @@ -1216,9 +1221,8 @@ /// Expand response files on a command line recursively using the given /// StringSaver and tokenization strategy. -bool ExpansionContext::expandResponseFiles( +Error ExpansionContext::expandResponseFiles( SmallVectorImpl &Argv) { - bool AllExpanded = true; struct ResponseFileRecord { std::string File; size_t End; @@ -1262,9 +1266,8 @@ 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; + return make_error( + CWD.getError(), Twine("cannot get absolute path for: ") + FName); } } else { CurrDir = CurrentDir; @@ -1272,43 +1275,51 @@ llvm::sys::path::append(CurrDir, FName); FName = CurrDir.c_str(); } - auto IsEquivalent = [FName, this](const ResponseFileRecord &RFile) { - llvm::ErrorOr 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 RHS = FS->status(RFile.File); - if (!RHS) { - // TODO: The error should be propagated up the stack. - llvm::consumeError(llvm::errorCodeToError(RHS.getError())); - return false; - } + auto IsEquivalent = + [FName, this](const ResponseFileRecord &RFile) -> ErrorOr { + ErrorOr LHS = FS->status(FName); + if (!LHS) + return LHS.getError(); + ErrorOr RHS = FS->status(RFile.File); + if (!RHS) + return RHS.getError(); return LHS->equivalent(*RHS); }; // 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; + for (const auto &F : drop_begin(FileStack)) { + if (ErrorOr R = IsEquivalent(F)) { + if (R.get()) + return make_error( + Twine("recursive expansion of: '") + F.File + "'", R.getError()); + } else { + return make_error(Twine("cannot open file: ") + F.File, + R.getError()); + } } // Replace this response file argument with the tokenization of its // contents. Nested response files are expanded in subsequent iterations. SmallVector ExpandedArgv; - 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 (!InConfigFile) { + // If the specified file does not exist, leave '@file' unexpanded, as + // libiberty does. + ErrorOr 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 (Error Err = expandResponseFile(FName, ExpandedArgv)) + return Err; for (ResponseFileRecord &Record : FileStack) { // Increase the end of all active records by the number of newly expanded @@ -1327,7 +1338,7 @@ // 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 Error::success(); } bool cl::expandResponseFiles(int Argc, const char *const *Argv, @@ -1344,7 +1355,21 @@ // Command line options can override the environment variable. NewArgv.append(Argv + 1, Argv + Argc); ExpansionContext ECtx(Saver.getAllocator(), Tokenize); - return ECtx.expandResponseFiles(NewArgv); + if (Error Err = ECtx.expandResponseFiles(NewArgv)) { + errs() << toString(std::move(Err)) << '\n'; + return false; + } + return true; +} + +bool cl::ExpandResponseFiles(StringSaver &Saver, TokenizerCallback Tokenizer, + SmallVectorImpl &Argv) { + ExpansionContext ECtx(Saver.getAllocator(), Tokenizer); + if (Error Err = ECtx.expandResponseFiles(Argv)) { + errs() << toString(std::move(Err)) << '\n'; + return false; + } + return true; } ExpansionContext::ExpansionContext(BumpPtrAllocator &A, TokenizerCallback T) @@ -1387,22 +1412,20 @@ return false; } -bool ExpansionContext::readConfigFile(StringRef CfgFile, - SmallVectorImpl &Argv) { +Error ExpansionContext::readConfigFile(StringRef CfgFile, + SmallVectorImpl &Argv) { SmallString<128> AbsPath; if (sys::path::is_relative(CfgFile)) { AbsPath.assign(CfgFile); if (std::error_code EC = FS->makeAbsolute(AbsPath)) - return false; + return make_error( + EC, Twine("cannot get absolute path for " + CfgFile)); CfgFile = AbsPath.str(); } InConfigFile = true; RelativeNames = true; - if (llvm::Error Err = expandResponseFile(CfgFile, Argv)) { - // TODO: The error should be propagated up the stack. - llvm::consumeError(std::move(Err)); - return false; - } + if (Error Err = expandResponseFile(CfgFile, Argv)) + return Err; return expandResponseFiles(Argv); } @@ -1458,25 +1481,28 @@ bool LongOptionsUseDoubleDash) { assert(hasOptions() && "No options specified!"); + ProgramOverview = Overview; + bool IgnoreErrors = Errs; + if (!Errs) + Errs = &errs(); + bool ErrorParsing = false; + // Expand response files. SmallVector newArgv(argv, argv + argc); BumpPtrAllocator A; ExpansionContext ECtx(A, Triple(sys::getProcessTriple()).isOSWindows() ? cl::TokenizeWindowsCommandLine : cl::TokenizeGNUCommandLine); - ECtx.expandResponseFiles(newArgv); + if (Error Err = ECtx.expandResponseFiles(newArgv)) { + *Errs << toString(std::move(Err)) << '\n'; + return false; + } argv = &newArgv[0]; argc = static_cast(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 --- a/llvm/unittests/Support/CommandLineTest.cpp +++ b/llvm/unittests/Support/CommandLineTest.cpp @@ -871,7 +871,7 @@ llvm::BumpPtrAllocator A; llvm::cl::ExpansionContext ECtx(A, llvm::cl::TokenizeGNUCommandLine); ECtx.setVFS(&FS).setCurrentDir(TestRoot).setRelativeNames(true); - ASSERT_TRUE(ECtx.expandResponseFiles(Argv)); + ASSERT_FALSE((bool)ECtx.expandResponseFiles(Argv)); EXPECT_THAT(Argv, testing::Pointwise( StringEquality(), {"test/test", "-flag_1", "-option_1", "-option_2", @@ -933,7 +933,14 @@ #endif llvm::cl::ExpansionContext ECtx(A, Tokenizer); ECtx.setVFS(&FS).setCurrentDir(TestRoot); - ASSERT_FALSE(ECtx.expandResponseFiles(Argv)); + 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); EXPECT_THAT(Argv, testing::Pointwise(StringEquality(), @@ -971,7 +978,7 @@ BumpPtrAllocator A; llvm::cl::ExpansionContext ECtx(A, cl::TokenizeGNUCommandLine); ECtx.setVFS(&FS).setCurrentDir(TestRoot); - ASSERT_FALSE(ECtx.expandResponseFiles(Argv)); + ASSERT_FALSE((bool)ECtx.expandResponseFiles(Argv)); // ASSERT instead of EXPECT to prevent potential out-of-bounds access. ASSERT_EQ(Argv.size(), 1 + NON_RSP_AT_ARGS + 2); @@ -1005,7 +1012,7 @@ BumpPtrAllocator A; llvm::cl::ExpansionContext ECtx(A, cl::TokenizeGNUCommandLine); ECtx.setVFS(&FS).setCurrentDir(TestRoot).setRelativeNames(true); - ASSERT_TRUE(ECtx.expandResponseFiles(Argv)); + ASSERT_FALSE((bool)ECtx.expandResponseFiles(Argv)); EXPECT_THAT(Argv, testing::Pointwise(StringEquality(), {"test/test", "-flag"})); } @@ -1025,7 +1032,7 @@ llvm::cl::ExpansionContext ECtx(A, cl::TokenizeWindowsCommandLine); ECtx.setVFS(&FS).setCurrentDir(TestRoot).setMarkEOLs(true).setRelativeNames( true); - ASSERT_TRUE(ECtx.expandResponseFiles(Argv)); + ASSERT_FALSE((bool)ECtx.expandResponseFiles(Argv)); const char *Expected[] = {"clang", "-Xclang", "-Wno-whatever", nullptr, "input.cpp"}; ASSERT_EQ(std::size(Expected), Argv.size()); @@ -1038,6 +1045,30 @@ } } +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 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(); @@ -1145,9 +1176,9 @@ llvm::BumpPtrAllocator A; llvm::cl::ExpansionContext ECtx(A, cl::tokenizeConfigFile); - bool Result = ECtx.readConfigFile(ConfigFile.path(), Argv); + llvm::Error Result = ECtx.readConfigFile(ConfigFile.path(), Argv); - EXPECT_TRUE(Result); + EXPECT_FALSE((bool)Result); EXPECT_EQ(Argv.size(), 13U); EXPECT_STREQ(Argv[0], "-option_1"); EXPECT_STREQ(Argv[1],