[PATCH 03/10 v2] libdw: Add locking to dwarf_getsrcfiles and dwarf_getsrclines

[Mark Wielaard]

Hi Aaron,

On Tue, 2025-02-04 at 16:50 -0500, Aaron Merey wrote:
> 	* libdw/dwarf_getsrcfiles.c (dwarf_getsrcfiles): Use dwarf_lock.
> 	* libdw/dwarf_getsrclines.c (dwarf_getsrclines): Use dwarf_lock.
> 
> Signed-off-by: Aaron Merey <amerey@redhat.com>
> ---
> v2 changes: Combined from v1 patches 04/15 and 05/15.
> 
>  libdw/dwarf_getsrcfiles.c | 11 +++++++----
>  libdw/dwarf_getsrclines.c | 25 +++++++++++++++++++------
>  2 files changed, 26 insertions(+), 10 deletions(-)

OK, so here dwarf_lock is used to guard access to the Dwarf_CU fields
lines and files. Might it make sense to turn these into a Dwarf_CU
specific lock?

> 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)
> diff --git a/libdw/dwarf_getsrclines.c b/libdw/dwarf_getsrclines.c
> index b0b5f2eb..79b672dd 100644
> --- a/libdw/dwarf_getsrclines.c
> +++ b/libdw/dwarf_getsrclines.c
> @@ -1444,8 +1444,10 @@ dwarf_getsrclines (Dwarf_Die *cudie, Dwarf_Lines **lines, size_t *nlines)
>        return -1;
>      }
>  
> -  /* Get the information if it is not already known.  */
>    struct Dwarf_CU *const cu = cudie->cu;
> +  mutex_lock (cu->dbg->dwarf_lock);
> +
> +  /* Get the information if it is not already known.  */
>    if (cu->lines == NULL)
>      {
>        /* For split units always pick the lines from the skeleton.  */
> @@ -1466,10 +1468,13 @@ dwarf_getsrclines (Dwarf_Die *cudie, Dwarf_Lines **lines, size_t *nlines)
>  		  *lines = cu->lines;
>  		  *nlines = cu->lines->nlines;
>  		}
> +
> +	      mutex_unlock (cu->dbg->dwarf_lock);
>  	      return res;
>  	    }
>  
>  	  __libdw_seterrno (DWARF_E_NO_DEBUG_LINE);
> +	  mutex_unlock (cu->dbg->dwarf_lock);
>  	  return -1;
>  	}
>  
> @@ -1487,21 +1492,29 @@ dwarf_getsrclines (Dwarf_Die *cudie, Dwarf_Lines **lines, size_t *nlines)
>        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 (cu->dbg->dwarf_lock);
> +	  return -1;
> +	}
>  
>        if (__libdw_getsrclines (cu->dbg, debug_line_offset,
>  			       __libdw_getcompdir (cudie),
>  			       cu->address_size, &cu->lines, &cu->files) < 0)
> -	return -1;
> +	{
> +	  mutex_unlock (cu->dbg->dwarf_lock);
> +	  return -1;
> +	}
>      }
>    else if (cu->lines == (void *) -1l)
> -    return -1;
> +    {
> +      mutex_unlock (cu->dbg->dwarf_lock);
> +      return -1;
> +    }
>  
>    *lines = cu->lines;
>    *nlines = cu->lines->nlines;
>  
> -  // XXX Eventually: unlocking here.
> -
> +  mutex_unlock (cu->dbg->dwarf_lock);
>    return 0;
>  }
>  INTDEF(dwarf_getsrclines)

As far as I can see the locking and unlocking is correct here.

Thanks,

Mark