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

[Mark Wielaard]

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

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?

Thanks,

Mark