Skip to content

[DNM, Incremental] Fix problem of priors injecting nodes for removed inputs into the graph. #703

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.

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

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,15 @@ extension ModuleDependencyGraph {
invalidatedNodes.formUnion(self.integrateExternal(.known(fed)))
}
}

/// If the priors were read from an invocation containing a subsuequently removed input,
/// the nodes defining decls from that input must be culled.
fileprivate func isForRemovedInput(_ node: Node) -> Bool {
guard let dependencySource = node.dependencySource else {
return false
}
return inputDependencySourceMap.inputIfKnown(for: dependencySource) == nil
}
}

// MARK: - Scheduling the first wave
Expand Down Expand Up @@ -528,7 +537,9 @@ extension ModuleDependencyGraph {
private var identifiers: [String] = [""]
private var currentDefKey: DependencyKey? = nil
private var nodeUses: [(DependencyKey, Int)] = []
public private(set) var allNodes: [Node] = []
/// Use nodes in def-use links are seliarized by index, so keep an array of all nodes read.
/// But, don't keep nodes that are for reomved inputs.
public private(set) var potentiallyUsedNodes: [Node?] = []

init?(_ info: IncrementalCompilationState.IncrementalDependencyAndInputSetup) {
self.fileSystem = info.fileSystem
Expand All @@ -541,8 +552,11 @@ extension ModuleDependencyGraph {

func finalizeGraph() -> ModuleDependencyGraph {
for (dependencyKey, useID) in self.nodeUses {
guard let use = self.potentiallyUsedNodes[useID] else {
continue
}
let isNewUse = self.graph.nodeFinder
.record(def: dependencyKey, use: self.allNodes[useID])
.record(def: dependencyKey, use: use)
assert(isNewUse, "Duplicate use def-use arc in graph?")
}
return self.graph
Expand All @@ -561,7 +575,12 @@ extension ModuleDependencyGraph {
mutating func didExitBlock() throws {}

private mutating func finalize(node newNode: Node) {
self.allNodes.append(newNode)
if graph.isForRemovedInput(newNode) {
// Preserve the mapping of Int to Node for reconstructing def-use links.
self.potentiallyUsedNodes.append(nil)
return
}
self.potentiallyUsedNodes.append(newNode)
let oldNode = self.graph.nodeFinder.insert(newNode)
assert(oldNode == nil,
"Integrated the same node twice: \(oldNode!), \(newNode)")
Expand Down
16 changes: 0 additions & 16 deletions Tests/SwiftDriverTests/IncrementalCompilationTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -796,22 +796,6 @@ extension IncrementalCompilationTests {

graph.verifyGraph()
if removeInputFromInvocation {
if afterRestoringBadPriors {
// FIXME: Fix the driver
// If you incrementally compile with a.swift and b.swift,
// at the end, the driver saves a serialized `ModuleDependencyGraph`
// contains nodes for declarations defined in both files.
// If you then later remove b.swift and recompile, the driver will
// see that a file was removed (via comparisons with the saved `BuildRecord`
// and will delete the saved priors. However, if for some reason the
// saved priors are not deleted, the driver will read saved priors
// containing entries for the deleted file. This test simulates that
// condition by restoring the deleted priors. The driver ought to be fixed
// to cull any entries for removed files from the deserialized priors.
print("*** WARNING: skipping checks, driver fails to cleaned out the graph ***",
to: &stderrStream); stderrStream.flush()
return graph
}
graph.ensureOmits(sourceBasenameWithoutExt: removedInput)
graph.ensureOmits(name: topLevelName)
}
Expand Down