Re: [PATCH 11/16] libdw: Add locking around __libdw_dieabbrev for dwarf_hasattr

[Mark Wielaard <mark@klomp.org>]

Hi Heather,

On Tue, Oct 17, 2023 at 02:57:39PM -0500, Heather McIntyre wrote:
> I see now that this is incomplete considering the other places that also
> call this function. I do agree that global locking may be heavy if 1)
> implemented in all of these locations, or 2) implemented directly in
> __libdw_dieabbrev. We could use atomics here directly in __libdw_dieabbrev.
> I have given this a try and it is currently passing all tests, including
> the new ones I added for data race detection.
> 
> I know you mentioned that taking any pthread lock at all might be a big
> overhead, but since I implemented a per dwarf struct lock, would using that
> be a possibility? Assuming multiple calls to __libdw_dieabbrev will be
> working on different dwarf objects.

I have been thinking about this issue and I think we made a mistake in
designing how a Dwarf_Die is lazy initialized. The abbrev field of a
Dwarf_Die is only set when needed by calling __libdw_dieabbrev, which
means we need some kind of locking or atomic swapping whenever we try
to use that field. I assume the idea originally was that calling
__libdw_dieabbrev is fairly "heavy" (it is, potentially reading the
whole .debug_abbrev for the CU). So we try to postpone it till it is
really needed.

But in practice it is always needed. Without the abbrev field set you
can just call dwarf_dieoffset, dwarf_cuoffset, dwarf_diecu and
dwarf_getabbrev. In theory you could avoid adding the abbrev for the
initial CU DIE for a Dwarf_CU when you are iterating over all CUs and
know you don't need the CU without inspecting the initial CU DIE. But
the Dwarf_Abbrev_Hash for the Dwarf_CU will already be
initialized. And normally the abbrev for the first DIE will also be
the first abbrev, so searching for it should be really quick.

So I think setting the Dwarf_Die abbrev field lazy is not really
helpful and makes the code needlessly complex. If we set the abbrev
field when a Dwarf_Die is created we can simplify the code and don't
need all this locking when we just want to access the field.

This of course is still a lot of coding, we'll have to check every
place that initializes a new Dwarf_Die. Which will have to call
__libdw_dieabbrev directly. But I think that will not need any extra
locking because the Dwarf_Abbrev_Hash used is already thread-safe (it
was written by Srđan Milaković also from Rice University).

What do you think?

Cheers,

Mark