On Tue, 2025-02-04 at 16:50 -0500, Aaron Merey wrote:
> Change type of dwarf_lock to mutex in order to take advantage of
> built-in support for recursive locking.
>
> * lib/locks.h: Add macros for locking, unlocking, initializing
> and destroying mutexes.
> * libdw/dwarf_begin_elf.c (dwarf_end): Replace rwlock macro with
> mutex macro.
> * libdw/dwarf_end.c (dwarf_end): Ditto.
> * libdw/dwarf_formref_die.c (dwarf_formref_die): Ditto.
> * libdw/dwarf_getalt.c (dwarf_getalt): Ditto.
> * libdw/dwarf_setalt.c (dwarf_setalt): Ditto.
> * libdw/libdwP.h (struct Dwarf): Ditto.
> * libdw/libdw_findcu.c (__libdw_findcu): Ditto.
I am still a bit nervous about needing recursive locking. IMHO it would
be better to rewrite the code so no recursive locking is required. But
it seems this should work and it is only used for things where we
always take a write lock already, so there is just one thread able to
access the code region anyway.
> Signed-off-by: Aaron Merey <ame...@redhat.com>
> ---
> v2 changes: Add comments to libdwP.h locks describing which functions
> they're intended to cover.
Thanks for that. But see below.
> lib/locks.h | 16 ++++++++++++++++
> libdw/dwarf_begin_elf.c | 2 +-
> libdw/dwarf_end.c | 2 +-
> libdw/dwarf_formref_die.c | 4 ++--
> libdw/dwarf_getalt.c | 10 +++++-----
> libdw/dwarf_setalt.c | 4 ++--
> libdw/libdwP.h | 12 +++++++-----
> libdw/libdw_findcu.c | 4 ++--
> 8 files changed, 36 insertions(+), 18 deletions(-)
>
> diff --git a/lib/locks.h b/lib/locks.h
> index 90fe3f1b..9c17eca7 100644
> --- a/lib/locks.h
> +++ b/lib/locks.h
> @@ -43,6 +43,17 @@
> # define rwlock_rdlock(lock) RWLOCK_CALL (rdlock (&lock))
> # define rwlock_wrlock(lock) RWLOCK_CALL (wrlock (&lock))
> # define rwlock_unlock(lock) RWLOCK_CALL (unlock (&lock))
> +# define mutex_define(class,name) class pthread_mutex_t name
> +# define MUTEX_CALL(call) \
> + ({ int _err = pthread_mutex_ ## call; assert_perror (_err); })
> +# define mutex_init(lock) \
> + ({ pthread_mutexattr_t _attr; \
> + pthread_mutexattr_init (&_attr); \
> + pthread_mutexattr_settype (&_attr, PTHREAD_MUTEX_RECURSIVE); \
> + MUTEX_CALL (init (&lock, &_attr)); })
> +# define mutex_lock(_lock) MUTEX_CALL (lock (&_lock))
> +# define mutex_unlock(lock) MUTEX_CALL (unlock (&lock))
> +# define mutex_fini(lock) MUTEX_CALL (destroy (&lock))
> # define once(once_control, init_routine) \
> ONCE_CALL (once (&once_control, init_routine))
> #else
> @@ -55,6 +66,11 @@
> # define rwlock_rdlock(lock) ((void) (lock))
> # define rwlock_wrlock(lock) ((void) (lock))
> # define rwlock_unlock(lock) ((void) (lock))
> +# define mutex_define(class,name) class int name
> +# define mutex_init(lock) ((void) (lock))
> +# define mutex_lock(lock) ((void) (lock))
> +# define mutex_unlock(lock) ((void) (lock))
> +# define mutex_fini(lock) ((void) (lock))
> # define once_define(class,name)
> # define once(once_control, init_routine) init_routine()
> #endif /* USE_LOCKS */
> diff --git a/libdw/dwarf_begin_elf.c b/libdw/dwarf_begin_elf.c
> index c8292913..8a3a939b 100644
> --- a/libdw/dwarf_begin_elf.c
> +++ b/libdw/dwarf_begin_elf.c
> @@ -579,7 +579,7 @@ dwarf_begin_elf (Elf *elf, Dwarf_Cmd cmd, Elf_Scn *scngrp)
> __libdw_seterrno (DWARF_E_NOMEM); /* no memory. */
> return NULL;
> }
> - rwlock_init (result->dwarf_lock);
> + mutex_init (result->dwarf_lock);
> eu_search_tree_init (&result->cu_tree);
> eu_search_tree_init (&result->tu_tree);
> eu_search_tree_init (&result->split_tree);
> diff --git a/libdw/dwarf_end.c b/libdw/dwarf_end.c
> index ee3ed74e..92389c07 100644
> --- a/libdw/dwarf_end.c
> +++ b/libdw/dwarf_end.c
> @@ -129,7 +129,7 @@ dwarf_end (Dwarf *dwarf)
> if (dwarf->mem_tails != NULL)
> free (dwarf->mem_tails);
> pthread_rwlock_destroy (&dwarf->mem_rwl);
> - rwlock_fini (dwarf->dwarf_lock);
> + mutex_fini (dwarf->dwarf_lock);
>
> /* Free the pubnames helper structure. */
> free (dwarf->pubnames_sets);
> diff --git a/libdw/dwarf_formref_die.c b/libdw/dwarf_formref_die.c
> index 69d1bf1c..85c484f6 100644
> --- a/libdw/dwarf_formref_die.c
> +++ b/libdw/dwarf_formref_die.c
> @@ -92,9 +92,9 @@ dwarf_formref_die (Dwarf_Attribute *attr, Dwarf_Die *result)
> bool scan_debug_types = false;
> do
> {
> - rwlock_wrlock(attr->cu->dbg->dwarf_lock);
> + mutex_lock (attr->cu->dbg->dwarf_lock);
> cu = __libdw_intern_next_unit (attr->cu->dbg, scan_debug_types);
> - rwlock_unlock(attr->cu->dbg->dwarf_lock);
> + mutex_unlock (attr->cu->dbg->dwarf_lock);
>
> if (cu == NULL)
> {
> diff --git a/libdw/dwarf_getalt.c b/libdw/dwarf_getalt.c
> index 26433b81..f4b8ad02 100644
> --- a/libdw/dwarf_getalt.c
> +++ b/libdw/dwarf_getalt.c
> @@ -170,12 +170,12 @@ dwarf_getalt (Dwarf *main)
> if (main == NULL)
> return NULL;
>
> - rwlock_wrlock(main->dwarf_lock);
> + mutex_lock (main->dwarf_lock);
>
> /* Only try once. */
> if (main->alt_dwarf == (void *) -1)
> {
> - rwlock_unlock (main->dwarf_lock);
> + mutex_unlock (main->dwarf_lock);
> return NULL;
> }
>
> @@ -185,7 +185,7 @@ dwarf_getalt (Dwarf *main)
> if (main->alt_dwarf != NULL)
> {
> result = main->alt_dwarf;
> - rwlock_unlock (main->dwarf_lock);
> + mutex_unlock (main->dwarf_lock);
> return result;
> }
>
> @@ -195,12 +195,12 @@ dwarf_getalt (Dwarf *main)
> if (main->alt_dwarf == NULL)
> {
> main->alt_dwarf = (void *) -1;
> - rwlock_unlock (main->dwarf_lock);
> + mutex_unlock (main->dwarf_lock);
> return NULL;
> }
>
> result = main->alt_dwarf;
> - rwlock_unlock (main->dwarf_lock);
> + mutex_unlock (main->dwarf_lock);
>
> return result;
> }
> diff --git a/libdw/dwarf_setalt.c b/libdw/dwarf_setalt.c
> index f98a457c..d8790fd3 100644
> --- a/libdw/dwarf_setalt.c
> +++ b/libdw/dwarf_setalt.c
> @@ -35,7 +35,7 @@
> void
> dwarf_setalt (Dwarf *main, Dwarf *alt)
> {
> - rwlock_wrlock (main->dwarf_lock);
> + mutex_lock (main->dwarf_lock);
>
> if (main->alt_fd != -1)
> {
> @@ -45,6 +45,6 @@ dwarf_setalt (Dwarf *main, Dwarf *alt)
> }
>
> main->alt_dwarf = alt;
> - rwlock_unlock (main->dwarf_lock);
> + mutex_unlock (main->dwarf_lock);
> }
> INTDEF (dwarf_setalt)
> diff --git a/libdw/libdwP.h b/libdw/libdwP.h
> index d6bab606..9d2ffab0 100644
> --- a/libdw/libdwP.h
> +++ b/libdw/libdwP.h
> @@ -264,9 +264,10 @@ struct Dwarf
> allocations for this Dwarf. */
> pthread_rwlock_t mem_rwl;
>
> - /* The dwarf_lock is a read-write lock designed to ensure thread-safe
> access
> - and modification of an entire Dwarf object. */
> - rwlock_define(, dwarf_lock);
> + /* Recursive mutex intended for all libdw functions which require locking,
> + except for functions covered by rwlocks (see rwlock declarations for
> + details). */
> + mutex_define(, dwarf_lock);
Maybe we can be more explicit here?
This mutex covers setting/getting the dwarf_alt field, getting the
next_tu_offset or next_cu_offset fields and should be held when calling
the __libdw_intern_next_unit function.
> /* Internal memory handling. This is basically a simplified thread-local
> reimplementation of obstacks. Unfortunately the standard obstack
> @@ -452,10 +453,11 @@ struct Dwarf_CU
> Dwarf_Off locs_base;
>
> /* Synchronize access to the abbrev member of a Dwarf_Die that
> - refers to this Dwarf_CU. */
> + refers to this Dwarf_CU. Covers __libdw_die_abbrev. */
> rwlock_define(, abbrev_lock);
>
> - /* Synchronize access to the split member of this Dwarf_CU. */
> + /* Synchronize access to the split member of this Dwarf_CU.
> + Covers __libdw_find_split_unit. */
> rwlock_define(, split_lock);
>
> /* Memory boundaries of this CU. */
> diff --git a/libdw/libdw_findcu.c b/libdw/libdw_findcu.c
> index bbbbee5d..613f61c8 100644
> --- a/libdw/libdw_findcu.c
> +++ b/libdw/libdw_findcu.c
> @@ -249,7 +249,7 @@ __libdw_findcu (Dwarf *dbg, Dwarf_Off start, bool
> v4_debug_types)
> if (found != NULL)
> return *found;
>
> - rwlock_wrlock (dbg->dwarf_lock);
> + mutex_lock (dbg->dwarf_lock);
>
> if (start < *next_offset)
> __libdw_seterrno (DWARF_E_INVALID_DWARF);
> @@ -276,7 +276,7 @@ __libdw_findcu (Dwarf *dbg, Dwarf_Off start, bool
> v4_debug_types)
> }
> }
>
> - rwlock_unlock (dbg->dwarf_lock);
> + mutex_unlock (dbg->dwarf_lock);
> return result;
> }
>
Thanks,
Mark
