[PATCH 2/3] libdw: Rewrite the memory handler to be thread-safe.

[Jonathon Anderson]

On Mon, Oct 21, 2019 at 18:13, Mark Wielaard <mark@klomp.org> wrote:
> Hi,
> 
> Finally back to this patch series.
> 
> On Thu, 2019-08-29 at 15:16 +0200, Mark Wielaard wrote:
>>  diff --git a/libdw/dwarf_end.c b/libdw/dwarf_end.c
>>  index 29795c10..fc573cb3 100644
>>  --- a/libdw/dwarf_end.c
>>  +++ b/libdw/dwarf_end.c
>>  @@ -94,14 +94,15 @@ dwarf_end (Dwarf *dwarf)
>>         /* And the split Dwarf.  */
>>         tdestroy (dwarf->split_tree, noop_free);
>> 
>>  -      struct libdw_memblock *memp = dwarf->mem_tail;
>>  -      /* The first block is allocated together with the Dwarf 
>> object.  */
>>  -      while (memp->prev != NULL)
>>  +      /* Free the internally allocated memory.  */
>>  +      struct libdw_memblock *memp = (struct libdw_memblock 
>> *)dwarf->mem_tail;
>>  +      while (memp != NULL)
>>   	{
>>   	  struct libdw_memblock *prevp = memp->prev;
>>   	  free (memp);
>>   	  memp = prevp;
>>   	}
>>  +      pthread_key_delete (dwarf->mem_key);
>> 
>>         /* Free the pubnames helper structure.  */
>>         free (dwarf->pubnames_sets);
> 
> This doesn't build on older GCCs (I am using 4.8.5) with this compile 
> error:
> 
> 
> libdw/dwarf_end.c: In function ‘dwarf_end’:
> libdw/dwarf_end.c:98:45: error: cannot convert to a pointer type
>        struct libdw_memblock *memp = (struct libdw_memblock 
> *)dwarf->mem_tail;
>                                              ^

Ah, whoops. Thanks for catching that one.

> 
> This is because mem_tail is defined as:
> 
>>  diff --git a/libdw/libdwP.h b/libdw/libdwP.h
>>  index eebb7d12..ad2599eb 100644
>>  --- a/libdw/libdwP.h
>>  +++ b/libdw/libdwP.h
>>  @@ -218,16 +231,11 @@ struct Dwarf
>>     /* Similar for addrx/constx, which will come from .debug_addr 
>> section.  */
>>     struct Dwarf_CU *fake_addr_cu;
>> 
>>  -  /* Internal memory handling.  This is basically a simplified
>>  -     reimplementation of obstacks.  Unfortunately the standard 
>> obstack
>>  -     implementation is not usable in libraries.  */
>>  -  struct libdw_memblock
>>  -  {
>>  -    size_t size;
>>  -    size_t remaining;
>>  -    struct libdw_memblock *prev;
>>  -    char mem[0];
>>  -  } *mem_tail;
>>  +  /* Internal memory handling.  Each thread allocates separately 
>> and only
>>  +     allocates from its own blocks, while all the blocks are 
>> pushed atomically
>>  +     onto a unified stack for easy deallocation.  */
>>  +  pthread_key_t mem_key;
>>  +  atomic_uintptr_t mem_tail;
>> 
>>     /* Default size of allocated memory blocks.  */
>>     size_t mem_default_size;
> 
> And for older compilers, without stdatomic.h, this means 
> atomic_uintptr_t is really:
> 
> typedef _Atomic(uintptr_t)              atomic_uintptr_t;
> #define _Atomic(T)                      struct { volatile 
> __typeof__(T) __val; }
> 
> And you cannot cast a struct to a pointer type directly.
> 
> To make this work on both older and newer gcc versions I changed this 
> to:
> 
> diff --git a/libdw/dwarf_end.c b/libdw/dwarf_end.c
> index fc573cb3..76ab9954 100644
> --- a/libdw/dwarf_end.c
> +++ b/libdw/dwarf_end.c
> @@ -95,7 +95,8 @@ dwarf_end (Dwarf *dwarf)
>        tdestroy (dwarf->split_tree, noop_free);
> 
>        /* Free the internally allocated memory.  */
> -      struct libdw_memblock *memp = (struct libdw_memblock 
> *)dwarf->mem_tail;
> +      struct libdw_memblock *memp;
> +      memp = (struct libdw_memblock *) atomic_load 
> (&dwarf->mem_tail);
>        while (memp != NULL)
>  	{
>  	  struct libdw_memblock *prevp = memp->prev;
> 
> Does that look reasonable?

It does, although I would prefer:

diff --git a/libdw/dwarf_end.c b/libdw/dwarf_end.c
index 9ca17212..6da9e0cd 100644
--- a/libdw/dwarf_end.c
+++ b/libdw/dwarf_end.c
@@ -95,7 +95,9 @@ dwarf_end (Dwarf *dwarf)
       tdestroy (dwarf->split_tree, noop_free);

       /* Free the internally allocated memory.  */
-      struct libdw_memblock *memp = (struct libdw_memblock 
*)dwarf->mem_tail;
+      struct libdw_memblock *memp;
+      memp  = (struct libdw_memblock *)atomic_load(&dwarf->mem_tail,
+                                                   
memory_order_relaxed);
       while (memp != NULL)
         {
           struct libdw_memblock *prevp = memp->prev;

Because some idiot thought making seq_cst the default was a good idea. 
And this way it notes in the code that this load is non-synchronizing.

-Jonathon

> 
> Thanks,
> 
> Mark