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

[Jonathon Anderson <jma14 at rice dot edu>]

Hello Mark,

Just finished some modifications to the patch series, git request-pull 
output below. This rebases onto the latest master and does a little 
diff cleaning, the major change is that I swapped out the memory 
management to use the pthread_key_* alternative mentioned before. I did 
some performance testing and found it to be notably faster in a 
high-stress microbenchmark (and no measurable difference in an 
application), and it makes the result easier to read overall. I've also 
removed the Valgrind configuration options patch since it doesn't add 
anything at the moment, if it becomes useful I'll submit it separately.

Thanks for taking the time to read through everything.

-Jonathon

The following changes since commit 
4bcc641d362de4236ae8f0f5bc933c6d84b6f105:

 libdw: fix latent bug in dwarf_getcfi.c not setting 
default_same_value. (2019-08-26 15:15:34 +0200)

are available in the Git repository at:

 https://github.com/blue42u/elfutils.git parallel-pr-v2

for you to fetch changes up to 1191d9ed292b508d732973a318a01051053e0f61:

 lib + libdw: Add and use a concurrent version of the dynamic-size 
hash table. (2019-08-26 22:29:45 -0500)

----------------------------------------------------------------
Jonathon Anderson (2):
     Add some supporting framework for C11-style atomics.
     libdw: Rewrite the memory handler to be thread-safe.

Srđan Milaković (1):
     lib + libdw: Add and use a concurrent version of the dynamic-size 
hash table.

configure.ac                     |  12 ++
lib/ChangeLog                    |  11 ++
lib/Makefile.am                  |   5 +-
lib/atomics.h                    |  37 +++++
lib/dynamicsizehash_concurrent.c | 522 
++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
lib/dynamicsizehash_concurrent.h | 118 +++++++++++++
lib/stdatomic-fbsd.h             | 442 
+++++++++++++++++++++++++++++++++++++++++++++++++
libdw/ChangeLog                  |  12 ++
libdw/Makefile.am                |   4 +-
libdw/dwarf_abbrev_hash.c        |   2 +-
libdw/dwarf_abbrev_hash.h        |   2 +-
libdw/dwarf_begin_elf.c          |  12 +-
libdw/dwarf_end.c                |   7 +-
libdw/libdwP.h                   |  59 ++++---
libdw/libdw_alloc.c              |   5 +-
15 files changed, 1210 insertions(+), 40 deletions(-)
create mode 100644 lib/atomics.h
create mode 100644 lib/dynamicsizehash_concurrent.c
create mode 100644 lib/dynamicsizehash_concurrent.h
create mode 100644 lib/stdatomic-fbsd.h

On Mon, Aug 26, 2019 at 8:37 AM, Jonathon Anderson <jma14@rice.edu> 
wrote:
> Looks correct to me (assuming it applies). I think there's another 
> latent bug in there somewhere (tests that use libdwfl used to leak 
> mem_tails, but now that dwarf_begin_elf doesn't do an initial malloc 
> it doesn't trigger), I'll try hunting it down when I have the time.
>
> Glad I could be of help testing the mailinglist :)
>
> -Jonathon
>
> On Mon, Aug 26, 2019 at 8:18 AM, Mark Wielaard <mark@klomp.org> wrote:
> > Hi Jonathon,
> >
> > Thanks for checking our new mailinglist settings :)
> > You message was accepted now and the HTML attachment stripped.
> >
> > On Sun, 2019-08-25 at 20:24 -0500, Jonathon Anderson wrote:
> > >  The one extra line in dwarf_getcfi.c
> > >  is to account for a failure in the test suite when Valgrind is
> > >  enabled (didn't catch it the first time around).
> >
> > That is an interesting catch. And obviously a latent bug. It works if
> > things are in the first memblock (as part of the larger Dwarf 
> > struct),
> > since that is allocated with calloc, and so zeroed out. But if the 
> > cfi
> > would get into a later memblock it would get a random value, since
> > those are malloced. In your patch the memblocks are always malloced,
> > and so the bug shows up immediately.
> >
> > Lets just cherry-pick this fixup since it is a good fixup to have.
> >
> > Does the attached look correct?
> >
> > Thanks,
> >
> > Mark