Skip to content

Commit fd3d7a9

Browse files
spavloffspavloff
committed
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
1 parent 1af81ab commit fd3d7a9

File tree

10 files changed

+370
-97
lines changed

10 files changed

+370
-97
lines changed

‎clang/include/clang/Basic/DiagnosticDriverKinds.td

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -214,14 +214,14 @@ def err_drv_malformed_sanitizer_coverage_ignorelist : Error<
214214
"malformed sanitizer coverage ignorelist: '%0'">;
215215
def err_drv_duplicate_config : Error<
216216
"no more than one option '--config' is allowed">;
217-
def err_drv_config_file_not_exist : Error<
218-
"configuration file '%0' does not exist">;
217+
def err_drv_cannot_open_config_file : Error<
218+
"configuration file '%0' cannot be opened: %1">;
219219
def err_drv_config_file_not_found : Error<
220220
"configuration file '%0' cannot be found">;
221221
def note_drv_config_file_searched_in : Note<
222222
"was searched for in the directory: %0">;
223223
def err_drv_cannot_read_config_file : Error<
224-
"cannot read configuration file '%0'">;
224+
"cannot read configuration file '%0': %1">;
225225
def err_drv_nested_config_file: Error<
226226
"option '--config' is not allowed inside configuration file">;
227227
def err_drv_arg_requires_bitcode_input: Error<

‎clang/lib/Driver/Driver.cpp

Lines changed: 19 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -940,10 +940,24 @@ static void appendOneArg(InputArgList &Args, const Arg *Opt,
940940

941941
bool Driver::readConfigFile(StringRef FileName,
942942
llvm::cl::ExpansionContext &ExpCtx) {
943+
// Try opening the given file.
944+
auto Status = getVFS().status(FileName);
945+
if (!Status) {
946+
Diag(diag::err_drv_cannot_open_config_file)
947+
<< FileName << Status.getError().message();
948+
return true;
949+
}
950+
if (Status->getType() != llvm::sys::fs::file_type::regular_file) {
951+
Diag(diag::err_drv_cannot_open_config_file)
952+
<< FileName << "not a regular file";
953+
return true;
954+
}
955+
943956
// Try reading the given file.
944957
SmallVector<const char *, 32> NewCfgArgs;
945-
if (!ExpCtx.readConfigFile(FileName, NewCfgArgs)) {
946-
Diag(diag::err_drv_cannot_read_config_file) << FileName;
958+
if (llvm::Error Err = ExpCtx.readConfigFile(FileName, NewCfgArgs)) {
959+
Diag(diag::err_drv_cannot_read_config_file)
960+
<< FileName << toString(std::move(Err));
947961
return true;
948962
}
949963

@@ -1025,12 +1039,9 @@ bool Driver::loadConfigFiles() {
10251039
if (llvm::sys::path::has_parent_path(CfgFileName)) {
10261040
CfgFilePath.assign(CfgFileName);
10271041
if (llvm::sys::path::is_relative(CfgFilePath)) {
1028-
if (getVFS().makeAbsolute(CfgFilePath))
1029-
return true;
1030-
auto Status = getVFS().status(CfgFilePath);
1031-
if (!Status ||
1032-
Status->getType() != llvm::sys::fs::file_type::regular_file) {
1033-
Diag(diag::err_drv_config_file_not_exist) << CfgFilePath;
1042+
if (getVFS().makeAbsolute(CfgFilePath)) {
1043+
Diag(diag::err_drv_cannot_open_config_file)
1044+
<< CfgFilePath << "cannot get absolute path";
10341045
return true;
10351046
}
10361047
}

‎clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -61,9 +61,11 @@ class ExpandResponseFilesDatabase : public CompilationDatabase {
6161
continue;
6262
llvm::BumpPtrAllocator Alloc;
6363
llvm::cl::ExpansionContext ECtx(Alloc, Tokenizer);
64-
ECtx.setVFS(FS.get())
65-
.setCurrentDir(Cmd.Directory)
66-
.expandResponseFiles(Argv);
64+
llvm::Error Err = ECtx.setVFS(FS.get())
65+
.setCurrentDir(Cmd.Directory)
66+
.expandResponseFiles(Argv);
67+
if (Err)
68+
llvm::errs() << Err;
6769
// Don't assign directly, Argv aliases CommandLine.
6870
std::vector<std::string> ExpandedArgv(Argv.begin(), Argv.end());
6971
Cmd.CommandLine = std::move(ExpandedArgv);

‎clang/test/Driver/config-file-errs.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@
1313
//--- Argument of '--config' must be existing file, if it is specified by path.
1414
//
1515
// RUN: not %clang --config somewhere/nonexistent-config-file 2>&1 | FileCheck %s -check-prefix CHECK-NONEXISTENT
16-
// CHECK-NONEXISTENT: configuration file '{{.*}}somewhere/nonexistent-config-file' does not exist
16+
// CHECK-NONEXISTENT: configuration file '{{.*}}somewhere{{.}}nonexistent-config-file' cannot be opened: {{[Nn]}}o such file or directory
1717

1818

1919
//--- All '--config' arguments must be existing files.

‎clang/tools/driver/driver.cpp

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -309,7 +309,10 @@ static int ExecuteCC1Tool(SmallVectorImpl<const char *> &ArgV) {
309309

310310
llvm::BumpPtrAllocator A;
311311
llvm::cl::ExpansionContext ECtx(A, llvm::cl::TokenizeGNUCommandLine);
312-
ECtx.expandResponseFiles(ArgV);
312+
if (llvm::Error Err = ECtx.expandResponseFiles(ArgV)) {
313+
llvm::errs() << toString(std::move(Err)) << '\n';
314+
return 1;
315+
}
313316
StringRef Tool = ArgV[1];
314317
void *GetExecutablePathVP = (void *)(intptr_t)GetExecutablePath;
315318
if (Tool == "-cc1")
@@ -373,7 +376,11 @@ int clang_main(int Argc, char **Argv) {
373376
if (MarkEOLs && Args.size() > 1 && StringRef(Args[1]).startswith("-cc1"))
374377
MarkEOLs = false;
375378
llvm::cl::ExpansionContext ECtx(A, Tokenizer);
376-
ECtx.setMarkEOLs(MarkEOLs).expandResponseFiles(Args);
379+
ECtx.setMarkEOLs(MarkEOLs);
380+
if (llvm::Error Err = ECtx.expandResponseFiles(Args)) {
381+
llvm::errs() << Err << '\n';
382+
return 1;
383+
}
377384

378385
// Handle -cc1 integrated tools, even if -cc1 was expanded from a response
379386
// file.

‎clang/unittests/Driver/ToolChainTest.cpp

Lines changed: 201 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -450,4 +450,205 @@ TEST(ToolChainTest, ConfigFileSearch) {
450450
}
451451
}
452452

453+
struct FileSystemWithError : public llvm::vfs::FileSystem {
454+
llvm::ErrorOr<llvm::vfs::Status> status(const Twine &Path) override {
455+
return std::make_error_code(std::errc::no_such_file_or_directory);
456+
}
457+
llvm::ErrorOr<std::unique_ptr<llvm::vfs::File>>
458+
openFileForRead(const Twine &Path) override {
459+
return std::make_error_code(std::errc::permission_denied);
460+
}
461+
llvm::vfs::directory_iterator dir_begin(const Twine &Dir,
462+
std::error_code &EC) override {
463+
return llvm::vfs::directory_iterator();
464+
}
465+
std::error_code setCurrentWorkingDirectory(const Twine &Path) override {
466+
return std::make_error_code(std::errc::permission_denied);
467+
}
468+
llvm::ErrorOr<std::string> getCurrentWorkingDirectory() const override {
469+
return std::make_error_code(std::errc::permission_denied);
470+
}
471+
};
472+
473+
TEST(ToolChainTest, ConfigFileError) {
474+
IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts = new DiagnosticOptions();
475+
IntrusiveRefCntPtr<DiagnosticIDs> DiagID(new DiagnosticIDs());
476+
std::unique_ptr<SimpleDiagnosticConsumer> DiagConsumer(
477+
new SimpleDiagnosticConsumer());
478+
DiagnosticsEngine Diags(DiagID, &*DiagOpts, DiagConsumer.get(), false);
479+
IntrusiveRefCntPtr<llvm::vfs::FileSystem> FS(new FileSystemWithError);
480+
481+
Driver TheDriver("/home/test/bin/clang", "arm-linux-gnueabi", Diags,
482+
"clang LLVM compiler", FS);
483+
std::unique_ptr<Compilation> C(
484+
TheDriver.BuildCompilation({"/home/test/bin/clang", "--no-default-config",
485+
"--config", "./root.cfg", "--version"}));
486+
ASSERT_TRUE(C);
487+
ASSERT_TRUE(C->containsError());
488+
EXPECT_EQ(1U, Diags.getNumErrors());
489+
EXPECT_STREQ("configuration file './root.cfg' cannot be opened: cannot get "
490+
"absolute path",
491+
DiagConsumer->Errors[0].c_str());
492+
}
493+
494+
TEST(ToolChainTest, BadConfigFile) {
495+
IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts = new DiagnosticOptions();
496+
IntrusiveRefCntPtr<DiagnosticIDs> DiagID(new DiagnosticIDs());
497+
std::unique_ptr<SimpleDiagnosticConsumer> DiagConsumer(
498+
new SimpleDiagnosticConsumer());
499+
DiagnosticsEngine Diags(DiagID, &*DiagOpts, DiagConsumer.get(), false);
500+
IntrusiveRefCntPtr<llvm::vfs::InMemoryFileSystem> FS(
501+
new llvm::vfs::InMemoryFileSystem);
502+
503+
#ifdef _WIN32
504+
const char *TestRoot = "C:\\";
505+
#define FILENAME "C:/opt/root.cfg"
506+
#define DIRNAME "C:/opt"
507+
#else
508+
const char *TestRoot = "/";
509+
#define FILENAME "/opt/root.cfg"
510+
#define DIRNAME "/opt"
511+
#endif
512+
// UTF-16 string must be aligned on 2-byte boundary. Strings and char arrays
513+
// do not provide necessary alignment, so copy constant string into properly
514+
// allocated memory in heap.
515+
llvm::BumpPtrAllocator Alloc;
516+
char *StrBuff = (char *)Alloc.Allocate(16, 4);
517+
std::memset(StrBuff, 0, 16);
518+
std::memcpy(StrBuff, "\xFF\xFE\x00\xD8\x00\x00", 6);
519+
StringRef BadUTF(StrBuff, 6);
520+
FS->setCurrentWorkingDirectory(TestRoot);
521+
FS->addFile("/opt/root.cfg", 0, llvm::MemoryBuffer::getMemBuffer(BadUTF));
522+
FS->addFile("/home/user/test.cfg", 0,
523+
llvm::MemoryBuffer::getMemBuffer("@file.rsp"));
524+
525+
{
526+
Driver TheDriver("/home/test/bin/clang", "arm-linux-gnueabi", Diags,
527+
"clang LLVM compiler", FS);
528+
std::unique_ptr<Compilation> C(TheDriver.BuildCompilation(
529+
{"/home/test/bin/clang", "--config", "/opt/root.cfg", "--version"}));
530+
ASSERT_TRUE(C);
531+
ASSERT_TRUE(C->containsError());
532+
EXPECT_EQ(1U, DiagConsumer->Errors.size());
533+
EXPECT_STREQ("cannot read configuration file '" FILENAME
534+
"': Could not convert UTF16 to UTF8",
535+
DiagConsumer->Errors[0].c_str());
536+
}
537+
DiagConsumer->clear();
538+
{
539+
Driver TheDriver("/home/test/bin/clang", "arm-linux-gnueabi", Diags,
540+
"clang LLVM compiler", FS);
541+
std::unique_ptr<Compilation> C(TheDriver.BuildCompilation(
542+
{"/home/test/bin/clang", "--config", "/opt", "--version"}));
543+
ASSERT_TRUE(C);
544+
ASSERT_TRUE(C->containsError());
545+
EXPECT_EQ(1U, DiagConsumer->Errors.size());
546+
EXPECT_STREQ("configuration file '" DIRNAME
547+
"' cannot be opened: not a regular file",
548+
DiagConsumer->Errors[0].c_str());
549+
}
550+
DiagConsumer->clear();
551+
{
552+
Driver TheDriver("/home/test/bin/clang", "arm-linux-gnueabi", Diags,
553+
"clang LLVM compiler", FS);
554+
std::unique_ptr<Compilation> C(TheDriver.BuildCompilation(
555+
{"/home/test/bin/clang", "--config", "root",
556+
"--config-system-dir=", "--config-user-dir=", "--version"}));
557+
ASSERT_TRUE(C);
558+
ASSERT_TRUE(C->containsError());
559+
EXPECT_EQ(1U, DiagConsumer->Errors.size());
560+
EXPECT_STREQ("configuration file 'root' cannot be found",
561+
DiagConsumer->Errors[0].c_str());
562+
}
563+
564+
#undef FILENAME
565+
#undef DIRNAME
566+
}
567+
568+
TEST(ToolChainTest, ConfigInexistentInclude) {
569+
IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts = new DiagnosticOptions();
570+
IntrusiveRefCntPtr<DiagnosticIDs> DiagID(new DiagnosticIDs());
571+
std::unique_ptr<SimpleDiagnosticConsumer> DiagConsumer(
572+
new SimpleDiagnosticConsumer());
573+
DiagnosticsEngine Diags(DiagID, &*DiagOpts, DiagConsumer.get(), false);
574+
IntrusiveRefCntPtr<llvm::vfs::InMemoryFileSystem> FS(
575+
new llvm::vfs::InMemoryFileSystem);
576+
577+
#ifdef _WIN32
578+
const char *TestRoot = "C:\\";
579+
#define USERCONFIG "C:\\home\\user\\test.cfg"
580+
#define UNEXISTENT "C:\\home\\user\\file.rsp"
581+
#else
582+
const char *TestRoot = "/";
583+
#define USERCONFIG "/home/user/test.cfg"
584+
#define UNEXISTENT "/home/user/file.rsp"
585+
#endif
586+
FS->setCurrentWorkingDirectory(TestRoot);
587+
FS->addFile("/home/user/test.cfg", 0,
588+
llvm::MemoryBuffer::getMemBuffer("@file.rsp"));
589+
590+
{
591+
Driver TheDriver("/home/test/bin/clang", "arm-linux-gnueabi", Diags,
592+
"clang LLVM compiler", FS);
593+
std::unique_ptr<Compilation> C(TheDriver.BuildCompilation(
594+
{"/home/test/bin/clang", "--config", "test.cfg",
595+
"--config-system-dir=", "--config-user-dir=/home/user", "--version"}));
596+
ASSERT_TRUE(C);
597+
ASSERT_TRUE(C->containsError());
598+
EXPECT_EQ(1U, DiagConsumer->Errors.size());
599+
EXPECT_STREQ("cannot read configuration file '" USERCONFIG
600+
"': cannot not open file '" UNEXISTENT "'",
601+
DiagConsumer->Errors[0].c_str());
602+
}
603+
604+
#undef USERCONFIG
605+
#undef UNEXISTENT
606+
}
607+
608+
TEST(ToolChainTest, ConfigRecursiveInclude) {
609+
IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts = new DiagnosticOptions();
610+
IntrusiveRefCntPtr<DiagnosticIDs> DiagID(new DiagnosticIDs());
611+
std::unique_ptr<SimpleDiagnosticConsumer> DiagConsumer(
612+
new SimpleDiagnosticConsumer());
613+
DiagnosticsEngine Diags(DiagID, &*DiagOpts, DiagConsumer.get(), false);
614+
IntrusiveRefCntPtr<llvm::vfs::InMemoryFileSystem> FS(
615+
new llvm::vfs::InMemoryFileSystem);
616+
617+
#ifdef _WIN32
618+
const char *TestRoot = "C:\\";
619+
#define USERCONFIG "C:\\home\\user\\test.cfg"
620+
#define INCLUDED1 "C:\\home\\user\\file1.cfg"
621+
#else
622+
const char *TestRoot = "/";
623+
#define USERCONFIG "/home/user/test.cfg"
624+
#define INCLUDED1 "/home/user/file1.cfg"
625+
#endif
626+
FS->setCurrentWorkingDirectory(TestRoot);
627+
FS->addFile("/home/user/test.cfg", 0,
628+
llvm::MemoryBuffer::getMemBuffer("@file1.cfg"));
629+
FS->addFile("/home/user/file1.cfg", 0,
630+
llvm::MemoryBuffer::getMemBuffer("@file2.cfg"));
631+
FS->addFile("/home/user/file2.cfg", 0,
632+
llvm::MemoryBuffer::getMemBuffer("@file3.cfg"));
633+
FS->addFile("/home/user/file3.cfg", 0,
634+
llvm::MemoryBuffer::getMemBuffer("@file1.cfg"));
635+
636+
{
637+
Driver TheDriver("/home/test/bin/clang", "arm-linux-gnueabi", Diags,
638+
"clang LLVM compiler", FS);
639+
std::unique_ptr<Compilation> C(TheDriver.BuildCompilation(
640+
{"/home/test/bin/clang", "--config", "test.cfg",
641+
"--config-system-dir=", "--config-user-dir=/home/user", "--version"}));
642+
ASSERT_TRUE(C);
643+
ASSERT_TRUE(C->containsError());
644+
EXPECT_EQ(1U, DiagConsumer->Errors.size());
645+
EXPECT_STREQ("cannot read configuration file '" USERCONFIG
646+
"': recursive expansion of: '" INCLUDED1 "'",
647+
DiagConsumer->Errors[0].c_str());
648+
}
649+
650+
#undef USERCONFIG
651+
#undef INCLUDED1
652+
}
653+
453654
} // end anonymous namespace.

‎flang/tools/flang-driver/driver.cpp

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -77,7 +77,9 @@ static void ExpandResponseFiles(
7777
// We're defaulting to the GNU syntax, since we don't have a CL mode.
7878
llvm::cl::TokenizerCallback tokenizer = &llvm::cl::TokenizeGNUCommandLine;
7979
llvm::cl::ExpansionContext ExpCtx(saver.getAllocator(), tokenizer);
80-
ExpCtx.expandResponseFiles(args);
80+
if (llvm::Error Err = ExpCtx.expandResponseFiles(args)) {
81+
llvm::errs() << toString(std::move(Err)) << '\n';
82+
}
8183
}
8284

8385
int main(int argc, const char **argv) {

‎llvm/include/llvm/Support/CommandLine.h

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -2206,10 +2206,10 @@ class ExpansionContext {
22062206
/// commands resolving file names in them relative to the directory where
22072207
/// CfgFilename resides. It also expands "<CFGDIR>" to the base path of the
22082208
/// current config file.
2209-
bool readConfigFile(StringRef CfgFile, SmallVectorImpl<const char *> &Argv);
2209+
Error readConfigFile(StringRef CfgFile, SmallVectorImpl<const char *> &Argv);
22102210

22112211
/// Expands constructs "@file" in the provided array of arguments recursively.
2212-
bool expandResponseFiles(SmallVectorImpl<const char *> &Argv);
2212+
Error expandResponseFiles(SmallVectorImpl<const char *> &Argv);
22132213
};
22142214

22152215
/// 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,
22212221

22222222
/// A convenience helper which supports the typical use case of expansion
22232223
/// function call.
2224-
inline bool ExpandResponseFiles(StringSaver &Saver, TokenizerCallback Tokenizer,
2225-
SmallVectorImpl<const char *> &Argv) {
2226-
ExpansionContext ECtx(Saver.getAllocator(), Tokenizer);
2227-
return ECtx.expandResponseFiles(Argv);
2228-
}
2224+
bool ExpandResponseFiles(StringSaver &Saver, TokenizerCallback Tokenizer,
2225+
SmallVectorImpl<const char *> &Argv);
22292226

22302227
/// A convenience helper which concatenates the options specified by the
22312228
/// environment variable EnvVar and command line options, then expands response

0 commit comments

Comments
 (0)