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

[Heather McIntyre <hsm2@rice.edu>]

[-- Attachment #1: Type: text/plain, Size: 2704 bytes --]

Hi Mark,

I see now that this is incomplete considering the other places that also
call this function. I do agree that global locking may be heavy if 1)
implemented in all of these locations, or 2) implemented directly in
__libdw_dieabbrev. We could use atomics here directly in __libdw_dieabbrev.
I have given this a try and it is currently passing all tests, including
the new ones I added for data race detection.

I know you mentioned that taking any pthread lock at all might be a big
overhead, but since I implemented a per dwarf struct lock, would using that
be a possibility? Assuming multiple calls to __libdw_dieabbrev will be
working on different dwarf objects.

On Tue, Oct 10, 2023 at 8:44 AM Mark Wielaard <mark@klomp.org> wrote:

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