Re: [PATCH] Fix thread-safety for elfutils

[Mark Wielaard <mark@klomp.org>]

Hi Heather,

On Tue, 2023-10-10 at 15:40 +0200, Mark Wielaard wrote:
> Very nice. That is a lot of work. And I must admit that I cannot hold
> that much work in my little head at the same time. So I have split up
> your commit into (what I hope are) logical independent parts. That will
> make it easier to review. I might have split it into too many parts,
> but that at least makes it easier to just add those parts that are
> trivially correct.
> 
> The only changes I made were:
> 1. Move the ChangeLog entries into the commit message
>    (This is something we do now and makes cherry picking small
>     changes easier, but I see it isn't actually documented
>     in CONTRIBUTING, sorry. I'll try to update that.)
> 2. Fixed up some Copyright notices as discussed off-list.
> 3. Made some whitespace/indentation changes which made the
>    diffs slightly smaller (in most cases).
> 
> I'll comment/review the individual commits. Which I'll post to the
> list.

So I commented on each one individually.
Below is the summary. I have pushed 4 patches to git trunk already.
I am suggesting to drop 2 changes, but please feel free to say we
really need them. I have some questions about the rest, but some are
minor issues. In general I really like the direction of this.

Hope my splitting of your patch isn't too confusing. But it really
helps me review separate smaller independent parts. And it makes it so
we can push more of your changes early.

> Heather McIntyre (16):
>       lib: Add new once_define and once macros to eu-config.h

Looks good. Added to main branch.

>       libelf: Make elf_version thread-safe

Looks good. Added to main branch.
Question whether this could also have been done with atomics?

>       libelf: Fix deadlock in __libelf_readall

I think the locking here is subtly wrong in the case of
elf->map_address != NULL

Suggest to look into rewriting libelf_acquire_all and
libelf_release_all to libelf_acquire_all_children and
libelf_release_all_children (which would only be called
with the elf->lock already acquired).

>       libelf: Fix deadlock in elf_cntl

Looks correct, but can probably be simplified by just relying on
if (__libelf_readall (elf) == NULL)

>       libelf: Fix elf_end deadlock

Looks good. Added to main branch.

>       libelf: Make elf32_getchdr and elf64_getchdr thread-safe

Looks good. Just needs elf32_getchdr.h added to noinst_HEADERS.
Pushed to the main branch with that change.

>       lib: Add eu_tsearch and eu_tfind

I think this would work. But I really like to discuss whether we can
have more fine grained locking. The static global lock will likely
cause too much contention. I added a similar change to noinst_HEADERS
as above for eu-search.h.

>       libcpu: Change calls for tsearch/tfind to eu_tsearch/eu_tfind.

Suggest to drop this for now. It will need a twalk wrapper. And I am
not sure it will be easy to make the bison parser concurrent as a
whole. This is for libasm, not libdw, so can imho be handled separately
if we really want to make libasm also completely thread-safe.

>       src: Use eu-search in nm and findtextrel.

Although this works, I don't think it is needed because eu-nm and eu-
findtextrel are single threaded utilities.

>       libdw: make dwarf_getalt thread-safe

I think this should also include dwarf_setalt. Also lets discuss
whether the locking is done correctly.

>       libdw: Add locking around __libdw_dieabbrev for dwarf_hasattr

I think this is also incomplete. dwarf_child, dwarf_getattrs,
dwarf_haschildren and dwarf_tag also call __libdw_dieabbrev. I am also
concerned this locking is very "heavy". Lets discuss whether there is
an alternative way to update the die->abbrev in a thead-safe way.

>       libdw: Make libdw_find_split_unit thread-safe

I have some concerns about whether the locking is complete/wide enough.

>       libdw: Make libdw_findcu thread-safe

Looks mostly correct. Question about whether this could be a per struct
Dwarf lock and whether the lock should also be added for the
dwarf_formref_die call to __libdw_intern_next_unit.

>       libdw,libdwfl: Use eu-search for thread-safety

These all look good.  Just have to think about whether the global
static lock currently used in eu-search is the right approach.

>       tests: Add eu-search tests

I am enthusiastic about having these new tests. Some suggestions to
simplify the mechanics. I also have updated the Copyright on the new
files. Could you look into the invocation of eu_search_macros. Maybe I
am misinterpreting how this should be used. But it looks like this will
always fail.

>       configure: No longer mark --enable-thread-safety as EXPERIMENTAL

Looks good. Will push once at least all libelf patches are in.
Note, your NEWS entry seems missing.

> Which can also be found here:
> https://code.wildebeest.org/git/user/mjw/elfutils/log/?h=thread-safety

I have updated that git branch with some of the suggestions above and
removed the commits that have already been pushed.

Thanks,

Mark