Refactor FATAL macro

Remove comment about usage of FATAL, UNREACHABLE and UNIMPLEMENTED,
which was deprecated since https://crrev.com/1410713006.
Also, refactor the FATAL macro and use it for implementing UNREACHABLE
and UNIMPLEMENTED, and in more code. The benefit over printf +
CHECK(false) is that the compiler knows that FATAL will never return.

R=bmeurer@chromium.org

Change-Id: I8c2ab3b4e6edfe8eff5ec6fdf3d92b15d0ed7126
Reviewed-on: https://chromium-review.googlesource.com/832726Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
Commit-Queue: Clemens Hammacher <clemensh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#50183}

src/base/logging.cc

@@ -119,8 +119,6 @@ DEFINE_CHECK_OP_IMPL(GT)
 }  // namespace base
 }  // namespace v8
 
-
-// Contains protection against recursive calls (faults while handling faults).
 void V8_Fatal(const char* file, int line, const char* format, ...) {
   fflush(stdout);
   fflush(stderr);

src/base/logging.h

@@ -20,23 +20,13 @@
 V8_BASE_EXPORT V8_NOINLINE void V8_Dcheck(const char* file, int line,
                                           const char* message);
 
-// The FATAL, UNREACHABLE and UNIMPLEMENTED macros are useful during
-// development, but they should not be relied on in the final product.
 #ifdef DEBUG
-#define FATAL(msg)                              \
-  V8_Fatal(__FILE__, __LINE__, "%s", (msg))
-#define UNIMPLEMENTED()                         \
-  V8_Fatal(__FILE__, __LINE__, "unimplemented code")
-#define UNREACHABLE()                           \
-  V8_Fatal(__FILE__, __LINE__, "unreachable code")
+#define FATAL(...) V8_Fatal(__FILE__, __LINE__, __VA_ARGS__)
 #else
-#define FATAL(msg)                              \
-  V8_Fatal("", 0, "%s", (msg))
-#define UNIMPLEMENTED()                         \
-  V8_Fatal("", 0, "unimplemented code")
-#define UNREACHABLE() V8_Fatal("", 0, "unreachable code")
+#define FATAL(...) V8_Fatal("", 0, __VA_ARGS__)
 #endif
-
+#define UNIMPLEMENTED() FATAL("unimplemented code")
+#define UNREACHABLE() FATAL("unreachable code")
 
 namespace v8 {
 namespace base {

src/compiler/code-assembler.cc

@@ -1357,13 +1357,13 @@ Node* CodeAssemblerVariable::value() const {
     str << "#Use of unbound variable:"
         << "#\n    Variable:      " << *this << "#\n    Current Block: ";
     state_->PrintCurrentBlock(str);
-    FATAL(str.str().c_str());
+    FATAL("%s", str.str().c_str());
   }
   if (!state_->InsideBlock()) {
     std::stringstream str;
     str << "#Accessing variable value outside a block:"
         << "#\n    Variable:      " << *this;
-    FATAL(str.str().c_str());
+    FATAL("%s", str.str().c_str());
   }
 #endif  // DEBUG
   return impl_->value_;
@@ -1456,7 +1456,7 @@ void CodeAssemblerLabel::MergeVariables() {
             }
             str << "\n#    Current Block: ";
             state_->PrintCurrentBlock(str);
-            FATAL(str.str().c_str());
+            FATAL("%s", str.str().c_str());
           }
 #endif  // DEBUG
         }
@@ -1472,7 +1472,7 @@ void CodeAssemblerLabel::Bind(AssemblerDebugInfo debug_info) {
     str << "Cannot bind the same label twice:"
         << "\n#    current:  " << debug_info
         << "\n#    previous: " << *label_->block();
-    FATAL(str.str().c_str());
+    FATAL("%s", str.str().c_str());
   }
   state_->raw_assembler_->Bind(label_, debug_info);
   UpdateVariablesAfterBind();
@@ -1524,7 +1524,7 @@ void CodeAssemblerLabel::UpdateVariablesAfterBind() {
           << " vs. found=" << (not_found ? 0 : i->second.size())
           << "\n# Variable:      " << *var_impl
           << "\n# Current Block: " << *label_->block();
-      FATAL(str.str().c_str());
+      FATAL("%s", str.str().c_str());
     }
 #endif  // DEBUG
     Node* phi = state_->raw_assembler_->Phi(

src/compiler/instruction-selector.cc

@@ -970,7 +970,7 @@ void InstructionSelector::VisitControl(BasicBlock* block) {
               << "only one predecessor." << std::endl
               << "# Current Block: " << *successor << std::endl
               << "#          Node: " << *node;
-          FATAL(str.str().c_str());
+          FATAL("%s", str.str().c_str());
         }
       }
     }

src/compiler/machine-graph-verifier.cc

@@ -558,7 +558,7 @@ class MachineRepresentationChecker {
               str << "Node #" << node->id() << ":" << *node->op()
                   << " in the machine graph is not being checked.";
               PrintDebugHelp(str, node);
-              FATAL(str.str().c_str());
+              FATAL("%s", str.str().c_str());
             }
             break;
         }
@@ -588,7 +588,7 @@ class MachineRepresentationChecker {
           << input_representation << " which doesn't have a " << representation
           << " representation.";
       PrintDebugHelp(str, node);
-      FATAL(str.str().c_str());
+      FATAL("%s", str.str().c_str());
     }
   }
 
@@ -607,7 +607,7 @@ class MachineRepresentationChecker {
         << " uses node #" << input->id() << ":" << *input->op()
         << " which doesn't have a tagged representation.";
     PrintDebugHelp(str, node);
-    FATAL(str.str().c_str());
+    FATAL("%s", str.str().c_str());
   }
 
   void CheckValueInputIsTaggedOrPointer(Node const* node, int index) {
@@ -640,7 +640,7 @@ class MachineRepresentationChecker {
           << " uses node #" << input->id() << ":" << *input->op()
           << " which doesn't have a tagged or pointer representation.";
       PrintDebugHelp(str, node);
-      FATAL(str.str().c_str());
+      FATAL("%s", str.str().c_str());
     }
   }
 
@@ -657,7 +657,7 @@ class MachineRepresentationChecker {
         str << "TypeError: node #" << input->id() << ":" << *input->op()
             << " is untyped.";
         PrintDebugHelp(str, node);
-        FATAL(str.str().c_str());
+        FATAL("%s", str.str().c_str());
         break;
       }
       default:
@@ -668,7 +668,7 @@ class MachineRepresentationChecker {
         << " uses node #" << input->id() << ":" << *input->op()
         << " which doesn't have an int32-compatible representation.";
     PrintDebugHelp(str, node);
-    FATAL(str.str().c_str());
+    FATAL("%s", str.str().c_str());
   }
 
   void CheckValueInputForInt64Op(Node const* node, int index) {
@@ -683,7 +683,7 @@ class MachineRepresentationChecker {
         str << "TypeError: node #" << input->id() << ":" << *input->op()
             << " is untyped.";
         PrintDebugHelp(str, node);
-        FATAL(str.str().c_str());
+        FATAL("%s", str.str().c_str());
         break;
       }
 
@@ -696,7 +696,7 @@ class MachineRepresentationChecker {
         << input_representation
         << " which doesn't have a kWord64 representation.";
     PrintDebugHelp(str, node);
-    FATAL(str.str().c_str());
+    FATAL("%s", str.str().c_str());
   }
 
   void CheckValueInputForFloat32Op(Node const* node, int index) {
@@ -710,7 +710,7 @@ class MachineRepresentationChecker {
         << " uses node #" << input->id() << ":" << *input->op()
         << " which doesn't have a kFloat32 representation.";
     PrintDebugHelp(str, node);
-    FATAL(str.str().c_str());
+    FATAL("%s", str.str().c_str());
   }
 
   void CheckValueInputForFloat64Op(Node const* node, int index) {
@@ -724,7 +724,7 @@ class MachineRepresentationChecker {
         << " uses node #" << input->id() << ":" << *input->op()
         << " which doesn't have a kFloat64 representation.";
     PrintDebugHelp(str, node);
-    FATAL(str.str().c_str());
+    FATAL("%s", str.str().c_str());
   }
 
   void CheckCallInputs(Node const* node) {
@@ -751,7 +751,7 @@ class MachineRepresentationChecker {
     }
     if (should_log_error) {
       PrintDebugHelp(str, node);
-      FATAL(str.str().c_str());
+      FATAL("%s", str.str().c_str());
     }
   }
 

src/compiler/raw-machine-assembler.cc

@@ -454,7 +454,7 @@ void RawMachineAssembler::Bind(RawMachineLabel* label,
     str << "Binding label without closing previous block:"
         << "\n#    label:          " << info
         << "\n#    previous block: " << *current_block_;
-    FATAL(str.str().c_str());
+    FATAL("%s", str.str().c_str());
   }
   Bind(label);
   current_block_->set_debug_info(info);
@@ -519,7 +519,7 @@ RawMachineLabel::~RawMachineLabel() {
   } else {
     str << "A label has been used but it's not bound.";
   }
-  FATAL(str.str().c_str());
+  FATAL("%s", str.str().c_str());
 #endif  // DEBUG
 }
 

src/compiler/verifier.cc

@@ -51,7 +51,7 @@ class Verifier::Visitor {
       std::ostringstream str;
       str << "TypeError: node #" << node->id() << ":" << *node->op()
           << " should never have a type";
-      FATAL(str.str().c_str());
+      FATAL("%s", str.str().c_str());
     }
   }
   void CheckTypeIs(Node* node, Type* type) {
@@ -62,7 +62,7 @@ class Verifier::Visitor {
       NodeProperties::GetType(node)->PrintTo(str);
       str << " is not ";
       type->PrintTo(str);
-      FATAL(str.str().c_str());
+      FATAL("%s", str.str().c_str());
     }
   }
   void CheckTypeMaybe(Node* node, Type* type) {
@@ -73,7 +73,7 @@ class Verifier::Visitor {
       NodeProperties::GetType(node)->PrintTo(str);
       str << " must intersect ";
       type->PrintTo(str);
-      FATAL(str.str().c_str());
+      FATAL("%s", str.str().c_str());
     }
   }
   void CheckValueInputIs(Node* node, int i, Type* type) {
@@ -86,7 +86,7 @@ class Verifier::Visitor {
       NodeProperties::GetType(input)->PrintTo(str);
       str << " is not ";
       type->PrintTo(str);
-      FATAL(str.str().c_str());
+      FATAL("%s", str.str().c_str());
     }
   }
   void CheckOutput(Node* node, Node* use, int count, const char* kind) {
@@ -95,7 +95,7 @@ class Verifier::Visitor {
       str << "GraphError: node #" << node->id() << ":" << *node->op()
           << " does not produce " << kind << " output used by node #"
           << use->id() << ":" << *use->op();
-      FATAL(str.str().c_str());
+      FATAL("%s", str.str().c_str());
     }
   }
 };

src/interpreter/constant-array-builder.cc

@@ -115,7 +115,7 @@ void ConstantArrayBuilder::ConstantArraySlice::CheckAllElementsAreUnique(
       for (const Entry& prev_entry : constants_) {
         os << i++ << ": " << Brief(*prev_entry.ToHandle(isolate)) << std::endl;
       }
-      FATAL(os.str().c_str());
+      FATAL("%s", os.str().c_str());
     }
   }
 }

test/cctest/test-cpu-profiler.cc

@@ -512,11 +512,7 @@ static const v8::CpuProfileNode* GetChild(v8::Local<v8::Context> context,
                                           const v8::CpuProfileNode* node,
                                           const char* name) {
   const v8::CpuProfileNode* result = FindChild(context, node, name);
-  if (!result) {
-    char buffer[100];
-    i::SNPrintF(i::ArrayVector(buffer), "Failed to GetChild: %s", name);
-    FATAL(buffer);
-  }
+  if (!result) FATAL("Failed to GetChild: %s", name);
   return result;
 }
 
@@ -1879,7 +1875,7 @@ TEST(CollectDeoptEvents) {
         GetBranchDeoptReason(env, iprofile, branch, arraysize(branch));
     if (deopt_reason != reason(i::DeoptimizeReason::kNotAHeapNumber) &&
         deopt_reason != reason(i::DeoptimizeReason::kNotASmi)) {
-      FATAL(deopt_reason);
+      FATAL("%s", deopt_reason);
     }
   }
   {
@@ -1889,7 +1885,7 @@ TEST(CollectDeoptEvents) {
     if (deopt_reason != reason(i::DeoptimizeReason::kNaN) &&
         deopt_reason != reason(i::DeoptimizeReason::kLostPrecisionOrNaN) &&
         deopt_reason != reason(i::DeoptimizeReason::kNotASmi)) {
-      FATAL(deopt_reason);
+      FATAL("%s", deopt_reason);
     }
   }
   {

test/cctest/test-log.cc

@@ -648,14 +648,12 @@ TEST(EquivalenceOfLoggingAndTraversal) {
     v8::Local<v8::Script> script = CompileWithOrigin(source_str, "");
     if (script.IsEmpty()) {
       v8::String::Utf8Value exception(isolate, try_catch.Exception());
-      printf("compile: %s\n", *exception);
-      CHECK(false);
+      FATAL("compile: %s\n", *exception);
     }
     v8::Local<v8::Value> result;
     if (!script->Run(logger.env()).ToLocal(&result)) {
       v8::String::Utf8Value exception(isolate, try_catch.Exception());
-      printf("run: %s\n", *exception);
-      CHECK(false);
+      FATAL("run: %s\n", *exception);
     }
     // The result either be the "true" literal or problem description.
     if (!result->IsTrue()) {
@@ -663,10 +661,7 @@ TEST(EquivalenceOfLoggingAndTraversal) {
       i::ScopedVector<char> data(s->Utf8Length() + 1);
       CHECK(data.start());
       s->WriteUtf8(data.start());
-      printf("%s\n", data.start());
-      // Make sure that our output is written prior crash due to CHECK failure.
-      fflush(stdout);
-      CHECK(false);
+      FATAL("%s\n", data.start());
     }
   }
   isolate->Dispose();

test/cctest/test-parsing.cc

@@ -1403,32 +1403,30 @@ void TestParserSyncWithFlags(i::Handle<i::String> source,
     isolate->clear_pending_exception();
 
     if (result == kSuccess) {
-      v8::base::OS::Print(
+      FATAL(
           "Parser failed on:\n"
           "\t%s\n"
           "with error:\n"
           "\t%s\n"
           "However, we expected no error.",
           source->ToCString().get(), message_string->ToCString().get());
-      CHECK(false);
     }
 
     if (test_preparser && !pending_error_handler.has_pending_error()) {
-      v8::base::OS::Print(
+      FATAL(
           "Parser failed on:\n"
           "\t%s\n"
           "with error:\n"
           "\t%s\n"
           "However, the preparser succeeded",
           source->ToCString().get(), message_string->ToCString().get());
-      CHECK(false);
     }
     // Check that preparser and parser produce the same error.
     if (test_preparser && !ignore_error_msg) {
       i::Handle<i::String> preparser_message =
           pending_error_handler.FormatErrorMessageForTest(CcTest::i_isolate());
       if (!i::String::Equals(message_string, preparser_message)) {
-        v8::base::OS::Print(
+        FATAL(
             "Expected parser and preparser to produce the same error on:\n"
             "\t%s\n"
             "However, found the following error messages\n"
@@ -1436,11 +1434,10 @@ void TestParserSyncWithFlags(i::Handle<i::String> source,
             "\tpreparser: %s\n",
             source->ToCString().get(), message_string->ToCString().get(),
             preparser_message->ToCString().get());
-        CHECK(false);
       }
     }
   } else if (test_preparser && pending_error_handler.has_pending_error()) {
-    v8::base::OS::Print(
+    FATAL(
         "Preparser failed on:\n"
         "\t%s\n"
         "with error:\n"
@@ -1450,14 +1447,12 @@ void TestParserSyncWithFlags(i::Handle<i::String> source,
         pending_error_handler.FormatErrorMessageForTest(CcTest::i_isolate())
             ->ToCString()
             .get());
-    CHECK(false);
   } else if (result == kError) {
-    v8::base::OS::Print(
+    FATAL(
         "Expected error on:\n"
         "\t%s\n"
         "However, parser and preparser succeeded",
         source->ToCString().get());
-    CHECK(false);
   }
 }
 
@@ -2387,12 +2382,11 @@ TEST(DontRegressPreParserDataSizes) {
     i::ParseData* pd = i::ParseData::FromCachedData(sd);
 
     if (pd->FunctionCount() != test_cases[i].functions) {
-      v8::base::OS::Print(
+      FATAL(
           "Expected preparse data for program:\n"
           "\t%s\n"
           "to contain %d functions, however, received %d functions.\n",
           program, test_cases[i].functions, pd->FunctionCount());
-      CHECK(false);
     }
     delete sd;
     delete pd;
@@ -5818,14 +5812,13 @@ TEST(BasicImportExportParsing) {
                 .ToHandleChecked());
         isolate->clear_pending_exception();
 
-        v8::base::OS::Print(
+        FATAL(
             "Parser failed on:\n"
             "\t%s\n"
             "with error:\n"
             "\t%s\n"
             "However, we expected no error.",
             source->ToCString().get(), message_string->ToCString().get());
-        CHECK(false);
       }
     }
 

test/cctest/test-profile-generator.cc

@@ -649,8 +649,7 @@ int GetFunctionLineNumber(CpuProfiler& profiler, LocalContext& env,
       v8::Utils::OpenHandle(*v8::Local<v8::Function>::Cast(
           env->Global()->Get(env.local(), v8_str(name)).ToLocalChecked())));
   CodeEntry* func_entry = code_map->FindEntry(func->abstract_code()->address());
-  if (!func_entry)
-    FATAL(name);
+  if (!func_entry) FATAL("%s", name);
   return func_entry->line_number();
 }
 

test/cctest/wasm/wasm-run-utils.cc

@@ -277,10 +277,8 @@ void TestBuildingGraphWithBuilder(compiler::WasmGraphBuilder* builder,
 #endif
 
     uint32_t pc = result.error_offset();
-    std::ostringstream str;
-    str << "Verification failed; pc = +" << pc
-        << ", msg = " << result.error_msg().c_str();
-    FATAL(str.str().c_str());
+    FATAL("Verification failed; pc = +%x, msg = %s", pc,
+          result.error_msg().c_str());
   }
   builder->LowerInt64();
   if (!CpuFeatures::SupportsWasmSimd128()) {