Allow Debugger and TimeLimit share the same flag

Jeffrey Tan · facebook-github-bot · commit aa74fc3a5cc0 · 2019-08-23T12:58:06.000-07:00
Summary:
Remove build mode for TimeLimit and allow debugger and timelimit to co-exist by sharing the same flag.
It also uses uncatchable JS error instead of C++ exception for time limit.

Reviewed By: davedets, ridiculousfish

Differential Revision: D16657227

fbshipit-source-id: 5c923128940062f01f78f25031cd596df8fcbd66
diff --git a/CMakeLists.txt b/CMakeLists.txt
@@ -57,9 +57,6 @@ set(HERMESVM_PROFILER_JSFUNCTION OFF CACHE BOOL
 set(HERMESVM_PROFILER_NATIVECALL OFF CACHE BOOL
   "Enable native call profiling in hermes VM")
 
-set(HERMESVM_TIMELIMIT OFF CACHE BOOL
-  "Enable time limit in hermes VM")
-
 set(HERMESVM_INDIRECT_THREADING ${DEFAULT_INTERPRETER_THREADING} CACHE BOOL
   "Enable the indirect threaded interpreter")
 
@@ -504,7 +501,6 @@ set(HERMES_LIT_TEST_PARAMS
   hbc_diff=${HERMES_BINARY_DIR}/bin/hbc-diff
   build_mode=${HERMES_ASSUMED_BUILD_MODE_IN_LIT_TEST}
   exception_on_oom_enabled=${HERMESVM_EXCEPTION_ON_OOM}
-  timelimit_enabled=${HERMESVM_TIMELIMIT}
   )
 
 set(LLVM_LIT_ARGS "-sv")
diff --git a/include/hermes/VM/Interpreter.h b/include/hermes/VM/Interpreter.h
@@ -183,14 +183,6 @@ class Interpreter {
   }
 #endif
 
-#ifdef HERMESVM_TIMELIMIT
-  LLVM_ATTRIBUTE_ALWAYS_INLINE
-  static inline void notifyTimeout(Runtime *runtime, const Inst *ip) {
-    runtime->notifyTimeout(ip);
-    llvm_unreachable("Timeout should terminate the execution.");
-  }
-#endif
-
   //===========================================================================
   // Out-of-line implementations of entire instructions.
 
diff --git a/include/hermes/VM/Runtime.h b/include/hermes/VM/Runtime.h
@@ -108,15 +108,6 @@ enum class PropCacheID {
       _COUNT
 };
 
-#ifdef HERMESVM_TIMELIMIT
-/// A std::runtime_error wrapper class for execution timeout.
-class JSTimeoutError : public std::runtime_error {
- public:
-  JSTimeoutError(const std::string &what_arg) : std::runtime_error(what_arg) {}
-  JSTimeoutError(const char *what_arg) : std::runtime_error(what_arg) {}
-};
-#endif
-
 /// The Runtime encapsulates the entire context of a VM. Multiple instances can
 /// exist and are completely independent from each other.
 class Runtime : public HandleRootOwner,
@@ -468,14 +459,22 @@ class Runtime : public HandleRootOwner,
     return commonStorage_.get();
   }
 
-#if defined(HERMES_ENABLE_DEBUGGER) || defined(HERMESVM_TIMELIMIT)
+#if defined(HERMES_ENABLE_DEBUGGER)
   /// Request the interpreter loop to take an asynchronous break at a convenient
-  /// point. This may be called from any thread, or a signal handler.
-  void triggerAsyncBreak() {
-    asyncBreakRequestFlag_.store(1, std::memory_order_relaxed);
+  /// point due to debugger UI request. This may be called from any thread, or a
+  /// signal handler.
+  void triggerDebuggerAsyncBreak() {
+    triggerAsyncBreak(AsyncBreakReasonBits::Debugger);
   }
 #endif
 
+  /// Request the interpreter loop to take an asynchronous break at a convenient
+  /// point due to previous registered timeout. This may be called from any
+  /// thread, or a signal handler.
+  void triggerTimeoutAsyncBreak() {
+    triggerAsyncBreak(AsyncBreakReasonBits::Timeout);
+  }
+
   /// Register \p callback which will be called
   /// during runtime destruction.
   void registerDestructionCallback(DestructionCallback callback) {
@@ -560,6 +559,12 @@ class Runtime : public HandleRootOwner,
   /// Raise an error for the quit function. This error is not catchable.
   ExecutionStatus raiseQuitError();
 
+  /// Raise an error for execution timeout. This error is not catchable.
+  ExecutionStatus raiseTimeoutError();
+
+  /// Utility function to raise a catchable JS error with \p errMessage.
+  ExecutionStatus raiseUncatchableError(llvm::StringRef errMessage);
+
   /// Interpret the current function until it returns or throws and return
   /// CallResult<HermesValue> or the thrown object in 'thrownObject'.
   CallResult<HermesValue> interpretFunction(CodeBlock *newCodeBlock);
@@ -989,44 +994,66 @@ class Runtime : public HandleRootOwner,
   /// A list of callbacks to call before runtime destruction.
   std::vector<DestructionCallback> destructionCallbacks_;
 
-#if defined(HERMES_ENABLE_DEBUGGER) || defined(HERMESVM_TIMELIMIT)
-  /// An atomic boolean set when an async pause is requested.
+  /// Bit flags for async break request reasons.
+  enum class AsyncBreakReasonBits : uint8_t {
+    Debugger = 0x1,
+    Timeout = 0x2,
+  };
+
+  /// An atomic flag set when an async pause is requested.
+  /// It is a bits flag with each bit reserved for different clients
+  /// defined by AsyncBreakReasonBits.
   /// This may be manipulated from multiple threads.
   std::atomic<uint8_t> asyncBreakRequestFlag_{0};
 
-  /// \return zero if no async pause was requsted, nonzero if an async pause was
-  /// requested. If nonzero is returned, the flag is reset to 0.
-  uint8_t testAndClearAsyncBreakRequest() {
+#if defined(HERMES_ENABLE_DEBUGGER)
+  /// \return zero if no debugger async pause was requested, nonzero if an async
+  /// pause was requested. If nonzero is returned, the flag is reset to 0.
+  bool testAndClearDebuggerAsyncBreakRequest() {
+    return testAndClearAsyncBreakRequest(AsyncBreakReasonBits::Debugger);
+  }
+
+  Debugger debugger_{this};
+#endif
+
+  /// \return whether any async break is requested or not.
+  bool hasAsyncBreak() const {
+    return asyncBreakRequestFlag_.load(std::memory_order_relaxed) != 0;
+  }
+
+  /// \return whether async break was requested or not for \p reasonBit. Clear
+  /// \p reasonBit request bit afterward.
+  bool testAndClearAsyncBreakRequest(AsyncBreakReasonBits reasonBit) {
+    /// Note that while the triggerTimeoutAsyncBreak() function may be called
+    /// from any thread, this one may only be called from within the Interpreter
+    /// loop.
     uint8_t flag = asyncBreakRequestFlag_.load(std::memory_order_relaxed);
-    if (LLVM_UNLIKELY(flag)) {
-      /// Note that while the triggerAsyncBreak() function may be called from
-      /// any thread, this one may only be called from within the Interpreter
-      /// loop; thus there is no need for a compare-and-swap. We might race with
-      /// setting the flag again, but currently we do not allow two callers who
-      /// both see a true return from a single flag set.
-      asyncBreakRequestFlag_.store(0, std::memory_order_relaxed);
+    if (LLVM_LIKELY((flag & (uint8_t)reasonBit) == 0)) {
+      // Fast path.
+      return false;
     }
-    return flag;
+    // Clear the flag using CAS.
+    uint8_t oldFlag = asyncBreakRequestFlag_.fetch_and(
+        ~(uint8_t)reasonBit, std::memory_order_relaxed);
+    assert(oldFlag != 0 && "Why is oldFlag zero?");
+    (void)oldFlag;
+    return true;
   }
-#endif
 
-#ifdef HERMESVM_TIMELIMIT
-  void notifyTimeout(const Inst *ip) {
-    char detailBuffer[400];
-    snprintf(
-        detailBuffer,
-        sizeof(detailBuffer),
-        // todo: include time out value.
-        "Javascript execution has timeout.");
-    throw JSTimeoutError(
-        std::string(detailBuffer) + "\ncall stack:\n" +
-        getCallStackNoAlloc(ip));
+  /// \return whether timeout async break was requsted or not. Clear the
+  /// timeout request bit afterward.
+  bool testAndClearTimeoutAsyncBreakRequest() {
+    return testAndClearAsyncBreakRequest(AsyncBreakReasonBits::Timeout);
   }
-#endif
 
-#ifdef HERMES_ENABLE_DEBUGGER
-  Debugger debugger_{this};
-#endif
+  /// Request the interpreter loop to take an asynchronous break
+  /// at a convenient point.
+  void triggerAsyncBreak(AsyncBreakReasonBits reason) {
+    asyncBreakRequestFlag_.fetch_or((uint8_t)reason, std::memory_order_relaxed);
+  }
+
+  /// Notify runtime execution has timeout.
+  ExecutionStatus notifyTimeout();
 
   /// Holds references to persistent BC providers for the lifetime of the
   /// Runtime. This is needed because the identifier table may contain pointers
diff --git a/include/hermes/VM/TimeLimitMonitor.h b/include/hermes/VM/TimeLimitMonitor.h
@@ -4,8 +4,6 @@
  * This source code is licensed under the MIT license found in the LICENSE
  * file in the root directory of this source tree.
  */
-#ifdef HERMESVM_TIMELIMIT
-
 #ifndef HERMES_VM_TIMELIMITMONITOR_H
 #define HERMES_VM_TIMELIMITMONITOR_H
 
@@ -60,7 +58,7 @@ class TimeLimitMonitor {
 
   /// Notify \p runtime to check timeout.
   void notifyRuntimeTimeout(Runtime *runtime) {
-    runtime->triggerAsyncBreak();
+    runtime->triggerTimeoutAsyncBreak();
   }
 
  private:
@@ -84,5 +82,3 @@ class TimeLimitMonitor {
 } // namespace hermes
 
 #endif // HERMES_VM_TIMELIMITMONITOR_H
-
-#endif // HERMESVM_TIMELIMIT
diff --git a/lib/CompilerDriver/CompilerDriver.cpp b/lib/CompilerDriver/CompilerDriver.cpp
@@ -813,11 +813,6 @@ bool validateFlags() {
     if (cl::DumpTarget != DumpBytecode)
       err("You can only dump bytecode for HBC bytecode file.");
   }
-
-  if (cl::EmitDebugInfo && cl::EmitAsyncBreakCheck) {
-    err("Debugger and execution time limit can't be used at the same time");
-  }
-
   return !errored;
 }
 
diff --git a/lib/ConsoleHost/ConsoleHost.cpp b/lib/ConsoleHost/ConsoleHost.cpp
@@ -167,15 +167,15 @@ namespace {
 
 template <typename F>
 auto maybeCatchException(const F &f) -> decltype(f()) {
-#if defined(HERMESVM_EXCEPTION_ON_OOM) || defined(HERMESVM_TIMELIMIT)
+#if defined(HERMESVM_EXCEPTION_ON_OOM)
   try {
     return f();
   } catch (const std::exception &ex) {
     // Report thrown exception and exit the process with failure code.
     llvm::errs() << ex.what();
     exit(1);
   }
-#else // HERMESVM_EXCEPTION_ON_OOM || HERMESVM_TIMELIMIT
+#else // HERMESVM_EXCEPTION_ON_OOM
   return f();
 #endif
 }
@@ -233,12 +233,10 @@ bool executeHBCBytecodeImpl(
   runtime->getJITContext().setDumpJITCode(options.dumpJITCode);
   runtime->getJITContext().setCrashOnError(options.jitCrashOnError);
 
-#ifdef HERMESVM_TIMELIMIT
   if (options.timeLimit > 0) {
     vm::TimeLimitMonitor::getInstance().watchRuntime(
         runtime.get(), options.timeLimit);
   }
-#endif
 
   if (shouldRecordGCStats) {
     statSampler = llvm::make_unique<vm::StatSamplingThread>(
@@ -291,11 +289,9 @@ bool executeHBCBytecodeImpl(
         llvm::errs(), runtime->makeHandle(runtime->getThrownValue()));
   }
 
-#ifdef HERMESVM_TIMELIMIT
   if (options.timeLimit > 0) {
     vm::TimeLimitMonitor::getInstance().unwatchRuntime(runtime.get());
   }
-#endif
 
 #ifdef HERMESVM_PROFILER_OPCODE
   runtime->dumpOpcodeStats(llvm::outs());
diff --git a/lib/VM/Debugger/Debugger.cpp b/lib/VM/Debugger/Debugger.cpp
@@ -87,7 +87,7 @@ static llvm::Optional<ScopeChain> scopeChainForBlock(
 }
 
 void Debugger::triggerAsyncPause() {
-  runtime_->triggerAsyncBreak();
+  runtime_->triggerDebuggerAsyncBreak();
 }
 
 void Debugger::breakAtJumpTarget(InterpreterState &state) {
diff --git a/lib/VM/Interpreter.cpp b/lib/VM/Interpreter.cpp
@@ -102,10 +102,6 @@ HERMES_SLOW_STATISTIC(
 PROFILER_SYMBOLS(INTERP_WRAPPER)
 #endif
 
-#if defined(HERMES_ENABLE_DEBUGGER) && defined(HERMESVM_TIMELIMIT)
-static_assert(false, "Debugger and TimeLimit can't be used at the same time");
-#endif
-
 namespace hermes {
 namespace vm {
 
@@ -1485,7 +1481,7 @@ CallResult<HermesValue> Interpreter::interpretFunction(
     doCall : {
 #ifdef HERMES_ENABLE_DEBUGGER
       // Check for an async debugger request.
-      if (LLVM_UNLIKELY(runtime->testAndClearAsyncBreakRequest())) {
+      if (LLVM_UNLIKELY(runtime->testAndClearDebuggerAsyncBreakRequest())) {
         if (runDebuggerUpdatingState(
                 Debugger::RunReason::AsyncBreak,
                 runtime,
@@ -1561,7 +1557,7 @@ CallResult<HermesValue> Interpreter::interpretFunction(
       CASE(CallDirectLongIndex) {
 #ifdef HERMES_ENABLE_DEBUGGER
         // Check for an async debugger request.
-        if (LLVM_UNLIKELY(runtime->testAndClearAsyncBreakRequest())) {
+        if (LLVM_UNLIKELY(runtime->testAndClearDebuggerAsyncBreakRequest())) {
           if (runDebuggerUpdatingState(
                   Debugger::RunReason::AsyncBreak,
                   runtime,
@@ -1716,7 +1712,7 @@ CallResult<HermesValue> Interpreter::interpretFunction(
       CASE(Ret) {
 #ifdef HERMES_ENABLE_DEBUGGER
         // Check for an async debugger request.
-        if (LLVM_UNLIKELY(runtime->testAndClearAsyncBreakRequest())) {
+        if (LLVM_UNLIKELY(runtime->testAndClearDebuggerAsyncBreakRequest())) {
           if (runDebuggerUpdatingState(
                   Debugger::RunReason::AsyncBreak,
                   runtime,
@@ -1858,25 +1854,26 @@ CallResult<HermesValue> Interpreter::interpretFunction(
       }
 
       CASE(AsyncBreakCheck) {
-#if defined(HERMES_ENABLE_DEBUGGER) || defined(HERMESVM_TIMELIMIT)
-        if (LLVM_UNLIKELY(runtime->testAndClearAsyncBreakRequest())) {
+        if (LLVM_UNLIKELY(runtime->hasAsyncBreak())) {
 #ifdef HERMES_ENABLE_DEBUGGER
-          if (runDebuggerUpdatingState(
-                  Debugger::RunReason::AsyncBreak,
-                  runtime,
-                  curCodeBlock,
-                  ip,
-                  frameRegs) == ExecutionStatus::EXCEPTION)
-            goto exception;
-#endif
-
-#ifdef HERMESVM_TIMELIMIT
-          // notifyTimeout does not return.
-          notifyTimeout(runtime, ip);
+          if (runtime->testAndClearDebuggerAsyncBreakRequest()) {
+            if (runDebuggerUpdatingState(
+                    Debugger::RunReason::AsyncBreak,
+                    runtime,
+                    curCodeBlock,
+                    ip,
+                    frameRegs) == ExecutionStatus::EXCEPTION)
+              goto exception;
+          }
 #endif
-          gcScope.flushToSmallCount(KEEP_HANDLES);
+          if (runtime->testAndClearTimeoutAsyncBreakRequest()) {
+            if (runtime->notifyTimeout() == ExecutionStatus::EXCEPTION) {
+              goto exception;
+            }
+          }
         }
-#endif
+        gcScope.flushToSmallCount(KEEP_HANDLES);
+
         ip = NEXTINST(AsyncBreakCheck);
         DISPATCH;
       }
diff --git a/lib/VM/Runtime.cpp b/lib/VM/Runtime.cpp
@@ -1033,13 +1033,22 @@ ExecutionStatus Runtime::raiseStackOverflow(StackOverflowKind kind) {
 }
 
 ExecutionStatus Runtime::raiseQuitError() {
+  return raiseUncatchableError("Quit");
+}
+
+ExecutionStatus Runtime::raiseTimeoutError() {
+  return raiseUncatchableError("Javascript execution has timeout.");
+}
+
+ExecutionStatus Runtime::raiseUncatchableError(llvm::StringRef errMessage) {
   auto res = JSError::createUncatchable(
       this, Handle<JSObject>::vmcast(&ErrorPrototype));
   if (res == ExecutionStatus::EXCEPTION) {
     return ExecutionStatus::EXCEPTION;
   }
   auto err = makeHandle<JSError>(*res);
-  res = StringPrimitive::create(this, llvm::ASCIIRef{"Quit"});
+  res = StringPrimitive::create(
+      this, llvm::ASCIIRef{errMessage.begin(), errMessage.end()});
   if (res == ExecutionStatus::EXCEPTION) {
     return ExecutionStatus::EXCEPTION;
   }
@@ -1771,5 +1780,10 @@ void Runtime::deserializeImpl(
 }
 #endif
 
+ExecutionStatus Runtime::notifyTimeout() {
+  // TODO: allow a vector of callbacks.
+  return raiseTimeoutError();
+}
+
 } // namespace vm
 } // namespace hermes
diff --git a/lib/VM/TimeLimitMonitor.cpp b/lib/VM/TimeLimitMonitor.cpp
diff --git a/test/hermes/execution-time-limit.js b/test/hermes/execution-time-limit.js
diff --git a/test/lit.cfg b/test/lit.cfg

Original file line number	Diff line number	Diff line change
`@@ -813,11 +813,6 @@ bool validateFlags() {`
`813`	`813`	`if (cl::DumpTarget != DumpBytecode)`
`814`	`814`	`err("You can only dump bytecode for HBC bytecode file.");`
`815`	`815`	`}`
`816`		`-`
`817`		`- if (cl::EmitDebugInfo && cl::EmitAsyncBreakCheck) {`
`818`		`- err("Debugger and execution time limit can't be used at the same time");`
`819`		`- }`
`820`		`-`
`821`	`816`	`return !errored;`
`822`	`817`	`}`
`823`	`818`
Original file line number	Diff line number	Diff line change
`@@ -87,7 +87,7 @@ static llvm::Optional<ScopeChain> scopeChainForBlock(`
`87`	`87`	`}`
`88`	`88`
`89`	`89`	`void Debugger::triggerAsyncPause() {`
`90`		`- runtime_->triggerAsyncBreak();`
	`90`	`+ runtime_->triggerDebuggerAsyncBreak();`
`91`	`91`	`}`
`92`	`92`
`93`	`93`	`void Debugger::breakAtJumpTarget(InterpreterState &state) {`