Use linked-list & spinlock to avoid non-safe functions. Closes #1102

* Most functions - including pthreads, malloc, free, etc are not safe to use in
  signal handlers. So we use a simple spin-lock and manual linked-list to manage
  our list of PIDs to wait on.

renderdoc/CMakeLists.txt

@@ -95,6 +95,7 @@ set(sources
     common/threading.h
     common/timing.h
     common/wrapped_pool.h
+    common/threading_tests.cpp
     core/core.cpp
     core/image_viewer.cpp
     core/core.h

renderdoc/common/threading.h

@@ -55,9 +55,36 @@ public:
 private:
   RWLock *m_RW;
 };
+
+class SpinLock
+{
+public:
+  void Lock()
+  {
+    while(!Trylock())
+    {
+      // spin!
+    }
+  }
+  bool Trylock() { return Atomic::CmpExch32(&val, 0, 1) == 0; }
+  void Unlock() { Atomic::CmpExch32(&val, 1, 0); }
+private:
+  volatile int32_t val = 0;
+};
+
+class ScopedSpinLock
+{
+public:
+  ScopedSpinLock(SpinLock &spin) : m_Spin(&spin) { m_Spin->Lock(); }
+  ~ScopedSpinLock() { m_Spin->Unlock(); }
+private:
+  SpinLock *m_Spin;
+};
 };
 
 #define SCOPED_LOCK(cs) Threading::ScopedLock CONCAT(scopedlock, __LINE__)(cs);
 
 #define SCOPED_READLOCK(rw) Threading::ScopedReadLock CONCAT(scopedlock, __LINE__)(rw);
 #define SCOPED_WRITELOCK(rw) Threading::ScopedWriteLock CONCAT(scopedlock, __LINE__)(rw);
+
+#define SCOPED_SPINLOCK(cs) Threading::SpinLock CONCAT(scopedlock, __LINE__)(cs);

renderdoc/common/threading_tests.cpp

@@ -0,0 +1,74 @@
+/******************************************************************************
+ * The MIT License (MIT)
+ *
+ * Copyright (c) 2017-2018 Baldur Karlsson
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ ******************************************************************************/
+
+#include "common/threading.h"
+#include "os/os_specific.h"
+
+#if ENABLED(ENABLE_UNIT_TESTS)
+
+#include "3rdparty/catch/catch.hpp"
+
+static int value = 0;
+
+TEST_CASE("Test spin lock", "[threading]")
+{
+  int finalValue = 0;
+
+  std::vector<Threading::ThreadHandle> threads;
+  std::vector<int> threadcount;
+
+  threads.resize(8);
+  threadcount.resize(8);
+
+  Threading::SpinLock lock;
+
+  for(int i = 0; i < 8; i++)
+  {
+    threadcount[i] = (rand() & 0xff) << 4;
+
+    finalValue += threadcount[i];
+  }
+
+  for(int i = 0; i < 8; i++)
+  {
+    threads[i] = Threading::CreateThread([&lock, &threadcount, i]() {
+      for(int c = 0; c < threadcount[i]; c++)
+      {
+        lock.Lock();
+        value++;
+        lock.Unlock();
+      }
+    });
+  }
+
+  for(Threading::ThreadHandle t : threads)
+  {
+    Threading::JoinThread(t);
+    Threading::CloseThread(t);
+  }
+
+  CHECK(finalValue == value);
+}
+
+#endif    // ENABLED(ENABLE_UNIT_TESTS)

renderdoc/core/core.cpp

@@ -366,6 +366,8 @@ RenderDoc::~RenderDoc()
     m_RemoteThread = 0;
   }
 
+  Process::Shutdown();
+
   Network::Shutdown();
 
   Threading::Shutdown();

renderdoc/os/os_specific.h

@@ -78,6 +78,8 @@ ExecuteResult LaunchAndInjectIntoProcess(const char *app, const char *workingDir
 void *LoadModule(const char *module);
 void *GetFunctionAddress(void *module, const char *function);
 uint32_t GetCurrentPID();
+
+void Shutdown();
 };
 
 namespace Timing

renderdoc/os/posix/posix_process.cpp

@@ -32,7 +32,6 @@
 #include <sys/types.h>
 #include <sys/wait.h>
 #include <unistd.h>
-#include <set>
 #include "api/app/renderdoc_app.h"
 #include "common/threading.h"
 #include "os/os_specific.h"
@@ -56,8 +55,83 @@ int GetIdentPort(pid_t childPid);
 
 #endif
 
-Threading::CriticalSection zombieLock;
-std::set<pid_t> children;
+Threading::SpinLock zombieLock;
+
+struct PIDNode
+{
+  PIDNode *next = NULL;
+  pid_t pid = 0;
+};
+
+struct PIDList
+{
+  PIDNode *head = NULL;
+
+  void append(PIDNode *node)
+  {
+    if(node == NULL)
+      return;
+
+    if(head == NULL)
+    {
+      head = node;
+      return;
+    }
+
+    // we keep this super simple, just always iterate to the tail rather than keeping a tail
+    // pointer. It's less efficient but these are short lists and accessed infrequently.
+    PIDNode *tail = head;
+    while(tail->next)
+      tail = tail->next;
+
+    tail->next = node;
+  }
+
+  void remove(PIDNode *node)
+  {
+    if(node == NULL)
+      return;
+
+    if(node == head)
+    {
+      // if the node we're removing is the head, just update our head pointer
+      head = head->next;
+      node->next = NULL;
+    }
+    else
+    {
+      // if it's not the head, then we iterate with two pointers - the previous element (starting at
+      // head) and the current element (starting at head's next pointer). If cur ever points to our
+      // node, we remove it from the list by updating prev's next pointer to point at cur's next
+      // pointer.
+      for(PIDNode *prev = head, *cur = head->next; cur;)
+      {
+        if(cur == node)
+        {
+          // remove cur from the list by modifying prev
+          prev->next = cur->next;
+          node->next = NULL;
+          return;
+        }
+
+        prev = cur;
+        cur = cur->next;
+      }
+
+      RDCERR("Couldn't find %p in list", node);
+    }
+  }
+
+  PIDNode *pop_front()
+  {
+    PIDNode *ret = head;
+    head = head->next;
+    ret->next = NULL;
+    return ret;
+  }
+};
+
+PIDList children, freeChildren;
 
 #if DISABLED(RDOC_ANDROID)
 
@@ -77,28 +151,42 @@ static void ZombieWaiter(int signum, siginfo_t *handler_info, void *handler_cont
       old_action.sa_handler(signum);
   }
 
-  std::vector<pid_t> waitedChildren;
-  std::set<pid_t> localChildren;
+  // we take the whole list here, process it and wait on all those PIDs, then restore it back at the
+  // end. We only take the list itself, not the free list
+  PIDList waitedChildren;
+  PIDList localChildren;
   {
-    SCOPED_LOCK(zombieLock);
-    localChildren = children;
+    SCOPED_SPINLOCK(zombieLock);
+    std::swap(localChildren.head, children.head);
   }
 
   // wait for any children, but don't block (hang). We must only wait for our own PIDs as waiting
   // for any other handler's PIDs (like Qt's) might lose the one chance they have to wait for them
   // themselves.
-  for(pid_t pid : localChildren)
+  for(PIDNode *cur = localChildren.head; cur;)
   {
+    // advance here immediately before potentially removing the node from the list
+    PIDNode *pid = cur;
+    cur = cur->next;
+
     // remember the child PIDs that we successfully waited on
-    if(waitpid(pid, NULL, WNOHANG) > 0)
-      waitedChildren.push_back(pid);
+    if(waitpid(pid->pid, NULL, WNOHANG) > 0)
+    {
+      // remove cur from the list
+      localChildren.remove(pid);
+
+      // add to the waitedChildren list
+      waitedChildren.append(pid);
+    }
   }
 
-  // remove any waited children from the list
+  // append the list back on rather than swapping again - since in between grabbing it above and
+  // putting it back here there might have been a new child added.
+  // Any waited children from the list are added to the free list
   {
-    SCOPED_LOCK(zombieLock);
-    for(pid_t pid : waitedChildren)
-      children.erase(pid);
+    SCOPED_SPINLOCK(zombieLock);
+    children.append(localChildren.head);
+    freeChildren.append(waitedChildren.head);
   }
 
   // restore errno
@@ -107,13 +195,15 @@ static void ZombieWaiter(int signum, siginfo_t *handler_info, void *handler_cont
 
 static void SetupZombieCollectionHandler()
 {
-  SCOPED_LOCK(zombieLock);
+  {
+    SCOPED_SPINLOCK(zombieLock);
 
-  static bool installed = false;
-  if(installed)
-    return;
+    static bool installed = false;
+    if(installed)
+      return;
 
-  installed = true;
+    installed = true;
+  }
 
   struct sigaction new_action = {};
   sigemptyset(&new_action.sa_mask);
@@ -502,8 +592,19 @@ static pid_t RunProcess(const char *app, const char *workingDir, const char *cmd
     else
     {
       // remember this PID so we can wait on it later
-      SCOPED_LOCK(zombieLock);
-      children.insert(childPid);
+      SCOPED_SPINLOCK(zombieLock);
+
+      PIDNode *node = NULL;
+
+      // take a child from the free list if available, otherwise allocate a new one
+      if(freeChildren.head)
+        node = freeChildren.pop_front();
+      else
+        node = new PIDNode();
+
+      node->pid = childPid;
+
+      children.append(node);
     }
   }
 
@@ -758,3 +859,116 @@ uint32_t Process::GetCurrentPID()
 {
   return (uint32_t)getpid();
 }
+
+void Process::Shutdown()
+{
+  // delete all items in the freeChildren list
+  for(PIDNode *cur = freeChildren.head; cur;)
+  {
+    PIDNode *del = cur;
+    cur = cur->next;
+    delete del;
+  }
+}
+#if ENABLED(ENABLE_UNIT_TESTS)
+
+#include "3rdparty/catch/catch.hpp"
+
+TEST_CASE("Test PID Node list handling", "[osspecific]")
+{
+  PIDNode *a = new PIDNode;
+  a->pid = (pid_t)500;
+
+  PIDList list1;
+
+  list1.append(a);
+
+  CHECK(list1.head == a);
+
+  PIDNode *b = new PIDNode;
+  b->pid = (pid_t)501;
+
+  list1.append(b);
+
+  CHECK(list1.head == a);
+  CHECK(list1.head->next == b);
+
+  PIDNode *c = new PIDNode;
+  c->pid = (pid_t)502;
+
+  list1.append(c);
+
+  CHECK(list1.head == a);
+  CHECK(list1.head->next == b);
+  CHECK(list1.head->next->next == c);
+
+  PIDNode *popped = list1.pop_front();
+
+  CHECK(popped == a);
+
+  CHECK(list1.head == b);
+  CHECK(list1.head->next == c);
+
+  list1.append(popped);
+
+  CHECK(list1.head == b);
+  CHECK(list1.head->next == c);
+  CHECK(list1.head->next->next == a);
+
+  list1.remove(c);
+
+  CHECK(list1.head == b);
+  CHECK(list1.head->next == a);
+
+  list1.append(c);
+
+  CHECK(list1.head == b);
+  CHECK(list1.head->next == a);
+  CHECK(list1.head->next->next == c);
+
+  list1.remove(c);
+
+  CHECK(list1.head == b);
+  CHECK(list1.head->next == a);
+
+  list1.append(c);
+
+  CHECK(list1.head == b);
+  CHECK(list1.head->next == a);
+  CHECK(list1.head->next->next == c);
+
+  list1.remove(b);
+
+  CHECK(list1.head == a);
+  CHECK(list1.head->next == c);
+
+  list1.append(b);
+
+  CHECK(list1.head == a);
+  CHECK(list1.head->next == c);
+  CHECK(list1.head->next->next == b);
+
+  PIDNode *d = new PIDNode;
+  d->pid = (pid_t)900;
+  PIDNode *e = new PIDNode;
+  b->pid = (pid_t)901;
+  PIDNode *f = new PIDNode;
+  c->pid = (pid_t)902;
+
+  PIDList list2;
+
+  list2.append(d);
+  list2.append(e);
+  list2.append(f);
+
+  list1.append(list2.head);
+
+  CHECK(list1.head == a);
+  CHECK(list1.head->next == c);
+  CHECK(list1.head->next->next == b);
+  CHECK(list1.head->next->next->next == d);
+  CHECK(list1.head->next->next->next->next == e);
+  CHECK(list1.head->next->next->next->next->next == f);
+};
+
+#endif    // ENABLED(ENABLE_UNIT_TESTS)

renderdoc/os/win32/win32_process.cpp

@@ -1621,3 +1621,8 @@ uint32_t Process::GetCurrentPID()
 {
   return (uint32_t)GetCurrentProcessId();
 }
+
+void Process::Shutdown()
+{
+  // nothing to do
+}

renderdoc/renderdoc.vcxproj

@@ -405,6 +405,7 @@
     <ClCompile Include="android\jdwp_util.cpp" />
     <ClCompile Include="common\common.cpp" />
     <ClCompile Include="common\dds_readwrite.cpp" />
+    <ClCompile Include="common\threading_tests.cpp" />
     <ClCompile Include="core\core.cpp" />
     <ClCompile Include="core\image_viewer.cpp" />
     <ClCompile Include="core\plugins.cpp" />

renderdoc/renderdoc.vcxproj.filters

@@ -809,6 +809,9 @@
     <ClCompile Include="3rdparty\miniz\miniz.c">
       <Filter>3rdparty\miniz</Filter>
     </ClCompile>
+    <ClCompile Include="common\threading_tests.cpp">
+      <Filter>Common</Filter>
+    </ClCompile>
   </ItemGroup>
   <ItemGroup>
     <None Include="os\win32\comexport.def">