diff mbox series

[7/9,v2] libdw: Make libdw_findcu thread-safe

Message ID	20240717223409.1788719-8-amerey@redhat.com
State	Superseded
Headers	DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 632C93860778 From: Aaron Merey <amerey@redhat.com> To: elfutils-devel@sourceware.org Cc: hsm2@rice.edu, johnmc@rice.edu, Aaron Merey <amerey@redhat.com>, Mark Wielaard <mark@klomp.org> Subject: [PATCH 7/9 v2] libdw: Make libdw_findcu thread-safe Date: Wed, 17 Jul 2024 18:34:06 -0400 Message-ID: <20240717223409.1788719-8-amerey@redhat.com> In-Reply-To: <20240717223409.1788719-1-amerey@redhat.com> References: <20240717223409.1788719-1-amerey@redhat.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Content-Type: text/plain; charset="US-ASCII"; x-default=true Precedence: list Errors-To: elfutils-devel-bounces~patchwork=sourceware.org@sourceware.org
Series	Fix thread-safety for elfutils \| [0/9,v2] Fix thread-safety for elfutils [1/9,v2] libelf: Fix deadlock in __libelf_readall [2/9,v2] libelf: Fix deadlock in elf_cntl [3/9,v2] lib: Add eu_tsearch, eu_tfind, eu_tdelete and eu_tdestroy [4/9,v2] libdw: make dwarf_getalt thread-safe [5/9,v2] libdwP.h: Add locking to __libdw_dieabbrev, [6/9,v2] libdw: Make libdw_find_split_unit thread-safe [7/9,v2] libdw: Make libdw_findcu thread-safe [8/9,v2] tests: Add eu-search tests [9/9,v2] configure: No longer mark --enable-thread-safety as EXPERIMENTAL

Commit Message

Aaron Merey July 17, 2024, 10:34 p.m. UTC

  From: Heather McIntyre <hsm2@rice.edu>

        * libdw/libdw_findcu.c (__libdw_findcu): Use eu_tfind
	and dwarf_lock
        (__libdw_intern_next_unit): Use per-Dwarf_CU locks.

Signed-off-by: Heather S. McIntyre <hsm2@rice.edu>
Signed-off-by: Aaron Merey <amerey@redhat.com>
Signed-off-by: Mark Wielaard <mark@klomp.org>

---
v2 changes:
Use per-Dwarf_CU lock instead of a global lock.

 libdw/libdw_findcu.c | 47 ++++++++++++++++++++++++++++----------------
 1 file changed, 30 insertions(+), 17 deletions(-)

Comments

Mark Wielaard July 19, 2024, 11:03 a.m. UTC | #1

Hi,

On Wed, 2024-07-17 at 18:34 -0400, Aaron Merey wrote:
> From: Heather McIntyre <hsm2@rice.edu>
> 
>         * libdw/libdw_findcu.c (__libdw_findcu): Use eu_tfind
> 	and dwarf_lock
>         (__libdw_intern_next_unit): Use per-Dwarf_CU locks.
> 
> Signed-off-by: Heather S. McIntyre <hsm2@rice.edu>
> Signed-off-by: Aaron Merey <amerey@redhat.com>
> Signed-off-by: Mark Wielaard <mark@klomp.org>
> 
> ---
> v2 changes:
> Use per-Dwarf_CU lock instead of a global lock.

Are you sure this description and the ChangeLog entry are correct?
This patch doesn't contain a change to use eu_tfind (there is already
one). And it seems to use a per-Dwarf lock, not a per-Dwarf_CU lock.


>  libdw/libdw_findcu.c | 47 ++++++++++++++++++++++++++++----------------
>  1 file changed, 30 insertions(+), 17 deletions(-)
> 
> diff --git a/libdw/libdw_findcu.c b/libdw/libdw_findcu.c
> index 72cf261c..8acff448 100644
> --- a/libdw/libdw_findcu.c
> +++ b/libdw/libdw_findcu.c
> @@ -177,6 +177,8 @@ __libdw_intern_next_unit (Dwarf *dbg, bool debug_types)
>    newp->startp = data->d_buf + newp->start;
>    newp->endp = data->d_buf + newp->end;
>    eu_search_tree_init (&newp->locs_tree);
> +  rwlock_init (newp->abbrev_lock);
> +  rwlock_init (newp->split_lock);
>  
>    /* v4 debug type units have version == 4 and unit_type == DW_UT_type.  */
>    if (debug_types)

Neither of these locks are used in the rest of the patch.

> @@ -243,27 +245,38 @@ __libdw_findcu (Dwarf *dbg, Dwarf_Off start, bool v4_debug_types)
>    /* Maybe we already know that CU.  */
>    struct Dwarf_CU fake = { .start = start, .end = 0 };
>    struct Dwarf_CU **found = eu_tfind (&fake, tree, findcu_cb);
> +  struct Dwarf_CU *result = NULL;
>    if (found != NULL)
>      return *found;
>  
> -  if (start < *next_offset)
> -    {
> -      __libdw_seterrno (DWARF_E_INVALID_DWARF);
> -      return NULL;
> -    }
> +  rwlock_wrlock (dbg->dwarf_lock);
>  
> -  /* No.  Then read more CUs.  */
> -  while (1)
> -    {
> -      struct Dwarf_CU *newp = __libdw_intern_next_unit (dbg, v4_debug_types);
> -      if (newp == NULL)
> -	return NULL;
> -
> -      /* Is this the one we are looking for?  */
> -      if (start < *next_offset || start == newp->start)
> -	return newp;
> -    }
> -  /* NOTREACHED */
> +  if (start < *next_offset)
> +    __libdw_seterrno (DWARF_E_INVALID_DWARF);
> +  else
> +    {   
> +      /* No.  Then read more CUs.  */
> +      while (1) 
> +        {   
> +          struct Dwarf_CU *newp = __libdw_intern_next_unit (dbg,
> +                                                            v4_debug_types);
> +          if (newp == NULL)
> +            {   
> +              result = NULL;
> +              break;
> +            }
> +
> +          /* Is this the one we are looking for?  */
> +          if (start < *next_offset || start == newp->start)
> +            {
> +              result = newp;
> +              break;
> +            }
> +        }
> +    }   
> +
> +  rwlock_unlock (dbg->dwarf_lock);
> +  return result;
>  }
>  
>  struct Dwarf_CU *

This uses the global Dwarf structure lock, not a per Dwarf_CU lock.

Cheers,

Mark

diff mbox series

Patch

diff --git a/libdw/libdw_findcu.c b/libdw/libdw_findcu.c
index 72cf261c..8acff448 100644
--- a/libdw/libdw_findcu.c
+++ b/libdw/libdw_findcu.c
@@ -177,6 +177,8 @@  __libdw_intern_next_unit (Dwarf *dbg, bool debug_types)
   newp->startp = data->d_buf + newp->start;
   newp->endp = data->d_buf + newp->end;
   eu_search_tree_init (&newp->locs_tree);
+  rwlock_init (newp->abbrev_lock);
+  rwlock_init (newp->split_lock);
 
   /* v4 debug type units have version == 4 and unit_type == DW_UT_type.  */
   if (debug_types)
@@ -243,27 +245,38 @@  __libdw_findcu (Dwarf *dbg, Dwarf_Off start, bool v4_debug_types)
   /* Maybe we already know that CU.  */
   struct Dwarf_CU fake = { .start = start, .end = 0 };
   struct Dwarf_CU **found = eu_tfind (&fake, tree, findcu_cb);
+  struct Dwarf_CU *result = NULL;
   if (found != NULL)
     return *found;
 
-  if (start < *next_offset)
-    {
-      __libdw_seterrno (DWARF_E_INVALID_DWARF);
-      return NULL;
-    }
+  rwlock_wrlock (dbg->dwarf_lock);
 
-  /* No.  Then read more CUs.  */
-  while (1)
-    {
-      struct Dwarf_CU *newp = __libdw_intern_next_unit (dbg, v4_debug_types);
-      if (newp == NULL)
-	return NULL;
-
-      /* Is this the one we are looking for?  */
-      if (start < *next_offset || start == newp->start)
-	return newp;
-    }
-  /* NOTREACHED */
+  if (start < *next_offset)
+    __libdw_seterrno (DWARF_E_INVALID_DWARF);
+  else
+    {   
+      /* No.  Then read more CUs.  */
+      while (1) 
+        {   
+          struct Dwarf_CU *newp = __libdw_intern_next_unit (dbg,
+                                                            v4_debug_types);
+          if (newp == NULL)
+            {   
+              result = NULL;
+              break;
+            }
+
+          /* Is this the one we are looking for?  */
+          if (start < *next_offset || start == newp->start)
+            {
+              result = newp;
+              break;
+            }
+        }
+    }   
+
+  rwlock_unlock (dbg->dwarf_lock);
+  return result;
 }
 
 struct Dwarf_CU *