Hi Aaron,
On Wed, Feb 19, 2025 at 11:36:37PM -0500, Aaron Merey wrote:
> * libdw/dwarf_getsrcfiles.c (dwarf_getsrcfiles): Use dwarf_lock.
> * libdw/dwarf_getsrclines.c (dwarf_getsrclines): Ditto.
> * libdw/dwarf_macro_getsrclines.c (dwarf_macro_getsrclines):
> Ditto.
>
> Signed-off-by: Aaron Merey <ame...@redhat.com>
>
> ---
> v3 changes:
> This patch replaces v2 02/10 and v2 03/10.
>
> The locking to dwarf_filesrc in 02/10 has been removed. The removal of
> dwarf_filesrc locking triggered an error from helgrind that is now fixed
> by the dwarf_macro_getsrclines locking added in this patch.
That makes sense.
> On Wed, Feb 12, 2025 at 8:17 AM Mark Wielaard <m...@klomp.org> wrote:
> >
> > 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?
>
> We could add another Dwarf_CU lock for this purpose. However we'll still
> need to use the dwarf_lock when lines or files haven't been read. For now
> I'd prefer to keep the number of locks as low as possible and focus on
> performance improvements once we've achieved thread safety.
>
> OTOH it shouldn't be too much work to add this cu lock so please let me
> know if you'd still like to see it added in this patch.
We should make dwarf_getsrcfiles and dwarf_getsrclines independent so
callers that onle need the file table don't need to "pay" for parsing
the whole line table. (I thought we had a bug for that, but cannot
find it now. If you also cannot find it, please let me know and I'll
file a new one.)
When we do that, we can reevaluate the locking situation.
I do think there should be lock per CU since now (especially since we
are using mutex) if you try to get the the file and/or line table for
one CU you block all threads working on other CUs in the Dwarf
Debug. So you are serializing the file/line table parsing.
> 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)
This looks correct. Minor nit about if { } indenting, which should be:
if (...)
{
x;
}
> diff --git a/libdw/dwarf_getsrclines.c b/libdw/dwarf_getsrclines.c
> index da78db67..c2e686b1 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->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 +1466,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,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->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)
This also looks correct. Same nit about if { } indenting.
> diff --git a/libdw/dwarf_macro_getsrcfiles.c b/libdw/dwarf_macro_getsrcfiles.c
> index 5e02935d..8d7c2d03 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->dwarf_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->dwarf_lock);
> return 0;
> }
>
> @@ -79,6 +82,7 @@ dwarf_macro_getsrcfiles (Dwarf *dbg, Dwarf_Macro *macro,
> table->files = (void *) -1;
> }
>
> + mutex_unlock (table->dbg->dwarf_lock);
> if (table->files == (void *) -1)
> return -1;
This feels like the lock is dropped too early. You are
accessing/comparing table->files after the lock is dropped. I would
extend it to the end of the function.
if (table->files == (void *) -1)
{
mutex_unlock (table->dbg->dwarf_lock);
return -1;
}
*files = table->files;
*nfiles = table->files->nfiles;
mutex_unlock (table->dbg->dwarf_lock);
return 0;
Cheers,
Mark
