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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
