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.

Sign up for GitHub

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
+27 −0
Merged

Flush pending diagnostics on crash. #4337

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5319c59
Flush pending diagnostics on crash.
jonmeow Sep 24, 2024
4dca0df
stream_errors support
jonmeow Sep 25, 2024
f90f267
Minor edits
jonmeow Sep 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions toolchain/diagnostics/diagnostic_consumer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,8 @@ class StreamDiagnosticConsumer : public DiagnosticConsumer {
auto HandleDiagnostic(Diagnostic diagnostic) -> void override;
auto Flush() -> void override { stream_->flush(); }

auto set_stream(llvm::raw_ostream* stream) -> void { stream_ = stream; }

private:
auto Print(const DiagnosticMessage& message, llvm::StringRef prefix) -> void;

Expand Down
1 change: 1 addition & 0 deletions toolchain/driver/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@ cc_library(
"//common:ostream",
"//common:version",
"//common:vlog",
"//toolchain/base:pretty_stack_trace_function",
"//toolchain/base:value_store",
"//toolchain/check",
"//toolchain/codegen",
Expand Down
17 changes: 17 additions & 0 deletions toolchain/driver/compile_subcommand.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

#include "common/vlog.h"
#include "llvm/ADT/ScopeExit.h"
#include "toolchain/base/pretty_stack_trace_function.h"
#include "toolchain/check/check.h"
#include "toolchain/codegen/codegen.h"
#include "toolchain/diagnostics/sorting_diagnostic_consumer.h"
Expand Down Expand Up @@ -485,6 +486,10 @@ class CompilationUnit {
consumer_->Flush();
}

// Flushes diagnostics, specifically as part of generating stack trace
// information.
auto FlushForStackTrace() -> void { consumer_->Flush(); }

auto input_filename() -> llvm::StringRef { return input_filename_; }
auto success() -> bool { return success_; }
auto has_source() -> bool { return source_.has_value(); }
Expand Down Expand Up @@ -660,6 +665,18 @@ auto CompileSubcommand::Run(DriverEnv& driver_env) -> DriverResult {
stream_consumer.Flush();
});

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 •
edited
Loading

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.

stream_consumer.set_stream(&out);
for (auto& unit : units) {
unit->FlushForStackTrace();
}
stream_consumer.Flush();
stream_consumer.set_stream(&driver_env.error_stream);
});

// Returns a DriverResult object. Called whenever Compile returns.
auto make_result = [&]() {
DriverResult result = {.success = true};
Expand Down
Loading