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

  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(+)
  

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

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

Hi Heather,

On Tue, Oct 17, 2023 at 02:57:39PM -0500, Heather McIntyre wrote:
> 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.

I have been thinking about this issue and I think we made a mistake in
designing how a Dwarf_Die is lazy initialized. The abbrev field of a
Dwarf_Die is only set when needed by calling __libdw_dieabbrev, which
means we need some kind of locking or atomic swapping whenever we try
to use that field. I assume the idea originally was that calling
__libdw_dieabbrev is fairly "heavy" (it is, potentially reading the
whole .debug_abbrev for the CU). So we try to postpone it till it is
really needed.

But in practice it is always needed. Without the abbrev field set you
can just call dwarf_dieoffset, dwarf_cuoffset, dwarf_diecu and
dwarf_getabbrev. In theory you could avoid adding the abbrev for the
initial CU DIE for a Dwarf_CU when you are iterating over all CUs and
know you don't need the CU without inspecting the initial CU DIE. But
the Dwarf_Abbrev_Hash for the Dwarf_CU will already be
initialized. And normally the abbrev for the first DIE will also be
the first abbrev, so searching for it should be really quick.

So I think setting the Dwarf_Die abbrev field lazy is not really
helpful and makes the code needlessly complex. If we set the abbrev
field when a Dwarf_Die is created we can simplify the code and don't
need all this locking when we just want to access the field.

This of course is still a lot of coding, we'll have to check every
place that initializes a new Dwarf_Die. Which will have to call
__libdw_dieabbrev directly. But I think that will not need any extra
locking because the Dwarf_Abbrev_Hash used is already thread-safe (it
was written by Srđan Milaković also from Rice University).

What do you think?

Cheers,

Mark