[5.4] fix silent crash when output from compiler is too large #3482
Conversation
motivation: silent crashes, confused uers changes: * change the order in swiftCompilerOutputParser such that print the stdout happens before any parsing attenpts are made * contraint the parsing attempts to output < 10k, since otherwise they struggle to complete rdar://77558631
|
@swift-ci please test |
tomerd
changed the title
|
|
||
| // next we want to try and scoop out any errors from the output (if reasonable size, otherwise this will be very slow), | ||
| // so they can later be passed to the advice provider in case of failure. | ||
| if output.utf8.count < 1024 * 10 { |
There was a problem hiding this comment.
Is the 8 MB limit calculated in some way, or is it arbitrarily chosen? I don't have an opinion either way. I'm just curious.
There was a problem hiding this comment.
base don experiments with SIL output in crashes. @abertelrud is working on a separate PR to fix this parsing logic and this will be removed then. this PR is focused on "damage control" given that this caused issues in 5.4
| // next we want to try and scoop out any errors from the output (if reasonable size, otherwise this will be very slow), | ||
| // so they can later be passed to the advice provider in case of failure. | ||
| if output.utf8.count < 1024 * 10 { | ||
| let regex = try! RegEx(pattern: #".*(error:[^\n]*)\n.*"#, options: .dotMatchesLineSeparators) |
There was a problem hiding this comment.
Can this parsing of error messages be replaced by using TSCUtility.SerializedDiagnostics, which I think is more performant and less reliant on the error messages' structure/format staying unchanged?
There was a problem hiding this comment.
Ah sorry I just saw that this PR is a cherry-pick of #3478, and my comment would've been more appropriate there, or in the form of a new PR.
There was a problem hiding this comment.
@abertelrud is working on a separate PR to fix this parsing logic, this PR is focused on "damage control" given that this caused issues in 5.4
There was a problem hiding this comment.
Using TSCUtility.SerializedDiagnostics is a great idea, and we should definitely adopt that across SwiftPM. That will be a larger effort than fixing this, also, linker diagnostics are unfortunately not yet part of the serialized diagnostics, so it wouldn't help this particular case. They are very source-code focused and currently (AFAIK) only emitted by Clang and Swiftc. But it's definitely where we should move to in SwiftPM, I believe, with tool-specific adapters that favor serialized diagnostics where supported, and that fall back on parsing stderr where we have to.
brings #3478 to 5.4