[compiler] Fix double error reporting for parser errors

Remove error reporting from parsing::Parse*, since in most cases we
didn't actually want them (clear errors afterward), and there was an
issue where Compiler::Compile would try to report errors already
reported in ParseAny, which ended up triggering unreachable code.

As a drive-by, move some one-off parse exception handling in
test-parsing into a CHECKED_PARSE_PROGRAM macro which replaces all the
"necessarily positive" calls to parsing::ParseProgram.

Bug: chromium:1091656
Change-Id: I4d463ec363312aea36ab92f1322cf66a416b9888
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2237134Reviewed-by: Ross McIlroy <rmcilroy@chromium.org>
Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
Reviewed-by: Jakob Gruber <jgruber@chromium.org>
Auto-Submit: Leszek Swirski <leszeks@chromium.org>
Commit-Queue: Leszek Swirski <leszeks@chromium.org>
Cr-Commit-Position: refs/heads/master@{#68281}

src/codegen/compiler.cc

@@ -1090,7 +1090,9 @@ MaybeHandle<SharedFunctionInfo> CompileToplevel(
   VMState<BYTECODE_COMPILER> state(isolate);
   if (parse_info->literal() == nullptr &&
       !parsing::ParseProgram(parse_info, script, maybe_outer_scope_info,
-                             isolate)) {
+                             isolate, parsing::ReportStatisticsMode::kYes)) {
+    FailWithPendingException(isolate, script, parse_info,
+                             Compiler::ClearExceptionFlag::KEEP_EXCEPTION);
     return MaybeHandle<SharedFunctionInfo>();
   }
   // Measure how long it takes to do the compilation; only take the
@@ -1456,7 +1458,7 @@ bool Compiler::CollectSourcePositions(Isolate* isolate,
   // Parse and update ParseInfo with the results. Don't update parsing
   // statistics since we've already parsed the code before.
   if (!parsing::ParseAny(&parse_info, shared_info, isolate,
-                         parsing::ReportErrorsAndStatisticsMode::kNo)) {
+                         parsing::ReportStatisticsMode::kNo)) {
     // Parsing failed probably as a result of stack exhaustion.
     bytecode->SetSourcePositionsFailedToCollect();
     return FailAndClearPendingException(isolate);
@@ -1548,7 +1550,8 @@ bool Compiler::Compile(Handle<SharedFunctionInfo> shared_info,
   }
 
   // Parse and update ParseInfo with the results.
-  if (!parsing::ParseAny(&parse_info, shared_info, isolate)) {
+  if (!parsing::ParseAny(&parse_info, shared_info, isolate,
+                         parsing::ReportStatisticsMode::kYes)) {
     return FailWithPendingException(isolate, script, &parse_info, flag);
   }
 

src/d8/d8.cc

@@ -584,7 +584,12 @@ bool Shell::ExecuteString(Isolate* isolate, Local<String> source,
 
     i::Handle<i::Script> script = parse_info.CreateScript(
         i_isolate, str, i::kNullMaybeHandle, ScriptOriginOptions());
-    if (!i::parsing::ParseProgram(&parse_info, script, i_isolate)) {
+    if (!i::parsing::ParseProgram(&parse_info, script, i_isolate,
+                                  i::parsing::ReportStatisticsMode::kYes)) {
+      parse_info.pending_error_handler()->PrepareErrors(
+          i_isolate, parse_info.ast_value_factory());
+      parse_info.pending_error_handler()->ReportErrors(i_isolate, script);
+
       fprintf(stderr, "Failed parsing\n");
       return false;
     }

src/debug/debug-scopes.cc

@@ -269,8 +269,9 @@ void ScopeIterator::TryParseAndRetrieveScopes(ReparseStrategy strategy) {
   const bool parse_result =
       flags.is_toplevel()
           ? parsing::ParseProgram(info_.get(), script, maybe_outer_scope,
-                                  isolate_)
-          : parsing::ParseFunction(info_.get(), shared_info, isolate_);
+                                  isolate_, parsing::ReportStatisticsMode::kNo)
+          : parsing::ParseFunction(info_.get(), shared_info, isolate_,
+                                   parsing::ReportStatisticsMode::kNo);
 
   if (parse_result) {
     DeclarationScope* literal_scope = info_->literal()->scope();
@@ -300,14 +301,13 @@ void ScopeIterator::TryParseAndRetrieveScopes(ReparseStrategy strategy) {
     UnwrapEvaluationContext();
   } else {
     // A failed reparse indicates that the preparser has diverged from the
-    // parser or that the preparse data given to the initial parse has been
-    // faulty. We fail in debug mode but in release mode we only provide the
-    // information we get from the context chain but nothing about
-    // completely stack allocated scopes or stack allocated locals.
-    // Or it could be due to stack overflow.
+    // parser, that the preparse data given to the initial parse was faulty, or
+    // a stack overflow.
+    // TODO(leszeks): This error is pretty unexpected, so we could report the
+    // error in debug mode. Better to not fail in release though, in case it's
+    // just a stack overflow.
+
     // Silently fail by presenting an empty context chain.
-    CHECK(isolate_->has_pending_exception());
-    isolate_->clear_pending_exception();
     context_ = Handle<Context>();
   }
 }

src/debug/liveedit.cc

@@ -757,7 +757,14 @@ bool ParseScript(Isolate* isolate, Handle<Script> script, ParseInfo* parse_info,
     success = Compiler::CompileForLiveEdit(parse_info, script, isolate)
                   .ToHandle(&shared);
   } else {
-    success = parsing::ParseProgram(parse_info, script, isolate);
+    success = parsing::ParseProgram(parse_info, script, isolate,
+                                    parsing::ReportStatisticsMode::kYes);
+    if (!success) {
+      // Throw the parser error.
+      parse_info->pending_error_handler()->PrepareErrors(
+          isolate, parse_info->ast_value_factory());
+      parse_info->pending_error_handler()->ReportErrors(isolate, script);
+    }
   }
   if (!success) {
     isolate->OptionalRescheduleException(false);

src/execution/messages.cc

@@ -1261,14 +1261,13 @@ Handle<String> RenderCallSite(Isolate* isolate, Handle<Object> object,
         isolate, *location->shared());
     UnoptimizedCompileState compile_state(isolate);
     ParseInfo info(isolate, flags, &compile_state);
-    if (parsing::ParseAny(&info, location->shared(), isolate)) {
+    if (parsing::ParseAny(&info, location->shared(), isolate,
+                          parsing::ReportStatisticsMode::kNo)) {
       info.ast_value_factory()->Internalize(isolate);
       CallPrinter printer(isolate, location->shared()->IsUserJavaScript());
       Handle<String> str = printer.Print(info.literal(), location->start_pos());
       *hint = printer.GetErrorHint();
       if (str->length() > 0) return str;
-    } else {
-      isolate->clear_pending_exception();
     }
   }
   return BuildDefaultCallSite(isolate, object);
@@ -1322,7 +1321,8 @@ Object ErrorUtils::ThrowSpreadArgError(Isolate* isolate, MessageTemplate id,
         isolate, *location.shared());
     UnoptimizedCompileState compile_state(isolate);
     ParseInfo info(isolate, flags, &compile_state);
-    if (parsing::ParseAny(&info, location.shared(), isolate)) {
+    if (parsing::ParseAny(&info, location.shared(), isolate,
+                          parsing::ReportStatisticsMode::kNo)) {
       info.ast_value_factory()->Internalize(isolate);
       CallPrinter printer(isolate, location.shared()->IsUserJavaScript(),
                           CallPrinter::SpreadErrorInArgsHint::kErrorInArgs);
@@ -1337,7 +1337,6 @@ Object ErrorUtils::ThrowSpreadArgError(Isolate* isolate, MessageTemplate id,
             MessageLocation(location.script(), pos, pos + 1, location.shared());
       }
     } else {
-      isolate->clear_pending_exception();
       callsite = BuildDefaultCallSite(isolate, object);
     }
   }
@@ -1399,7 +1398,8 @@ Object ErrorUtils::ThrowLoadFromNullOrUndefined(Isolate* isolate,
         isolate, *location.shared());
     UnoptimizedCompileState compile_state(isolate);
     ParseInfo info(isolate, flags, &compile_state);
-    if (parsing::ParseAny(&info, location.shared(), isolate)) {
+    if (parsing::ParseAny(&info, location.shared(), isolate,
+                          parsing::ReportStatisticsMode::kNo)) {
       info.ast_value_factory()->Internalize(isolate);
       CallPrinter printer(isolate, location.shared()->IsUserJavaScript());
       Handle<String> str = printer.Print(info.literal(), location.start_pos());
@@ -1434,8 +1434,6 @@ Object ErrorUtils::ThrowLoadFromNullOrUndefined(Isolate* isolate,
       }
 
       if (str->length() > 0) callsite = str;
-    } else {
-      isolate->clear_pending_exception();
     }
   }
 

src/parsing/parser.h

@@ -170,10 +170,10 @@ class V8_EXPORT_PRIVATE Parser : public NON_EXPORTED_BASE(ParserBase<Parser>) {
   friend class i::ArrowHeadParsingScope<ParserTypes<Parser>>;
   friend bool v8::internal::parsing::ParseProgram(
       ParseInfo*, Handle<Script>, MaybeHandle<ScopeInfo> maybe_outer_scope_info,
-      Isolate*, parsing::ReportErrorsAndStatisticsMode stats_mode);
+      Isolate*, parsing::ReportStatisticsMode stats_mode);
   friend bool v8::internal::parsing::ParseFunction(
       ParseInfo*, Handle<SharedFunctionInfo> shared_info, Isolate*,
-      parsing::ReportErrorsAndStatisticsMode stats_mode);
+      parsing::ReportStatisticsMode stats_mode);
 
   bool AllowsLazyParsingWithoutUnresolvedVariables() const {
     return !MaybeParsingArrowhead() &&

src/parsing/parsing.cc

@@ -7,6 +7,7 @@
 #include <memory>
 
 #include "src/ast/ast.h"
+#include "src/base/v8-fallthrough.h"
 #include "src/execution/vm-state-inl.h"
 #include "src/handles/maybe-handles.h"
 #include "src/objects/objects-inl.h"
@@ -24,14 +25,13 @@ namespace {
 
 void MaybeReportErrorsAndStatistics(ParseInfo* info, Handle<Script> script,
                                     Isolate* isolate, Parser* parser,
-                                    ReportErrorsAndStatisticsMode mode) {
-  if (mode == ReportErrorsAndStatisticsMode::kYes) {
-    if (info->literal() == nullptr) {
-      info->pending_error_handler()->PrepareErrors(isolate,
-                                                   info->ast_value_factory());
-      info->pending_error_handler()->ReportErrors(isolate, script);
-    }
-    parser->UpdateStatistics(isolate, script);
+                                    ReportStatisticsMode mode) {
+  switch (mode) {
+    case ReportStatisticsMode::kYes:
+      parser->UpdateStatistics(isolate, script);
+      break;
+    case ReportStatisticsMode::kNo:
+      break;
   }
 }
 
@@ -39,7 +39,7 @@ void MaybeReportErrorsAndStatistics(ParseInfo* info, Handle<Script> script,
 
 bool ParseProgram(ParseInfo* info, Handle<Script> script,
                   MaybeHandle<ScopeInfo> maybe_outer_scope_info,
-                  Isolate* isolate, ReportErrorsAndStatisticsMode mode) {
+                  Isolate* isolate, ReportStatisticsMode mode) {
   DCHECK(info->flags().is_toplevel());
   DCHECK_NULL(info->literal());
 
@@ -62,12 +62,12 @@ bool ParseProgram(ParseInfo* info, Handle<Script> script,
 }
 
 bool ParseProgram(ParseInfo* info, Handle<Script> script, Isolate* isolate,
-                  ReportErrorsAndStatisticsMode mode) {
+                  ReportStatisticsMode mode) {
   return ParseProgram(info, script, kNullMaybeHandle, isolate, mode);
 }
 
 bool ParseFunction(ParseInfo* info, Handle<SharedFunctionInfo> shared_info,
-                   Isolate* isolate, ReportErrorsAndStatisticsMode mode) {
+                   Isolate* isolate, ReportStatisticsMode mode) {
   DCHECK(!info->flags().is_toplevel());
   DCHECK(!shared_info.is_null());
   DCHECK_NULL(info->literal());
@@ -93,7 +93,7 @@ bool ParseFunction(ParseInfo* info, Handle<SharedFunctionInfo> shared_info,
 }
 
 bool ParseAny(ParseInfo* info, Handle<SharedFunctionInfo> shared_info,
-              Isolate* isolate, ReportErrorsAndStatisticsMode mode) {
+              Isolate* isolate, ReportStatisticsMode mode) {
   DCHECK(!shared_info.is_null());
   if (info->flags().is_toplevel()) {
     MaybeHandle<ScopeInfo> maybe_outer_scope_info;

src/parsing/parsing.h

@@ -15,36 +15,37 @@ class SharedFunctionInfo;
 
 namespace parsing {
 
-enum class ReportErrorsAndStatisticsMode { kYes, kNo };
+enum class ReportStatisticsMode { kYes, kNo };
 
 // Parses the top-level source code represented by the parse info and sets its
 // function literal. Returns false (and deallocates any allocated AST nodes) if
 // parsing failed.
-V8_EXPORT_PRIVATE bool ParseProgram(
-    ParseInfo* info, Handle<Script> script, Isolate* isolate,
-    ReportErrorsAndStatisticsMode mode = ReportErrorsAndStatisticsMode::kYes);
+V8_EXPORT_PRIVATE bool ParseProgram(ParseInfo* info, Handle<Script> script,
+                                    Isolate* isolate,
+                                    ReportStatisticsMode mode);
 
 // Parses the top-level source code represented by the parse info and sets its
 // function literal. Allows passing an |outer_scope| for programs that exist in
 // another scope (e.g. eval). Returns false (and deallocates any allocated AST
 // nodes) if parsing failed.
-V8_EXPORT_PRIVATE bool ParseProgram(
-    ParseInfo* info, Handle<Script> script, MaybeHandle<ScopeInfo> outer_scope,
-    Isolate* isolate,
-    ReportErrorsAndStatisticsMode mode = ReportErrorsAndStatisticsMode::kYes);
+V8_EXPORT_PRIVATE bool ParseProgram(ParseInfo* info, Handle<Script> script,
+                                    MaybeHandle<ScopeInfo> outer_scope,
+                                    Isolate* isolate,
+                                    ReportStatisticsMode mode);
 
 // Like ParseProgram but for an individual function which already has a
 // allocated shared function info.
-V8_EXPORT_PRIVATE bool ParseFunction(
-    ParseInfo* info, Handle<SharedFunctionInfo> shared_info, Isolate* isolate,
-    ReportErrorsAndStatisticsMode mode = ReportErrorsAndStatisticsMode::kYes);
+V8_EXPORT_PRIVATE bool ParseFunction(ParseInfo* info,
+                                     Handle<SharedFunctionInfo> shared_info,
+                                     Isolate* isolate,
+                                     ReportStatisticsMode mode);
 
 // If you don't know whether info->is_toplevel() is true or not, use this method
 // to dispatch to either of the above functions. Prefer to use the above methods
 // whenever possible.
-V8_EXPORT_PRIVATE bool ParseAny(
-    ParseInfo* info, Handle<SharedFunctionInfo> shared_info, Isolate* isolate,
-    ReportErrorsAndStatisticsMode mode = ReportErrorsAndStatisticsMode::kYes);
+V8_EXPORT_PRIVATE bool ParseAny(ParseInfo* info,
+                                Handle<SharedFunctionInfo> shared_info,
+                                Isolate* isolate, ReportStatisticsMode mode);
 
 }  // namespace parsing
 }  // namespace internal

src/parsing/pending-compilation-error-handler.cc

@@ -5,6 +5,7 @@
 #include "src/parsing/pending-compilation-error-handler.h"
 
 #include "src/ast/ast-value-factory.h"
+#include "src/base/export-template.h"
 #include "src/base/logging.h"
 #include "src/debug/debug.h"
 #include "src/execution/isolate.h"
@@ -139,10 +140,13 @@ void PendingCompilationErrorHandler::PrepareErrors(
   ast_value_factory->Internalize(isolate);
   error_details_.Prepare(isolate);
 }
-template void PendingCompilationErrorHandler::PrepareErrors(
-    Isolate* isolate, AstValueFactory* ast_value_factory);
-template void PendingCompilationErrorHandler::PrepareErrors(
-    OffThreadIsolate* isolate, AstValueFactory* ast_value_factory);
+template EXPORT_TEMPLATE_DEFINE(
+    V8_EXPORT_PRIVATE) void PendingCompilationErrorHandler::
+    PrepareErrors(Isolate* isolate, AstValueFactory* ast_value_factory);
+template EXPORT_TEMPLATE_DEFINE(
+    V8_EXPORT_PRIVATE) void PendingCompilationErrorHandler::
+    PrepareErrors(OffThreadIsolate* isolate,
+                  AstValueFactory* ast_value_factory);
 
 void PendingCompilationErrorHandler::ReportErrors(Isolate* isolate,
                                                   Handle<Script> script) const {

src/parsing/pending-compilation-error-handler.h

@@ -49,8 +49,10 @@ class PendingCompilationErrorHandler {
 
   // Handle errors detected during parsing.
   template <typename LocalIsolate>
+  EXPORT_TEMPLATE_DECLARE(V8_EXPORT_PRIVATE)
   void PrepareErrors(LocalIsolate* isolate, AstValueFactory* ast_value_factory);
-  void ReportErrors(Isolate* isolate, Handle<Script> script) const;
+  V8_EXPORT_PRIVATE void ReportErrors(Isolate* isolate,
+                                      Handle<Script> script) const;
 
   // Handle warnings detected during compilation.
   template <typename LocalIsolate>
@@ -139,6 +141,15 @@ class PendingCompilationErrorHandler {
   DISALLOW_COPY_AND_ASSIGN(PendingCompilationErrorHandler);
 };
 
+extern template void PendingCompilationErrorHandler::PrepareErrors(
+    Isolate* isolate, AstValueFactory* ast_value_factory);
+extern template void PendingCompilationErrorHandler::PrepareErrors(
+    OffThreadIsolate* isolate, AstValueFactory* ast_value_factory);
+extern template void PendingCompilationErrorHandler::PrepareWarnings(
+    Isolate* isolate);
+extern template void PendingCompilationErrorHandler::PrepareWarnings(
+    OffThreadIsolate* isolate);
+
 }  // namespace internal
 }  // namespace v8
 #endif  // V8_PARSING_PENDING_COMPILATION_ERROR_HANDLER_H_

test/cctest/parsing/test-preparser.cc

@@ -716,7 +716,8 @@ TEST(PreParserScopeAnalysis) {
       i::ParseInfo using_scope_data(isolate, flags, &using_scope_state);
       using_scope_data.set_consumed_preparse_data(
           i::ConsumedPreparseData::For(isolate, produced_data_on_heap));
-      CHECK(i::parsing::ParseFunction(&using_scope_data, shared, isolate));
+      CHECK(i::parsing::ParseFunction(&using_scope_data, shared, isolate,
+                                      i::parsing::ReportStatisticsMode::kYes));
 
       // Verify that we skipped at least one function inside that scope.
       i::DeclarationScope* scope_with_skipped_functions =
@@ -727,7 +728,8 @@ TEST(PreParserScopeAnalysis) {
       // Parse the lazy function again eagerly to produce baseline data.
       i::UnoptimizedCompileState not_using_scope_state(isolate);
       i::ParseInfo not_using_scope_data(isolate, flags, &not_using_scope_state);
-      CHECK(i::parsing::ParseFunction(&not_using_scope_data, shared, isolate));
+      CHECK(i::parsing::ParseFunction(&not_using_scope_data, shared, isolate,
+                                      i::parsing::ReportStatisticsMode::kYes));
 
       // Verify that we didn't skip anything (there's no preparsed scope data,
       // so we cannot skip).
@@ -764,7 +766,8 @@ TEST(Regress753896) {
 
   // We don't assert that parsing succeeded or that it failed; currently the
   // error is not detected inside lazy functions, but it might be in the future.
-  i::parsing::ParseProgram(&info, script, isolate);
+  i::parsing::ParseProgram(&info, script, isolate,
+                           i::parsing::ReportStatisticsMode::kYes);
 }
 
 TEST(ProducingAndConsumingByteData) {

test/fuzzer/parser.cc

@@ -85,7 +85,12 @@ extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) {
       v8::internal::UnoptimizedCompileFlags::ForScriptCompile(i_isolate,
                                                               *script);
   v8::internal::ParseInfo info(i_isolate, flags, &state);
-  if (!v8::internal::parsing::ParseProgram(&info, script, i_isolate)) {
+  if (!v8::internal::parsing::ParseProgram(
+          &info, script, i_isolate, i::parsing::ReportStatisticsMode::kYes)) {
+    info.pending_error_handler()->PrepareErrors(i_isolate,
+                                                info.ast_value_factory());
+    info.pending_error_handler()->ReportErrors(i_isolate, script);
+
     i_isolate->OptionalRescheduleException(true);
   }
   isolate->RequestGarbageCollectionForTesting(