From 8c354ca23247ff5c91ebdb7ce3ec8af8bc96e34d Mon Sep 17 00:00:00 2001 From: Jon Ross-Perkins Date: Fri, 4 Nov 2022 14:59:39 -0700 Subject: [PATCH] Switch to PrettyStackTrace for CHECK/FATAL (#2373) At present, CHECK/FATAL print their own stack trace. This switches to just using std::abort for the stack trace, as well as the CHECK printing more completely. This has a few consequences: 1) I'm now buffering the FATAL strings in order to print it later. 2) We now print the bug report message and program arguments on failure. This is part of pretty printing and was elided before. 3) We can now have pretty printing on FATAL, e.g. to show the stacks we're building in the parser. --- common/BUILD | 5 +++- common/check_internal.cpp | 34 +++++++++++++++++++++++ common/check_internal.h | 44 ++++++++++-------------------- common/check_test.cpp | 18 ++++++++---- toolchain/lexer/tokenized_buffer.h | 2 ++ toolchain/parser/parser2.cpp | 25 +++++++++++++++++ toolchain/parser/parser2.h | 2 ++ 7 files changed, 94 insertions(+), 36 deletions(-) create mode 100644 common/check_internal.cpp diff --git a/common/BUILD b/common/BUILD index 21bcdea218a0..25984d531c24 100644 --- a/common/BUILD +++ b/common/BUILD @@ -14,7 +14,10 @@ cc_library( cc_library( name = "check", - srcs = ["check_internal.h"], + srcs = [ + "check_internal.cpp", + "check_internal.h", + ], hdrs = ["check.h"], deps = [ "@llvm-project//llvm:Support", diff --git a/common/check_internal.cpp b/common/check_internal.cpp new file mode 100644 index 000000000000..c00d9fd23265 --- /dev/null +++ b/common/check_internal.cpp @@ -0,0 +1,34 @@ +// Part of the Carbon Language project, under the Apache License v2.0 with LLVM +// Exceptions. See /LICENSE for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception + +#include "common/check_internal.h" + +namespace Carbon::Internal { + +// Prints the buffered message. +auto PrintAfterStackTrace(void* str) -> void { + llvm::errs() << reinterpret_cast(str); +} + +ExitingStream::~ExitingStream() { + llvm_unreachable( + "Exiting streams should only be constructed by check.h macros that " + "ensure the special operator| exits the program prior to their " + "destruction!"); +} + +auto ExitingStream::Done() -> void { + buffer_ << "\n"; + // Register another signal handler to print the buffered message. This is + // because we want it at the bottom of output, after LLVM's builtin stack + // output, rather than the top. + llvm::sys::AddSignalHandler(PrintAfterStackTrace, + const_cast(buffer_str_.c_str())); + // It's useful to exit the program with `std::abort()` for integration with + // debuggers and other tools. We also assume LLVM's exit handling is + // installed, which will stack trace on `std::abort()`. + std::abort(); +} + +} // namespace Carbon::Internal diff --git a/common/check_internal.h b/common/check_internal.h index 7d01e751447d..7afa12172433 100644 --- a/common/check_internal.h +++ b/common/check_internal.h @@ -25,21 +25,12 @@ class ExitingStream { // Internal type used in macros to dispatch to the `operator|` overload. struct Helper {}; - ExitingStream() { - // Start all messages with a stack trace. Printing this first ensures the - // error message is the last thing written which makes it easier to find. It - // also helps ensure we report the most useful stack trace and location - // information. - llvm::errs() << "Stack trace:\n"; - llvm::sys::PrintStackTrace(llvm::errs()); - } + ExitingStream() + // Prefix the buffer with the current bug report message. + : buffer_(buffer_str_) {} - [[noreturn]] ~ExitingStream() { - llvm_unreachable( - "Exiting streams should only be constructed by check.h macros that " - "ensure the special operator| exits the program prior to their " - "destruction!"); - } + // Never called. + [[noreturn]] ~ExitingStream(); // If the bool cast occurs, it's because the condition is false. This supports // && short-circuiting the creation of ExitingStream. @@ -49,10 +40,10 @@ class ExitingStream { template auto operator<<(const T& message) -> ExitingStream& { if (separator_) { - llvm::errs() << ": "; + buffer_ << ": "; separator_ = false; } - llvm::errs() << message; + buffer_ << message; return *this; } @@ -64,24 +55,19 @@ class ExitingStream { // Low-precedence binary operator overload used in check.h macros to flush the // output and exit the program. We do this in a binary operator rather than // the destructor to ensure good debug info and backtraces for errors. - [[noreturn]] friend auto operator|(Helper /*helper*/, - ExitingStream& /*rhs*/) { - // Finish with a newline. - llvm::errs() << "\n"; - // We assume LLVM's exit handling is installed, which will stack trace on - // `std::abort()`. We print a more user friendly stack trace on - // construction, but it is still useful to exit the program with - // `std::abort()` for integration with debuggers and other tools. We also - // want to do any pending cleanups. So we replicate the signal handling here - // and unregister LLVM's handlers right before we abort. - llvm::sys::RunInterruptHandlers(); - llvm::sys::unregisterHandlers(); - std::abort(); + [[noreturn]] friend auto operator|(Helper /*helper*/, ExitingStream& stream) + -> void { + stream.Done(); } private: + [[noreturn]] auto Done() -> void; + // Whether a separator should be printed if << is used again. bool separator_ = false; + + std::string buffer_str_; + llvm::raw_string_ostream buffer_; }; } // namespace Carbon::Internal diff --git a/common/check_test.cpp b/common/check_test.cpp index c6853150752c..f51b39538e40 100644 --- a/common/check_test.cpp +++ b/common/check_test.cpp @@ -13,9 +13,11 @@ TEST(CheckTest, CheckTrue) { CARBON_CHECK(true); } TEST(CheckTest, CheckFalse) { ASSERT_DEATH({ CARBON_CHECK(false); }, - "Stack trace:\n" - "(.|\n)+\n" - "CHECK failure at common/check_test.cpp:\\d+: false\n"); + "\nCHECK failure at common/check_test.cpp:\\d+: false\n"); +} + +TEST(CheckTest, CheckFalseHasStackDump) { + ASSERT_DEATH({ CARBON_CHECK(false); }, "\nStack dump:\n"); } TEST(CheckTest, CheckTrueCallbackNotUsed) { @@ -30,7 +32,7 @@ TEST(CheckTest, CheckTrueCallbackNotUsed) { TEST(CheckTest, CheckFalseMessage) { ASSERT_DEATH({ CARBON_CHECK(false) << "msg"; }, - "CHECK failure at common/check_test.cpp:.+: false: msg\n"); + "\nCHECK failure at common/check_test.cpp:.+: false: msg\n"); } TEST(CheckTest, CheckOutputForms) { @@ -42,14 +44,18 @@ TEST(CheckTest, CheckOutputForms) { TEST(CheckTest, Fatal) { ASSERT_DEATH({ CARBON_FATAL() << "msg"; }, - "FATAL failure at common/check_test.cpp:.+: msg\n"); + "\nFATAL failure at common/check_test.cpp:.+: msg\n"); +} + +TEST(CheckTest, FatalHasStackDump) { + ASSERT_DEATH({ CARBON_FATAL() << "msg"; }, "\nStack dump:\n"); } auto FatalNoReturnRequired() -> int { CARBON_FATAL() << "msg"; } TEST(ErrorTest, FatalNoReturnRequired) { ASSERT_DEATH({ FatalNoReturnRequired(); }, - "FATAL failure at common/check_test.cpp:.+: msg\n"); + "\nFATAL failure at common/check_test.cpp:.+: msg\n"); } } // namespace diff --git a/toolchain/lexer/tokenized_buffer.h b/toolchain/lexer/tokenized_buffer.h index 2991b690da89..70164535af51 100644 --- a/toolchain/lexer/tokenized_buffer.h +++ b/toolchain/lexer/tokenized_buffer.h @@ -68,6 +68,8 @@ class TokenizedBufferToken { return lhs.index_ >= rhs.index_; } + auto Print(llvm::raw_ostream& output) const -> void { output << index_; } + private: friend TokenizedBuffer; diff --git a/toolchain/parser/parser2.cpp b/toolchain/parser/parser2.cpp index 6f700ad5ad83..f24115debd89 100644 --- a/toolchain/parser/parser2.cpp +++ b/toolchain/parser/parser2.cpp @@ -9,6 +9,7 @@ #include "common/check.h" #include "llvm/ADT/Optional.h" +#include "llvm/Support/PrettyStackTrace.h" #include "toolchain/lexer/token_kind.h" #include "toolchain/lexer/tokenized_buffer.h" #include "toolchain/parser/parse_node_kind.h" @@ -16,6 +17,26 @@ namespace Carbon { +class Parser2::PrettyStackTraceParseState : public llvm::PrettyStackTraceEntry { + public: + explicit PrettyStackTraceParseState(const Parser2* parser) + : parser_(parser) {} + ~PrettyStackTraceParseState() override = default; + + auto print(llvm::raw_ostream& output) const -> void override { + output << "Parser stack:\n"; + for (int i = 0; i < static_cast(parser_->state_stack_.size()); ++i) { + const auto& entry = parser_->state_stack_[i]; + output << "\t" << i << ".\t" << entry.state << " @ " << entry.start_token + << ":" << parser_->tokens_.GetKind(entry.start_token).Name() + << "\n"; + } + } + + private: + const Parser2* parser_; +}; + Parser2::Parser2(ParseTree& tree_arg, TokenizedBuffer& tokens_arg, TokenDiagnosticEmitter& emitter) : tree_(tree_arg), @@ -71,6 +92,10 @@ auto Parser2::ConsumeIf(TokenKind kind) } auto Parser2::Parse() -> void { +#ifndef NDEBUG + PrettyStackTraceParseState pretty_stack(this); +#endif + PushState(ParserState::Declaration()); while (!state_stack_.empty()) { switch (state_stack_.back().state) { diff --git a/toolchain/parser/parser2.h b/toolchain/parser/parser2.h index 67452ce2cc6a..4c761d362f95 100644 --- a/toolchain/parser/parser2.h +++ b/toolchain/parser/parser2.h @@ -28,6 +28,8 @@ class Parser2 { } private: + class PrettyStackTraceParseState; + // Used to track state on state_stack_. struct StateStackEntry { // The state.