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

@@ -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",

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<char*>(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<char*>(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

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 <typename T>
   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

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

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;
 

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<int>(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) {

toolchain/parser/parser2.h

@@ -28,6 +28,8 @@ class Parser2 {
   }
 
  private:
+  class PrettyStackTraceParseState;
+
   // Used to track state on state_stack_.
   struct StateStackEntry {
     // The state.