Hi Aaron, On Tue, 2026-10-27 at 20:34 -0400, Aaron Merey wrote: > Use a CFI cache's corresponding dwfl load_lock to serialize its > initialization. Rename __libdwfl_set_cfi to __libdwfl_set_cfi_locked > to indicate that the caller must hold the load_lock. Atomics are used > to acquire an initialized CFI cache without needing to obtain the > load_lock.
This is slightly more complicated because __libdwfl_set_cfi is used to set either the Dwfl_Module eh_cfi or dwarf_cfi field and we assume the caller provides the correct/matching arguments. Luckily there are only two callers of __libdwfl_set_cfi, so it is easy to double check. > Signed-off-by: Aaron Merey <[email protected]> > --- > libdwfl/dwfl_module.c | 2 +- > libdwfl/dwfl_module_dwarf_cfi.c | 25 ++++++++++++++++++------- > libdwfl/dwfl_module_eh_cfi.c | 26 ++++++++++++++++++-------- > libdwfl/libdwflP.h | 7 ++++--- > 4 files changed, 41 insertions(+), 19 deletions(-) > > diff --git a/libdwfl/dwfl_module.c b/libdwfl/dwfl_module.c > index b4ddf806..27e6c064 100644 > --- a/libdwfl/dwfl_module.c > +++ b/libdwfl/dwfl_module.c > @@ -74,7 +74,7 @@ __libdwfl_module_free (Dwfl_Module *mod) > } > > /* We might have primed the Dwarf_CFI ebl cache with our own ebl > - in __libdwfl_set_cfi. Make sure we don't free it twice. */ > + in __libdwfl_set_cfi_locked. Make sure we don't free it twice. */ > if (mod->eh_cfi != NULL) > { > if (mod->eh_cfi->ebl != NULL && mod->eh_cfi->ebl == mod->ebl) OK, just comment update, __libdwfl_module_free doesn't need any locking. > diff --git a/libdwfl/dwfl_module_dwarf_cfi.c b/libdwfl/dwfl_module_dwarf_cfi.c > index 05a14f7c..2da02b32 100644 > --- a/libdwfl/dwfl_module_dwarf_cfi.c > +++ b/libdwfl/dwfl_module_dwarf_cfi.c > @@ -35,7 +35,7 @@ > > Dwarf_CFI * > internal_function > -__libdwfl_set_cfi (Dwfl_Module *mod, Dwarf_CFI **slot, Dwarf_CFI *cfi) > +__libdwfl_set_cfi_locked (Dwfl_Module *mod, Dwarf_CFI **slot, Dwarf_CFI *cfi) > { > if (cfi != NULL && cfi->ebl == NULL) > { > @@ -51,7 +51,9 @@ __libdwfl_set_cfi (Dwfl_Module *mod, Dwarf_CFI **slot, > Dwarf_CFI *cfi) > } > } > > - return *slot = cfi; > + if (cfi != NULL) > + atomic_store_release (slot, cfi); > + return cfi; > } OK, name changed. This just uses atomic_store to signal the slot is set. No other locking needed. Maybe there should be a sanity check that *slot == &mod->eh_cfi || *slot == &mod->dwarf->cfi? > Dwarf_CFI * > @@ -60,14 +62,23 @@ dwfl_module_dwarf_cfi (Dwfl_Module *mod, Dwarf_Addr *bias) > if (mod == NULL) > return NULL; > > - if (mod->dwarf_cfi != NULL) > + Dwarf_CFI *cfi = atomic_load_acquire (&mod->dwarf_cfi); > + if (cfi != NULL) > { > *bias = dwfl_adjusted_dwarf_addr (mod, 0); > - return mod->dwarf_cfi; > + return cfi; > } OK, use atomic_load to get the field, return it directly instead of reloading through mod. > - return __libdwfl_set_cfi (mod, &mod->dwarf_cfi, > - INTUSE(dwarf_getcfi) > - (INTUSE(dwfl_module_getdwarf) (mod, bias))); > + mutex_lock (mod->dwfl->load_lock); > + cfi = mod->dwarf_cfi; > + if (cfi == NULL) > + cfi = __libdwfl_set_cfi_locked (mod, &mod->dwarf_cfi, > + INTUSE(dwarf_getcfi) > + (INTUSE(dwfl_module_getdwarf) (mod, bias))); > + else > + *bias = dwfl_adjusted_dwarf_addr (mod, 0); > + > + mutex_unlock (mod->dwfl->load_lock); > + return cfi; OK, get the lock, reload the dwarf_cfi through mod to see if someone raced passed us and set it. > } > INTDEF (dwfl_module_dwarf_cfi) > diff --git a/libdwfl/dwfl_module_eh_cfi.c b/libdwfl/dwfl_module_eh_cfi.c > index aac6657d..cc28ad60 100644 > --- a/libdwfl/dwfl_module_eh_cfi.c > +++ b/libdwfl/dwfl_module_eh_cfi.c > @@ -39,21 +39,31 @@ dwfl_module_eh_cfi (Dwfl_Module *mod, Dwarf_Addr *bias) > if (mod == NULL) > return NULL; > > - if (mod->eh_cfi != NULL) > + Dwarf_CFI *cfi = atomic_load_acquire (&mod->eh_cfi); > + if (cfi != NULL) > { > *bias = dwfl_adjusted_address (mod, 0); > - return mod->eh_cfi; > + return cfi; > } OK, like above but for eh_cfi field. > - __libdwfl_getelf (mod); > - if (mod->elferr != DWFL_E_NOERROR) > + mutex_lock (mod->dwfl->load_lock); > + cfi = mod->eh_cfi; > + if (cfi == NULL) > { > - __libdwfl_seterrno (mod->elferr); > - return NULL; > + __libdwfl_getelf (mod); > + if (mod->elferr != DWFL_E_NOERROR) > + { > + __libdwfl_seterrno (mod->elferr); > + mutex_unlock (mod->dwfl->load_lock); > + return NULL; > + } > + > + cfi = __libdwfl_set_cfi_locked (mod, &mod->eh_cfi, > + INTUSE(dwarf_getcfi_elf) (mod->main.elf)); > } > > + mutex_unlock (mod->dwfl->load_lock); > *bias = dwfl_adjusted_address (mod, 0); > - return __libdwfl_set_cfi (mod, &mod->eh_cfi, > - INTUSE(dwarf_getcfi_elf) (mod->main.elf)); > + return cfi; > } > INTDEF (dwfl_module_eh_cfi) OK, likewise. > diff --git a/libdwfl/libdwflP.h b/libdwfl/libdwflP.h > index 7a181029..56014ff6 100644 > --- a/libdwfl/libdwflP.h > +++ b/libdwfl/libdwflP.h > @@ -521,9 +521,10 @@ extern Dwfl_Error __libdwfl_relocate_value (Dwfl_Module > *mod, Elf *elf, > /* Ensure that MOD->ebl is set up. */ > extern Dwfl_Error __libdwfl_module_getebl (Dwfl_Module *mod) > internal_function; > > -/* Install a new Dwarf_CFI in *SLOT (MOD->eh_cfi or MOD->dwarf_cfi). */ > -extern Dwarf_CFI *__libdwfl_set_cfi (Dwfl_Module *mod, Dwarf_CFI **slot, > - Dwarf_CFI *cfi) > +/* Install a new Dwarf_CFI in *SLOT (MOD->eh_cfi or MOD->dwarf_cfi). > + MOD->dwfl->load_lock must be held by the caller. */ > +extern Dwarf_CFI *__libdwfl_set_cfi_locked (Dwfl_Module *mod, Dwarf_CFI > **slot, > + Dwarf_CFI *cfi) > internal_function; > > /* Iterate through all the CU's in the module. Start by passing a null And rename it. Looks ok and correct. Thanks, Mark
