Hi rafael,
This is the Clang side of the patch in http://reviews.llvm.org/D4899
This patch aims at fixing PR17239.
Note: it depends on http://reviews.llvm.org/D4899 the llvm side of the patch,
landing in the LLVM tree.
This bug happens because the /link (clang-cl.exe argument) is marked as
"consume all remaining arguments". However, when inside a response file, /link
should only consume all remaining arguments inside the response file where it
is located, not the entire command line after expansion. The LLVM side of the
patch will change the semantics of the RemainingArgsClass kind to always
consume only until the end of the response file when the option originally came
from a response file. There are only two options in this class: dash dash (--)
and /link.
In the Clang side, I just used the ExpandedArgs class created in the LLVM side
of the patch to fix this bug.
http://reviews.llvm.org/D4900
Files:
include/clang/Driver/Driver.h
lib/Driver/Driver.cpp
test/Driver/cl-link-at-file.c
tools/driver/cc1_main.cpp
tools/driver/cc1as_main.cpp
tools/driver/driver.cpp
Index: include/clang/Driver/Driver.h
===================================================================
--- include/clang/Driver/Driver.h
+++ include/clang/Driver/Driver.h
@@ -26,6 +26,9 @@
#include <string>
namespace llvm {
+namespace cl {
+ class ExpandedArgs;
+}
namespace opt {
class Arg;
class ArgList;
@@ -239,7 +242,9 @@
/// argument vector. A null return value does not necessarily
/// indicate an error condition, the diagnostics should be queried
/// to determine if an error occurred.
- Compilation *BuildCompilation(ArrayRef<const char *> Args);
+ Compilation *
+ BuildCompilation(ArrayRef<const char *> Args,
+ const llvm::cl::ExpandedArgs *ResponseFilesInfo = nullptr);
/// @name Driver Steps
/// @{
@@ -249,7 +254,9 @@
/// ParseArgStrings - Parse the given list of strings into an
/// ArgList.
- llvm::opt::InputArgList *ParseArgStrings(ArrayRef<const char *> Args);
+ llvm::opt::InputArgList *
+ ParseArgStrings(ArrayRef<const char *> Args,
+ const llvm::cl::ExpandedArgs *ResponseFilesInfo = nullptr);
/// BuildInputs - Construct the list of inputs and their types from
/// the given arguments.
Index: lib/Driver/Driver.cpp
===================================================================
--- lib/Driver/Driver.cpp
+++ lib/Driver/Driver.cpp
@@ -29,6 +29,7 @@
#include "llvm/Option/OptSpecifier.h"
#include "llvm/Option/OptTable.h"
#include "llvm/Option/Option.h"
+#include "llvm/Support/CommandLine.h"
#include "llvm/Support/Debug.h"
#include "llvm/Support/ErrorHandling.h"
#include "llvm/Support/FileSystem.h"
@@ -102,7 +103,9 @@
}
}
-InputArgList *Driver::ParseArgStrings(ArrayRef<const char *> ArgList) {
+InputArgList *
+Driver::ParseArgStrings(ArrayRef<const char *> ArgList,
+ const llvm::cl::ExpandedArgs *ResponseFilesInfo) {
llvm::PrettyStackTraceString CrashInfo("Command line argument parsing");
unsigned IncludedFlagsBitmask;
@@ -114,7 +117,8 @@
InputArgList *Args = getOpts().ParseArgs(ArgList.begin(), ArgList.end(),
MissingArgIndex, MissingArgCount,
IncludedFlagsBitmask,
- ExcludedFlagsBitmask);
+ ExcludedFlagsBitmask,
+ ResponseFilesInfo);
// Check for missing argument error.
if (MissingArgCount)
@@ -287,7 +291,9 @@
return DAL;
}
-Compilation *Driver::BuildCompilation(ArrayRef<const char *> ArgList) {
+Compilation *
+Driver::BuildCompilation(ArrayRef<const char *> ArgList,
+ const llvm::cl::ExpandedArgs *ResponseFilesInfo) {
llvm::PrettyStackTraceString CrashInfo("Compilation construction");
// FIXME: Handle environment options which affect driver behavior, somewhere
@@ -312,7 +318,7 @@
// FIXME: This stuff needs to go into the Compilation, not the driver.
bool CCCPrintActions;
- InputArgList *Args = ParseArgStrings(ArgList.slice(1));
+ InputArgList *Args = ParseArgStrings(ArgList.slice(1), ResponseFilesInfo);
// -no-canonical-prefixes is used very early in main.
Args->ClaimAllArgs(options::OPT_no_canonical_prefixes);
Index: test/Driver/cl-link-at-file.c
===================================================================
--- test/Driver/cl-link-at-file.c
+++ test/Driver/cl-link-at-file.c
@@ -0,0 +1,44 @@
+// PR17239 - The /link option, when inside a response file, should only extend
+// until the end of the response file (and not the entire command line)
+
+// Don't attempt slash switches on msys bash.
+// REQUIRES: shell-preserves-root
+
+// Note: %s must be preceded by -- or bound to another option, otherwise it may
+// be interpreted as a command-line option, e.g. on Mac where %s is commonly
+// under /Users.
+
+// RUN: echo /link bar.lib baz.lib > %t.args
+// RUN: touch %t.obj
+// RUN: %clang_cl -### @%t.args -- %t.obj 2>&1 | FileCheck %s -check-prefix=ARGS
+// If the "/link" option captures all remaining args beyond its response file,
+// it will also capture "--" and our input argument. In this case, Clang will
+// be clueless and will emit "argument unused" warnings. If PR17239 is properly
+// fixed, this should not happen because the "/link" option is restricted to
+// consume only remaining args in its response file.
+// ARGS-NOT: warning
+// ARGS-NOT: argument unused during compilation
+// Identify the linker command
+// ARGS: link.exe
+
+// RUN: echo -- %t.obj > %t.2.args
+// RUN: %clang_cl -### @%t.2.args /link /base:0x10000 2>&1 | FileCheck %s \
+// RUN: -check-prefix=BASE
+// Notice how the "--" arg (which consumes remaining args) should only take
+// effect until the end of its container response file.
+// Even though "/base:0x10000" is not a valid clang argument, it should not
+// fail because we are passing it as a linker argument (via /link)
+// BASE-NOT: no such file or directory: '/base:0x10000'
+// BASE: link.exe
+// BASE: "/base:0x10000"
+
+// RUN: echo /align:4096 > %t.3.args
+// RUN: %clang_cl -### @%t.2.args /link @%t3.args /base:0x10000 2>&1 | \
+// RUN: FileCheck %s -check-prefix=ALIGN
+// This test checks whether a "/link" command outside a response file is able
+// to consume all remaining args even if followed by response files (whether
+// this response file should be expanded or not is another question that is
+// outside the scope of this test).
+// ALIGN-NOT: no such file or directory: '/base:0x10000'
+// ALIGN: link.exe
+// ALIGN: "/base:0x10000"
Index: tools/driver/cc1_main.cpp
===================================================================
--- tools/driver/cc1_main.cpp
+++ tools/driver/cc1_main.cpp
@@ -63,7 +63,7 @@
}
#endif
-int cc1_main(const char **ArgBegin, const char **ArgEnd,
+int cc1_main(const char * const *ArgBegin, const char * const *ArgEnd,
const char *Argv0, void *MainAddr) {
std::unique_ptr<CompilerInstance> Clang(new CompilerInstance());
IntrusiveRefCntPtr<DiagnosticIDs> DiagID(new DiagnosticIDs());
Index: tools/driver/cc1as_main.cpp
===================================================================
--- tools/driver/cc1as_main.cpp
+++ tools/driver/cc1as_main.cpp
@@ -141,15 +141,15 @@
DwarfVersion = 3;
}
- static bool CreateFromArgs(AssemblerInvocation &Res, const char **ArgBegin,
- const char **ArgEnd, DiagnosticsEngine &Diags);
+ static bool CreateFromArgs(AssemblerInvocation &Res, const char * const *ArgBegin,
+ const char * const *ArgEnd, DiagnosticsEngine &Diags);
};
}
bool AssemblerInvocation::CreateFromArgs(AssemblerInvocation &Opts,
- const char **ArgBegin,
- const char **ArgEnd,
+ const char * const *ArgBegin,
+ const char * const *ArgEnd,
DiagnosticsEngine &Diags) {
bool Success = true;
@@ -420,7 +420,7 @@
exit(1);
}
-int cc1as_main(const char **ArgBegin, const char **ArgEnd,
+int cc1as_main(const char * const *ArgBegin, const char * const *ArgEnd,
const char *Argv0, void *MainAddr) {
// Print a stack trace if we signal out.
sys::PrintStackTraceOnErrorSignal();
Index: tools/driver/driver.cpp
===================================================================
--- tools/driver/driver.cpp
+++ tools/driver/driver.cpp
@@ -50,6 +50,7 @@
using namespace clang;
using namespace clang::driver;
using namespace llvm::opt;
+using llvm::cl::ExpandedArgs;
std::string GetExecutablePath(const char *Argv0, bool CanonicalPrefixes) {
if (!CanonicalPrefixes)
@@ -94,19 +95,17 @@
/// \param Edit - The override command to perform.
/// \param SavedStrings - Set to use for storing string representations.
static void ApplyOneQAOverride(raw_ostream &OS,
- SmallVectorImpl<const char*> &Args,
- StringRef Edit,
- std::set<std::string> &SavedStrings) {
+ ExpandedArgs &Args,
+ StringRef Edit) {
// This does not need to be efficient.
if (Edit[0] == '^') {
- const char *Str =
- SaveStringInSet(SavedStrings, Edit.substr(1));
+ const char *Str = Args.SaveString(Edit.substr(1));
OS << "### Adding argument " << Str << " at beginning\n";
Args.insert(Args.begin() + 1, Str);
} else if (Edit[0] == '+') {
const char *Str =
- SaveStringInSet(SavedStrings, Edit.substr(1));
+ Args.SaveString(Edit.substr(1));
OS << "### Adding argument " << Str << " at end\n";
Args.push_back(Str);
} else if (Edit[0] == 's' && Edit[1] == '/' && Edit.endswith("/") &&
@@ -120,7 +119,7 @@
if (Repl != Args[i]) {
OS << "### Replacing '" << Args[i] << "' with '" << Repl << "'\n";
- Args[i] = SaveStringInSet(SavedStrings, Repl);
+ Args[i] = Args.SaveString(Repl);
}
}
} else if (Edit[0] == 'x' || Edit[0] == 'X') {
@@ -152,17 +151,16 @@
++i;
}
OS << "### Adding argument " << Edit << " at end\n";
- Args.push_back(SaveStringInSet(SavedStrings, '-' + Edit.str()));
+ Args.push_back(Args.SaveString('-' + Edit.str()));
} else {
OS << "### Unrecognized edit: " << Edit << "\n";
}
}
/// ApplyQAOverride - Apply a comma separate list of edits to the
/// input argument lists. See ApplyOneQAOverride.
-static void ApplyQAOverride(SmallVectorImpl<const char*> &Args,
- const char *OverrideStr,
- std::set<std::string> &SavedStrings) {
+static void ApplyQAOverride(ExpandedArgs &Args,
+ const char *OverrideStr) {
raw_ostream *OS = &llvm::errs();
if (OverrideStr[0] == '#') {
@@ -180,20 +178,19 @@
if (!End)
End = S + strlen(S);
if (End != S)
- ApplyOneQAOverride(*OS, Args, std::string(S, End), SavedStrings);
+ ApplyOneQAOverride(*OS, Args, std::string(S, End));
S = End;
if (*S != '\0')
++S;
}
}
-extern int cc1_main(const char **ArgBegin, const char **ArgEnd,
+extern int cc1_main(const char * const *ArgBegin, const char * const *ArgEnd,
const char *Argv0, void *MainAddr);
-extern int cc1as_main(const char **ArgBegin, const char **ArgEnd,
+extern int cc1as_main(const char * const *ArgBegin, const char * const *ArgEnd,
const char *Argv0, void *MainAddr);
-static void ParseProgName(SmallVectorImpl<const char *> &ArgVector,
- std::set<std::string> &SavedStrings,
+static void ParseProgName(ExpandedArgs &ArgVector,
Driver &TheDriver)
{
// Try to infer frontend type and default target from the program name.
@@ -275,12 +272,25 @@
if (it != ArgVector.end())
++it;
const char* Strings[] =
- { SaveStringInSet(SavedStrings, std::string("-target")),
- SaveStringInSet(SavedStrings, Prefix) };
+ { ArgVector.SaveString(std::string("-target")),
+ ArgVector.SaveString(Prefix) };
ArgVector.insert(it, Strings, Strings + llvm::array_lengthof(Strings));
}
}
+static
+SmallVector<const char *, 256> GetArgumentVector(ArrayRef<const char *> Args,
+ llvm::SpecificBumpPtrAllocator<char> &ArgAllocator) {
+ SmallVector<const char *, 256> argv;
+ std::error_code EC = llvm::sys::Process::GetArgumentVector(
+ argv, Args, ArgAllocator);
+ if (EC) {
+ llvm::errs() << "error: couldn't get arguments: " << EC.message() << '\n';
+ exit(1);
+ }
+ return argv;
+}
+
namespace {
class StringSetSaver : public llvm::cl::StringSaver {
public:
@@ -297,18 +307,15 @@
llvm::sys::PrintStackTraceOnErrorSignal();
llvm::PrettyStackTraceProgram X(argc_, argv_);
- SmallVector<const char *, 256> argv;
llvm::SpecificBumpPtrAllocator<char> ArgAllocator;
- std::error_code EC = llvm::sys::Process::GetArgumentVector(
- argv, ArrayRef<const char *>(argv_, argc_), ArgAllocator);
- if (EC) {
- llvm::errs() << "error: couldn't get arguments: " << EC.message() << '\n';
- return 1;
- }
std::set<std::string> SavedStrings;
StringSetSaver Saver(SavedStrings);
- llvm::cl::ExpandResponseFiles(Saver, llvm::cl::TokenizeGNUCommandLine, argv);
+ ExpandedArgs ExpArgs = ExpandedArgs::ExpandResponseFiles(
+ ExpandedArgs::GNU,
+ GetArgumentVector(ArrayRef<const char *>(argv_, argc_), ArgAllocator),
+ &Saver);
+ const SmallVectorImpl<const char *> &argv = ExpArgs.get();
// Handle -cc1 integrated tools.
if (argv.size() > 1 && StringRef(argv[1]).startswith("-cc1")) {
@@ -338,7 +345,7 @@
// scenes.
if (const char *OverrideStr = ::getenv("CCC_OVERRIDE_OPTIONS")) {
// FIXME: Driver shouldn't take extra initial argument.
- ApplyQAOverride(argv, OverrideStr, SavedStrings);
+ ApplyQAOverride(ExpArgs, OverrideStr);
}
std::string Path = GetExecutablePath(argv[0], CanonicalPrefixes);
@@ -394,7 +401,7 @@
}
llvm::InitializeAllTargets();
- ParseProgName(argv, SavedStrings, TheDriver);
+ ParseProgName(ExpArgs, TheDriver);
// Handle CC_PRINT_OPTIONS and CC_PRINT_OPTIONS_FILE.
TheDriver.CCPrintOptions = !!::getenv("CC_PRINT_OPTIONS");
@@ -411,7 +418,7 @@
if (TheDriver.CCLogDiagnostics)
TheDriver.CCLogDiagnosticsFilename = ::getenv("CC_LOG_DIAGNOSTICS_FILE");
- std::unique_ptr<Compilation> C(TheDriver.BuildCompilation(argv));
+ std::unique_ptr<Compilation> C(TheDriver.BuildCompilation(argv, &ExpArgs));
int Res = 0;
SmallVector<std::pair<int, const Command *>, 4> FailingCommands;
if (C.get())
@@ -448,7 +455,7 @@
// If any timers were active but haven't been destroyed yet, print their
// results now. This happens in -disable-free mode.
llvm::TimerGroup::printAll(llvm::errs());
-
+
llvm::llvm_shutdown();
#ifdef LLVM_ON_WIN32
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
