[parser] Only log messages using the pending error handling

This shares the pending_error_handler from the parser to the preparser, allowing the preparser to directly log errors to it. This removes LogMessage from the loggers. ParserLogger::LogMessage was already unused, so this also removes error info from the preparse data altogether.

BUG=

Review-Url: https://codereview.chromium.org/2502633002
Cr-Commit-Position: refs/heads/master@{#40984}

src/parsing/parser.cc

@@ -68,7 +68,6 @@ bool ParseData::IsSane() {
   if (data_length < PreparseDataConstants::kHeaderSize) return false;
   if (Magic() != PreparseDataConstants::kMagicNumber) return false;
   if (Version() != PreparseDataConstants::kCurrentVersion) return false;
-  if (HasError()) return false;
   // Check that the space allocated for function entries is sane.
   int functions_size = FunctionsSize();
   if (functions_size < 0) return false;
@@ -90,11 +89,6 @@ void ParseData::Initialize() {
 }
 
 
-bool ParseData::HasError() {
-  return Data()[PreparseDataConstants::kHasErrorOffset];
-}
-
-
 unsigned ParseData::Magic() {
   return Data()[PreparseDataConstants::kMagicOffset];
 }
@@ -2788,14 +2782,11 @@ Parser::LazyParsingResult Parser::SkipFunction(
     *ok = false;
     return kLazyParsingComplete;
   }
-  PreParserLogger* logger = reusable_preparser_->logger();
-  if (logger->has_error()) {
-    ReportMessageAt(Scanner::Location(logger->start(), logger->end()),
-                    logger->message(), logger->argument_opt(),
-                    logger->error_type());
+  if (pending_error_handler_.has_pending_error()) {
     *ok = false;
     return kLazyParsingComplete;
   }
+  PreParserLogger* logger = reusable_preparser_->logger();
   function_scope->set_end_position(logger->end());
   Expect(Token::RBRACE, CHECK_OK_VALUE(kLazyParsingComplete));
   total_preparse_skipped_ +=
@@ -3292,8 +3283,8 @@ PreParser::PreParseResult Parser::ParseFunctionWithPreParser(
   TRACE_EVENT0(TRACE_DISABLED_BY_DEFAULT("v8.compile"), "V8.PreParse");
 
   if (reusable_preparser_ == NULL) {
-    reusable_preparser_ =
-        new PreParser(zone(), &scanner_, ast_value_factory(), stack_limit_);
+    reusable_preparser_ = new PreParser(zone(), &scanner_, ast_value_factory(),
+                                        &pending_error_handler_, stack_limit_);
     reusable_preparser_->set_allow_lazy(true);
 #define SET_ALLOW(name) reusable_preparser_->set_allow_##name(allow_##name());
     SET_ALLOW(natives);

src/parsing/parser.h

@@ -102,8 +102,6 @@ class ParseData {
   FunctionEntry GetFunctionEntry(int start);
   int FunctionCount();
 
-  bool HasError();
-
   unsigned* Data() {  // Writable data as unsigned int array.
     return reinterpret_cast<unsigned*>(const_cast<byte*>(script_data_->data()));
   }

src/parsing/preparse-data-format.h

@@ -14,22 +14,13 @@ struct PreparseDataConstants {
  public:
   // Layout and constants of the preparse data exchange format.
   static const unsigned kMagicNumber = 0xBadDead;
-  static const unsigned kCurrentVersion = 12;
+  static const unsigned kCurrentVersion = 13;
 
   static const int kMagicOffset = 0;
   static const int kVersionOffset = 1;
-  static const int kHasErrorOffset = 2;
-  static const int kFunctionsSizeOffset = 3;
-  static const int kSizeOffset = 4;
-  static const int kHeaderSize = 5;
-
-  // If encoding a message, the following positions are fixed.
-  static const int kMessageStartPos = 0;
-  static const int kMessageEndPos = 1;
-  static const int kMessageArgCountPos = 2;
-  static const int kParseErrorTypePos = 3;
-  static const int kMessageTemplatePos = 4;
-  static const int kMessageArgPos = 5;
+  static const int kFunctionsSizeOffset = 2;
+  static const int kSizeOffset = 3;
+  static const int kHeaderSize = 4;
 
   static const unsigned char kNumberTerminator = 0x80u;
 };

src/parsing/preparse-data.cc

@@ -33,42 +33,14 @@ ParserLogger::ParserLogger() {
       PreparseDataConstants::kMagicNumber;
   preamble_[PreparseDataConstants::kVersionOffset] =
       PreparseDataConstants::kCurrentVersion;
-  preamble_[PreparseDataConstants::kHasErrorOffset] = false;
   preamble_[PreparseDataConstants::kFunctionsSizeOffset] = 0;
   preamble_[PreparseDataConstants::kSizeOffset] = 0;
-  DCHECK_EQ(5, PreparseDataConstants::kHeaderSize);
+  DCHECK_EQ(4, PreparseDataConstants::kHeaderSize);
 #ifdef DEBUG
   prev_start_ = -1;
 #endif
 }
 
-void ParserLogger::LogMessage(int start_pos, int end_pos,
-                              MessageTemplate::Template message,
-                              const char* arg_opt, ParseErrorType error_type) {
-  if (HasError()) return;
-  preamble_[PreparseDataConstants::kHasErrorOffset] = true;
-  function_store_.Reset();
-  STATIC_ASSERT(PreparseDataConstants::kMessageStartPos == 0);
-  function_store_.Add(start_pos);
-  STATIC_ASSERT(PreparseDataConstants::kMessageEndPos == 1);
-  function_store_.Add(end_pos);
-  STATIC_ASSERT(PreparseDataConstants::kMessageArgCountPos == 2);
-  function_store_.Add((arg_opt == NULL) ? 0 : 1);
-  STATIC_ASSERT(PreparseDataConstants::kParseErrorTypePos == 3);
-  function_store_.Add(error_type);
-  STATIC_ASSERT(PreparseDataConstants::kMessageTemplatePos == 4);
-  function_store_.Add(static_cast<unsigned>(message));
-  STATIC_ASSERT(PreparseDataConstants::kMessageArgPos == 5);
-  if (arg_opt != NULL) WriteString(CStrVector(arg_opt));
-}
-
-void ParserLogger::WriteString(Vector<const char> str) {
-  function_store_.Add(str.length());
-  for (int i = 0; i < str.length(); i++) {
-    function_store_.Add(str[i]);
-  }
-}
-
 ScriptData* ParserLogger::GetScriptData() {
   int function_size = function_store_.size();
   int total_size = PreparseDataConstants::kHeaderSize + function_size;

src/parsing/preparse-data.h

@@ -49,18 +49,14 @@ class ScriptData {
 class PreParserLogger final {
  public:
   PreParserLogger()
-      : has_error_(false),
-        start_(-1),
-        end_(-1),
+      : end_(-1),
         num_parameters_(-1),
         function_length_(-1),
-        has_duplicate_parameters_(false),
-        error_type_(kSyntaxError) {}
+        has_duplicate_parameters_(false) {}
 
   void LogFunction(int end, int num_parameters, int function_length,
                    bool has_duplicate_parameters, int literals,
                    int properties) {
-    DCHECK(!has_error_);
     end_ = end;
     num_parameters_ = num_parameters;
     function_length_ = function_length;
@@ -69,60 +65,24 @@ class PreParserLogger final {
     properties_ = properties;
   }
 
-  // Logs an error message and marks the log as containing an error.
-  // Further logging will be ignored, and ExtractData will return a vector
-  // representing the error only.
-  void LogMessage(int start, int end, MessageTemplate::Template message,
-                  const char* argument_opt, ParseErrorType error_type) {
-    if (has_error_) return;
-    has_error_ = true;
-    start_ = start;
-    end_ = end;
-    message_ = message;
-    argument_opt_ = argument_opt;
-    error_type_ = error_type;
-  }
-
-  bool has_error() const { return has_error_; }
-
-  int start() const { return start_; }
   int end() const { return end_; }
   int num_parameters() const {
-    DCHECK(!has_error_);
     return num_parameters_;
   }
   int function_length() const {
-    DCHECK(!has_error_);
     return function_length_;
   }
   bool has_duplicate_parameters() const {
-    DCHECK(!has_error_);
     return has_duplicate_parameters_;
   }
   int literals() const {
-    DCHECK(!has_error_);
     return literals_;
   }
   int properties() const {
-    DCHECK(!has_error_);
     return properties_;
   }
-  ParseErrorType error_type() const {
-    DCHECK(has_error_);
-    return error_type_;
-  }
-  MessageTemplate::Template message() {
-    DCHECK(has_error_);
-    return message_;
-  }
-  const char* argument_opt() const {
-    DCHECK(has_error_);
-    return argument_opt_;
-  }
 
  private:
-  bool has_error_;
-  int start_;
   int end_;
   // For function entries.
   int num_parameters_;
@@ -130,19 +90,10 @@ class PreParserLogger final {
   bool has_duplicate_parameters_;
   int literals_;
   int properties_;
-  // For error messages.
-  MessageTemplate::Template message_;
-  const char* argument_opt_;
-  ParseErrorType error_type_;
 };
 
 class ParserLogger final {
  public:
-  struct Key {
-    bool is_one_byte;
-    Vector<const byte> literal_bytes;
-  };
-
   ParserLogger();
 
   void LogFunction(int start, int end, int num_parameters, int function_length,
@@ -150,24 +101,9 @@ class ParserLogger final {
                    LanguageMode language_mode, bool uses_super_property,
                    bool calls_eval);
 
-  // Logs an error message and marks the log as containing an error.
-  // Further logging will be ignored, and ExtractData will return a vector
-  // representing the error only.
-  void LogMessage(int start, int end, MessageTemplate::Template message,
-                  const char* argument_opt, ParseErrorType error_type);
   ScriptData* GetScriptData();
 
-  bool HasError() {
-    return static_cast<bool>(preamble_[PreparseDataConstants::kHasErrorOffset]);
-  }
-  Vector<unsigned> ErrorMessageData() {
-    DCHECK(HasError());
-    return function_store_.ToVector();
-  }
-
  private:
-  void WriteString(Vector<const char> str);
-
   Collector<unsigned> function_store_;
   unsigned preamble_[PreparseDataConstants::kHeaderSize];
 

src/parsing/preparser.cc

@@ -133,7 +133,7 @@ PreParser::PreParseResult PreParser::PreParseFunction(
   } else if (stack_overflow()) {
     return kPreParseStackOverflow;
   } else if (!*ok) {
-    DCHECK(log_.has_error());
+    DCHECK(pending_error_handler_->has_pending_error());
   } else {
     DCHECK_EQ(Token::RBRACE, scanner()->peek());
 

src/parsing/preparser.h

@@ -839,11 +839,13 @@ class PreParser : public ParserBase<PreParser> {
   };
 
   PreParser(Zone* zone, Scanner* scanner, AstValueFactory* ast_value_factory,
+            PendingCompilationErrorHandler* pending_error_handler,
             uintptr_t stack_limit)
       : ParserBase<PreParser>(zone, scanner, stack_limit, nullptr,
                               ast_value_factory),
         use_counts_(nullptr),
-        track_unresolved_variables_(false) {}
+        track_unresolved_variables_(false),
+        pending_error_handler_(pending_error_handler) {}
 
   PreParserLogger* logger() { return &log_; }
 
@@ -1274,8 +1276,9 @@ class PreParser : public ParserBase<PreParser> {
                                  MessageTemplate::Template message,
                                  const char* arg = NULL,
                                  ParseErrorType error_type = kSyntaxError) {
-    log_.LogMessage(source_location.beg_pos, source_location.end_pos, message,
-                    arg, error_type);
+    pending_error_handler_->ReportMessageAt(source_location.beg_pos,
+                                            source_location.end_pos, message,
+                                            arg, error_type);
   }
 
   V8_INLINE void ReportMessageAt(Scanner::Location source_location,
@@ -1509,6 +1512,7 @@ class PreParser : public ParserBase<PreParser> {
   int* use_counts_;
   bool track_unresolved_variables_;
   PreParserLogger log_;
+  PendingCompilationErrorHandler* pending_error_handler_;
 };
 
 PreParserExpression PreParser::SpreadCall(PreParserExpression function,

src/pending-compilation-error-handler.cc

@@ -13,20 +13,29 @@
 namespace v8 {
 namespace internal {
 
+Handle<String> PendingCompilationErrorHandler::ArgumentString(
+    Isolate* isolate) {
+  if (arg_ != NULL) return arg_->string();
+  if (char_arg_ != NULL) {
+    return isolate->factory()
+        ->NewStringFromUtf8(CStrVector(char_arg_))
+        .ToHandleChecked();
+  }
+  if (!handle_arg_.is_null()) return handle_arg_;
+  return isolate->factory()->undefined_string();
+}
+
+Handle<String> PendingCompilationErrorHandler::FormatMessage(Isolate* isolate) {
+  return MessageTemplate::FormatMessage(isolate, message_,
+                                        ArgumentString(isolate));
+}
+
 void PendingCompilationErrorHandler::ThrowPendingError(Isolate* isolate,
                                                        Handle<Script> script) {
   if (!has_pending_error_) return;
   MessageLocation location(script, start_position_, end_position_);
   Factory* factory = isolate->factory();
-  Handle<String> argument;
-  if (arg_ != NULL) {
-    argument = arg_->string();
-  } else if (char_arg_ != NULL) {
-    argument =
-        factory->NewStringFromUtf8(CStrVector(char_arg_)).ToHandleChecked();
-  } else if (!handle_arg_.is_null()) {
-    argument = handle_arg_;
-  }
+  Handle<String> argument = ArgumentString(isolate);
   isolate->debug()->OnCompileError(script);
 
   Handle<Object> error;

src/pending-compilation-error-handler.h

@@ -75,8 +75,11 @@ class PendingCompilationErrorHandler {
   bool has_pending_error() const { return has_pending_error_; }
 
   void ThrowPendingError(Isolate* isolate, Handle<Script> script);
+  Handle<String> FormatMessage(Isolate* isolate);
 
  private:
+  Handle<String> ArgumentString(Isolate* isolate);
+
   bool has_pending_error_;
   int start_position_;
   int end_position_;

test/cctest/test-parsing.cc

@@ -173,11 +173,13 @@ TEST(ScanHTMLEndComments) {
     i::Zone zone(CcTest::i_isolate()->allocator(), ZONE_NAME);
     i::AstValueFactory ast_value_factory(
         &zone, CcTest::i_isolate()->heap()->HashSeed());
-    i::PreParser preparser(&zone, &scanner, &ast_value_factory, stack_limit);
+    i::PendingCompilationErrorHandler pending_error_handler;
+    i::PreParser preparser(&zone, &scanner, &ast_value_factory,
+                           &pending_error_handler, stack_limit);
     preparser.set_allow_lazy(true);
     i::PreParser::PreParseResult result = preparser.PreParseProgram();
     CHECK_EQ(i::PreParser::kPreParseSuccess, result);
-    CHECK(!preparser.logger()->has_error());
+    CHECK(!pending_error_handler.has_pending_error());
   }
 
   for (int i = 0; fail_tests[i]; i++) {
@@ -188,12 +190,14 @@ TEST(ScanHTMLEndComments) {
     i::Zone zone(CcTest::i_isolate()->allocator(), ZONE_NAME);
     i::AstValueFactory ast_value_factory(
         &zone, CcTest::i_isolate()->heap()->HashSeed());
-    i::PreParser preparser(&zone, &scanner, &ast_value_factory, stack_limit);
+    i::PendingCompilationErrorHandler pending_error_handler;
+    i::PreParser preparser(&zone, &scanner, &ast_value_factory,
+                           &pending_error_handler, stack_limit);
     preparser.set_allow_lazy(true);
     i::PreParser::PreParseResult result = preparser.PreParseProgram();
     // Even in the case of a syntax error, kPreParseSuccess is returned.
     CHECK_EQ(i::PreParser::kPreParseSuccess, result);
-    CHECK(preparser.logger()->has_error());
+    CHECK(pending_error_handler.has_pending_error());
   }
 }
 
@@ -360,12 +364,14 @@ TEST(StandAlonePreParser) {
     i::Zone zone(CcTest::i_isolate()->allocator(), ZONE_NAME);
     i::AstValueFactory ast_value_factory(
         &zone, CcTest::i_isolate()->heap()->HashSeed());
-    i::PreParser preparser(&zone, &scanner, &ast_value_factory, stack_limit);
+    i::PendingCompilationErrorHandler pending_error_handler;
+    i::PreParser preparser(&zone, &scanner, &ast_value_factory,
+                           &pending_error_handler, stack_limit);
     preparser.set_allow_lazy(true);
     preparser.set_allow_natives(true);
     i::PreParser::PreParseResult result = preparser.PreParseProgram();
     CHECK_EQ(i::PreParser::kPreParseSuccess, result);
-    CHECK(!preparser.logger()->has_error());
+    CHECK(!pending_error_handler.has_pending_error());
   }
 }
 
@@ -392,11 +398,13 @@ TEST(StandAlonePreParserNoNatives) {
     i::Zone zone(CcTest::i_isolate()->allocator(), ZONE_NAME);
     i::AstValueFactory ast_value_factory(
         &zone, CcTest::i_isolate()->heap()->HashSeed());
-    i::PreParser preparser(&zone, &scanner, &ast_value_factory, stack_limit);
+    i::PendingCompilationErrorHandler pending_error_handler;
+    i::PreParser preparser(&zone, &scanner, &ast_value_factory,
+                           &pending_error_handler, stack_limit);
     preparser.set_allow_lazy(true);
     i::PreParser::PreParseResult result = preparser.PreParseProgram();
     CHECK_EQ(i::PreParser::kPreParseSuccess, result);
-    CHECK(preparser.logger()->has_error());
+    CHECK(pending_error_handler.has_pending_error());
   }
 }
 
@@ -457,13 +465,15 @@ TEST(RegressChromium62639) {
   i::Zone zone(CcTest::i_isolate()->allocator(), ZONE_NAME);
   i::AstValueFactory ast_value_factory(&zone,
                                        CcTest::i_isolate()->heap()->HashSeed());
+  i::PendingCompilationErrorHandler pending_error_handler;
   i::PreParser preparser(&zone, &scanner, &ast_value_factory,
+                         &pending_error_handler,
                          CcTest::i_isolate()->stack_guard()->real_climit());
   preparser.set_allow_lazy(true);
   i::PreParser::PreParseResult result = preparser.PreParseProgram();
   // Even in the case of a syntax error, kPreParseSuccess is returned.
   CHECK_EQ(i::PreParser::kPreParseSuccess, result);
-  CHECK(preparser.logger()->has_error());
+  CHECK(pending_error_handler.has_pending_error());
 }
 
 
@@ -530,7 +540,9 @@ TEST(PreParseOverflow) {
   i::Zone zone(CcTest::i_isolate()->allocator(), ZONE_NAME);
   i::AstValueFactory ast_value_factory(&zone,
                                        CcTest::i_isolate()->heap()->HashSeed());
-  i::PreParser preparser(&zone, &scanner, &ast_value_factory, stack_limit);
+  i::PendingCompilationErrorHandler pending_error_handler;
+  i::PreParser preparser(&zone, &scanner, &ast_value_factory,
+                         &pending_error_handler, stack_limit);
   preparser.set_allow_lazy(true);
   i::PreParser::PreParseResult result = preparser.PreParseProgram();
   CHECK_EQ(i::PreParser::kPreParseStackOverflow, result);
@@ -1270,27 +1282,6 @@ const char* ReadString(unsigned* start) {
 }
 
 
-i::Handle<i::String> FormatMessage(i::Vector<unsigned> data) {
-  i::Isolate* isolate = CcTest::i_isolate();
-  int message = data[i::PreparseDataConstants::kMessageTemplatePos];
-  int arg_count = data[i::PreparseDataConstants::kMessageArgCountPos];
-  i::Handle<i::Object> arg_object;
-  if (arg_count == 1) {
-    // Position after text found by skipping past length field and
-    // length field content words.
-    const char* arg =
-        ReadString(&data[i::PreparseDataConstants::kMessageArgPos]);
-    arg_object = v8::Utils::OpenHandle(*v8_str(arg));
-    i::DeleteArray(arg);
-  } else {
-    CHECK_EQ(0, arg_count);
-    arg_object = isolate->factory()->undefined_value();
-  }
-
-  data.Dispose();
-  return i::MessageTemplate::FormatMessage(isolate, message, arg_object);
-}
-
 enum ParserFlag {
   kAllowLazy,
   kAllowNatives,
@@ -1338,7 +1329,7 @@ void TestParserSyncWithFlags(i::Handle<i::String> source,
   int parser_materialized_literals = -2;
 
   // Preparse the data.
-  i::ParserLogger log;
+  i::PendingCompilationErrorHandler pending_error_handler;
   if (test_preparser) {
     i::Scanner scanner(isolate->unicode_cache());
     std::unique_ptr<i::Utf16CharacterStream> stream(
@@ -1346,20 +1337,14 @@ void TestParserSyncWithFlags(i::Handle<i::String> source,
     i::Zone zone(CcTest::i_isolate()->allocator(), ZONE_NAME);
     i::AstValueFactory ast_value_factory(
         &zone, CcTest::i_isolate()->heap()->HashSeed());
-    i::PreParser preparser(&zone, &scanner, &ast_value_factory, stack_limit);
+    i::PreParser preparser(&zone, &scanner, &ast_value_factory,
+                           &pending_error_handler, stack_limit);
     SetParserFlags(&preparser, flags);
     scanner.Initialize(stream.get());
     i::PreParser::PreParseResult result =
         preparser.PreParseProgram(&preparser_materialized_literals, is_module);
     CHECK_EQ(i::PreParser::kPreParseSuccess, result);
-    i::PreParserLogger* logger = preparser.logger();
-    // Convert to complete log.
-    if (logger->has_error()) {
-      log.LogMessage(logger->start(), logger->end(), logger->message(),
-                     logger->argument_opt(), logger->error_type());
-    }
   }
-  bool preparse_error = log.HasError();
 
   // Parse the data
   i::FunctionLiteral* function;
@@ -1399,7 +1384,7 @@ void TestParserSyncWithFlags(i::Handle<i::String> source,
       CHECK(false);
     }
 
-    if (test_preparser && !preparse_error) {
+    if (test_preparser && !pending_error_handler.has_pending_error()) {
       v8::base::OS::Print(
           "Parser failed on:\n"
           "\t%s\n"
@@ -1412,7 +1397,7 @@ void TestParserSyncWithFlags(i::Handle<i::String> source,
     // Check that preparser and parser produce the same error.
     if (test_preparser) {
       i::Handle<i::String> preparser_message =
-          FormatMessage(log.ErrorMessageData());
+          pending_error_handler.FormatMessage(CcTest::i_isolate());
       if (!i::String::Equals(message_string, preparser_message)) {
         v8::base::OS::Print(
             "Expected parser and preparser to produce the same error on:\n"
@@ -1425,7 +1410,7 @@ void TestParserSyncWithFlags(i::Handle<i::String> source,
         CHECK(false);
       }
     }
-  } else if (test_preparser && preparse_error) {
+  } else if (test_preparser && pending_error_handler.has_pending_error()) {
     v8::base::OS::Print(
         "Preparser failed on:\n"
         "\t%s\n"
@@ -1433,7 +1418,9 @@ void TestParserSyncWithFlags(i::Handle<i::String> source,
         "\t%s\n"
         "However, the parser succeeded",
         source->ToCString().get(),
-        FormatMessage(log.ErrorMessageData())->ToCString().get());
+        pending_error_handler.FormatMessage(CcTest::i_isolate())
+            ->ToCString()
+            .get());
     CHECK(false);
   } else if (result == kError) {
     v8::base::OS::Print(