[05/15] dwarf_getsrclines.c: Add locking

Message ID 20250120032041.280173-5-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_getsrclines.c (dwarf_getsrclines): Use dwarf_lock.

Signed-off-by: Aaron Merey <amerey@redhat.com>
---
 libdw/dwarf_getsrclines.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)
  

Comments

Mark Wielaard Jan. 22, 2025, 11:35 p.m. UTC | #1
On Sun, Jan 19, 2025 at 10:20:31PM -0500, Aaron Merey wrote:
> 	* libdw/dwarf_getsrclines.c (dwarf_getsrclines): Use dwarf_lock.

So same lock used for getting/setting Dwarf_Lines. It is a little
unfortunate that lines and files table reading is so intertwined.

I understand why you use the same lock for both.  But it would be good
to document somewhere fields the lock really protects.

I think I am spotting a missed unlock below.

> diff --git a/libdw/dwarf_getsrclines.c b/libdw/dwarf_getsrclines.c
> index da78db67..fb2c431b 100644
> --- a/libdw/dwarf_getsrclines.c
> +++ b/libdw/dwarf_getsrclines.c
> @@ -1197,8 +1197,6 @@ read_srclines (Dwarf *dbg,
>                         + nnewfiles * sizeof (Dwarf_Fileinfo)
>                         + (ndirs + 1) * sizeof (char *),
>                         1);
> -
> -
>        /* Copy prevfiles to newfiles.  */
>        for (size_t n = 0; n < nprevfiles; n++)
>  	newfiles->info[n] = prevfiles->info[n];
> @@ -1442,8 +1440,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.  */
> @@ -1464,10 +1464,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;
>  	}
>  
> @@ -1485,12 +1488,18 @@ 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;

Aren't we still holding the lock here?

> @@ -1498,8 +1507,7 @@ dwarf_getsrclines (Dwarf_Die *cudie, Dwarf_Lines **lines, size_t *nlines)
>    *lines = cu->lines;
>    *nlines = cu->lines->nlines;
>  
> -  // XXX Eventually: unlocking here.
> -
> +  mutex_unlock (cu->dbg->dwarf_lock);
>    return 0;
>  }
>  INTDEF(dwarf_getsrclines)
> -- 
> 2.47.1
>
  
Aaron Merey Jan. 24, 2025, 2:15 a.m. UTC | #2
Hi Mark,

On Wed, Jan 22, 2025 at 6:35 PM Mark Wielaard <mark@klomp.org> wrote:
>
> On Sun, Jan 19, 2025 at 10:20:31PM -0500, Aaron Merey wrote:
> >    else if (cu->lines == (void *) -1l)
> >      return -1;
>
> Aren't we still holding the lock here?

You're right, will fix in v2.

Aaron
  

Patch

diff --git a/libdw/dwarf_getsrclines.c b/libdw/dwarf_getsrclines.c
index da78db67..fb2c431b 100644
--- a/libdw/dwarf_getsrclines.c
+++ b/libdw/dwarf_getsrclines.c
@@ -1197,8 +1197,6 @@  read_srclines (Dwarf *dbg,
                        + nnewfiles * sizeof (Dwarf_Fileinfo)
                        + (ndirs + 1) * sizeof (char *),
                        1);
-
-
       /* Copy prevfiles to newfiles.  */
       for (size_t n = 0; n < nprevfiles; n++)
 	newfiles->info[n] = prevfiles->info[n];
@@ -1442,8 +1440,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.  */
@@ -1464,10 +1464,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;
 	}
 
@@ -1485,12 +1488,18 @@  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;
@@ -1498,8 +1507,7 @@  dwarf_getsrclines (Dwarf_Die *cudie, Dwarf_Lines **lines, size_t *nlines)
   *lines = cu->lines;
   *nlines = cu->lines->nlines;
 
-  // XXX Eventually: unlocking here.
-
+  mutex_unlock (cu->dbg->dwarf_lock);
   return 0;
 }
 INTDEF(dwarf_getsrclines)