Skip to content

Commit 56a49ca

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 f62e6da commit 56a49ca

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
@@ -537,7 +537,7 @@ static bool emitReferenceDependencies(CompilerInstance &Instance,
537537
});
538538
}
539539

540-
static void emitReferenceDependenciesForAllPrimaryInputsIfNeeded(
540+
static void emitSwiftdepsForAllPrimaryInputsIfNeeded(
541541
CompilerInstance &Instance) {
542542
const auto &Invocation = Instance.getInvocation();
543543
if (Invocation.getFrontendOptions()
@@ -547,6 +547,22 @@ static void emitReferenceDependenciesForAllPrimaryInputsIfNeeded(
547547
SourceLoc(), diag::emit_reference_dependencies_without_primary_file);
548548
return;
549549
}
550+
551+
// Do not write out swiftdeps for any primaries if we've encountered an
552+
// error. Without this, the driver will attempt to integrate swiftdeps
553+
// from broken swift files. One way this could go wrong is if a primary that
554+
// fails to build in an early wave has dependents in a later wave. The
555+
// driver will not schedule those later dependents after this batch exits,
556+
// so they will have no opportunity to bring their swiftdeps files up to
557+
// date. With this early exit, the driver sees the same priors in the
558+
// swiftdeps files from before errors were introduced into the batch, and
559+
// integration therefore always hops from "known good" to "known good" states.
560+
//
561+
// FIXME: It seems more appropriate for the driver to notice the early-exit
562+
// and react by always enqueuing the jobs it dropped in the other waves.
563+
if (Instance.getDiags().hadAnyError())
564+
return;
565+
550566
for (auto *SF : Instance.getPrimarySourceFiles()) {
551567
const std::string &referenceDependenciesFilePath =
552568
Invocation.getReferenceDependenciesFilePathForPrimary(
@@ -955,8 +971,10 @@ static void performEndOfPipelineActions(CompilerInstance &Instance) {
955971
emitIndexData(Instance);
956972
}
957973

958-
// Emit dependencies.
959-
emitReferenceDependenciesForAllPrimaryInputsIfNeeded(Instance);
974+
// Emit Swiftdeps for every file in the batch.
975+
emitSwiftdepsForAllPrimaryInputsIfNeeded(Instance);
976+
977+
// Emit Make-style dependencies.
960978
emitMakeDependenciesIfNeeded(Instance.getDiags(),
961979
Instance.getDependencyTracker(), opts);
962980

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)