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
>
