Skip to content
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

Flush pending diagnostics on crash. #4337

Merged
merged 3 commits into from
Sep 25, 2024

Conversation

jonmeow
Copy link
Contributor

@jonmeow jonmeow commented Sep 24, 2024

This risks diagnsotic formatting crashing, but I think we more frequently see cases where it'd be interesting to know what diagnostics were being delayed as part of the default sorting.

@josh11b josh11b enabled auto-merge September 24, 2024 17:46
PrettyStackTraceFunction flush_on_crash([&](llvm::raw_ostream& out) {
// When crashing, print pending diagnostics to the crash stream.
// TODO: Eventually we'll want to limit the count.
out << "Pending diagnostics\n";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we skip printing this if diagnostic streaming is enabled? (And maybe if there are no diagnostics?)

Copy link
Contributor Author

@jonmeow jonmeow Sep 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PrettyStackTrace requires a stack allocation. A unique_ptr sort of works, but only if you immediately allocate it (or never allocate it) so that [construction and] destruction order is still stack-like. I believe it's infeasible to skip printing here (without removing the PrettyStackTraceFunction altogether): you end up with a number and malformatted stack trace output. We can print something different, but we cannot choose to skip printing based on later state.

I've added a slightly different label if streaming is enabled. We could skip both printing and flushing for streamed errors (i.e., just don't allocate a PrettyStackTraceFunction in this case), but my leaning would be that it's potentially helpful and more consistent to flush in both modes.

@josh11b josh11b added this pull request to the merge queue Sep 24, 2024
@josh11b josh11b removed this pull request from the merge queue due to a manual request Sep 24, 2024
@jonmeow jonmeow requested a review from zygoloid September 25, 2024 17:45
@josh11b josh11b added this pull request to the merge queue Sep 25, 2024
Merged via the queue into carbon-language:trunk with commit ee38363 Sep 25, 2024
8 checks passed
@jonmeow jonmeow deleted the flush-on-crash branch September 25, 2024 22:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants