-
Notifications
You must be signed in to change notification settings - Fork 97
[Explicit Module Builds] Add support for creating a reproducer when clang process crashes. #455
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
332ca30
d831b09
d1af3ae
127cb89
144bc49
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 |
---|---|---|
|
@@ -301,6 +301,19 @@ public final class ClangCompileTaskAction: TaskAction, BuildValueValidatingTaskA | |
outputDelegate.emitOutput("Failed frontend command:\n") | ||
outputDelegate.emitOutput(ByteString(encodingAsUTF8: commandString) + "\n") | ||
} | ||
|
||
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. Related to the question about serialization overhead, we should consider whether this should be opt-in or opt-out. I'm not sure I have a strong opinion right now 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. We also have plans to add reproducer-on-error. That one should be opt-in. But crashes seem to be rare enough that generating a reproducer can be don unconditionally. Honestly, the crashes seem to be rare enough that this entire feature in its current form isn't particularly useful. The value should come from iterating on the feature. |
||
if case .some(.failed) = lastResult, case .some(.exit(.uncaughtSignal, _)) = outputDelegate.result { | ||
do { | ||
if let reproducerMessage = try clangModuleDependencyGraph.generateReproducer( | ||
forFailedDependency: dependencyInfo, | ||
libclangPath: explicitModulesPayload.libclangPath, | ||
casOptions: explicitModulesPayload.casOptions) { | ||
outputDelegate.emitOutput(ByteString(encodingAsUTF8: reproducerMessage) + "\n") | ||
} | ||
} catch { | ||
outputDelegate.error(error.localizedDescription) | ||
} | ||
} | ||
return lastResult ?? .failed | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -215,16 +215,30 @@ final public class PrecompileClangModuleTaskAction: TaskAction, BuildValueValida | |
enableStrictCASErrors: key.casOptions!.enableStrictCASErrors | ||
) | ||
} | ||
} else if result == .failed && !executionDelegate.userPreferences.enableDebugActivityLogs && !executionDelegate.emitFrontendCommandLines { | ||
let commandString = UNIXShellCommandCodec( | ||
encodingStrategy: .backslashes, | ||
encodingBehavior: .fullCommandLine | ||
).encode(commandLine) | ||
|
||
// <rdar://59354519> We need to find a way to use the generic infrastructure for displaying the command line in | ||
// the build log. | ||
outputDelegate.emitOutput("Failed frontend command:\n") | ||
outputDelegate.emitOutput(ByteString(encodingAsUTF8: commandString) + "\n") | ||
} else if result == .failed { | ||
if !executionDelegate.userPreferences.enableDebugActivityLogs && !executionDelegate.emitFrontendCommandLines { | ||
let commandString = UNIXShellCommandCodec( | ||
encodingStrategy: .backslashes, | ||
encodingBehavior: .fullCommandLine | ||
).encode(commandLine) | ||
|
||
// <rdar://59354519> We need to find a way to use the generic infrastructure for displaying the command line in | ||
// the build log. | ||
outputDelegate.emitOutput("Failed frontend command:\n") | ||
outputDelegate.emitOutput(ByteString(encodingAsUTF8: commandString) + "\n") | ||
} | ||
if case .some(.exit(.uncaughtSignal, _)) = outputDelegate.result { | ||
do { | ||
if let reproducerMessage = try clangModuleDependencyGraph.generateReproducer( | ||
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. Do you have a PR for how 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. libclang part is swiftlang/llvm-project#10577 We detect crashes in cc1 but when we generate a reproducer, we don't run those commands. So it is not likely to crash again. In fact, generating a reproducer mostly does dependency scanning that we've done already. It is possible that a successful action can crash when repeated again but I don't think I would describe it as "likely". 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. I see. This reproducer has no preprocessing steps more than the dependency scanning which is probably fine even I might still prefer an out of process tool for that, but that will need a bigger change to squeeze multiple tool execution into one task. |
||
forFailedDependency: dependencyInfo, | ||
libclangPath: key.libclangPath, | ||
casOptions: key.casOptions) { | ||
outputDelegate.emitOutput(ByteString(encodingAsUTF8: reproducerMessage) + "\n") | ||
} | ||
} catch { | ||
outputDelegate.error(error.localizedDescription) | ||
} | ||
} | ||
} | ||
return result | ||
} catch { | ||
|
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.
We should try to measure the impact of serializing an extra command line here, it should be shorter than the cc1 command but we do want to keep this data small...
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.
Is the impact in the consumed storage space? Just to know what to measure.
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've tested the storage space changes on Sparkle and here are my findings. The build directory has grown by 2.3MB (2,441,200 bytes) which is 0.5% of the build directory size (growth from 450,234,890 bytes to 452,676,090). Most of the growth (1.8MB) is in ExplicitPrecompiledModules where each .scan file (out of 647) has grown by 2.5-3KB. For the record, the smallest .pcm in my testing is 20KB, average is 366KB. I wasn't using short names or shallow directory structure, so the measurements should be fairly representative.
Is it a sufficient measurement or would you like to see more data?