Re: [PATCH] libdw: add thread-safety to dwarf_getabbrev()

[Mark Wielaard <mark@klomp.org>]

Hi Jonathon and Srđan,

On Fri, 2019-08-16 at 14:24 -0500, Jonathon Anderson wrote:
> For parallel applications that need the information in the DIEs, the
> Dwarf_Abbrev hash table et al. become a massive data race. This fixes 
> that by:
> 
> 1. Adding atomics & locks to the hash table to manage concurrency
>    (lib/dynamicsizehash_concurrent.{c,h})
> 2. Adding a lock & array structure to the memory manager (pseudo-TLS)
>    (libdwP.h, libdw_alloc.c)
> 3. Adding extra configure options for Helgrind/DRD annotations
>    (configure.ac)
> 4. Including "stdatomic.h" from FreeBSD, to support C11-style   atomics.
>    (lib/stdatomic.h)

This looks like really nice work. Thanks!

I am splitting review in some smaller parts if you don't mind.
Simply because it is large and I cannot keep everything in my head at
once :) But here some initial overall comments.

> Notes:
>  - GCC >= 4.9 provides <stdatomic.h> natively; for those versions
>    lib/stdatomic.h could be removed or disabled. We can also rewrite the
>    file if the copyright becomes an issue.

If the compiler provides stdatomic.h then I think it would be good to
use that instead of our own implementation. The copyright isn't a
problem. But do you have a reference/URL to the upstream version? I
like to add that somewhere, so we can sync with it in the future. I see
various commented out parts. Was that already upstream? Should we just
remove those parts?

>  - Currently the concurrent hash table is always enabled, 
>    performance-wise there is no known difference between it  
>    and the non-concurrent  version.
>    This can be changed to toggle with --enable-thread-safety
>    if preferred.

I would prefer it always enabled, unless there is a massive slowdown of
the single-threaded case. The problem with --enable-thread-safety is
that it is a) known broken (sigh) and b) it basically introduces two
separate libraries that behave subtly differently. I would very much
like to get rid of --enable-thread-safety by fixing the broken locking
and simply making it the default.

>  - Another implementation of #2 above might use dynamic TLS 
>    (pthread_key_*),
>    we chose this implementation to reduce the overall complexity.

Are there any other trade-offs?

Thanks,

Mark