-
Notifications
You must be signed in to change notification settings - Fork 105
Validate MODULE_DEPENDENCIES using tracing from Clang #635
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -432,7 +432,10 @@ public struct ClangTaskPayload: ClangModuleVerifierPayloadType, DependencyInfoEd | |
|
||
public let fileNameMapPath: Path? | ||
|
||
fileprivate init(serializedDiagnosticsPath: Path?, indexingPayload: ClangIndexingPayload?, explicitModulesPayload: ClangExplicitModulesPayload? = nil, outputObjectFilePath: Path? = nil, fileNameMapPath: Path? = nil, developerPathString: String? = nil) { | ||
public let moduleDependenciesContext: ModuleDependenciesContext? | ||
public let traceFile: Path? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should have the |
||
|
||
fileprivate init(serializedDiagnosticsPath: Path?, indexingPayload: ClangIndexingPayload?, explicitModulesPayload: ClangExplicitModulesPayload? = nil, outputObjectFilePath: Path? = nil, fileNameMapPath: Path? = nil, developerPathString: String? = nil, moduleDependenciesContext: ModuleDependenciesContext? = nil, traceFile: Path? = nil) { | ||
if let developerPathString, explicitModulesPayload == nil { | ||
self.dependencyInfoEditPayload = .init(removablePaths: [], removableBasenames: [], developerPath: Path(developerPathString)) | ||
} else { | ||
|
@@ -443,27 +446,33 @@ public struct ClangTaskPayload: ClangModuleVerifierPayloadType, DependencyInfoEd | |
self.explicitModulesPayload = explicitModulesPayload | ||
self.outputObjectFilePath = outputObjectFilePath | ||
self.fileNameMapPath = fileNameMapPath | ||
self.moduleDependenciesContext = moduleDependenciesContext | ||
self.traceFile = traceFile | ||
} | ||
|
||
public func serialize<T: Serializer>(to serializer: T) { | ||
serializer.serializeAggregate(6) { | ||
serializer.serializeAggregate(8) { | ||
serializer.serialize(serializedDiagnosticsPath) | ||
serializer.serialize(indexingPayload) | ||
serializer.serialize(explicitModulesPayload) | ||
serializer.serialize(outputObjectFilePath) | ||
serializer.serialize(fileNameMapPath) | ||
serializer.serialize(dependencyInfoEditPayload) | ||
serializer.serialize(moduleDependenciesContext) | ||
serializer.serialize(traceFile) | ||
} | ||
} | ||
|
||
public init(from deserializer: any Deserializer) throws { | ||
try deserializer.beginAggregate(6) | ||
try deserializer.beginAggregate(8) | ||
self.serializedDiagnosticsPath = try deserializer.deserialize() | ||
self.indexingPayload = try deserializer.deserialize() | ||
self.explicitModulesPayload = try deserializer.deserialize() | ||
self.outputObjectFilePath = try deserializer.deserialize() | ||
self.fileNameMapPath = try deserializer.deserialize() | ||
self.dependencyInfoEditPayload = try deserializer.deserialize() | ||
self.moduleDependenciesContext = try deserializer.deserialize() | ||
self.traceFile = try deserializer.deserialize() | ||
} | ||
} | ||
|
||
|
@@ -1156,6 +1165,22 @@ public class ClangCompilerSpec : CompilerSpec, SpecIdentifierType, GCCCompatible | |
dependencyData = nil | ||
} | ||
|
||
let moduleDependenciesContext = cbc.producer.moduleDependenciesContext | ||
let traceFile: Path? | ||
if clangInfo?.hasFeature("print-headers-direct-per-file") ?? false, | ||
(moduleDependenciesContext?.validate ?? .defaultValue) != .no { | ||
let file = Path(outputNode.path.str + ".trace.json") | ||
commandLine += [ | ||
"-Xclang", "-header-include-file", | ||
"-Xclang", file.str, | ||
"-Xclang", "-header-include-filtering=direct-per-file", | ||
"-Xclang", "-header-include-format=json" | ||
] | ||
traceFile = file | ||
} else { | ||
traceFile = nil | ||
} | ||
|
||
// Add the diagnostics serialization flag. We currently place the diagnostics file right next to the output object file. | ||
let diagFilePath: Path? | ||
if let serializedDiagnosticsOptions = self.serializedDiagnosticsOptions(scope: cbc.scope, outputPath: outputNode.path) { | ||
|
@@ -1266,7 +1291,9 @@ public class ClangCompilerSpec : CompilerSpec, SpecIdentifierType, GCCCompatible | |
explicitModulesPayload: explicitModulesPayload, | ||
outputObjectFilePath: shouldGenerateRemarks ? outputNode.path : nil, | ||
fileNameMapPath: verifierPayload?.fileNameMapPath, | ||
developerPathString: recordSystemHeaderDepsOutsideSysroot ? cbc.scope.evaluate(BuiltinMacros.DEVELOPER_DIR).str : nil | ||
developerPathString: recordSystemHeaderDepsOutsideSysroot ? cbc.scope.evaluate(BuiltinMacros.DEVELOPER_DIR).str : nil, | ||
moduleDependenciesContext: moduleDependenciesContext, | ||
traceFile: traceFile | ||
) | ||
|
||
var inputNodes: [any PlannedNode] = inputDeps.map { delegate.createNode($0) } | ||
|
@@ -1316,6 +1343,10 @@ public class ClangCompilerSpec : CompilerSpec, SpecIdentifierType, GCCCompatible | |
extraInputs = [] | ||
} | ||
|
||
if let moduleDependenciesContext { | ||
additionalSignatureData += moduleDependenciesContext.signatureData() | ||
} | ||
|
||
// Finally, create the task. | ||
delegate.createTask(type: self, dependencyData: dependencyData, payload: payload, ruleInfo: ruleInfo, additionalSignatureData: additionalSignatureData, commandLine: commandLine, additionalOutput: additionalOutput, environment: environmentBindings, workingDirectory: compilerWorkingDirectory(cbc), inputs: inputNodes + extraInputs, outputs: [outputNode], action: action ?? delegate.taskActionCreationDelegate.createDeferredExecutionTaskActionIfRequested(userPreferences: cbc.producer.userPreferences), execDescription: resolveExecutionDescription(cbc, delegate), enableSandboxing: enableSandboxing, additionalTaskOrderingOptions: [.compilationForIndexableSourceFile], usesExecutionInputs: usesExecutionInputs, showEnvironment: true, priority: .preferred) | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -248,6 +248,20 @@ public final class ClangCompileTaskAction: TaskAction, BuildValueValidatingTaskA | |
casDBs = nil | ||
} | ||
|
||
// Check if verifying dependencies from trace data is enabled. | ||
var traceFile: Path? = nil | ||
var moduleDependenciesContext: ModuleDependenciesContext? = nil | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit but typically we'd make these two |
||
if let payload = task.payload as? ClangTaskPayload { | ||
traceFile = payload.traceFile | ||
moduleDependenciesContext = payload.moduleDependenciesContext | ||
} | ||
if let traceFile { | ||
// Remove the trace output file if it already exists. | ||
if executionDelegate.fs.exists(traceFile) { | ||
try executionDelegate.fs.remove(traceFile) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does clang create or append to this particular trace file just like the ld trace we looked at? |
||
} | ||
} | ||
|
||
var lastResult: CommandResult? = nil | ||
for command in dependencyInfo.commands { | ||
if let casDBs { | ||
|
@@ -304,6 +318,24 @@ public final class ClangCompileTaskAction: TaskAction, BuildValueValidatingTaskA | |
return lastResult ?? .failed | ||
} | ||
} | ||
|
||
if let moduleDependenciesContext, let traceFile, lastResult == .succeeded { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If clang doesn't have the feature, traceFile will be nil here, so we'll skip checking altogether, even if the user requested |
||
// Verify the dependencies from the trace data. | ||
let fs = executionDelegate.fs | ||
let traceData = try JSONDecoder().decode(Array<TraceData>.self, from: fs.readMemoryMapped(traceFile)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why use readMemoryMapped here? I think we don't have a lot of clarity around when to use read vs readMemoryMapped and should probably default to read. |
||
|
||
var allFiles = Set<Path>() | ||
traceData.forEach { allFiles.formUnion(Set($0.includes)) } | ||
let diagnostics = moduleDependenciesContext.makeDiagnostics(files: Array(allFiles)) | ||
for diagnostic in diagnostics { | ||
outputDelegate.emit(diagnostic) | ||
} | ||
|
||
if diagnostics.contains(where: { $0.behavior == .error }) { | ||
return .failed | ||
} | ||
} | ||
|
||
return lastResult ?? .failed | ||
} catch { | ||
outputDelegate.emitError("\(error)") | ||
|
@@ -431,3 +463,10 @@ public final class ClangCompileTaskAction: TaskAction, BuildValueValidatingTaskA | |
) | ||
} | ||
} | ||
|
||
// Results from tracing header includes with "direct-per-file" filtering. | ||
// This is used to validate dependencies. | ||
fileprivate struct TraceData: Decodable { | ||
let source: Path | ||
let includes: [Path] | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this quite belongs here. For one, the type is serializable/codable, so clients can serialize it to get a signature blob. And two, what actually goes into a signature depends on each specific client task, so there can't be a central definition like this. That said, I think the clang task signature actually should contain the full value of ModuleDependenciesContext (and probably the entire task Payload tbh? @owenv is that not already the case?) instead of just validate+names, because things like access level and fixit context do influence the diagnostics the clang task emits, so we'd want to re-run when the user changes any of those to avoid ending up with stale diagnostics that give you incorrect fixits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably not the entire payload because it's fairly large and a lot of the content doesn't need to be included because it's redundant with the command line. But yeah, I would just serialize ModuleDependenciesContext to form a signature for it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, that means that my Swift change is incorrect for incremental builds, since it doesn't add this context to its signature. Going to fix that.