Re: BUG #17095: ./configure fails thread_test.c when run under TSAN - Mailing list pgsql-bugs
| From | Vitali Lovich |
|---|---|
| Subject | Re: BUG #17095: ./configure fails thread_test.c when run under TSAN |
| Date | |
| Msg-id | CAF8PYMiRWgMhFCrd3Gyxj1o=SDx0bj4+mJJyeDF_qkZcnFMPpQ@mail.gmail.com Whole thread Raw |
| In response to | BUG #17095: ./configure fails thread_test.c when run under TSAN (PG Bug reporting form <noreply@postgresql.org>) |
| Responses |
Re: BUG #17095: ./configure fails thread_test.c when run under TSAN
|
| List | pgsql-bugs |
I've confirmed the following patch (relative to 45b88269a353ad93744772791feb6d01bc7e1e42) resolves the issue. It's predicated on the assumption that gcc or clang (or a compiler with the relevant builtins) are being used to build this.
diff --git a/src/test/thread/thread_test.c b/src/test/thread/thread_test.c
index 5392439146..cef8eb6c82 100644
--- a/src/test/thread/thread_test.c
+++ b/src/test/thread/thread_test.c
@@ -80,6 +80,14 @@ static volatile int thread2_done = 0;
static volatile int errno1_set = 0;
static volatile int errno2_set = 0;
+static void atomic_write(volatile int* ptr, int value) {
+ __atomic_store_n(ptr, value, __ATOMIC_RELAXED);
+}
+
+static int atomic_read(volatile int* ptr) {
+ return __atomic_load_n(ptr, __ATOMIC_RELAXED);
+}
+
#ifndef HAVE_STRERROR_R
static char *strerror_p1;
static char *strerror_p2;
@@ -163,7 +171,7 @@ main(int argc, char *argv[])
exit(1);
}
- while (thread1_done == 0 || thread2_done == 0)
+ while (atomic_read(&thread1_done) == 0 || atomic_read(&thread2_done) == 0)
sched_yield(); /* if this is a portability problem, remove it */
/* Test things while we have thread-local storage */
@@ -312,8 +320,8 @@ func_call_1(void)
* Wait for other thread to set errno. We can't use thread-specific
* locking here because it might affect errno.
*/
- errno1_set = 1;
- while (errno2_set == 0)
+ atomic_write(&errno1_set, 1);
+ while (atomic_read(&errno2_set) == 0)
sched_yield();
#ifdef WIN32
@@ -368,7 +376,7 @@ func_call_1(void)
}
#endif
- thread1_done = 1;
+ atomic_write(&thread1_done, 1);
pthread_mutex_lock(&init_mutex); /* wait for parent to test */
pthread_mutex_unlock(&init_mutex);
}
@@ -403,8 +411,8 @@ func_call_2(void)
* Wait for other thread to set errno. We can't use thread-specific
* locking here because it might affect errno.
*/
- errno2_set = 1;
- while (errno1_set == 0)
+ atomic_write(&errno2_set, 1);
+ while (atomic_read(&errno1_set) == 0)
sched_yield();
#ifdef WIN32
@@ -451,7 +459,7 @@ func_call_2(void)
}
#endif
- thread2_done = 1;
+ atomic_write(&thread2_done, 1);
pthread_mutex_lock(&init_mutex); /* wait for parent to test */
pthread_mutex_unlock(&init_mutex);
}
diff --git a/src/test/thread/thread_test.c b/src/test/thread/thread_test.c
index 5392439146..cef8eb6c82 100644
--- a/src/test/thread/thread_test.c
+++ b/src/test/thread/thread_test.c
@@ -80,6 +80,14 @@ static volatile int thread2_done = 0;
static volatile int errno1_set = 0;
static volatile int errno2_set = 0;
+static void atomic_write(volatile int* ptr, int value) {
+ __atomic_store_n(ptr, value, __ATOMIC_RELAXED);
+}
+
+static int atomic_read(volatile int* ptr) {
+ return __atomic_load_n(ptr, __ATOMIC_RELAXED);
+}
+
#ifndef HAVE_STRERROR_R
static char *strerror_p1;
static char *strerror_p2;
@@ -163,7 +171,7 @@ main(int argc, char *argv[])
exit(1);
}
- while (thread1_done == 0 || thread2_done == 0)
+ while (atomic_read(&thread1_done) == 0 || atomic_read(&thread2_done) == 0)
sched_yield(); /* if this is a portability problem, remove it */
/* Test things while we have thread-local storage */
@@ -312,8 +320,8 @@ func_call_1(void)
* Wait for other thread to set errno. We can't use thread-specific
* locking here because it might affect errno.
*/
- errno1_set = 1;
- while (errno2_set == 0)
+ atomic_write(&errno1_set, 1);
+ while (atomic_read(&errno2_set) == 0)
sched_yield();
#ifdef WIN32
@@ -368,7 +376,7 @@ func_call_1(void)
}
#endif
- thread1_done = 1;
+ atomic_write(&thread1_done, 1);
pthread_mutex_lock(&init_mutex); /* wait for parent to test */
pthread_mutex_unlock(&init_mutex);
}
@@ -403,8 +411,8 @@ func_call_2(void)
* Wait for other thread to set errno. We can't use thread-specific
* locking here because it might affect errno.
*/
- errno2_set = 1;
- while (errno1_set == 0)
+ atomic_write(&errno2_set, 1);
+ while (atomic_read(&errno1_set) == 0)
sched_yield();
#ifdef WIN32
@@ -451,7 +459,7 @@ func_call_2(void)
}
#endif
- thread2_done = 1;
+ atomic_write(&thread2_done, 1);
pthread_mutex_lock(&init_mutex); /* wait for parent to test */
pthread_mutex_unlock(&init_mutex);
}
I didn't predicate using the atomic on TSAN actually being used to build this but that could also be done with something like:
#if defined(__has_feature)
# if __has_feature(thread_sanitizer)
# define ATOMIC_VOLATILES 1
# endif
#endif
#if defined(__has_feature)
# if __has_feature(thread_sanitizer)
# define ATOMIC_VOLATILES 1
# endif
#endif
#if __SANITIZE_THREAD__
#define ATOMIC_VOLATILES 1
#endif
& fixing up the body of atomic_write/atomic_read to change behavior based on #if ATOMIC_VOLATILES.
On Thu, Jul 8, 2021 at 1:02 PM PG Bug reporting form <noreply@postgresql.org> wrote:
The following bug has been logged on the website:
Bug reference: 17095
Logged by: Vitali Lovich
Email address: vlovich+github@gmail.com
PostgreSQL version: 14beta2
Operating system: Arch Linux
Description:
When I run configure with -fsanitize=address (using clang 13 at revision
d3676d4b666ead794fc58bbc7e07aa406dcf487a), it errors out because
`thread_test.c` is violating thread safety.
Specifically it complains about the access of errno2_set, errno1_set,
thread1_done, & thread2_done being read/modified from multiple threads
without any kind of synchronization. AFAICT these TSAN errors are correct.
pgsql-bugs by date: