Skip to content

Commit 9d06eef

Browse files
committed
Close Soundness Hole in Incremental Build
Fix a longstanding bug in the driver where the incremental build would fail to execute dependent jobs and allow errors in primaries in later waves to become buried. A simplified version of a situation that exposes this bug has been committed into the tests. Imagine two files: ``` // one.swift struct A {} // two.swift func use() { let _ = A() } ``` After a clean build, we modify one.swift as follows: ``` // one.swift struct B< {} ``` The syntax error in this file caused the baseline driver to fail the build immediately and thus we would not schedule two.swift for execution. This is incorrect! Consider correcting the syntax error in one.swift: ``` // one.swift struct B {} ``` two.swift _still_ won't be rebuilt because 1) It was never modified during any step of this process 2) The swiftdeps of two.swift have not been updated to flush out the dependency on A. rdar://72800626
1 parent d710cfc commit 9d06eef

File tree

8 files changed

+75
-3
lines changed

8 files changed

+75
-3
lines changed

lib/FrontendTool/FrontendTool.cpp

Lines changed: 21 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -617,7 +617,7 @@ constructDetailedTaskDescription(const CompilerInvocation &Invocation,
617617
Outputs};
618618
}
619619

620-
static void emitReferenceDependenciesForAllPrimaryInputsIfNeeded(
620+
static void emitSwiftdepsForAllPrimaryInputsIfNeeded(
621621
CompilerInstance &Instance) {
622622
const auto &Invocation = Instance.getInvocation();
623623
if (Invocation.getFrontendOptions()
@@ -627,6 +627,22 @@ static void emitReferenceDependenciesForAllPrimaryInputsIfNeeded(
627627
SourceLoc(), diag::emit_reference_dependencies_without_primary_file);
628628
return;
629629
}
630+
631+
// Do not write out swiftdeps for any primaries if we've encountered an
632+
// error. Without this, the driver will attempt to integrate swiftdeps
633+
// from broken swift files. One way this could go wrong is if a primary that
634+
// fails to build in an early wave has dependents in a later wave. The
635+
// driver will not schedule those later dependents after this batch exits,
636+
// so they will have no opportunity to bring their swiftdeps files up to
637+
// date. With this early exit, the driver sees the same priors in the
638+
// swiftdeps files from before errors were introduced into the batch, and
639+
// integration therefore always hops from "known good" to "known good" states.
640+
//
641+
// FIXME: It seems more appropriate for the driver to notice the early-exit
642+
// and react by always enqueuing the jobs it dropped in the other waves.
643+
if (Instance.getDiags().hadAnyError())
644+
return;
645+
630646
for (auto *SF : Instance.getPrimarySourceFiles()) {
631647
const std::string &referenceDependenciesFilePath =
632648
Invocation.getReferenceDependenciesFilePathForPrimary(
@@ -1035,8 +1051,10 @@ static void performEndOfPipelineActions(CompilerInstance &Instance) {
10351051
emitIndexData(Instance);
10361052
}
10371053

1038-
// Emit dependencies.
1039-
emitReferenceDependenciesForAllPrimaryInputsIfNeeded(Instance);
1054+
// Emit Swiftdeps for every file in the batch.
1055+
emitSwiftdepsForAllPrimaryInputsIfNeeded(Instance);
1056+
1057+
// Emit Make-style dependencies.
10401058
emitMakeDependenciesIfNeeded(Instance.getDiags(),
10411059
Instance.getDependencyTracker(), opts);
10421060

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
struct A {}
Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
struct B {}
Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
struct B< {}
Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
{
2+
"main.swift": {
3+
"object": "./main.o",
4+
"swift-dependencies": "./main.swiftdeps"
5+
},
6+
"definesA.swift": {
7+
"object": "./definesA.o",
8+
"swift-dependencies": "./definesA.swiftdeps"
9+
},
10+
"usesA.swift": {
11+
"object": "./usesA.o",
12+
"swift-dependencies": "./usesA.swiftdeps"
13+
},
14+
"": {
15+
"swift-dependencies": "./main~buildrecord.swiftdeps"
16+
}
17+
}
18+
Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
func use() {
2+
let _ = A()
3+
}
Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
// Establish baseline
2+
3+
// RUN: %empty-directory(%t)
4+
// RUN: cp %S/Inputs/cross-file-failure/* %t
5+
// RUN: cp %t/definesA{-one,}.swift
6+
// RUN: touch -t 200101010101 %t/*.swift
7+
// RUN: cd %t
8+
9+
// RUN: %target-swiftc_driver -enable-batch-mode -j2 -incremental -driver-show-incremental main.swift definesA.swift usesA.swift -module-name main -output-file-map ofm.json >&output-baseline
10+
11+
// Change one type and cause a syntax error. This should cause _both_ files to
12+
// rebuild.
13+
14+
// RUN: cp %t/definesA{-two,}.swift
15+
// RUN: touch -t 200201010101 %t/*
16+
// RUN: touch -t 200101010101 %t/*.swift
17+
// RUN: touch -t 200301010101 %t/definesA.swift
18+
19+
// RUN: not %target-swiftc_driver -enable-batch-mode -j2 -incremental -driver-show-incremental main.swift definesA.swift usesA.swift -module-name main -output-file-map ofm.json
20+
21+
// RUN: cp %t/definesA{-three,}.swift
22+
// RUN: touch -t 200401010101 %t/definesA.swift
23+
24+
// RUN: not %target-swiftc_driver -enable-batch-mode -j2 -incremental -driver-show-incremental main.swift definesA.swift usesA.swift -module-name main -output-file-map ofm.json >&output-incremental
25+
26+
// RUN: %FileCheck -check-prefix=CHECK-RECOMPILED %s --dump-input=always < %t/output-incremental
27+
28+
// CHECK-RECOMPILED: Queuing (initial): {compile: definesA.o <= definesA.swift}
29+
// CHECK-RECOMPILED: Queuing because of dependencies discovered later: {compile: usesA.o <= usesA.swift}

0 commit comments

Comments
 (0)