Hi Mark,
On Tue, Feb 10, 2026 at 8:42 AM Mark Wielaard <[email protected]> wrote:
>
> Hi Aaron,
>
> On Mon, 2026-02-09 at 08:16 -0500, Aaron Merey wrote:
> > Acquire abbrev_lock before reading cu->last_abbrev_offset in dwarf_tag.
>
> This seems correct. Getting and setting the cu->last_abbrev_offset
> value should only be done under a lock.
>
> > Also double check for abbrev hash table entry once lock is held to
> > avoid multiple threads attempting to populate the abbrev hash table.
>
> But I think this isn't necessary because __libdw_getabbrev already does
> that.
>
> > Signed-off-by: Aaron Merey <[email protected]>
> > ---
> > libdw/dwarf_tag.c | 48 ++++++++++++++++++++++++++++-------------------
> > 1 file changed, 29 insertions(+), 19 deletions(-)
> >
> > diff --git a/libdw/dwarf_tag.c b/libdw/dwarf_tag.c
> > index 7daa4813..69e3bdc2 100644
> > --- a/libdw/dwarf_tag.c
> > +++ b/libdw/dwarf_tag.c
> > @@ -47,30 +47,40 @@ __libdw_findabbrev (struct Dwarf_CU *cu, unsigned int
> > code)
> > /* See whether the entry is already in the hash table. */
> > abb = Dwarf_Abbrev_Hash_find (&cu->abbrev_hash, code);
> > if (abb == NULL)
> > - while (cu->last_abbrev_offset != (size_t) -1l)
> > - {
> > - size_t length;
> > + {
> > + mutex_lock (cu->abbrev_lock);
>
> This is good.
>
> >
> > - /* Find the next entry. It gets automatically added to the
> > - hash table. */
> > - mutex_lock (cu->abbrev_lock);
> > - abb = __libdw_getabbrev (cu->dbg, cu, cu->last_abbrev_offset,
> > &length);
> > + /* Check once more in case entry was added before abbrev_lock
> > + was aquired. */
> > + abb = Dwarf_Abbrev_Hash_find (&cu->abbrev_hash, code);
> >
> > - if (abb == NULL || abb == DWARF_END_ABBREV)
> > + if (abb == NULL)
>
> But this part seems unnecessary because...
>
> > + while (cu->last_abbrev_offset != (size_t) -1l)
The issue is this check `last_abbrev_offset != (size_t) -1l`. If
multiple threads have Dwarf_Abbrev_Hash_find return NULL,
last_abbrev_offset != -1 for only the first thread to grab the lock.
The other threads will grab the lock and see last_abbrev_offset == -1.
In this case dwarf_tag returns DWARF_END_ABBREV instead of the abbrev
cached by the first thread.
Aaron
> > {
> > - /* Make sure we do not try to search for it again. */
> > - cu->last_abbrev_offset = (size_t) -1l;
> > - mutex_unlock (cu->abbrev_lock);
> > - return DWARF_END_ABBREV;
> > + size_t length;
> > +
> > + /* Find the next entry. It gets automatically added to the
> > + hash table. */
> > + abb = __libdw_getabbrev (cu->dbg, cu, cu->last_abbrev_offset,
> > + &length);
>
> This will work just fine if the abbrev is already in the hash table.
>
> > +
> > + if (abb == NULL || abb == DWARF_END_ABBREV)
> > + {
> > + /* Make sure we do not try to search for it again. */
> > + cu->last_abbrev_offset = (size_t) -1l;
> > + mutex_unlock (cu->abbrev_lock);
> > + return DWARF_END_ABBREV;
> > + }
> > +
> > + cu->last_abbrev_offset += length;
> > +
> > + /* Is this the code we are looking for? */
> > + if (abb->code == code)
> > + break;
> > }
> >
> > - cu->last_abbrev_offset += length;
> > - mutex_unlock (cu->abbrev_lock);
> > -
> > - /* Is this the code we are looking for? */
> > - if (abb->code == code)
> > - break;
> > - }
> > + mutex_unlock (cu->abbrev_lock);
> > + }
>
> All good.
>
> >
> > /* This is our second (or third, etc.) call to __libdw_findabbrev
> > and the code is invalid. */
>
> Cheers,
>
> Mark
>
