Skip to content

Commit 7a97b31

Browse files
committed
<rdar://problem/28676638> Need BuildSystem support for build
cancellation - Implement the ability to cancel running builds - Invoke `cancel()` on `SIGINT` when executing build systems commands via CLI. - Test for SIGINT handling and for SIGKILL escalation - Unit test for cancellation in `LaneBasedExecutionQueue` - Reactivate Ninja cancellation test (issue was sh vs. bash)
1 parent bf3b11a commit 7a97b31

17 files changed

+562
-40
lines changed

include/llbuild/BuildSystem/BuildExecutionQueue.h

+3
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,9 @@ class BuildExecutionQueue {
8585
/// Add a job to be executed.
8686
virtual void addJob(QueueJob job) = 0;
8787

88+
/// Cancel all jobs and subprocesses of this queue.
89+
virtual void cancelAllJobs() = 0;
90+
8891
/// @name Execution Interfaces
8992
///
9093
/// These are additional interfaces provided by the execution queue which can

include/llbuild/BuildSystem/BuildSystem.h

+3
Original file line numberDiff line numberDiff line change
@@ -201,6 +201,9 @@ class BuildSystem {
201201
/// if a cycle was discovered).
202202
bool build(StringRef target);
203203

204+
/// Cancel the current build
205+
void cancel();
206+
204207
/// @}
205208
};
206209

include/llbuild/BuildSystem/BuildSystemFrontend.h

+8
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
#include "llvm/ADT/ArrayRef.h"
2020
#include "llvm/ADT/StringRef.h"
2121

22+
#include <atomic>
2223
#include <string>
2324
#include <vector>
2425

@@ -91,6 +92,7 @@ class BuildSystemFrontendDelegate : public BuildSystemDelegate {
9192

9293
private:
9394
void* impl;
95+
std::atomic<bool> isCancelled_;
9496

9597
/// Default implementation, cannot be overriden by subclasses.
9698
virtual void setFileContentsBeingParsed(StringRef buffer) override;
@@ -126,6 +128,12 @@ class BuildSystemFrontendDelegate : public BuildSystemDelegate {
126128
/// Provides a default cancellation implementation that will cancel when any
127129
/// command has failed.
128130
virtual bool isCancelled() override;
131+
132+
/// Cancels the current build.
133+
virtual void cancel();
134+
135+
/// Reset mutable build state before a new build operation.
136+
void resetForBuild();
129137

130138
/// Provides a default handler.
131139
///

lib/BuildSystem/BuildSystem.cpp

+17
Original file line numberDiff line numberDiff line change
@@ -281,6 +281,10 @@ static BuildSystemImpl& getBuildSystem(BuildEngine& engine) {
281281
return static_cast<BuildSystemEngineDelegate*>(
282282
engine.getDelegate())->getBuildSystem();
283283
}
284+
285+
static bool isCancelled(BuildEngine& engine) {
286+
return getBuildSystem(engine).getCommandInterface().getDelegate().isCancelled();
287+
}
284288

285289
/// This is the task used to "build" a target, it translates between the request
286290
/// for building a target key and the requests for all of its nodes.
@@ -328,6 +332,12 @@ class TargetTask : public Task {
328332
}
329333

330334
virtual void inputsAvailable(BuildEngine& engine) override {
335+
// If the build should cancel, do nothing.
336+
if (isCancelled(engine)) {
337+
engine.taskIsComplete(this, BuildValue::makeSkippedCommand().toData());
338+
return;
339+
}
340+
331341
if (hasMissingInput) {
332342
// FIXME: Design the logging and status output APIs.
333343
auto& system = getBuildSystem(engine);
@@ -2041,3 +2051,10 @@ bool BuildSystem::enableTracing(StringRef path,
20412051
bool BuildSystem::build(StringRef name) {
20422052
return static_cast<BuildSystemImpl*>(impl)->build(name);
20432053
}
2054+
2055+
void BuildSystem::cancel() {
2056+
if (impl) {
2057+
auto buildSystemImpl = static_cast<BuildSystemImpl*>(impl);
2058+
buildSystemImpl->getCommandInterface().getExecutionQueue().cancelAllJobs();
2059+
}
2060+
}

lib/BuildSystem/BuildSystemFrontend.cpp

+14-2
Original file line numberDiff line numberDiff line change
@@ -221,7 +221,7 @@ BuildSystemFrontendDelegate(llvm::SourceMgr& sourceMgr,
221221
StringRef name,
222222
uint32_t version)
223223
: BuildSystemDelegate(name, version),
224-
impl(new BuildSystemFrontendDelegateImpl(sourceMgr, invocation))
224+
impl(new BuildSystemFrontendDelegateImpl(sourceMgr, invocation)), isCancelled_(false)
225225
{
226226

227227
}
@@ -330,7 +330,19 @@ BuildSystemFrontendDelegate::createExecutionQueue() {
330330

331331
bool BuildSystemFrontendDelegate::isCancelled() {
332332
// Stop the build after any command failures.
333-
return getNumFailedCommands() > 0;
333+
return getNumFailedCommands() > 0 || isCancelled_;
334+
}
335+
336+
void BuildSystemFrontendDelegate::cancel() {
337+
// FIXME: We should audit that a build is happening.
338+
isCancelled_ = true;
339+
340+
auto delegateImpl = static_cast<BuildSystemFrontendDelegateImpl*>(impl);
341+
delegateImpl->system->cancel();
342+
}
343+
344+
void BuildSystemFrontendDelegate::resetForBuild() {
345+
isCancelled_ = false;
334346
}
335347

336348
void BuildSystemFrontendDelegate::hadCommandFailure() {

lib/BuildSystem/LaneBasedExecutionQueue.cpp

+82-16
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616

1717
#include "llvm/ADT/ArrayRef.h"
1818
#include "llvm/ADT/SmallString.h"
19+
#include "llvm/ADT/STLExtras.h"
1920
#include "llvm/ADT/Twine.h"
2021
#include "llvm/Support/Path.h"
2122
#include "llvm/Support/Program.h"
@@ -28,6 +29,7 @@
2829
#include <thread>
2930
#include <vector>
3031
#include <string>
32+
#include <unordered_set>
3133

3234
#include <fcntl.h>
3335
#include <pthread.h>
@@ -64,6 +66,16 @@ class LaneBasedExecutionQueue : public BuildExecutionQueue {
6466
std::mutex readyJobsMutex;
6567
std::condition_variable readyJobsCondition;
6668

69+
/// The set of spawned processes to terminate if we get cancelled.
70+
std::unordered_set<pid_t> spawnedProcesses;
71+
std::mutex spawnedProcessesMutex;
72+
73+
/// Management of cancellation and SIGKILL escalation
74+
std::unique_ptr<std::thread> killAfterTimeoutThread = nullptr;
75+
std::atomic<bool> cancelled { false };
76+
std::condition_variable stopKillingCondition;
77+
std::mutex stopKillingMutex;
78+
6779
void executeLane(unsigned laneNumber) {
6880
// Set the thread name, if available.
6981
#if defined(__APPLE__)
@@ -78,7 +90,7 @@ class LaneBasedExecutionQueue : public BuildExecutionQueue {
7890
#endif
7991

8092
// Execute items from the queue until shutdown.
81-
while (true) {
93+
while (!cancelled) {
8294
// Take a job from the ready queue.
8395
QueueJob job{};
8496
{
@@ -87,6 +99,10 @@ class LaneBasedExecutionQueue : public BuildExecutionQueue {
8799
// While the queue is empty, wait for an item.
88100
while (readyJobs.empty()) {
89101
readyJobsCondition.wait(lock);
102+
103+
if (cancelled) {
104+
return;
105+
}
90106
}
91107

92108
// Take an item according to the chosen policy.
@@ -106,6 +122,21 @@ class LaneBasedExecutionQueue : public BuildExecutionQueue {
106122
}
107123
}
108124

125+
void killAfterTimeout() {
126+
std::unique_lock<std::mutex> lock(stopKillingMutex);
127+
stopKillingCondition.wait_for(lock, std::chrono::seconds(10));
128+
sendSignalToProcesses(SIGKILL);
129+
}
130+
131+
void sendSignalToProcesses(int signal) {
132+
std::unique_lock<std::mutex> lock(spawnedProcessesMutex);
133+
134+
for (pid_t pid: spawnedProcesses) {
135+
// We are killing the whole process group here, this depends on us spawning each process in its own group earlier
136+
::kill(-pid, signal);
137+
}
138+
}
139+
109140
public:
110141
LaneBasedExecutionQueue(BuildExecutionQueueDelegate& delegate,
111142
unsigned numLanes)
@@ -120,20 +151,43 @@ class LaneBasedExecutionQueue : public BuildExecutionQueue {
120151

121152
virtual ~LaneBasedExecutionQueue() {
122153
// Shut down the lanes.
123-
for (unsigned i = 0; i != numLanes; ++i) {
124-
addJob({});
125-
}
154+
cancelled = true;
155+
readyJobsCondition.notify_all();
156+
126157
for (unsigned i = 0; i != numLanes; ++i) {
127158
lanes[i]->join();
128159
}
160+
161+
if (killAfterTimeoutThread) {
162+
stopKillingCondition.notify_all();
163+
killAfterTimeoutThread->join();
164+
}
129165
}
130166

131167
virtual void addJob(QueueJob job) override {
168+
if (cancelled) {
169+
// FIXME: We should eventually raise an error here as new work should not be enqueued after cancellation
170+
return;
171+
}
172+
132173
std::lock_guard<std::mutex> guard(readyJobsMutex);
133174
readyJobs.push_back(job);
134175
readyJobsCondition.notify_one();
135176
}
136177

178+
virtual void cancelAllJobs() override {
179+
auto wasAlreadyCancelled = cancelled.exchange(true);
180+
// If we were already cancelled, do nothing.
181+
if (wasAlreadyCancelled) {
182+
return;
183+
}
184+
185+
readyJobsCondition.notify_all();
186+
187+
sendSignalToProcesses(SIGINT);
188+
killAfterTimeoutThread = llvm::make_unique<std::thread>(&LaneBasedExecutionQueue::killAfterTimeout, this);
189+
}
190+
137191
virtual bool
138192
executeProcess(QueueJobContext* opaqueContext,
139193
ArrayRef<StringRef> commandLine,
@@ -274,19 +328,24 @@ class LaneBasedExecutionQueue : public BuildExecutionQueue {
274328
}
275329

276330
// Spawn the command.
277-
//
278-
// FIXME: Need to track spawned processes for the purposes of cancellation.
279-
280331
pid_t pid;
281-
if (posix_spawn(&pid, args[0], /*file_actions=*/&fileActions,
282-
/*attrp=*/&attributes, const_cast<char**>(args.data()),
283-
envp) != 0) {
284-
getDelegate().commandProcessHadError(
285-
context.job.getForCommand(), handle,
286-
Twine("unable to spawn process (") + strerror(errno) + ")");
287-
getDelegate().commandProcessFinished(context.job.getForCommand(), handle,
288-
-1);
289-
return false;
332+
{
333+
// We need to hold the spawn processes lock when we spawn, to ensure that
334+
// we don't create a process in between when we are cancelled.
335+
std::lock_guard<std::mutex> guard(spawnedProcessesMutex);
336+
337+
if (posix_spawn(&pid, args[0], /*file_actions=*/&fileActions,
338+
/*attrp=*/&attributes, const_cast<char**>(args.data()),
339+
envp) != 0) {
340+
getDelegate().commandProcessHadError(
341+
context.job.getForCommand(), handle,
342+
Twine("unable to spawn process (") + strerror(errno) + ")");
343+
getDelegate().commandProcessFinished(context.job.getForCommand(), handle,
344+
-1);
345+
return false;
346+
}
347+
348+
spawnedProcesses.insert(pid);
290349
}
291350

292351
posix_spawn_file_actions_destroy(&fileActions);
@@ -323,6 +382,13 @@ class LaneBasedExecutionQueue : public BuildExecutionQueue {
323382
int status, result = waitpid(pid, &status, 0);
324383
while (result == -1 && errno == EINTR)
325384
result = waitpid(pid, &status, 0);
385+
386+
// Update the set of spawned processes.
387+
{
388+
std::lock_guard<std::mutex> guard(spawnedProcessesMutex);
389+
spawnedProcesses.erase(pid);
390+
}
391+
326392
if (result == -1) {
327393
getDelegate().commandProcessHadError(
328394
context.job.getForCommand(), handle,

lib/BuildSystem/SwiftTools.cpp

+6
Original file line numberDiff line numberDiff line change
@@ -106,6 +106,12 @@ class SwiftGetVersionCommand : public Command {
106106

107107
virtual void inputsAvailable(BuildSystemCommandInterface& bsci,
108108
core::Task* task) override {
109+
// If the build should cancel, do nothing.
110+
if (bsci.getDelegate().isCancelled()) {
111+
bsci.taskIsComplete(task, BuildValue::makeSkippedCommand());
112+
return;
113+
}
114+
109115
// Dispatch a task to query the compiler version.
110116
auto fn = [this, &bsci=bsci, task=task](QueueJobContext* context) {
111117
// Suppress static analyzer false positive on generalized lambda capture

0 commit comments

Comments
 (0)