[PATCH] D38704: [libunwind] Emulate pthread rwlocks via SRW locks for windows

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

In https://reviews.llvm.org/D38704#892479, @zturner wrote:

> I'm a little nervous about re-inventing a poor man's version of a reader writer lock.  Can we not just copy LLVM's?


I guess I could have a look to see how much extra either kitchen sink it would bring. Since it almost mapped 1:1 to the windows functions, I thought it wouldn't end up too large - unfortunately the return values and unlock functions made it a bit harder though.



================
Comment at: src/UnwindCursor.hpp:20
 #ifndef _LIBUNWIND_HAS_NO_THREADS
-  #include <pthread.h>
+  #ifdef _WIN32
+    #include <windows.h>
----------------
zturner wrote:
> Maybe you want to check `_MSC_VER` here?  I think MinGW compilers will have `pthread` support.
MinGW compilers can have optional pthread support (it's not a feature of the compiler itself but an extra library that one can choose to build), but not everybody wants to use it and at least I wouldn't want to use it as mandatory dependency for any C++ support based on libunwind.


================
Comment at: src/UnwindCursor.hpp:50
+static bool lockedForWrite = false;
+static int pthread_rwlock_rdlock(pthread_rwlock_t *lock) {
+  AcquireSRWLockShared(lock);
----------------
zturner wrote:
> This doesn't seem like what you want.  a global `static` function / variable in a header file is going to be duplicated in every translation unit.  i.e. two translation units will have different copies of `lockedForWrite`.  Same goes for the rest of the functions.
Oh, right - I would need to make it match the static class member `_lock` instead.


================
Comment at: src/UnwindCursor.hpp:61-64
+  if (lockedForWrite) {
+    lockedForWrite = false;
+    ReleaseSRWLockExclusive(lock);
+  } else {
----------------
zturner wrote:
> Doesn't `lockedForWrite` need to be atomic?  If it is not atomic, there is no guarantee that thread 2 will see the results of thread 1's modifications with any kind of reasonable order.
I don't think it needs to be atomic, although the `rdlock` function perhaps shouldn't touch it at all. It only ever gets set to true once we have an exclusive lock, and in those cases gets set back to false before the exclusive lock gets released. So without touching it in the `rdlock` function, we only ever write to it while holding the exclusive write lock.


https://reviews.llvm.org/D38704