-
Notifications
You must be signed in to change notification settings - Fork 90
[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 | ||
} | ||
} | ||
|
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?