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

[Zachary Turner via Phabricator via cfe-commits]

zturner added a comment.

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?



================
Comment at: src/UnwindCursor.hpp:20
 #ifndef _LIBUNWIND_HAS_NO_THREADS
-  #include <pthread.h>
+  #ifdef _WIN32
+    #include <windows.h>
----------------
Maybe you want to check `_MSC_VER` here?  I think MinGW compilers will have `pthread` support.


================
Comment at: src/UnwindCursor.hpp:43-45
+#if !defined(_LIBUNWIND_HAS_NO_THREADS) && defined(_WIN32)
+#define pthread_rwlock_t SRWLOCK
+#define PTHREAD_RWLOCK_INITIALIZER SRWLOCK_INIT
----------------
As a matter of principle, I'm not a huge fan of naming things with posix specific names if the implementation is not actually posix.  For example, these functions always return success, but this is not true of the posix functions which can fail for various reasons.  In general, if people are unaware that there is some abstraction behind the scenes for Windows, and they just see a function named `posix_rwlock_rdlock`, then they are going to assume that they can expect the semantics documented on the posix man pages, which is not true.

The alternative is to create a proper abstraction, which might be more work than you're interested in taking on, so consider this just an advisory suggestion.


================
Comment at: src/UnwindCursor.hpp:50
+static bool lockedForWrite = false;
+static int pthread_rwlock_rdlock(pthread_rwlock_t *lock) {
+  AcquireSRWLockShared(lock);
----------------
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.


================
Comment at: src/UnwindCursor.hpp:61-64
+  if (lockedForWrite) {
+    lockedForWrite = false;
+    ReleaseSRWLockExclusive(lock);
+  } else {
----------------
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.


https://reviews.llvm.org/D38704