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 <ame...@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
