[PATCH 1/2] configure: add support for thread sanitizer (--enable-tsan)

[PATCH 1/2] configure: add support for thread sanitizer (--enable-tsan)

[Matthias Maennich]

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 <maennich@google.com>
---
 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}
-- 
2.25.1.481.gfbce0eb801-goog

[PATCH 2/2] abg-workers: guard bring_workers_down to avoid dead lock

[Matthias Maennich]

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 <maennich@google.com>
---
 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<bool>, 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<worker>::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;
 }
-- 
2.25.1.481.gfbce0eb801-goog

Re: [PATCH 2/2] abg-workers: guard bring_workers_down to avoid dead lock

[Giuliano Procida]

Hi.

This looks good to me.

On Thu, 12 Mar 2020 at 11:32, 'Matthias Maennich' via kernel-team
<kernel-team@android.com> wrote:
>
> 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.
>

Reviewed-by: Giuliano Procida <gprocida@google.com>

Regards,
Giuliano.

> Signed-off-by: Matthias Maennich <maennich@google.com>
> ---
>  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<bool>, 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<worker>::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;
>  }
> --
> 2.25.1.481.gfbce0eb801-goog
>
> --
> To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com.
>

Re: [PATCH 2/2] abg-workers: guard bring_workers_down to avoid dead lock

[Dodji Seketeli]

Hello Matthias,

Matthias Maennich <maennich@google.com> a écrit:

> 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.

Applied to master.

Thanks!

-- 
		Dodji

Re: [PATCH 1/2] configure: add support for thread sanitizer (--enable-tsan)

[Giuliano Procida]

On Thu, 12 Mar 2020 at 11:32, 'Matthias Maennich' via kernel-team
<kernel-team@android.com> wrote:
>
> 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

Please verify that there's no need to add the various flags to LDFLAGS
(or no difference if you do so) thanks to CXX anf CFLAGS.
msan (for which there isn't support yet; it also requires Clang and
lld in particular) definitely requires its flag at link time.

Assuming this is the case:
Reviewed-by: Giuliano Procida <gprocida@google.com>

It's also noticeable that turning on some sanitisers affects abidiff
output. This can't be good and we should look into it when we have
time.

Regards,
Giuliano.

> Signed-off-by: Matthias Maennich <maennich@google.com>
> ---
>  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}
> --
> 2.25.1.481.gfbce0eb801-goog
>
> --
> To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com.
>

Re: [PATCH 1/2] configure: add support for thread sanitizer (--enable-tsan)

[Matthias Maennich]

Hi!

On Mon, Mar 16, 2020 at 03:00:02PM +0000, Giuliano Procida wrote:
>On Thu, 12 Mar 2020 at 11:32, 'Matthias Maennich' via kernel-team
><kernel-team@android.com> wrote:
>>
>> 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
>
>Please verify that there's no need to add the various flags to LDFLAGS
>(or no difference if you do so) thanks to CXX anf CFLAGS.
>msan (for which there isn't support yet; it also requires Clang and

Will post a patch for msan right away :-)

>lld in particular) definitely requires its flag at link time.

Autotools generates the makefiles such that CXX or CC is used for
linking and that gets passed the CXXFLAGS / CFLAGS. Having the compiler
drive the link step allows it to translate the compile flag into the
full set of linker flags. Hence, we do not need to set any LDFLAGS.

TL;DR: we are good :-)

>
>Assuming this is the case:
>Reviewed-by: Giuliano Procida <gprocida@google.com>
>
>It's also noticeable that turning on some sanitisers affects abidiff
>output. This can't be good and we should look into it when we have
>time.

When running this in our hermetic build toolchain that appears to be not
an issue. Though I do not yet see where this happens when running with
a normal autotools based build.

Cheers,
Matthias

>
>Regards,
>Giuliano.
>
>> Signed-off-by: Matthias Maennich <maennich@google.com>
>> ---
>>  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}
>> --
>> 2.25.1.481.gfbce0eb801-goog
>>
>> --
>> To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com.
>>

Re: [PATCH 1/2] configure: add support for thread sanitizer (--enable-tsan)

[Matthias Maennich]

On Mon, Mar 16, 2020 at 03:00:02PM +0000, Giuliano Procida wrote:
>On Thu, 12 Mar 2020 at 11:32, 'Matthias Maennich' via kernel-team
><kernel-team@android.com> wrote:
>>
>> 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
>
>Please verify that there's no need to add the various flags to LDFLAGS
>(or no difference if you do so) thanks to CXX anf CFLAGS.
>msan (for which there isn't support yet; it also requires Clang and
>lld in particular) definitely requires its flag at link time.
>
>Assuming this is the case:
>Reviewed-by: Giuliano Procida <gprocida@google.com>
>
>It's also noticeable that turning on some sanitisers affects abidiff
>output. This can't be good and we should look into it when we have
>time.

This mystery I could now solve: elfutils before 0.178 is loading the ebl
backends with dlopen() at runtime. At least on Debian based systems,
those are not in the same directory as libelf.so and some minor magic
is used to discover them. When using the asan runtimes, the loading gets
confused and silently fails (this is due to the fact that some
architectures do not have an ebl backend and failure isn't bad then).
This leads to wrong relocations when using elfutils and eventually to
different extracted data in libabigail, which makes the tests fail. The
workaround is to use a newer elfutils version. Since 0.178, elfutils has
the backends builtin and does not load them anymore at runtime. In my
case, I just needed to rerun `make check` with a newer elfutils in
LD_LIBRARY_PATH.

Cheers,
Matthias

>
>Regards,
>Giuliano.
>
>> Signed-off-by: Matthias Maennich <maennich@google.com>
>> ---
>>  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}
>> --
>> 2.25.1.481.gfbce0eb801-goog
>>
>> --
>> To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com.
>>

Re: [PATCH 1/2] configure: add support for thread sanitizer (--enable-tsan)

[Dodji Seketeli]

Hello Matthias,

Matthias Maennich <maennich@google.com> a écrit:

> 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
>

Applied to master.

Thanks!

-- 
		Dodji