Debugging quality-of-life improvements (#810)

* Debugging quality-of-life improvements

- Use std::abort for `CHECK`/`FATAL` failures, which acts as a debugger breakpoint as well as automatically printing a stack trace.
- Re-enable printing continuations in `--trace` mode.
- Log the source location of each step in `--trace` mode.

Co-authored-by: Jon Meow <46229924+jonmeow@users.noreply.github.com>

common/check.h

@@ -17,19 +17,24 @@ class ExitingStream {
   LLVM_ATTRIBUTE_NORETURN ~ExitingStream() {
     // Finish with a newline.
     llvm::errs() << "\n";
-    exit(-1);
+    if (treat_as_bug) {
+      std::abort();
+    } else {
+      std::exit(-1);
+    }
   }
 
   // Indicates that initial input is in, so this is where a ": " should be added
   // before user input.
-  ExitingStream& add_separator() {
+  ExitingStream& AddSeparator() {
     separator = true;
     return *this;
   }
 
-  // Prints a stack traces.
-  ExitingStream& print_stack() {
-    llvm::sys::PrintStackTrace(llvm::errs());
+  // Indicates that the program is exiting due to a bug in the program, rather
+  // than, e.g., invalid input.
+  ExitingStream& TreatAsBug() {
+    treat_as_bug = true;
     return *this;
   }
 
@@ -51,6 +56,9 @@ class ExitingStream {
  private:
   // Whether a separator should be printed if << is used again.
   bool separator = false;
+
+  // Whether the program is exiting due to a bug.
+  bool treat_as_bug = false;
 };
 
 // Checks the given condition, and if it's false, prints a stack, streams the
@@ -60,16 +68,16 @@ class ExitingStream {
 // For example:
 //   CHECK(is_valid) << "Data is not valid!";
 #define CHECK(condition)                                                      \
-  (!(condition)) &&                                                           \
-      (Carbon::ExitingStream().print_stack() << "CHECK failure: " #condition) \
-          .add_separator()
+  (!(condition)) && (Carbon::ExitingStream() << "CHECK failure: " #condition) \
+                        .AddSeparator()                                       \
+                        .TreatAsBug()
 
 // This is similar to CHECK, but is unconditional. Writing FATAL() is clearer
 // than CHECK(false) because it avoids confusion about control flow.
 //
 // For example:
 //   FATAL() << "Unreachable!";
-#define FATAL() Carbon::ExitingStream().print_stack() << "FATAL: "
+#define FATAL() Carbon::ExitingStream().TreatAsBug() << "FATAL: "
 
 }  // namespace Carbon
 

common/check_test.cpp

@@ -11,7 +11,7 @@ namespace Carbon {
 TEST(CheckTest, CheckTrue) { CHECK(true); }
 
 TEST(CheckTest, CheckFalse) {
-  ASSERT_DEATH({ CHECK(false); }, "\nCHECK failure: false\n");
+  ASSERT_DEATH({ CHECK(false); }, "CHECK failure: false\n");
 }
 
 TEST(CheckTest, CheckTrueCallbackNotUsed) {
@@ -25,7 +25,7 @@ TEST(CheckTest, CheckTrueCallbackNotUsed) {
 }
 
 TEST(CheckTest, CheckFalseMessage) {
-  ASSERT_DEATH({ CHECK(false) << "msg"; }, "\nCHECK failure: false: msg\n");
+  ASSERT_DEATH({ CHECK(false) << "msg"; }, "CHECK failure: false: msg\n");
 }
 
 TEST(CheckTest, CheckOutputForms) {
@@ -36,13 +36,13 @@ TEST(CheckTest, CheckOutputForms) {
 }
 
 TEST(CheckTest, Fatal) {
-  ASSERT_DEATH({ FATAL() << "msg"; }, "\nFATAL: msg\n");
+  ASSERT_DEATH({ FATAL() << "msg"; }, "FATAL: msg\n");
 }
 
 auto FatalNoReturnRequired() -> int { FATAL() << "msg"; }
 
 TEST(ErrorTest, FatalNoReturnRequired) {
-  ASSERT_DEATH({ FatalNoReturnRequired(); }, "\nFATAL: msg\n");
+  ASSERT_DEATH({ FatalNoReturnRequired(); }, "FATAL: msg\n");
 }
 
 }  // namespace Carbon

executable_semantics/interpreter/BUILD

@@ -4,18 +4,31 @@
 
 package(default_visibility = ["//executable_semantics:__pkg__"])
 
+# These currently have to be a single build rule because of a dependency cycle
+# in printing.
 cc_library(
-    name = "action",
-    srcs = ["action.cpp"],
-    hdrs = ["action.h"],
+    name = "action_frame_and_value",
+    srcs = [
+        "action.cpp",
+        "frame.cpp",
+        "value.cpp",
+    ],
+    hdrs = [
+        "action.h",
+        "frame.h",
+        "value.h",
+    ],
     deps = [
+        ":address",
+        ":dictionary",
+        ":field_path",
         ":stack",
-        ":value",
         "//common:ostream",
         "//executable_semantics/ast:expression",
         "//executable_semantics/ast:function_definition",
         "//executable_semantics/ast:statement",
         "//executable_semantics/common:arena",
+        "//executable_semantics/common:error",
         "@llvm-project//llvm:Support",
     ],
 )
@@ -56,27 +69,13 @@ cc_library(
     ],
 )
 
-cc_library(
-    name = "frame",
-    srcs = ["frame.cpp"],
-    hdrs = ["frame.h"],
-    deps = [
-        ":action",
-        ":address",
-        ":dictionary",
-        ":stack",
-        "//common:ostream",
-        "@llvm-project//llvm:Support",
-    ],
-)
-
 cc_library(
     name = "heap",
     srcs = ["heap.cpp"],
     hdrs = ["heap.h"],
     deps = [
+        ":action_frame_and_value",
         ":address",
-        ":value",
         "//common:ostream",
         "@llvm-project//llvm:Support",
     ],
@@ -91,11 +90,9 @@ cc_library(
         "interpreter.h",
     ],
     deps = [
-        ":action",
+        ":action_frame_and_value",
         ":address",
-        ":frame",
         ":heap",
-        ":value",
         "//common:check",
         "//common:ostream",
         "//executable_semantics/ast:declaration",
@@ -129,20 +126,3 @@ cc_library(
         "@llvm-project//llvm:Support",
     ],
 )
-
-cc_library(
-    name = "value",
-    srcs = ["value.cpp"],
-    hdrs = ["value.h"],
-    deps = [
-        ":address",
-        ":field_path",
-        ":stack",
-        "//common:ostream",
-        "//executable_semantics/ast:function_definition",
-        "//executable_semantics/ast:statement",
-        "//executable_semantics/common:arena",
-        "//executable_semantics/common:error",
-        "@llvm-project//llvm:Support",
-    ],
-)

executable_semantics/interpreter/interpreter.cpp

@@ -362,7 +362,8 @@ auto Interpreter::StepLvalue() -> Transition {
   Ptr<Action> act = stack.Top()->todo.Top();
   Ptr<const Expression> exp = cast<LValAction>(*act).Exp();
   if (tracing_output) {
-    llvm::outs() << "--- step lvalue " << *exp << " --->\n";
+    llvm::outs() << "--- step lvalue " << *exp << " (" << exp->SourceLoc()
+                 << ") --->\n";
   }
   switch (exp->Tag()) {
     case Expression::Kind::IdentifierExpression: {
@@ -449,7 +450,8 @@ auto Interpreter::StepExp() -> Transition {
   Ptr<Action> act = stack.Top()->todo.Top();
   Ptr<const Expression> exp = cast<ExpressionAction>(*act).Exp();
   if (tracing_output) {
-    llvm::outs() << "--- step exp " << *exp << " --->\n";
+    llvm::outs() << "--- step exp " << *exp << " (" << exp->SourceLoc()
+                 << ") --->\n";
   }
   switch (exp->Tag()) {
     case Expression::Kind::IndexExpression: {
@@ -647,7 +649,8 @@ auto Interpreter::StepPattern() -> Transition {
   Ptr<Action> act = stack.Top()->todo.Top();
   Ptr<const Pattern> pattern = cast<PatternAction>(*act).Pat();
   if (tracing_output) {
-    llvm::outs() << "--- step pattern " << *pattern << " --->\n";
+    llvm::outs() << "--- step pattern " << *pattern << " ("
+                 << pattern->SourceLoc() << ") --->\n";
   }
   switch (pattern->Tag()) {
     case Pattern::Kind::AutoPattern: {
@@ -745,7 +748,7 @@ auto Interpreter::StepStmt() -> Transition {
   if (tracing_output) {
     llvm::outs() << "--- step stmt ";
     stmt->PrintDepth(1, llvm::outs());
-    llvm::outs() << " --->\n";
+    llvm::outs() << " (" << stmt->SourceLoc() << ") --->\n";
   }
   switch (stmt->Tag()) {
     case Statement::Kind::Match: {

executable_semantics/interpreter/value.cpp

@@ -9,6 +9,7 @@
 #include "common/check.h"
 #include "executable_semantics/common/arena.h"
 #include "executable_semantics/common/error.h"
+#include "executable_semantics/interpreter/frame.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/Support/Casting.h"
 
@@ -232,11 +233,15 @@ void Value::Print(llvm::raw_ostream& out) const {
     case Value::Kind::VariableType:
       out << cast<VariableType>(*this).Name();
       break;
-    case Value::Kind::ContinuationValue:
-      out << "continuation";
-      // TODO: Find a way to print useful information about the continuation
-      // without creating a dependency cycle.
+    case Value::Kind::ContinuationValue: {
+      out << "{";
+      llvm::ListSeparator sep(" :: ");
+      for (Ptr<Frame> frame : cast<ContinuationValue>(*this).Stack()) {
+        out << sep << *frame;
+      }
+      out << "}";
       break;
+    }
     case Value::Kind::StringType:
       out << "String";
       break;