Re: [PATCH 11/16] libdw: Add locking around __libdw_dieabbrev for dwarf_hasattr

[Mark Wielaard <mark@klomp.org>]

Hi Heather,

On Tue, 2023-10-10 at 15:42 +0200, Mark Wielaard wrote:
> From: Heather McIntyre <hsm2@rice.edu>
> 
> 	* libdw/dwarf_hasattr.c (dwarf_hasattr): Use die_abbrev_lock
> 	around __libdw_dieabbrev call.
> 
> Signed-off-by: Heather S. McIntyre <hsm2@rice.edu>
> Signed-off-by: Mark Wielaard <mark@klomp.org>
> ---
>  libdw/dwarf_hasattr.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/libdw/dwarf_hasattr.c b/libdw/dwarf_hasattr.c
> index eca08394..92f8de68 100644
> --- a/libdw/dwarf_hasattr.c
> +++ b/libdw/dwarf_hasattr.c
> @@ -34,6 +34,10 @@
>  #include <dwarf.h>
>  #include "libdwP.h"
>  
> +/* dwarf_hasattr() calls __libdw_dieabbrev() in libdwP.h.
> +   __libdw_dieabbrev() reads/writes "die->abbrev".
> +   Mutual exclusion is enforced around the call to __libdw_dieabbrev to prevent a race. */
> +rwlock_define(static, die_abbrev_lock);

dwarf_child, dwarf_getattrs, dwarf_haschildren and dwarf_tag also use
__libdw_dieabbrev to get the Dwarf_Abbrev pointer for the given
Dwarf_DIE. Shouldn't they also use such locking? Or have the locking
inside __libdw_dieabbrev itself?

Also there are many Dwarf_Dies which all start out "lazy" without
abbrev set. So taking a global static lock, or even taking any pthread
lock at all might be a big overhead. Is there some way we can do this
with atomics instead?

>  
>  int
>  dwarf_hasattr (Dwarf_Die *die, unsigned int search_name)
> @@ -41,8 +45,13 @@ dwarf_hasattr (Dwarf_Die *die, unsigned int search_name)
>    if (die == NULL)
>      return 0;
>  
> +  rwlock_wrlock(die_abbrev_lock);
> +
>    /* Find the abbreviation entry.  */
>    Dwarf_Abbrev *abbrevp = __libdw_dieabbrev (die, NULL);
> +
> +  rwlock_unlock(die_abbrev_lock);
> +
>    if (unlikely (abbrevp == DWARF_END_ABBREV))
>      {
>        __libdw_seterrno (DWARF_E_INVALID_DWARF);