[offthread] Allow errors in off-thread compile

Use the PendingCompilationErrorHandler in the UnoptimizedCompileState
class to prepare compilation errors off-thread, and report them during
the merge into the main thread.

Bug: chromium:1011762
Change-Id: I3ad5078e25c176aa30743500714b2fad838d3ce8
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2105354
Commit-Queue: Leszek Swirski <leszeks@chromium.org>
Reviewed-by: Ross McIlroy <rmcilroy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#67353}

src/codegen/compiler.cc

@@ -45,6 +45,7 @@
 #include "src/parsing/parse-info.h"
 #include "src/parsing/parser.h"
 #include "src/parsing/parsing.h"
+#include "src/parsing/pending-compilation-error-handler.h"
 #include "src/parsing/rewriter.h"
 #include "src/parsing/scanner-character-streams.h"
 #include "src/snapshot/code-serializer.h"
@@ -1014,8 +1015,29 @@ bool FailWithPendingException(Isolate* isolate, Handle<Script> script,
 bool FailWithPendingException(OffThreadIsolate* isolate, Handle<Script> script,
                               ParseInfo* parse_info,
                               Compiler::ClearExceptionFlag flag) {
-  // TODO(leszeks): Implement.
-  UNREACHABLE();
+  // Off-thread compilation is a "standard compilation path", on which we don't
+  // want to override existing Isolate errors (after merging with the main
+  // thread), so we should expect to always keep the exception.
+  DCHECK_EQ(flag, Compiler::KEEP_EXCEPTION);
+  if (parse_info->pending_error_handler()->has_pending_error()) {
+    parse_info->pending_error_handler()->PrepareErrorsOffThread(
+        isolate, script, parse_info->ast_value_factory());
+  }
+  return false;
+}
+
+bool FailWithPendingExceptionAfterOffThreadFinalization(
+    Isolate* isolate, Handle<Script> script,
+    PendingCompilationErrorHandler* pending_error_handler) {
+  if (!isolate->has_pending_exception()) {
+    if (pending_error_handler->has_pending_error()) {
+      pending_error_handler->ReportErrorsAfterOffThreadFinalization(isolate,
+                                                                    script);
+    } else {
+      isolate->StackOverflow();
+    }
+  }
+  return false;
 }
 
 void FinalizeScriptCompilation(Isolate* isolate, Handle<Script> script,
@@ -1290,41 +1312,58 @@ void BackgroundCompileTask::Run() {
     // Parsing has succeeded, compile.
     outer_function_job_ = CompileOnBackgroundThread(
         info_.get(), compile_state_.allocator(), &inner_function_jobs_);
+  }
   // Save the language mode and record whether we collected source positions.
   language_mode_ = info_->language_mode();
   collected_source_positions_ = info_->flags().collect_source_positions();
 
-    if (finalize_on_background_thread_) {
+  if (!finalize_on_background_thread_) return;
+
+  // ---
+  // At this point, off-thread compilation has completed and we are off-thread
+  // finalizing.
+  // ---
+
   DCHECK(info_->flags().is_toplevel());
 
   TRACE_EVENT0(TRACE_DISABLED_BY_DEFAULT("v8.compile"),
                "V8.FinalizeCodeBackground");
 
-      off_thread_isolate_->PinToCurrentThread();
+  OffThreadIsolate* isolate = off_thread_isolate();
+  isolate->PinToCurrentThread();
 
-      OffThreadHandleScope handle_scope(off_thread_isolate_.get());
+  OffThreadHandleScope handle_scope(isolate);
 
   // We don't have the script source or the script origin yet, so use a few
   // default values for them. These will be fixed up during the main-thread
   // merge.
   Handle<Script> script = info_->CreateScript(
-          off_thread_isolate_.get(),
-          off_thread_isolate_->factory()->empty_string(), kNullMaybeHandle,
+      isolate, isolate->factory()->empty_string(), kNullMaybeHandle,
       ScriptOriginOptions(), NOT_NATIVES_CODE);
 
-      Handle<SharedFunctionInfo> outer_function_sfi =
-          FinalizeTopLevel(info_.get(), script, off_thread_isolate_.get(),
-                           outer_function_job_.get(), &inner_function_jobs_)
-              .ToHandleChecked();
+  MaybeHandle<SharedFunctionInfo> maybe_result;
+  if (info_->literal() != nullptr) {
+    maybe_result =
+        FinalizeTopLevel(info_.get(), script, isolate,
+                         outer_function_job_.get(), &inner_function_jobs_);
+
+    parser_->HandleSourceURLComments(isolate, script);
+  } else {
+    DCHECK(!outer_function_job_);
+  }
 
-      parser_->HandleSourceURLComments(off_thread_isolate_.get(), script);
+  Handle<SharedFunctionInfo> result;
+  if (!maybe_result.ToHandle(&result)) {
+    compile_state_.pending_error_handler()->PrepareErrorsOffThread(
+        isolate, script, info_->ast_value_factory());
+  }
 
-      outer_function_sfi_ =
-          off_thread_isolate_->TransferHandle(outer_function_sfi);
+  outer_function_sfi_ = isolate->TransferHandle(maybe_result);
+  script_ = isolate->TransferHandle(script);
 
   TRACE_EVENT0(TRACE_DISABLED_BY_DEFAULT("v8.compile"),
                "V8.FinalizeCodeBackground.Finish");
-      off_thread_isolate_->FinishOffThread();
+  isolate->FinishOffThread();
 
   TRACE_EVENT0(TRACE_DISABLED_BY_DEFAULT("v8.compile"),
                "V8.FinalizeCodeBackground.ReleaseParser");
@@ -1334,8 +1373,6 @@ void BackgroundCompileTask::Run() {
   info_.reset();
   outer_function_job_.reset();
   inner_function_jobs_.clear();
-    }
-  }
 }
 
 // ----------------------------------------------------------------------------
@@ -2441,12 +2478,22 @@ Compiler::GetSharedFunctionInfoForStreamedScript(
 
       task->off_thread_isolate()->Publish(isolate);
 
-      Handle<SharedFunctionInfo> sfi = task->outer_function_sfi();
-      Handle<Script> script(Script::cast(sfi->script()), isolate);
+      maybe_result = task->outer_function_sfi();
+      Handle<Script> script = task->script();
 
       FixUpOffThreadAllocatedScript(isolate, script, source, script_details,
                                     origin_options, NOT_NATIVES_CODE);
 
+      if (maybe_result.is_null()) {
+        // Parsing has failed - report error messages.
+        FailWithPendingExceptionAfterOffThreadFinalization(
+            isolate, script, task->pending_error_handler());
+      } else {
+        // Report any warnings generated during compilation.
+        if (task->pending_error_handler()->has_pending_warnings()) {
+          task->pending_error_handler()->ReportWarnings(isolate, script);
+        }
+
         // It's possible that source position collection was enabled after the
         // background compile was started (for instance by enabling the cpu
         // profiler), and the compiled bytecode is missing source positions. So,
@@ -2466,8 +2513,7 @@ Compiler::GetSharedFunctionInfoForStreamedScript(
             }
           });
         }
-
-      maybe_result = sfi;
+      }
     } else {
       ParseInfo* parse_info = task->info();
       DCHECK(parse_info->flags().is_toplevel());

src/codegen/compiler.h

@@ -16,6 +16,7 @@
 #include "src/logging/code-events.h"
 #include "src/objects/contexts.h"
 #include "src/parsing/parse-info.h"
+#include "src/parsing/pending-compilation-error-handler.h"
 #include "src/utils/allocation.h"
 #include "src/zone/zone.h"
 
@@ -389,10 +390,17 @@ class V8_EXPORT_PRIVATE BackgroundCompileTask {
     return finalize_on_background_thread_;
   }
   OffThreadIsolate* off_thread_isolate() { return off_thread_isolate_.get(); }
-  Handle<SharedFunctionInfo> outer_function_sfi() {
+  PendingCompilationErrorHandler* pending_error_handler() {
+    return compile_state_.pending_error_handler();
+  }
+  MaybeHandle<SharedFunctionInfo> outer_function_sfi() {
     DCHECK_NOT_NULL(off_thread_isolate_);
     return outer_function_sfi_.ToHandle();
   }
+  Handle<Script> script() {
+    DCHECK_NOT_NULL(off_thread_isolate_);
+    return script_.ToHandle();
+  }
 
  private:
   // Data needed for parsing, and data needed to to be passed between thread
@@ -412,7 +420,8 @@ class V8_EXPORT_PRIVATE BackgroundCompileTask {
   // should add some stricter type-safety or DCHECKs to ensure that the user of
   // the task knows this.
   std::unique_ptr<OffThreadIsolate> off_thread_isolate_;
-  OffThreadTransferHandle<SharedFunctionInfo> outer_function_sfi_;
+  OffThreadTransferMaybeHandle<SharedFunctionInfo> outer_function_sfi_;
+  OffThreadTransferHandle<Script> script_;
 
   // Single function data for top-level function compilation.
   int start_position_;

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/logging.h"
 #include "src/debug/debug.h"
 #include "src/execution/isolate.h"
 #include "src/execution/messages.h"
@@ -14,15 +15,30 @@
 namespace v8 {
 namespace internal {
 
+void PendingCompilationErrorHandler::MessageDetails::TransferOffThreadHandle(
+    OffThreadIsolate* isolate) {
+  DCHECK_NE(type_, kMainThreadHandle);
+  if (type_ != kAstRawString) return;
+  arg_transfer_handle_ = isolate->TransferHandle(arg_->string());
+  type_ = kOffThreadTransferHandle;
+}
+
 Handle<String> PendingCompilationErrorHandler::MessageDetails::ArgumentString(
     Isolate* isolate) const {
-  if (arg_ != nullptr) return arg_->string();
-  if (char_arg_ != nullptr) {
+  switch (type_) {
+    case kAstRawString:
+      return arg_->string();
+    case kNone:
+      return isolate->factory()->undefined_string();
+    case kConstCharString:
       return isolate->factory()
           ->NewStringFromUtf8(CStrVector(char_arg_))
           .ToHandleChecked();
+    case kMainThreadHandle:
+      return arg_handle_;
+    case kOffThreadTransferHandle:
+      return arg_transfer_handle_.ToHandle();
   }
-  return isolate->factory()->undefined_string();
 }
 
 MessageLocation PendingCompilationErrorHandler::MessageDetails::GetLocation(
@@ -37,8 +53,7 @@ void PendingCompilationErrorHandler::ReportMessageAt(int start_position,
   if (has_pending_error_) return;
   has_pending_error_ = true;
 
-  error_details_ =
-      MessageDetails(start_position, end_position, message, nullptr, arg);
+  error_details_ = MessageDetails(start_position, end_position, message, arg);
 }
 
 void PendingCompilationErrorHandler::ReportMessageAt(int start_position,
@@ -48,8 +63,7 @@ void PendingCompilationErrorHandler::ReportMessageAt(int start_position,
   if (has_pending_error_) return;
   has_pending_error_ = true;
 
-  error_details_ =
-      MessageDetails(start_position, end_position, message, arg, nullptr);
+  error_details_ = MessageDetails(start_position, end_position, message, arg);
 }
 
 void PendingCompilationErrorHandler::ReportWarningAt(int start_position,
@@ -57,7 +71,7 @@ void PendingCompilationErrorHandler::ReportWarningAt(int start_position,
                                                      MessageTemplate message,
                                                      const char* arg) {
   warning_messages_.emplace_front(
-      MessageDetails(start_position, end_position, message, nullptr, arg));
+      MessageDetails(start_position, end_position, message, arg));
 }
 
 void PendingCompilationErrorHandler::ReportWarnings(Isolate* isolate,
@@ -77,8 +91,12 @@ void PendingCompilationErrorHandler::ReportWarnings(Isolate* isolate,
 
 void PendingCompilationErrorHandler::ReportWarnings(OffThreadIsolate* isolate,
                                                     Handle<Script> script) {
-  // TODO(leszeks): Do nothing, re-report on the main thread.
-  UNREACHABLE();
+  // Change any AstRawStrings to raw object pointers before the Ast Zone dies,
+  // re-report later on the main thread.
+  DCHECK(!has_pending_error());
+  for (MessageDetails& warning : warning_messages_) {
+    warning.TransferOffThreadHandle(isolate);
+  }
 }
 
 void PendingCompilationErrorHandler::ReportErrors(
@@ -94,6 +112,28 @@ void PendingCompilationErrorHandler::ReportErrors(
   }
 }
 
+void PendingCompilationErrorHandler::PrepareErrorsOffThread(
+    OffThreadIsolate* isolate, Handle<Script> script,
+    AstValueFactory* ast_value_factory) {
+  if (!stack_overflow()) {
+    DCHECK(has_pending_error());
+    // Internalize ast values for later throwing the pending error.
+    ast_value_factory->Internalize(isolate);
+    error_details_.TransferOffThreadHandle(isolate);
+  }
+}
+
+void PendingCompilationErrorHandler::ReportErrorsAfterOffThreadFinalization(
+    Isolate* isolate, Handle<Script> script) {
+  if (stack_overflow()) {
+    isolate->StackOverflow();
+  } else {
+    DCHECK(has_pending_error());
+    // Ast values should already be internalized.
+    ThrowPendingError(isolate, script);
+  }
+}
+
 void PendingCompilationErrorHandler::ThrowPendingError(Isolate* isolate,
                                                        Handle<Script> script) {
   if (!has_pending_error_) return;

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

@@ -10,6 +10,7 @@
 #include "src/base/macros.h"
 #include "src/common/globals.h"
 #include "src/common/message-template.h"
+#include "src/execution/off-thread-isolate.h"
 #include "src/handles/handles.h"
 
 namespace v8 {
@@ -49,6 +50,14 @@ class PendingCompilationErrorHandler {
   // Handle errors detected during parsing.
   void ReportErrors(Isolate* isolate, Handle<Script> script,
                     AstValueFactory* ast_value_factory);
+  // Prepare errors detected during off-thread parsing, to be reported later on
+  // the main thread.
+  void PrepareErrorsOffThread(OffThreadIsolate* isolate, Handle<Script> script,
+                              AstValueFactory* ast_value_factory);
+  // Report errors detected during off-thread parsing, which were prepared
+  // off-thread during finalization by the above method.
+  void ReportErrorsAfterOffThreadFinalization(Isolate* isolate,
+                                              Handle<Script> script);
 
   // Handle warnings detected during compilation.
   void ReportWarnings(Isolate* isolate, Handle<Script> script);
@@ -77,27 +86,49 @@ class PendingCompilationErrorHandler {
         : start_position_(-1),
           end_position_(-1),
           message_(MessageTemplate::kNone),
-          arg_(nullptr),
-          char_arg_(nullptr) {}
+          type_(kNone) {}
     MessageDetails(int start_position, int end_position,
-                   MessageTemplate message, const AstRawString* arg,
-                   const char* char_arg)
+                   MessageTemplate message, const AstRawString* arg)
         : start_position_(start_position),
           end_position_(end_position),
           message_(message),
           arg_(arg),
-          char_arg_(char_arg) {}
+          type_(arg ? kAstRawString : kNone) {}
+    MessageDetails(int start_position, int end_position,
+                   MessageTemplate message, const char* char_arg)
+        : start_position_(start_position),
+          end_position_(end_position),
+          message_(message),
+          char_arg_(char_arg),
+          type_(char_arg_ ? kConstCharString : kNone) {}
 
     Handle<String> ArgumentString(Isolate* isolate) const;
     MessageLocation GetLocation(Handle<Script> script) const;
     MessageTemplate message() const { return message_; }
 
+    // After off-thread finalization, the Ast Zone will be deleted, so before
+    // that happens we have to transfer any string handles.
+    void TransferOffThreadHandle(OffThreadIsolate* isolate);
+
    private:
+    enum Type {
+      kNone,
+      kAstRawString,
+      kConstCharString,
+      kOffThreadTransferHandle,
+      kMainThreadHandle
+    };
+
     int start_position_;
     int end_position_;
     MessageTemplate message_;
+    union {
       const AstRawString* arg_;
       const char* char_arg_;
+      OffThreadTransferHandle<String> arg_transfer_handle_;
+      Handle<String> arg_handle_;
+    };
+    Type type_;
   };
 
   void ThrowPendingError(Isolate* isolate, Handle<Script> script);