Preserve ThreadNexus phase_flag state.

The ThreadNexus::phase_flag is part of the thread synchronization mechanism and additionally used to put the system into a sort of 'singled-threaded mode' that can run arbitrary code. In order to preserve this, we have to know that when we 'acquired' the lock whether the lock was already held by us. If it was, we don't want to release it.
rubinius · Jun 9, 2016 · 3f7bc1e · 3f7bc1e
1 parent ec8d11c
commit 3f7bc1e
Show file tree

Hide file tree

Showing 4 changed files with 38 additions and 15 deletions.
diff --git a/machine/builtin/system.cpp b/machine/builtin/system.cpp
@@ -386,14 +386,16 @@ namespace rubinius {
     state->shared().machine_threads()->before_fork_exec(state);
     state->memory()->set_interrupt();
 
-    state->vm()->thread_nexus()->lock(state->vm());
+    ThreadNexus::LockStatus status = state->vm()->thread_nexus()->lock(state->vm());
 
     // If execvp() succeeds, we'll read EOF and know.
     fcntl(errors_fd, F_SETFD, FD_CLOEXEC);
 
     int pid = ::fork();
 
-    state->vm()->thread_nexus()->unlock();
+    if(status == ThreadNexus::eLocked) {
+      state->vm()->thread_nexus()->unlock();
+    }
 
     if(pid == 0) {
       // We're in the child...
@@ -721,7 +723,7 @@ namespace rubinius {
 
     state->shared().machine_threads()->before_exec(state);
 
-    state->vm()->thread_nexus()->lock(state->vm());
+    ThreadNexus::LockStatus status = state->vm()->thread_nexus()->lock(state->vm());
 
     void* old_handlers[NSIG];
 
@@ -762,7 +764,9 @@ namespace rubinius {
       sigaction(i, &action, NULL);
     }
 
-    state->vm()->thread_nexus()->unlock();
+    if(status == ThreadNexus::eLocked) {
+      state->vm()->thread_nexus()->unlock();
+    }
 
     state->shared().machine_threads()->after_exec(state);
 
@@ -855,11 +859,13 @@ namespace rubinius {
     state->shared().machine_threads()->before_fork(state);
     state->memory()->set_interrupt();
 
-    state->vm()->thread_nexus()->lock(state->vm());
+    ThreadNexus::LockStatus status = state->vm()->thread_nexus()->lock(state->vm());
 
     int pid = ::fork();
 
-    state->vm()->thread_nexus()->unlock();
+    if(status == ThreadNexus::eLocked) {
+      state->vm()->thread_nexus()->unlock();
+    }
 
     if(pid > 0) {
       // We're in the parent...

diff --git a/machine/thread_nexus.hpp b/machine/thread_nexus.hpp
@@ -49,6 +49,12 @@ namespace rubinius {
       eWaiting    = 0x82,
     };
 
+    enum LockStatus {
+      eNotLocked  = 0x0,
+      eHeldLock   = 0x1,
+      eLocked     = 0x2,
+    };
+
     const static int cYieldingPhase = 0x80;
 
     ThreadNexus()
@@ -67,6 +73,12 @@ namespace rubinius {
       rubinius::bug("attempt to destroy ThreadNexus");
     }
 
+  private:
+    LockStatus to_lock_status(bool flag) {
+      return flag ? eHeldLock : eLocked;
+    }
+
+  public:
     std::mutex& fork_mutex() {
       return fork_mutex_;
     }
@@ -119,7 +131,7 @@ namespace rubinius {
 
     bool waiting_lock(VM* vm);
 
-    bool try_lock(VM* vm) {
+    LockStatus try_lock(VM* vm) {
       while(stop_p()) {
         bool held = waiting_lock(vm);
 
@@ -128,7 +140,7 @@ namespace rubinius {
           if(try_checkpoint(vm)) {
             if(stop_p()) {
               unset_stop();
-              return true;
+              return to_lock_status(held);
             }
           }
         }
@@ -137,14 +149,15 @@ namespace rubinius {
         if(!held) unlock();
       }
 
-      return false;
+      return eNotLocked;
     }
 
-    void lock(VM* vm) {
-      waiting_lock(vm);
+    LockStatus lock(VM* vm) {
+      bool held = waiting_lock(vm);
       set_stop();
       checkpoint(vm);
       unset_stop();
+      return to_lock_status(held);
     }
 
     void unlock() {

diff --git a/machine/thread_phase.hpp b/machine/thread_phase.hpp
@@ -14,16 +14,19 @@ namespace rubinius {
    */
   class LockPhase {
     State* state_;
+    ThreadNexus::LockStatus status_;
 
   public:
     LockPhase(STATE)
       : state_(state)
     {
-      state->vm()->thread_nexus()->lock(state->vm());
+      status_ = state->vm()->thread_nexus()->lock(state->vm());
     }
 
     ~LockPhase() {
-      state_->vm()->thread_nexus()->unlock();
+      if(status_ == ThreadNexus::eLocked) {
+        state_->vm()->thread_nexus()->unlock();
+      }
     }
   };
 

diff --git a/machine/vm.hpp b/machine/vm.hpp
@@ -436,12 +436,13 @@ namespace rubinius {
     void checkpoint(STATE) {
       metrics().machine.checkpoints++;
 
-      if(thread_nexus_->try_lock(this)) {
+      ThreadNexus::LockStatus status = thread_nexus_->try_lock(this);
+      if(status != ThreadNexus::eNotLocked) {
         metrics().machine.stops++;
 
         collect_maybe(state);
 
-        thread_nexus_->unlock();
+        if(status == ThreadNexus::eLocked) thread_nexus_->unlock();
       }
 
       if(profile_counter_++ >= profile_interval_) {