Hi Aaron, On Fri, 2025-12-12 at 20:34 -0500, Aaron Merey wrote: > Individual debuginfod_client objects and their underlying CURLM handles > should not be used concurrently during calls to > __libdwfl_debuginfod_find_debuginfo and > __libdwfl_debuginfod_find_executable. > > Add a mutex field to struct Dwfl to serialize calls to > __libdwfl_debuginfod_find_*. > > Signed-off-by: Aaron Merey <[email protected]> > --- > > v2 changes: Reverted changes to a comment for __libdwfl_debuginfod_init. > Rename dwfl->lock to dwfl->debuginfod_lock.
That does make things more clear, thanks. > On Thu, Dec 4, 2025 at 7:03 PM Aaron Merey <[email protected]> wrote: > > On Wed, Dec 3, 2025 at 11:19 AM Mark Wielaard <[email protected]> wrote: > > > So if I understand correctly there are two issues here. > > > > > > The debuginfo_client is created and assigned to a Dwfl field lazily. So > > > you want to prevent concurrent calls to dwfl_get_debuginfod_client. > > > > > > A debuginfo_client has CURLM handle which cannot be used by different > > > threads concurrently. > > > > > > Since we have one unique debuginfo_client per dwfl you use one and the > > > same lock? > > > > > > I believe this patch should work. But have you considered having a lock > > > inside debuginfod-client itself to protect access to the CURLM handles? > > > That would (maybe) result is slightly simpler code. And would help > > > other users of libdebuginfod that might use debuginfod_client handles > > > from multiple threads (gdb?). > > > > I agree that it is better to have dedicated per-client locks instead. > > Theres no need for libdwfl functions to wait on locks held for > > independent debuginfod purposes. > > I experimented with adding thread safety at the libdebuginfod level > instead of libdwfl. Reasoning about its correctness ended up being > more complex than expected, in part due to the client progressfn > callback that can be called during a query. > > After some discussion with Frank I think that the effort to make > libdebuginfod thread safe isn't trivial and may not be worth the time, > as we don't currently have a clear use case that would see a worthwhile > performance benefit. OK, at least internal libdwfl usage is now thread-safe. If the Dwfl functions that use the debuginfod_client handle are called from different threads. The only remaining issue is that the user can call dwfl_get_debuginfod_client (dwfl) to get the debuginfod_client handle and use it concurrently with other dwfl operations that might trigger operations on the same handle. Not sure what the best "solution" for that is. We could add a new method to get the debuginfod_lock of the dwfl and require the user uses that when invoking any method on the debuginfod_client handle. Or maybe we need dwfl_put_back_debuginfod_client (dwfl) to signal the user is done with it? Or we could just add documentation that the handle should only be used to when not using any other dwfl methods? The patch looks good otherwise. Thanks, Mark > libdwfl/debuginfod-client.c | 6 +++++- > libdwfl/dwfl_begin.c | 2 ++ > libdwfl/dwfl_end.c | 1 + > libdwfl/libdwflP.h | 3 +++ > 4 files changed, 11 insertions(+), 1 deletion(-) > > diff --git a/libdwfl/debuginfod-client.c b/libdwfl/debuginfod-client.c > index 882a5eff..ecb053fc 100644 > --- a/libdwfl/debuginfod-client.c > +++ b/libdwfl/debuginfod-client.c > @@ -49,7 +49,7 @@ static void __libdwfl_debuginfod_init (void); > > static pthread_once_t init_control = PTHREAD_ONCE_INIT; > > -/* NB: this is slightly thread-unsafe */ > +/* dwfl->debuginfod_lock must be held when calling this function. */ > > debuginfod_client * > dwfl_get_debuginfod_client (Dwfl *dwfl) > @@ -77,10 +77,12 @@ __libdwfl_debuginfod_find_executable (Dwfl *dwfl, > int fd = -1; > if (build_id_len > 0) > { > + mutex_lock (dwfl->debuginfod_lock); > debuginfod_client *c = INTUSE (dwfl_get_debuginfod_client) (dwfl); > if (c != NULL) > fd = (*fp_debuginfod_find_executable) (c, build_id_bits, > build_id_len, NULL); > + mutex_unlock (dwfl->debuginfod_lock); > } > > return fd; > @@ -94,10 +96,12 @@ __libdwfl_debuginfod_find_debuginfo (Dwfl *dwfl, > int fd = -1; > if (build_id_len > 0) > { > + mutex_lock (dwfl->debuginfod_lock); > debuginfod_client *c = INTUSE (dwfl_get_debuginfod_client) (dwfl); > if (c != NULL) > fd = (*fp_debuginfod_find_debuginfo) (c, build_id_bits, > build_id_len, NULL); > + mutex_unlock (dwfl->debuginfod_lock); > } > > return fd; > diff --git a/libdwfl/dwfl_begin.c b/libdwfl/dwfl_begin.c > index b03f5cf4..835de8e3 100644 > --- a/libdwfl/dwfl_begin.c > +++ b/libdwfl/dwfl_begin.c > @@ -50,6 +50,8 @@ dwfl_begin (const Dwfl_Callbacks *callbacks) > dwfl->offline_next_address = OFFLINE_REDZONE; > } > > + mutex_init (dwfl->debuginfod_lock); > + > return dwfl; > } > INTDEF (dwfl_begin) > diff --git a/libdwfl/dwfl_end.c b/libdwfl/dwfl_end.c > index d9cf569b..c82d83fb 100644 > --- a/libdwfl/dwfl_end.c > +++ b/libdwfl/dwfl_end.c > @@ -53,6 +53,7 @@ dwfl_end (Dwfl *dwfl) > free (dwfl->lookup_module); > free (dwfl->lookup_segndx); > free (dwfl->sysroot); > + mutex_fini (dwfl->debuginfod_lock); > > Dwfl_Module *next = dwfl->modulelist; > while (next != NULL) > diff --git a/libdwfl/libdwflP.h b/libdwfl/libdwflP.h > index a5d88d60..6e394c26 100644 > --- a/libdwfl/libdwflP.h > +++ b/libdwfl/libdwflP.h > @@ -141,6 +141,9 @@ struct Dwfl > struct Dwfl_User_Core *user_core; > char *sysroot; /* sysroot, or NULL to search standard system > paths */ > + > + /* Serialize debuginfod_client usage. */ > + mutex_define (, debuginfod_lock); > }; > > #define OFFLINE_REDZONE 0x10000
