Re: [PATCH] libdw: add thread-safety to dwarf_getabbrev()
Hi,
On Fri, Aug 16, 2019 at 02:24:28PM -0500, Jonathon Anderson wrote:
> 2. Adding a lock & array structure to the memory manager (pseudo-TLS)
> (libdwP.h, libdw_alloc.c)
Specifically concentrating on this part.
> diff --git a/libdw/ChangeLog b/libdw/ChangeLog
> index bf1f4857..87abf7a7 100644
> --- a/libdw/ChangeLog
> +++ b/libdw/ChangeLog
> @@ -1,3 +1,12 @@
> +2019-08-15 Jonathon Anderson
> +
> + * libdw_alloc.c (__libdw_allocate): Added thread-safe stack allocator.
> + * libdwP.h (Dwarf): Likewise.
> + * dwarf_begin_elf.c (dwarf_begin_elf): Support for above.
> + * dwarf_end.c (dwarf_end): Likewise.
> [...]
> + * Makefile.am: Link -lpthread to provide rwlocks.
> diff --git a/libdw/Makefile.am b/libdw/Makefile.am
> index 7a3d5322..6d0a0187 100644
> --- a/libdw/Makefile.am
> +++ b/libdw/Makefile.am
> @@ -108,7 +108,7 @@ am_libdw_pic_a_OBJECTS = $(libdw_a_SOURCES:.c=.os)
> libdw_so_LIBS = libdw_pic.a ../libdwelf/libdwelf_pic.a \
> ../libdwfl/libdwfl_pic.a ../libebl/libebl.a
> libdw_so_DEPS = ../lib/libeu.a ../libelf/libelf.so
> -libdw_so_LDLIBS = $(libdw_so_DEPS) -ldl -lz $(argp_LDADD) $(zip_LIBS)
> +libdw_so_LDLIBS = $(libdw_so_DEPS) -ldl -lz $(argp_LDADD) $(zip_LIBS)
> -lpthread
> libdw_so_SOURCES =
> libdw.so$(EXEEXT): $(srcdir)/libdw.map $(libdw_so_LIBS) $(libdw_so_DEPS)
> # The rpath is necessary for libebl because its $ORIGIN use will
Do we also need -pthread for CFLAGS?
> diff --git a/libdw/dwarf_begin_elf.c b/libdw/dwarf_begin_elf.c
> index 38c8f5c6..b3885bb5 100644
> --- a/libdw/dwarf_begin_elf.c
> +++ b/libdw/dwarf_begin_elf.c
> @@ -417,11 +417,14 @@ dwarf_begin_elf (Elf *elf, Dwarf_Cmd cmd, Elf_Scn
> *scngrp)
> /* Initialize the memory handling. */
> result->mem_default_size = mem_default_size;
> result->oom_handler = __libdw_oom;
> - result->mem_tail = (struct libdw_memblock *) (result + 1);
> - result->mem_tail->size = (result->mem_default_size
> - - offsetof (struct libdw_memblock, mem));
> - result->mem_tail->remaining = result->mem_tail->size;
> - result->mem_tail->prev = NULL;
> + pthread_rwlock_init(&result->mem_rwl, NULL);
> + result->mem_stacks = 1;
> + result->mem_tails = malloc (sizeof (struct libdw_memblock *));
> + result->mem_tails[0] = (struct libdw_memblock *) (result + 1);
> + result->mem_tails[0]->size = (result->mem_default_size
> +- offsetof (struct libdw_memblock, mem));
> + result->mem_tails[0]->remaining = result->mem_tails[0]->size;
> + result->mem_tails[0]->prev = NULL;
Can't we just set mem_tails[0] = NULL?
Wouldn't __libdw_alloc_tail () then initialize it?
> diff --git a/libdw/dwarf_end.c b/libdw/dwarf_end.c
> index 29795c10..6317bcda 100644
> --- a/libdw/dwarf_end.c
> +++ b/libdw/dwarf_end.c
> @@ -94,14 +94,22 @@ 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)
> - {
> - struct libdw_memblock *prevp = memp->prev;
> - free (memp);
> - memp = prevp;
> - }
> + for (size_t i = 0; i < dwarf->mem_stacks; i++)
> +{
> + struct libdw_memblock *memp = dwarf->mem_tails[i];
> + /* The first block is allocated together with the Dwarf object.
> */
Then this wouldn't be true again.
> + while (memp != NULL && memp->prev != NULL)
> + {
> + struct libdw_memblock *prevp = memp->prev;
> + free (memp);
> + memp = prevp;
> + }
> + /* Only for stack 0 though, the others are allocated
> individually. */
> + if (memp != NULL && i > 0)
> +free (memp);
> +}
And then you don't need this special case.
> + free (dwarf->mem_tails);
> + pthread_rwlock_destroy (&dwarf->mem_rwl);
>
> /* Free the pubnames helper structure. */
> free (dwarf->pubnames_sets);
> diff --git a/libdw/libdwP.h b/libdw/libdwP.h
> index eebb7d12..442d493d 100644
> --- a/libdw/libdwP.h
> +++ b/libdw/libdwP.h
> @@ -31,6 +31,7 @@
>
> #include
> #include
> +#include
>
> #include
> #include
> @@ -218,16 +219,18 @@ 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
> + /* Internal memory handling. This is basically a simplified thread-local
> reimplementation of obstacks. Unfortunately the standard obstack
> implementation is not usable in libraries. */
> + pthread_rwlock_t mem_rwl;
> + size_t mem_stacks;
> struct libdw_memblock
> {
> size_t size;
> size_t remaining;
> struct libdw_memblock *prev;
> char mem[0];
> - } *mem_tail;
> + } **mem_tails;
Please document what/how exactly mem_rwl protects which other fields.
> /* Default si
Re: [PATCH] libdw: add thread-safety to dwarf_getabbrev()
On Sat, Aug 24, 2019 at 6:24 PM, Mark Wielaard wrote:
Hi,
On Fri, Aug 16, 2019 at 02:24:28PM -0500, Jonathon Anderson wrote:
2. Adding a lock & array structure to the memory manager
(pseudo-TLS)
(libdwP.h, libdw_alloc.c)
Specifically concentrating on this part.
diff --git a/libdw/ChangeLog b/libdw/ChangeLog
index bf1f4857..87abf7a7 100644
--- a/libdw/ChangeLog
+++ b/libdw/ChangeLog
@@ -1,3 +1,12 @@
+2019-08-15 Jonathon Anderson
+
+ * libdw_alloc.c (__libdw_allocate): Added thread-safe stack
allocator.
+ * libdwP.h (Dwarf): Likewise.
+ * dwarf_begin_elf.c (dwarf_begin_elf): Support for above.
+ * dwarf_end.c (dwarf_end): Likewise.
[...]
+ * Makefile.am: Link -lpthread to provide rwlocks.
diff --git a/libdw/Makefile.am b/libdw/Makefile.am
index 7a3d5322..6d0a0187 100644
--- a/libdw/Makefile.am
+++ b/libdw/Makefile.am
@@ -108,7 +108,7 @@ am_libdw_pic_a_OBJECTS =
$(libdw_a_SOURCES:.c=.os)
libdw_so_LIBS = libdw_pic.a ../libdwelf/libdwelf_pic.a \
../libdwfl/libdwfl_pic.a ../libebl/libebl.a
libdw_so_DEPS = ../lib/libeu.a ../libelf/libelf.so
-libdw_so_LDLIBS = $(libdw_so_DEPS) -ldl -lz $(argp_LDADD)
$(zip_LIBS)
+libdw_so_LDLIBS = $(libdw_so_DEPS) -ldl -lz $(argp_LDADD)
$(zip_LIBS)
-lpthread
libdw_so_SOURCES =
libdw.so$(EXEEXT): $(srcdir)/libdw.map $(libdw_so_LIBS)
$(libdw_so_DEPS)
# The rpath is necessary for libebl because its $ORIGIN use will
Do we also need -pthread for CFLAGS?
Yep, that would be the smart thing to do. Will be handled in the next
version of the patch (once I figure out how to autotools...).
diff --git a/libdw/dwarf_begin_elf.c b/libdw/dwarf_begin_elf.c
index 38c8f5c6..b3885bb5 100644
--- a/libdw/dwarf_begin_elf.c
+++ b/libdw/dwarf_begin_elf.c
@@ -417,11 +417,14 @@ dwarf_begin_elf (Elf *elf, Dwarf_Cmd cmd,
Elf_Scn
*scngrp)
/* Initialize the memory handling. */
result->mem_default_size = mem_default_size;
result->oom_handler = __libdw_oom;
- result->mem_tail = (struct libdw_memblock *) (result + 1);
- result->mem_tail->size = (result->mem_default_size
- - offsetof (struct libdw_memblock, mem));
- result->mem_tail->remaining = result->mem_tail->size;
- result->mem_tail->prev = NULL;
+ pthread_rwlock_init(&result->mem_rwl, NULL);
+ result->mem_stacks = 1;
+ result->mem_tails = malloc (sizeof (struct libdw_memblock *));
+ result->mem_tails[0] = (struct libdw_memblock *) (result + 1);
+ result->mem_tails[0]->size = (result->mem_default_size
+ - offsetof (struct libdw_memblock, mem));
+ result->mem_tails[0]->remaining = result->mem_tails[0]->size;
+ result->mem_tails[0]->prev = NULL;
Can't we just set mem_tails[0] = NULL?
Wouldn't __libdw_alloc_tail () then initialize it?
Mostly an artifact of preserving the previous behavior for
single-threaded programs, the malloc for the Dwarf then simplifies too
(and less memory usage for opened but otherwise unused Dwarfs).
Downside is an extra malloc on first usage (which shouldn't be an issue
for any real program).
diff --git a/libdw/dwarf_end.c b/libdw/dwarf_end.c
index 29795c10..6317bcda 100644
--- a/libdw/dwarf_end.c
+++ b/libdw/dwarf_end.c
@@ -94,14 +94,22 @@ 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)
- {
-struct libdw_memblock *prevp = memp->prev;
-free (memp);
-memp = prevp;
- }
+ for (size_t i = 0; i < dwarf->mem_stacks; i++)
+{
+ struct libdw_memblock *memp = dwarf->mem_tails[i];
+ /* The first block is allocated together with the Dwarf
object.
*/
Then this wouldn't be true again.
+ while (memp != NULL && memp->prev != NULL)
+ {
+struct libdw_memblock *prevp = memp->prev;
+free (memp);
+memp = prevp;
+ }
+ /* Only for stack 0 though, the others are allocated
individually. */
+ if (memp != NULL && i > 0)
+free (memp);
+}
And then you don't need this special case.
+ free (dwarf->mem_tails);
+ pthread_rwlock_destroy (&dwarf->mem_rwl);
/* Free the pubnames helper structure. */
free (dwarf->pubnames_sets);
diff --git a/libdw/libdwP.h b/libdw/libdwP.h
index eebb7d12..442d493d 100644
--- a/libdw/libdwP.h
+++ b/libdw/libdwP.h
@@ -31,6 +31,7 @@
#include
#include
+#include
#include
#include
@@ -218,16 +219,18 @@ 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
+ /* Internal memory handling. This is basically a simplified
thread-local
re
