From d8294d8a8877deae01ed362b5b43d3f39a5c597e Mon Sep 17 00:00:00 2001 From: baldurk Date: Mon, 17 Sep 2018 15:58:10 +0100 Subject: [PATCH] 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 | 1 + renderdoc/common/threading.h | 27 +++ renderdoc/common/threading_tests.cpp | 74 ++++++++ renderdoc/core/core.cpp | 2 + renderdoc/os/os_specific.h | 2 + renderdoc/os/posix/posix_process.cpp | 256 ++++++++++++++++++++++++--- renderdoc/os/win32/win32_process.cpp | 5 + renderdoc/renderdoc.vcxproj | 1 + renderdoc/renderdoc.vcxproj.filters | 3 + 9 files changed, 350 insertions(+), 21 deletions(-) create mode 100644 renderdoc/common/threading_tests.cpp diff --git a/renderdoc/CMakeLists.txt b/renderdoc/CMakeLists.txt index 21035ac4f..5a19da10f 100644 --- a/renderdoc/CMakeLists.txt +++ b/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 diff --git a/renderdoc/common/threading.h b/renderdoc/common/threading.h index 841d36712..59c8ff459 100644 --- a/renderdoc/common/threading.h +++ b/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); diff --git a/renderdoc/common/threading_tests.cpp b/renderdoc/common/threading_tests.cpp new file mode 100644 index 000000000..c20714db0 --- /dev/null +++ b/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 threads; + std::vector 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) \ No newline at end of file diff --git a/renderdoc/core/core.cpp b/renderdoc/core/core.cpp index 74a6dac84..378953f51 100644 --- a/renderdoc/core/core.cpp +++ b/renderdoc/core/core.cpp @@ -366,6 +366,8 @@ RenderDoc::~RenderDoc() m_RemoteThread = 0; } + Process::Shutdown(); + Network::Shutdown(); Threading::Shutdown(); diff --git a/renderdoc/os/os_specific.h b/renderdoc/os/os_specific.h index dde271b69..8df373743 100644 --- a/renderdoc/os/os_specific.h +++ b/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 diff --git a/renderdoc/os/posix/posix_process.cpp b/renderdoc/os/posix/posix_process.cpp index 207e627e9..06ee88f7d 100644 --- a/renderdoc/os/posix/posix_process.cpp +++ b/renderdoc/os/posix/posix_process.cpp @@ -32,7 +32,6 @@ #include #include #include -#include #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 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 waitedChildren; - std::set 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) diff --git a/renderdoc/os/win32/win32_process.cpp b/renderdoc/os/win32/win32_process.cpp index 9332a38da..e6f4c6f7f 100644 --- a/renderdoc/os/win32/win32_process.cpp +++ b/renderdoc/os/win32/win32_process.cpp @@ -1621,3 +1621,8 @@ uint32_t Process::GetCurrentPID() { return (uint32_t)GetCurrentProcessId(); } + +void Process::Shutdown() +{ + // nothing to do +} \ No newline at end of file diff --git a/renderdoc/renderdoc.vcxproj b/renderdoc/renderdoc.vcxproj index 13b2fa5b5..ede7962a9 100644 --- a/renderdoc/renderdoc.vcxproj +++ b/renderdoc/renderdoc.vcxproj @@ -405,6 +405,7 @@ + diff --git a/renderdoc/renderdoc.vcxproj.filters b/renderdoc/renderdoc.vcxproj.filters index 2efbe2b94..72c7357c4 100644 --- a/renderdoc/renderdoc.vcxproj.filters +++ b/renderdoc/renderdoc.vcxproj.filters @@ -809,6 +809,9 @@ 3rdparty\miniz + + Common +