From fd3d7a9f8cbb5ad81fb96d92d5f7b1d51a4d4127 Mon Sep 17 00:00:00 2001 From: Serge Pavlov Date: Mon, 31 Oct 2022 12:59:15 +0700 Subject: [PATCH] Handle errors in expansion of response files Previously an error raised during an expansion of response files (including configuration files) was ignored and only the fact of its presence was reported to the user with generic error messages. This made it difficult to analyze problems. For example, if a configuration file tried to read an inexistent file, the error message said that 'configuration file cannot be found', which is wrong and misleading. This change enhances handling errors in the expansion so that users could get more informative error messages. Differential Revision: https://reviews.llvm.org/D136090 --- clang/include/clang/Basic/DiagnosticDriverKinds.td | 6 +- clang/lib/Driver/Driver.cpp | 27 ++- .../ExpandResponseFilesCompilationDatabase.cpp | 8 +- clang/test/Driver/config-file-errs.c | 2 +- clang/tools/driver/driver.cpp | 11 +- clang/unittests/Driver/ToolChainTest.cpp | 201 +++++++++++++++++++++ flang/tools/flang-driver/driver.cpp | 4 +- llvm/include/llvm/Support/CommandLine.h | 11 +- llvm/lib/Support/CommandLine.cpp | 152 +++++++++------- llvm/unittests/Support/CommandLineTest.cpp | 45 ++++- 10 files changed, 370 insertions(+), 97 deletions(-) diff --git a/clang/include/clang/Basic/DiagnosticDriverKinds.td b/clang/include/clang/Basic/DiagnosticDriverKinds.td index b09e124..e477d93 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_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 index 26729f1..80e6ec7 100644 --- a/clang/lib/Driver/Driver.cpp +++ b/clang/lib/Driver/Driver.cpp @@ -940,10 +940,24 @@ 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 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,12 +1039,9 @@ 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)) - 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 (getVFS().makeAbsolute(CfgFilePath)) { + Diag(diag::err_drv_cannot_open_config_file) + << CfgFilePath << "cannot get absolute path"; return true; } } diff --git a/clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp b/clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp index c4b3abc..88d20ba 100644 --- a/clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp +++ b/clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp @@ -61,9 +61,11 @@ private: 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 index 497e6cb..85b3443 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' does not exist +// CHECK-NONEXISTENT: configuration file '{{.*}}somewhere{{.}}nonexistent-config-file' cannot be opened: {{[Nn]}}o 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 index 11eba44..2cc3b48 100644 --- a/clang/tools/driver/driver.cpp +++ b/clang/tools/driver/driver.cpp @@ -309,7 +309,10 @@ static int ExecuteCC1Tool(SmallVectorImpl &ArgV) { 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 @@ 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).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 index 10b20a9..b143cd6 100644 --- a/clang/unittests/Driver/ToolChainTest.cpp +++ b/clang/unittests/Driver/ToolChainTest.cpp @@ -450,4 +450,205 @@ TEST(ToolChainTest, ConfigFileSearch) { } } +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 + // 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(16, 4); + std::memset(StrBuff, 0, 16); + 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 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 index 3ab6fc6..28a8db2 100644 --- a/flang/tools/flang-driver/driver.cpp +++ b/flang/tools/flang-driver/driver.cpp @@ -77,7 +77,9 @@ 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); - 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 index f30d844..e68addca 100644 --- a/llvm/include/llvm/Support/CommandLine.h +++ b/llvm/include/llvm/Support/CommandLine.h @@ -2206,10 +2206,10 @@ public: /// 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 @@ bool expandResponseFiles(int Argc, const char *const *Argv, const char *EnvVar, /// 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 index 3986c91..136b813 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. -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,30 @@ ExpansionContext::expandResponseFile(StringRef FName, // 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] != '@') + // Get expanded file name. + StringRef FileName(Arg); + if (!FileName.consume_front("@")) 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); @@ -1216,9 +1217,8 @@ ExpansionContext::expandResponseFile(StringRef FName, /// 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 +1262,8 @@ bool ExpansionContext::expandResponseFiles( 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 +1271,51 @@ bool ExpansionContext::expandResponseFiles( 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 +1334,7 @@ bool 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 AllExpanded; + return Error::success(); } bool cl::expandResponseFiles(int Argc, const char *const *Argv, @@ -1344,7 +1351,21 @@ 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); - 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 +1408,20 @@ bool ExpansionContext::findConfigFile(StringRef FileName, 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 +1477,28 @@ 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 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 index dd6e122..26e82d1 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_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 @@ TEST(CommandLineTest, RecursiveResponseFiles) { #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 @@ TEST(CommandLineTest, ResponseFilesAtArguments) { 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 @@ TEST(CommandLineTest, ResponseFileRelativePath) { 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 @@ TEST(CommandLineTest, ResponseFileEOLs) { 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, 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 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 @@ TEST(CommandLineTest, ReadConfigFile) { 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], -- 2.7.4