From patchwork Thu Mar 12 11:31:57 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: =?utf-8?q?Matthias_M=C3=A4nnich?= X-Patchwork-Id: 39030 From: maennich@google.com (Matthias Maennich) Date: Thu, 12 Mar 2020 12:31:57 +0100 Subject: [PATCH 1/2] configure: add support for thread sanitizer (--enable-tsan) Message-ID: <20200312113158.24055-1-maennich@google.com> Similarly to asan and ubsan, add support for tsan conditionally at configure time. This allows us to track down race conditions etc. * configure.ac: Add configure options for -fsanitize=thread Signed-off-by: Matthias Maennich Reviewed-by: Giuliano Procida --- configure.ac | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/configure.ac b/configure.ac index ba800a72ac7a..50a50c6c8932 100644 --- a/configure.ac +++ b/configure.ac @@ -120,6 +120,12 @@ AC_ARG_ENABLE(asan, ENABLE_ASAN=$enableval, ENABLE_ASAN=no) +AC_ARG_ENABLE(tsan, + AS_HELP_STRING([--enable-tsan=yes|no], + [enable the support of building with -fsanitize=thread)]), + ENABLE_TSAN=$enableval, + ENABLE_TSAN=no) + AC_ARG_ENABLE(ubsan, AS_HELP_STRING([--enable-ubsan=yes|no], [enable the support of building with -fsanitize=undefined)]), @@ -614,6 +620,11 @@ if test x$ENABLE_ASAN = xyes; then CXXFLAGS="$CXXFLAGS -fsanitize=address" fi +if test x$ENABLE_TSAN = xyes; then + CFLAGS="$CFLAGS -fsanitize=thread" + CXXFLAGS="$CXXFLAGS -fsanitize=thread" +fi + if test x$ENABLE_UBSAN = xyes; then CFLAGS="$CFLAGS -fsanitize=undefined" CXXFLAGS="$CXXFLAGS -fsanitize=undefined" @@ -907,6 +918,7 @@ AC_MSG_NOTICE([ Enable python 3 : ${ENABLE_PYTHON3} Enable running tests under Valgrind : ${enable_valgrind} Enable build with -fsanitize=address : ${ENABLE_ASAN} + Enable build with -fsanitize=thread : ${ENABLE_TSAN} Enable build with -fsanitize=undefined : ${ENABLE_UBSAN} Generate html apidoc : ${ENABLE_APIDOC} Generate html manual : ${ENABLE_MANUAL} From patchwork Thu Mar 12 11:31:58 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: =?utf-8?q?Matthias_M=C3=A4nnich?= X-Patchwork-Id: 39031 From: maennich@google.com (Matthias Maennich) Date: Thu, 12 Mar 2020 12:31:58 +0100 Subject: [PATCH 2/2] abg-workers: guard bring_workers_down to avoid dead lock In-Reply-To: <20200312113158.24055-1-maennich@google.com> References: <20200312113158.24055-1-maennich@google.com> Message-ID: <20200312113158.24055-2-maennich@google.com> Since bring_workers_down is not atomic, a worker thread can make a racy read on it while do_bring_workers_down() writes it. That can lead to a deadlock between the worker thread waiting for more work and do_bring_workers_down() waiting for the worker to finish. Address this by guarding all access to bring_workers_down with locking tasks_todo_mutex. This is likely to be dropped after migrating to newer C++ standards supporting std::atomic. * src/abg-workers.cc(do_bring_workers_down): keep task_todo_mutex locked while writing bring_workers_down, (wait_to_execute_a_task): rewrite the loop condition to ensure safe access to bring_workers_down. Signed-off-by: Matthias Maennich Reviewed-by: Giuliano Procida --- src/abg-workers.cc | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/src/abg-workers.cc b/src/abg-workers.cc index eb892688c489..9035854df958 100644 --- a/src/abg-workers.cc +++ b/src/abg-workers.cc @@ -114,7 +114,9 @@ struct worker struct queue::priv { // A boolean to say if the user wants to shutdown the worker - // threads. + // threads. guarded by tasks_todo_mutex. + // TODO: once we have std::atomic, use it and reconsider the + // synchronization around its reads and writes bool bring_workers_down; // The number of worker threads. size_t num_workers; @@ -249,11 +251,11 @@ struct queue::priv while (!tasks_todo.empty()) pthread_cond_wait(&tasks_done_cond, &tasks_todo_mutex); + bring_workers_down = true; pthread_mutex_unlock(&tasks_todo_mutex); // Now that the task queue is empty, drain the workers by waking them up, // letting them finish their final task before termination. - bring_workers_down = true; ABG_ASSERT(pthread_cond_broadcast(&tasks_todo_cond) == 0); for (std::vector::const_iterator i = workers.begin(); @@ -388,7 +390,7 @@ queue::task_done_notify::operator()(const task_sptr&/*task_done*/) queue::priv* worker::wait_to_execute_a_task(queue::priv* p) { - do + while (true) { pthread_mutex_lock(&p->tasks_todo_mutex); // If there is no more tasks to perform and the queue is not to @@ -426,8 +428,15 @@ worker::wait_to_execute_a_task(queue::priv* p) pthread_mutex_unlock(&p->tasks_done_mutex); pthread_cond_signal(&p->tasks_done_cond); } + + // ensure we access bring_workers_down always guarded + bool drop_out = false; + pthread_mutex_lock(&p->tasks_todo_mutex); + drop_out = p->bring_workers_down; + pthread_mutex_unlock(&p->tasks_todo_mutex); + if (drop_out) + break; } - while (!p->bring_workers_down); return p; }