[execution] Only take a single lock while handling interrupts

StackGuard::HandleInterrupts used to take a lock for testing and
clearing each individual interrupt bit. This CL changes that to a
single read up front.

Slight behavioral changes:

1. A TERMINATE_EXECUTION interrupt is now handled first; we
immediately exit and preserve all other interrupts (in case V8 is
later resumed).
2. Since interrupts are read once, it is no longer possible to request
an interrupt *within* HandleInterrupts that will later be processed
within the same HandleInterrupts call.
3. Stack limits are reset immediately after reading the interrupt
bits, and prior to actually processing the interrupts.

Bug: v8:9328
Change-Id: I3048bb413213d11307df49e0014b64a2b43444e0
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1653115
Commit-Queue: Jakob Gruber <jgruber@chromium.org>
Reviewed-by: Yang Guo <yangguo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#62132}

src/execution/execution.cc

@@ -537,11 +537,24 @@ void StackGuard::ClearInterrupt(InterruptFlag flag) {
   if (!has_pending_interrupts(access)) reset_limits(access);
 }
 
-bool StackGuard::CheckAndClearInterrupt(InterruptFlag flag) {
+int StackGuard::FetchAndClearInterrupts() {
   ExecutionAccess access(isolate_);
-  bool result = (thread_local_.interrupt_flags_ & flag);
-  thread_local_.interrupt_flags_ &= ~flag;
-  if (!has_pending_interrupts(access)) reset_limits(access);
+
+  int result = 0;
+  if (thread_local_.interrupt_flags_ & TERMINATE_EXECUTION) {
+    // The TERMINATE_EXECUTION interrupt is special, since it terminates
+    // execution but should leave V8 in a resumable state. If it exists, we only
+    // fetch and clear that bit. On resume, V8 can continue processing other
+    // interrupts.
+    result = TERMINATE_EXECUTION;
+    thread_local_.interrupt_flags_ &= ~TERMINATE_EXECUTION;
+    if (!has_pending_interrupts(access)) reset_limits(access);
+  } else {
+    result = thread_local_.interrupt_flags_;
+    thread_local_.interrupt_flags_ = 0;
+    reset_limits(access);
+  }
+
   return result;
 }
 
@@ -618,6 +631,29 @@ void StackGuard::InitThread(const ExecutionAccess& lock) {
 
 // --- C a l l s   t o   n a t i v e s ---
 
+namespace {
+
+bool TestAndClear(int* bitfield, int mask) {
+  bool result = (*bitfield & mask);
+  *bitfield &= ~mask;
+  return result;
+}
+
+class ShouldBeZeroOnReturnScope final {
+ public:
+#ifndef DEBUG
+  explicit ShouldBeZeroOnReturnScope(int*) {}
+#else   // DEBUG
+  explicit ShouldBeZeroOnReturnScope(int* v) : v_(v) {}
+  ~ShouldBeZeroOnReturnScope() { DCHECK_EQ(*v_, 0); }
+
+ private:
+  int* v_;
+#endif  // DEBUG
+};
+
+}  // namespace
+
 Object StackGuard::HandleInterrupts() {
   TRACE_EVENT0("v8.execute", "V8.HandleInterrupts");
 
@@ -626,48 +662,55 @@ Object StackGuard::HandleInterrupts() {
     isolate_->heap()->MonotonicallyIncreasingTimeInMs();
   }
 
-  if (CheckAndClearInterrupt(GC_REQUEST)) {
+  // Fetch and clear interrupt bits in one go. See comments inside the method
+  // for special handling of TERMINATE_EXECUTION.
+  int interrupt_flags = FetchAndClearInterrupts();
+
+  // All interrupts should be fully processed when returning from this method.
+  ShouldBeZeroOnReturnScope should_be_zero_on_return(&interrupt_flags);
+
+  if (TestAndClear(&interrupt_flags, TERMINATE_EXECUTION)) {
+    TRACE_EVENT0("v8.execute", "V8.TerminateExecution");
+    return isolate_->TerminateExecution();
+  }
+
+  if (TestAndClear(&interrupt_flags, GC_REQUEST)) {
     TRACE_EVENT0(TRACE_DISABLED_BY_DEFAULT("v8.gc"), "V8.GCHandleGCRequest");
     isolate_->heap()->HandleGCRequest();
   }
 
-  if (CheckAndClearInterrupt(GROW_SHARED_MEMORY)) {
+  if (TestAndClear(&interrupt_flags, GROW_SHARED_MEMORY)) {
     TRACE_EVENT0(TRACE_DISABLED_BY_DEFAULT("v8.wasm"),
                  "V8.WasmGrowSharedMemory");
     isolate_->wasm_engine()->memory_tracker()->UpdateSharedMemoryInstances(
         isolate_);
   }
 
-  if (CheckAndClearInterrupt(TERMINATE_EXECUTION)) {
-    TRACE_EVENT0("v8.execute", "V8.TerminateExecution");
-    return isolate_->TerminateExecution();
-  }
-
-  if (CheckAndClearInterrupt(DEOPT_MARKED_ALLOCATION_SITES)) {
+  if (TestAndClear(&interrupt_flags, DEOPT_MARKED_ALLOCATION_SITES)) {
     TRACE_EVENT0(TRACE_DISABLED_BY_DEFAULT("v8.gc"),
                  "V8.GCDeoptMarkedAllocationSites");
     isolate_->heap()->DeoptMarkedAllocationSites();
   }
 
-  if (CheckAndClearInterrupt(INSTALL_CODE)) {
+  if (TestAndClear(&interrupt_flags, INSTALL_CODE)) {
     TRACE_EVENT0(TRACE_DISABLED_BY_DEFAULT("v8.compile"),
                  "V8.InstallOptimizedFunctions");
     DCHECK(isolate_->concurrent_recompilation_enabled());
     isolate_->optimizing_compile_dispatcher()->InstallOptimizedFunctions();
   }
 
-  if (CheckAndClearInterrupt(API_INTERRUPT)) {
+  if (TestAndClear(&interrupt_flags, API_INTERRUPT)) {
     TRACE_EVENT0("v8.execute", "V8.InvokeApiInterruptCallbacks");
     // Callbacks must be invoked outside of ExecutionAccess lock.
     isolate_->InvokeApiInterruptCallbacks();
   }
 
-  if (CheckAndClearInterrupt(LOG_WASM_CODE)) {
+  if (TestAndClear(&interrupt_flags, LOG_WASM_CODE)) {
     TRACE_EVENT0(TRACE_DISABLED_BY_DEFAULT("v8.wasm"), "LogCode");
     isolate_->wasm_engine()->LogOutstandingCodesForIsolate(isolate_);
   }
 
-  if (CheckAndClearInterrupt(WASM_CODE_GC)) {
+  if (TestAndClear(&interrupt_flags, WASM_CODE_GC)) {
     TRACE_EVENT0(TRACE_DISABLED_BY_DEFAULT("v8.wasm"), "WasmCodeGC");
     isolate_->wasm_engine()->ReportLiveCodeFromStackForGC(isolate_);
   }

src/execution/execution.h

@@ -101,7 +101,6 @@ class V8_EXPORT_PRIVATE StackGuard final {
 
 #define V(NAME, Name, id)                                                    \
   inline bool Check##Name() { return CheckInterrupt(NAME); }                 \
-  inline bool CheckAndClear##Name() { return CheckAndClearInterrupt(NAME); } \
   inline void Request##Name() { RequestInterrupt(NAME); }                    \
   inline void Clear##Name() { ClearInterrupt(NAME); }
   INTERRUPT_LIST(V)
@@ -139,7 +138,7 @@ class V8_EXPORT_PRIVATE StackGuard final {
   bool CheckInterrupt(InterruptFlag flag);
   void RequestInterrupt(InterruptFlag flag);
   void ClearInterrupt(InterruptFlag flag);
-  bool CheckAndClearInterrupt(InterruptFlag flag);
+  int FetchAndClearInterrupts();
 
   // You should hold the ExecutionAccess lock when calling this method.
   bool has_pending_interrupts(const ExecutionAccess& lock) {