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 <m...@klomp.org> wrote:
> Hi Heather,
>
> On Tue, 2023-10-10 at 15:42 +0200, Mark Wielaard wrote:
> From: Heather McIntyre <h...@rice.edu>
>
> * libdw/dwarf_hasattr.c (dwarf_hasattr): Use die_abbrev_lock
> around __libdw_dieabbrev call.
>
> Signed-off-by: Heather S. McIntyre <h...@rice.edu>
> Signed-off-by: Mark Wielaard <m...@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);
>
