[04/15] dwarf_getsrcfiles.c: Add locking

Message ID 20250120032041.280173-4-amerey@redhat.com
State Superseded
Headers
Series [01/15] Add eu_search_tree_init |

Commit Message

Aaron Merey Jan. 20, 2025, 3:20 a.m. UTC
  * libdw/dwarf_getsrcfiles.c (dwarf_getsrcfiles): Use dwarf_lock.

Signed-off-by: Aaron Merey <amerey@redhat.com>

---
 libdw/dwarf_getsrcfiles.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)
  

Comments

Mark Wielaard Jan. 22, 2025, 11:17 p.m. UTC | #1
Hi Aaron,

On Sun, Jan 19, 2025 at 10:20:30PM -0500, Aaron Merey wrote:
> 	* libdw/dwarf_getsrcfiles.c (dwarf_getsrcfiles): Use dwarf_lock.
>
> ---
>  libdw/dwarf_getsrcfiles.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/libdw/dwarf_getsrcfiles.c b/libdw/dwarf_getsrcfiles.c
> index 24e4b7d2..3029ce69 100644
> --- a/libdw/dwarf_getsrcfiles.c
> +++ b/libdw/dwarf_getsrcfiles.c
> @@ -47,9 +47,10 @@ dwarf_getsrcfiles (Dwarf_Die *cudie, Dwarf_Files **files, size_t *nfiles)
>      }
>  
>    int res = -1;
> +  struct Dwarf_CU *const cu = cudie->cu;
> +  mutex_lock (cudie->cu->dbg->dwarf_lock);

OK, another Dwarf_Files access, now getting at the dwarf_lock another
way through the cu.

>    /* Get the information if it is not already known.  */
> -  struct Dwarf_CU *const cu = cudie->cu;
>    if (cu->files == NULL)
>      {
>        /* For split units there might be a simple file table (without lines).
> @@ -96,7 +97,10 @@ dwarf_getsrcfiles (Dwarf_Die *cudie, Dwarf_Files **files, size_t *nfiles)
>  	  Dwarf_Off debug_line_offset;
>  	  if (__libdw_formptr (stmt_list, IDX_debug_line, DWARF_E_NO_DEBUG_LINE,
>  			       NULL, &debug_line_offset) == NULL)
> -	    return -1;
> +	    {
> +	      mutex_unlock (cudie->cu->dbg->dwarf_lock);
> +	      return -1;
> +	    }
>  
>  	  res = __libdw_getsrcfiles (cu->dbg, debug_line_offset,
>  				     __libdw_getcompdir (cudie),

So is the lock to protect all paths through __libdw_getsrcfiles?

> @@ -115,8 +119,7 @@ dwarf_getsrcfiles (Dwarf_Die *cudie, Dwarf_Files **files, size_t *nfiles)
>  	*nfiles = cu->files->nfiles;
>      }
>  
> -  // XXX Eventually: unlocking here.
> -
> +  mutex_unlock (cudie->cu->dbg->dwarf_lock);
>    return res;
>  }
>  INTDEF (dwarf_getsrcfiles)
> -- 
> 2.47.1
>
  
Aaron Merey Jan. 24, 2025, 2:15 a.m. UTC | #2
Hi Mark,

> On Sun, Jan 19, 2025 at 10:20:30PM -0500, Aaron Merey wrote:
> >         res = __libdw_getsrcfiles (cu->dbg, debug_line_offset,
> >                                    __libdw_getcompdir (cudie),
>
> So is the lock to protect all paths through __libdw_getsrcfiles?

This patch series contains enough locking to get the testsuite (in
particular the new eu-search tests) passing.  There is another path
into __libdw_getsrcfiles in dwarf_macro_getsrcfiles that does not have
any locking at the moment.

Aaron
  
Mark Wielaard Jan. 24, 2025, 3:33 p.m. UTC | #3
Hi Aaron,

On Thu, Jan 23, 2025 at 09:15:10PM -0500, Aaron Merey wrote:
> > On Sun, Jan 19, 2025 at 10:20:30PM -0500, Aaron Merey wrote:
> > >         res = __libdw_getsrcfiles (cu->dbg, debug_line_offset,
> > >                                    __libdw_getcompdir (cudie),
> >
> > So is the lock to protect all paths through __libdw_getsrcfiles?
> 
> This patch series contains enough locking to get the testsuite (in
> particular the new eu-search tests) passing.  There is another path
> into __libdw_getsrcfiles in dwarf_macro_getsrcfiles that does not have
> any locking at the moment.

It might make reviewing a little easier if the locks for the same
field/function were in the same patch. Or if the patches referred to
each other. Then it is easier to see why the locking is
correct/complete.

Thanks,

Mark
  

Patch

diff --git a/libdw/dwarf_getsrcfiles.c b/libdw/dwarf_getsrcfiles.c
index 24e4b7d2..3029ce69 100644
--- a/libdw/dwarf_getsrcfiles.c
+++ b/libdw/dwarf_getsrcfiles.c
@@ -47,9 +47,10 @@  dwarf_getsrcfiles (Dwarf_Die *cudie, Dwarf_Files **files, size_t *nfiles)
     }
 
   int res = -1;
+  struct Dwarf_CU *const cu = cudie->cu;
+  mutex_lock (cudie->cu->dbg->dwarf_lock);
 
   /* Get the information if it is not already known.  */
-  struct Dwarf_CU *const cu = cudie->cu;
   if (cu->files == NULL)
     {
       /* For split units there might be a simple file table (without lines).
@@ -96,7 +97,10 @@  dwarf_getsrcfiles (Dwarf_Die *cudie, Dwarf_Files **files, size_t *nfiles)
 	  Dwarf_Off debug_line_offset;
 	  if (__libdw_formptr (stmt_list, IDX_debug_line, DWARF_E_NO_DEBUG_LINE,
 			       NULL, &debug_line_offset) == NULL)
-	    return -1;
+	    {
+	      mutex_unlock (cudie->cu->dbg->dwarf_lock);
+	      return -1;
+	    }
 
 	  res = __libdw_getsrcfiles (cu->dbg, debug_line_offset,
 				     __libdw_getcompdir (cudie),
@@ -115,8 +119,7 @@  dwarf_getsrcfiles (Dwarf_Die *cudie, Dwarf_Files **files, size_t *nfiles)
 	*nfiles = cu->files->nfiles;
     }
 
-  // XXX Eventually: unlocking here.
-
+  mutex_unlock (cudie->cu->dbg->dwarf_lock);
   return res;
 }
 INTDEF (dwarf_getsrcfiles)