Skip to content

Wrap Command Line Arguments in a Response File if System Limits are Exceeded #16362

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 17 commits into from
Jun 21, 2018
Merged

Wrap Command Line Arguments in a Response File if System Limits are Exceeded #16362

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
5976901
Wrap command line arguments in a response file if they exceed system …
dabelknap May 2, 2018
da19625
Merge branch 'master' into frontend_responsefile
dabelknap May 11, 2018
2297f75
Move the "allowsResponseFiles" flag to the ToolChain.
dabelknap May 11, 2018
2b5e3c7
Reuse existing string-escaping code, and use raw_fd_ostream.
dabelknap May 14, 2018
62813f4
Reorganize response file tests.
dabelknap May 14, 2018
fe4f21d
Elaborate on the allowsResponseFiles flag.
dabelknap May 16, 2018
2a30e25
Only keep a "const char*" for the response file argument.
dabelknap May 18, 2018
3fe62b9
Reformat verbose and stack trace printouts for response-files.
dabelknap May 23, 2018
3b38fa6
Preserve argv for the stack trace, and only create a copy if necessary.
dabelknap Jun 1, 2018
6592b42
Style fixes.
dabelknap Jun 1, 2018
8e6abee
Pass ArrayRefs by value.
dabelknap Jun 13, 2018
e65cb23
Remove reference to vector in the comment.
dabelknap Jun 15, 2018
d71373c
Encapsulate the logic for returning a response file or the full arg v…
dabelknap Jun 15, 2018
f6b5cf5
Further clarify the purpose of "allowsResponseFiles" flag.
dabelknap Jun 15, 2018
9d51350
Use an ArrayRef insead of a const reference.
dabelknap Jun 19, 2018
d6ec8fa
Replace accessor methods in getArgumentsForTaskExecution.
dabelknap Jun 19, 2018
be229fc
Use subCommandArgs instead of argv within subcommand if block.
dabelknap Jun 20, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 20 additions & 2 deletions include/swift/Driver/Job.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -271,6 +271,14 @@ class Job {
/// Whether the job wants a list of input or output files created.
std::vector<FilelistInfo> FilelistFileInfos;

/// Response file path
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would remove this comment. It is completely redundant.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the point of this comment? @jrose-apple Is there some coding standard I don't know about? Why not remove it entirely?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This purpose of the comment is to remain consistent with the rest of the file. A few lines above reads:

/// The output of this command.
std::unique_ptr<CommandOutput> Output;

/// The executable to run.
const char *Executable;

for example.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand. Since I believe that this sort of comment degrades maintainability (yes, it's a tiny cut but there are virtually thousands of them), and since I try to enhance the maintainabilty of the code with each review, I asked for its deletion. However, a consistent-but-redundant comment is only a mere bagatelle.

const char *ResponseFilePath;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bit of a nitpick, but I would prefer it with another "const" after the star (const char * const) in order to signal that this variable is never assigned to (assuming that is the case). I'm assuming it is initialized in the constructor and never changed thereafter. (Also applies in other cases, such as ResponseFileArg.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still like the const after the star here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const char * is what is conventionally used in the rest of the class.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I count about 100 lines of code in methods of that class. If I had to maintain this code and needed to know if that instance variable ever changed, I would have to read them. Adding the "cost" would save me that work. Therefore, I would not approve this PR without this "const". (Of course you don't need my approval to land it & that's OK. I would just feel derelict in my duty to favor consistency over maintainability in this case.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Meta: At this point you've expressed a strong enough opinion that Austin does need your approval to land the PR. If you want to rescind that, you'd have to signify that, perhaps by "dismissing" your X review.

Personally, I'm with Austin on this one. Being inconsistent can imply that the inconsistency is deliberate, and therefore it is better to be consistent with existing code in a PR that's adding functionality. (That's especially true for someone who's a relative newcomer.)

Copy link
Contributor

@davidungar davidungar Jun 21, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Meta: @jrose-apple Please help me out here. Since the merge box is green, I figured he did not need my approval to land the PR. (I seem to recall reading verbiage to that effect on github, too.) Is there some policy (or implicit understanding) that I've missed? Where are you thinking that I express such a strong enough opinion? Was the need expressed, or implied? I don't understand.

You have a fair point about inconsistency, but I weigh that point against the benefits of this particular proposed inconsistency. In this case I would argue (on your side) that if the const is added to one variable, a future reader might infer that the other variables are not const. However, the counter argument would be that it would be clear for anyone with a passing familiarity with the code base that const is often not used when it could be. Thus, it seems to me that the consistency benefit is outweighed by the not-having-to-read-100-lines-of-code benefit of the const.

My experience as a novitiate to this code base has been that such consts would save me significant time. Ironically, it was my experience with adopting Swift for my code that convinced me of the significance of the benefits of write-once variables.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the merge box is green, I figured he did not need my approval to land the PR. (I seem to recall reading verbiage to that effect on github, too.) Is there some policy (or implicit understanding) that I've missed? Where are you thinking that I express such a strong enough opinion? Was the need expressed, or implied?

We aren't using GitHub's built-in gating for reviewers, true. But only a little above the green merge button is the part where "@davidungar requested changes", with a red X. Even without that, though, I think we want to take expressed concerns seriously unless the reviewer explicitly says something like "this is fine as is (but I think it could be better in these ways)".


/// This contains a single argument pointing to the response file path with
/// the '@' prefix.
/// The argument string must be kept alive as long as the Job is alive.
const char *ResponseFileArg;

/// The modification time of the main input file, if any.
llvm::sys::TimePoint<> InputModTime = llvm::sys::TimePoint<>::max();

Expand All @@ -281,12 +289,16 @@ class Job {
const char *Executable,
llvm::opt::ArgStringList Arguments,
EnvironmentVector ExtraEnvironment = {},
std::vector<FilelistInfo> Infos = {})
std::vector<FilelistInfo> Infos = {},
const char *ResponseFilePath = nullptr,
const char *ResponseFileArg = nullptr)
: SourceAndCondition(&Source, Condition::Always),
Inputs(std::move(Inputs)), Output(std::move(Output)),
Executable(Executable), Arguments(std::move(Arguments)),
ExtraEnvironment(std::move(ExtraEnvironment)),
FilelistFileInfos(std::move(Infos)) {}
FilelistFileInfos(std::move(Infos)),
ResponseFilePath(ResponseFilePath),
ResponseFileArg(ResponseFileArg) {}

virtual ~Job();

Expand All @@ -296,7 +308,9 @@ class Job {

const char *getExecutable() const { return Executable; }
const llvm::opt::ArgStringList &getArguments() const { return Arguments; }
ArrayRef<const char *> getResponseFileArg() const { return ResponseFileArg; }
ArrayRef<FilelistInfo> getFilelistInfos() const { return FilelistFileInfos; }
ArrayRef<const char *> getArgumentsForTaskExecution() const;

ArrayRef<const Job *> getInputs() const { return Inputs; }
const CommandOutput &getOutput() const { return *Output; }
Expand Down Expand Up @@ -347,6 +361,10 @@ class Job {

static void printArguments(raw_ostream &Stream,
const llvm::opt::ArgStringList &Args);

bool hasResponseFile() const { return ResponseFilePath != nullptr; }

bool writeArgsToResponseFile() const;
};

/// A BatchJob comprises a _set_ of jobs, each of which is sufficiently similar
Expand Down
6 changes: 6 additions & 0 deletions include/swift/Driver/ToolChain.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,12 @@ class ToolChain {
std::vector<std::pair<const char *, const char *>> ExtraEnvironment;
std::vector<FilelistInfo> FilelistInfos;

// Not all platforms and jobs support the use of response files, so assume
// "false" by default. If the executable specified in the InvocationInfo
// constructor supports response files, this can be overridden and set to
// "true".
bool allowsResponseFiles = false;

InvocationInfo(const char *name, llvm::opt::ArgStringList args = {},
decltype(ExtraEnvironment) extraEnv = {})
: ExecutableName(name), Arguments(std::move(args)),
Expand Down
4 changes: 2 additions & 2 deletions lib/Driver/Compilation.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -291,8 +291,8 @@ namespace driver {
"not implemented for compilations with multiple jobs");
if (Comp.ShowJobLifecycle)
llvm::outs() << "Added to TaskQueue: " << LogJob(Cmd) << "\n";
TQ->addTask(Cmd->getExecutable(), Cmd->getArguments(), llvm::None,
(void *)Cmd);
TQ->addTask(Cmd->getExecutable(), Cmd->getArgumentsForTaskExecution(),
llvm::None, (void *)Cmd);
}

/// When a task finishes, check other Jobs that may be blocked.
Expand Down
30 changes: 30 additions & 0 deletions lib/Driver/Job.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
#include "llvm/ADT/STLExtras.h"
#include "llvm/Option/Arg.h"
#include "llvm/Support/Compiler.h"
#include "llvm/Support/FileSystem.h"
#include "llvm/Support/Path.h"
#include "llvm/Support/Program.h"
#include "llvm/Support/raw_ostream.h"
Expand Down Expand Up @@ -337,6 +338,15 @@ void Job::dump() const {
printCommandLineAndEnvironment(llvm::errs());
}

ArrayRef<const char *> Job::getArgumentsForTaskExecution() const {
if (hasResponseFile()) {
writeArgsToResponseFile();
return getResponseFileArg();
} else {
return getArguments();
}
}

void Job::printCommandLineAndEnvironment(raw_ostream &Stream,
StringRef Terminator) const {
printCommandLine(Stream, /*Terminator=*/"");
Expand All @@ -352,7 +362,12 @@ void Job::printCommandLineAndEnvironment(raw_ostream &Stream,
void Job::printCommandLine(raw_ostream &os, StringRef Terminator) const {
escapeAndPrintString(os, Executable);
os << ' ';
if (hasResponseFile()) {
printArguments(os, {ResponseFileArg});
os << " # ";
}
printArguments(os, Arguments);

os << Terminator;
}

Expand Down Expand Up @@ -403,6 +418,21 @@ void Job::printSummary(raw_ostream &os) const {
os << "}";
}

bool Job::writeArgsToResponseFile() const {
std::error_code EC;
llvm::raw_fd_ostream OS(ResponseFilePath, EC, llvm::sys::fs::F_None);
if (EC) {
return true;
}
for (const char *arg : Arguments) {
OS << "\"";
escapeAndPrintString(OS, arg);
OS << "\" ";
}
OS.flush();
return false;
}

BatchJob::BatchJob(const JobAction &Source,
SmallVectorImpl<const Job *> &&Inputs,
std::unique_ptr<CommandOutput> Output,
Expand Down
13 changes: 12 additions & 1 deletion lib/Driver/ToolChain.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,11 +114,22 @@ std::unique_ptr<Job> ToolChain::constructJob(
}
}

const char *responseFilePath = nullptr;
const char *responseFileArg = nullptr;
if (invocationInfo.allowsResponseFiles &&
!llvm::sys::commandLineFitsWithinSystemLimits(
executablePath, invocationInfo.Arguments)) {
responseFilePath = context.getTemporaryFilePath("arguments", "resp");
responseFileArg = C.getArgs().MakeArgString(Twine("@") + responseFilePath);
}

return llvm::make_unique<Job>(JA, std::move(inputs), std::move(output),
executablePath,
std::move(invocationInfo.Arguments),
std::move(invocationInfo.ExtraEnvironment),
std::move(invocationInfo.FilelistInfos));
std::move(invocationInfo.FilelistInfos),
responseFilePath,
responseFileArg);
}

std::string
Expand Down
1 change: 1 addition & 0 deletions lib/Driver/ToolChains.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,6 +247,7 @@ ToolChain::constructInvocation(const CompileJobAction &job,
const JobContext &context) const {
InvocationInfo II{SWIFT_EXECUTABLE_NAME};
ArgStringList &Arguments = II.Arguments;
II.allowsResponseFiles = true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is it true in this case? Comment?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I remain puzzled by that line. Please add a comment.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Never mind, see below.


Arguments.push_back("-frontend");

Expand Down
5 changes: 4 additions & 1 deletion lib/Driver/UnixToolChains.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -325,7 +325,10 @@ toolchains::GenericUnix::constructInvocation(const LinkJobAction &job,
Arguments.push_back(
context.Args.MakeArgString(context.Output.getPrimaryOutputFilename()));

return {Clang, Arguments};
InvocationInfo II{Clang, Arguments};
II.allowsResponseFiles = true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why true? Comment?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there are a bunch of similar lines, could you please factor them into an intention-revealing method? Or add a comment to each?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Never mind, see below.


return II;
}

std::string toolchains::Android::getTargetForLinker() const {
Expand Down
5 changes: 4 additions & 1 deletion lib/Driver/WindowsToolChains.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,5 +168,8 @@ toolchains::Windows::constructInvocation(const LinkJobAction &job,
Arguments.push_back(
context.Args.MakeArgString(context.Output.getPrimaryOutputFilename()));

return {Clang, Arguments};
InvocationInfo II{Clang, Arguments};
II.allowsResponseFiles = true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, this is the third time I'm seeing the setting to true. Is there something in common that the code should be reifying?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope. It's all based on what other tool is getting invoked, which is not necessarily the same across each toolchain and certainly not the same across each job.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is, this is the customization point, so that the rest of the code can treat everything the same.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, but how about comments explaining why allowsResponseFiles is true in each case? Why might it be false vs true?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment would read: // clang supports response files. Given the way in which the code is written, such a comment would be redundant.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am still with Austin on this. If the executable specified in the InvocationInfo constructor supports response files, you can put true here. I guess the doc comment for allowsResponseFiles could say that a little more clearly, but it doesn't belong at the use sites.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the comment in include/swift/Driver/ToolChain.h.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for updating the comment. In thinking about this item, the comment you added is good and it is sufficient. I'm not asking for the following, but if I were to try to make it even clearer, I would use a one-line intention-revealing function. But, never mind.


return II;
}
39 changes: 27 additions & 12 deletions test/Driver/response-file.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,24 +6,35 @@
// RUN: %target-build-swift -typecheck @%t.1.resp %s 2>&1 | %FileCheck %s -check-prefix=MULTIPLE
// MULTIPLE: warning: result of call to 'abs' is unused

// RUN: echo "-DTEST2A -DTEST2B" > %t.2.resp
// RUN: echo "-DTEST2C -DTEST2D" > %t.3.resp
// RUN: %target-build-swift -typecheck @%t.2.resp %s @%t.3.resp 2>&1 | %FileCheck %s -check-prefix=MIXED
// RUN: echo "-DTEST2A -DTEST2B" > %t.2A.resp
// RUN: echo "-DTEST2C -DTEST2D" > %t.2B.resp
// RUN: %target-build-swift -typecheck @%t.2A.resp %s @%t.2B.resp 2>&1 | %FileCheck %s -check-prefix=MIXED
// MIXED: warning: result of call to 'abs' is unused

// RUN: echo "-DTEST3A" > %t.4.resp
// RUN: echo "%s" >> %t.4.resp
// RUN: echo "-DTEST3B" >> %t.4.resp
// RUN: %target-build-swift -typecheck @%t.4.resp 2>&1 | %FileCheck %s -check-prefix=RESPONLY
// RUN: echo "-DTEST3A" > %t.3.resp
// RUN: echo "%s" >> %t.3.resp
// RUN: echo "-DTEST3B" >> %t.3.resp
// RUN: %target-build-swift -typecheck @%t.3.resp 2>&1 | %FileCheck %s -check-prefix=RESPONLY
// RESPONLY: warning: result of call to 'abs' is unused

// RUN: echo "-DTEST4A" > %t.5.resp
// RUN: echo "%s" >> %t.5.resp
// RUN: echo "@%t.5.resp" > %t.6.resp
// RUN: echo "-DTEST4B" >> %t.6.resp
// RUN: %target-build-swift -typecheck @%t.6.resp 2>&1 | %FileCheck %s -check-prefix=RECURSIVE
// RUN: echo "-DTEST4A" > %t.4A.resp
// RUN: echo "%s" >> %t.4A.resp
// RUN: echo "@%t.4A.resp" > %t.4B.resp
// RUN: echo "-DTEST4B" >> %t.4B.resp
// RUN: %target-build-swift -typecheck @%t.4B.resp 2>&1 | %FileCheck %s -check-prefix=RECURSIVE
// RECURSIVE: warning: result of call to 'abs' is unused

// RUN: python -c 'for i in range(500001): print "-DTEST5_" + str(i)' > %t.5.resp
// RUN: %target-build-swift -typecheck @%t.5.resp %s 2>&1 | %FileCheck %s -check-prefix=LONG
// LONG: warning: result of call to 'abs' is unused
// RUN: %empty-directory(%t/tmp)
// RUN: env TMPDIR=%t/tmp/ %target-swiftc_driver %s @%t.5.resp -save-temps 2>&1
// RUN: ls %t/tmp/arguments-*.resp
// RUN: %target-build-swift -v @%t.5.resp %s 2>&1 | %FileCheck %s -check-prefix=VERBOSE
// VERBOSE: @{{[^ ]*}}arguments-{{[0-9a-zA-Z]+}}.resp # -frontend -c -primary-file
// RUN: not %target-swiftc_driver %s @%t.5.resp -Xfrontend -debug-crash-immediately 2>&1 | %FileCheck %s -check-prefix=CRASH
// CRASH: Program arguments: {{[^ ]*}}swift -frontend -c -primary-file

#if TEST0
abs(-5)
#endif
Expand All @@ -43,3 +54,7 @@ abs(-5)
#if TEST4A && TEST4B
abs(-5)
#endif

#if TEST5_0 && TEST5_10 && TEST5_100 && TEST5_1000 && TEST5_10000 && TEST5_100000 && TEST5_500000
abs(-5)
#endif
Loading