Validate MODULE_DEPENDENCIES using tracing from Clang #635
Conversation
This currently only checks Clang compilations with modules because there were some issues with introducing a new task action for non-modular Clang compilations. rdar://150313957
| @@ -181,4 +223,9 @@ public struct ModuleDependenciesContext: Sendable, SerializableCodable { | |||
| return Diagnostic.FixIt(sourceRange: sourceRange, newText: newText) | |||
| } | |||
| } | |||
|
|
|||
| func signatureData() -> String { | |||
There was a problem hiding this comment.
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.
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.
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.
| @@ -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.
Should have the Path suffix like fileNameMapPath
| 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.
Does clang create or append to this particular trace file just like the ld trace we looked at?
| if let moduleDependenciesContext, let traceFile, lastResult == .succeeded { | ||
| // 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.
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.
Tests/SWBBuildSystemTests/DependencyVerificationBuildOperationTests.swift
Outdated
Show resolved
Hide resolved
| @@ -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.
Nit but typically we'd make these two let and have else set them to nil, just like what you did with let traceFile: Path? in CCompiler.swift
| @@ -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.
If clang doesn't have the feature, traceFile will be nil here, so we'll skip checking altogether, even if the user requested VALIDATE_MODULE_DEPENDENCIES. The Swift version on the other hand emits an error if VALIDATE_MODULE_DEPENDENCIES is set but the toolchain doesn't have the required support, mainly so that the test can check for that when it's running against older toolchains.
This currently only checks Clang compilations with modules because there were some issues with introducing a new task action for non-modular Clang compilations.
rdar://150313957