[05/15] dwarf_getsrclines.c: Add locking
Commit Message
* 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
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
>
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
@@ -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)