Hi Aaron, On Sun, 2025-03-16 at 21:51 -0400, Aaron Merey wrote: > * libdw/dwarf_begin_elf.c (dwarf_begin_elf): Init macro_lock. > * libdw/dwarf_end.c (cu_free): Free src_lock. > (dwarf_end): Free macro_lock. > * libdw/dwarf_getsrcfiles.c (dwarf_getsrcfiles): Use src_lock. > * libdw/dwarf_getsrclines.c (dwarf_getsrclines): Ditto. > * libdw/dwarf_macro_getsrclines.c (dwarf_macro_getsrclines): Use > macro_lock. > * libdw/libdwP.h (struct Dwarf): Define macro_lock. > (struct Dwarf_CU): Define src_lock. > * libdw/libdw_findcu.c (__libdw_intern_next_unit): Init src_lock.
Looks good. Just one comment about a comment below. > Signed-off-by: Aaron Merey <ame...@redhat.com> > --- > v3: > https://patchwork.sourceware.org/project/elfutils/patch/20250220043644.2058519-3-ame...@redhat.com/ > > v4: Instead of using dwarf_lock, define a new lock for dwarf_getsrclines and > dwarf_getsrcfiles as well as dwarf_macro_getsrcfiles. > > libdw/dwarf_begin_elf.c | 1 + > libdw/dwarf_end.c | 2 ++ > libdw/dwarf_getsrcfiles.c | 11 +++++++---- > libdw/dwarf_getsrclines.c | 25 +++++++++++++++++++------ > libdw/dwarf_macro_getsrcfiles.c | 10 +++++++++- > libdw/libdwP.h | 10 ++++++++-- > libdw/libdw_findcu.c | 1 + > 7 files changed, 47 insertions(+), 13 deletions(-) > > diff --git a/libdw/dwarf_begin_elf.c b/libdw/dwarf_begin_elf.c > index 8a3a939b..58a309e9 100644 > --- a/libdw/dwarf_begin_elf.c > +++ b/libdw/dwarf_begin_elf.c > @@ -580,6 +580,7 @@ dwarf_begin_elf (Elf *elf, Dwarf_Cmd cmd, Elf_Scn *scngrp) > return NULL; > } > mutex_init (result->dwarf_lock); > + mutex_init (result->macro_lock); > eu_search_tree_init (&result->cu_tree); > eu_search_tree_init (&result->tu_tree); > eu_search_tree_init (&result->split_tree); > diff --git a/libdw/dwarf_end.c b/libdw/dwarf_end.c > index 92389c07..c12815e1 100644 > --- a/libdw/dwarf_end.c > +++ b/libdw/dwarf_end.c > @@ -70,6 +70,7 @@ cu_free (void *arg) > Dwarf_Abbrev_Hash_free (&p->abbrev_hash); > rwlock_fini (p->abbrev_lock); > rwlock_fini (p->split_lock); > + mutex_fini (p->src_lock); > > /* Free split dwarf one way (from skeleton to split). */ > if (p->unit_type == DW_UT_skeleton > @@ -130,6 +131,7 @@ dwarf_end (Dwarf *dwarf) > free (dwarf->mem_tails); > pthread_rwlock_destroy (&dwarf->mem_rwl); > mutex_fini (dwarf->dwarf_lock); > + mutex_fini (dwarf->macro_lock); > > /* Free the pubnames helper structure. */ > free (dwarf->pubnames_sets); > diff --git a/libdw/dwarf_getsrcfiles.c b/libdw/dwarf_getsrcfiles.c > index 24e4b7d2..9becd1d5 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->src_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->src_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->src_lock); > return res; > } > INTDEF (dwarf_getsrcfiles) > diff --git a/libdw/dwarf_getsrclines.c b/libdw/dwarf_getsrclines.c > index da78db67..be10cdee 100644 > --- a/libdw/dwarf_getsrclines.c > +++ b/libdw/dwarf_getsrclines.c > @@ -1442,8 +1442,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->src_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 +1466,13 @@ dwarf_getsrclines (Dwarf_Die *cudie, Dwarf_Lines > **lines, size_t *nlines) > *lines = cu->lines; > *nlines = cu->lines->nlines; > } > + > + mutex_unlock (cu->src_lock); > return res; > } > > __libdw_seterrno (DWARF_E_NO_DEBUG_LINE); > + mutex_unlock (cu->src_lock); > return -1; > } > > @@ -1485,21 +1490,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->src_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->src_lock); > + return -1; > + } > } > else if (cu->lines == (void *) -1l) > - return -1; > + { > + mutex_unlock (cu->src_lock); > + return -1; > + } > > *lines = cu->lines; > *nlines = cu->lines->nlines; > > - // XXX Eventually: unlocking here. > - > + mutex_unlock (cu->src_lock); > return 0; > } > INTDEF(dwarf_getsrclines) > diff --git a/libdw/dwarf_macro_getsrcfiles.c b/libdw/dwarf_macro_getsrcfiles.c > index 5e02935d..6ccbeadb 100644 > --- a/libdw/dwarf_macro_getsrcfiles.c > +++ b/libdw/dwarf_macro_getsrcfiles.c > @@ -41,6 +41,8 @@ dwarf_macro_getsrcfiles (Dwarf *dbg, Dwarf_Macro *macro, > > /* macro is declared NN */ > Dwarf_Macro_Op_Table *const table = macro->table; > + > + mutex_lock (table->dbg->macro_lock); > if (table->files == NULL) > { > Dwarf_Off line_offset = table->line_offset; > @@ -48,6 +50,7 @@ dwarf_macro_getsrcfiles (Dwarf *dbg, Dwarf_Macro *macro, > { > *files = NULL; > *nfiles = 0; > + mutex_unlock (table->dbg->macro_lock); > return 0; > } > > @@ -80,9 +83,14 @@ dwarf_macro_getsrcfiles (Dwarf *dbg, Dwarf_Macro *macro, > } > > if (table->files == (void *) -1) > - return -1; > + { > + mutex_unlock (table->dbg->macro_lock); > + return -1; > + } > > *files = table->files; > *nfiles = table->files->nfiles; > + > + mutex_unlock (table->dbg->macro_lock); > return 0; > } > diff --git a/libdw/libdwP.h b/libdw/libdwP.h > index d121914d..974d6a50 100644 > --- a/libdw/libdwP.h > +++ b/libdw/libdwP.h > @@ -265,11 +265,13 @@ struct Dwarf > pthread_rwlock_t mem_rwl; > > /* Recursive mutex intended for setting/getting alt_dwarf, next_tu_offset, > - and next_cu_offset. Covers dwarf_getsrclines, dwarf_getsrcfiles and > - dwarf_macro_getsrcfiles. Should also be held when calling > + and next_cu_offset. Should be held when calling > __libdw_intern_next_unit. */ > mutex_define(, dwarf_lock); > > + /* Synchronize access to dwarf_macro_getsrcfiles. */ > + mutex_define(, macro_lock); > + Maybe say: /* Used inside dwarf_macro_getsrcfiles to guard access to the Dwarf_Macro table struct. */ ? > /* Internal memory handling. This is basically a simplified thread-local > reimplementation of obstacks. Unfortunately the standard obstack > implementation is not usable in libraries. */ > @@ -461,6 +463,10 @@ struct Dwarf_CU > Covers __libdw_find_split_unit. */ > rwlock_define(, split_lock); > > + /* Synchronize access to the lines and files members. > + Covers dwarf_getsrclines and dwarf_getsrcfiles. */ > + mutex_define(, src_lock); > + > /* Memory boundaries of this CU. */ > void *startp; > void *endp; > diff --git a/libdw/libdw_findcu.c b/libdw/libdw_findcu.c > index 613f61c8..f0243643 100644 > --- a/libdw/libdw_findcu.c > +++ b/libdw/libdw_findcu.c > @@ -179,6 +179,7 @@ __libdw_intern_next_unit (Dwarf *dbg, bool debug_types) > eu_search_tree_init (&newp->locs_tree); > rwlock_init (newp->abbrev_lock); > rwlock_init (newp->split_lock); > + mutex_init (newp->src_lock); > > /* v4 debug type units have version == 4 and unit_type == DW_UT_type. */ > if (debug_types)
