☠ Buildbot (Sourceware): elfutils - failed test (failure) (master)
A new failure has been detected on builder elfutils-gentoo-sparc while building elfutils. Full details are available at: https://builder.sourceware.org/buildbot/#builders/225/builds/107 Build state: failed test (failure) Revision: 6d84c62cebeb2768dcb4a1843d503537d86a153e Worker: gentoo-sparc Build Reason: (unknown) Blamelist: Heather McIntyre Steps: - 0: worker_preparation ( success ) - 1: set package name ( success ) - 2: git checkout ( success ) Logs: - stdio: https://builder.sourceware.org/buildbot/#builders/225/builds/107/steps/2/logs/stdio - 3: autoreconf ( success ) Logs: - stdio: https://builder.sourceware.org/buildbot/#builders/225/builds/107/steps/3/logs/stdio - 4: configure ( success ) Logs: - stdio: https://builder.sourceware.org/buildbot/#builders/225/builds/107/steps/4/logs/stdio - config.log: https://builder.sourceware.org/buildbot/#builders/225/builds/107/steps/4/logs/config_log - 5: get version ( success ) Logs: - stdio: https://builder.sourceware.org/buildbot/#builders/225/builds/107/steps/5/logs/stdio - property changes: https://builder.sourceware.org/buildbot/#builders/225/builds/107/steps/5/logs/property_changes - 6: make ( warnings ) Logs: - stdio: https://builder.sourceware.org/buildbot/#builders/225/builds/107/steps/6/logs/stdio - warnings (3): https://builder.sourceware.org/buildbot/#builders/225/builds/107/steps/6/logs/warnings__3_ - 7: make check ( failure ) Logs: - stdio: https://builder.sourceware.org/buildbot/#builders/225/builds/107/steps/7/logs/stdio - test-suite.log: https://builder.sourceware.org/buildbot/#builders/225/builds/107/steps/7/logs/test-suite_log - 8: prep ( success ) Logs: - stdio: https://builder.sourceware.org/buildbot/#builders/225/builds/107/steps/8/logs/stdio - 9: build bunsen.cpio.gz ( success ) Logs: - stdio: https://builder.sourceware.org/buildbot/#builders/225/builds/107/steps/9/logs/stdio - 10: fetch bunsen.cpio.gz ( success ) Logs: - stdio: https://builder.sourceware.org/buildbot/#builders/225/builds/107/steps/10/logs/stdio - 11: unpack bunsen.cpio.gz ( success ) Logs: - stdio: https://builder.sourceware.org/buildbot/#builders/225/builds/107/steps/11/logs/stdio - 12: pass .bunsen.source.gitname ( success ) Logs: - stdio: https://builder.sourceware.org/buildbot/#builders/225/builds/107/steps/12/logs/stdio - 13: pass .bunsen.source.gitdescribe ( success ) Logs: - stdio: https://builder.sourceware.org/buildbot/#builders/225/builds/107/steps/13/logs/stdio - 14: pass .bunsen.source.gitbranch ( success ) Logs: - stdio: https://builder.sourceware.org/buildbot/#builders/225/builds/107/steps/14/logs/stdio - 15: pass .bunsen.source.gitrepo ( success ) Logs: - stdio: https://builder.sourceware.org/buildbot/#builders/225/builds/107/steps/15/logs/stdio - 16: upload to bunsen ( success ) Logs: - stdio: https://builder.sourceware.org/buildbot/#builders/225/builds/107/steps/16/logs/stdio - 17: clean up ( success ) Logs: - stdio: https://builder.sourceware.org/buildbot/#builders/225/builds/107/steps/17/logs/stdio - 18: make distclean ( success ) Logs: - stdio: https://builder.sourceware.org/buildbot/#builders/225/builds/107/steps/18/logs/stdio
Re: [PATCH] Fix thread-safety for elfutils
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
Re: ☠ Buildbot (Sourceware): elfutils - failed test (failure) (master)
Hi Heather, On Sat, 2023-10-14 at 15:32 +, buil...@sourceware.org wrote: > A new failure has been detected on builder elfutils-gentoo-sparc while > building elfutils. > > Full details are available at: > https://builder.sourceware.org/buildbot/#builders/225/builds/107 > > Build state: failed test (failure) > Revision: 6d84c62cebeb2768dcb4a1843d503537d86a153e > Worker: gentoo-sparc > Build Reason: (unknown) > Blamelist: Heather McIntyre > [...] > - 7: make check ( failure ) > Logs: > - stdio: > https://builder.sourceware.org/buildbot/#builders/225/builds/107/steps/7/logs/stdio > - test-suite.log: > https://builder.sourceware.org/buildbot/#builders/225/builds/107/steps/7/logs/test-suite_log This was not caused by your patch. For some reason the sparc-gentoo builder hang (maybe because of the run-large-elf-file.sh test?) Happily things look pretty green for everything else: https://builder.sourceware.org/buildbot/#/builders?tags=elfutils Cheers, Mark
☺ Buildbot (Sourceware): elfutils - build successful (master)
A restored build has been detected on builder elfutils-gentoo-sparc while building elfutils. Full details are available at: https://builder.sourceware.org/buildbot/#builders/225/builds/108 Build state: build successful Revision: 4ecc50a568a8d8b4ddbc924a37d0364b2ae6ba69 Worker: gentoo-sparc-big Build Reason: (unknown) Blamelist: Heather McIntyre Steps: - 0: worker_preparation ( success ) - 1: set package name ( success ) - 2: git checkout ( success ) Logs: - stdio: https://builder.sourceware.org/buildbot/#builders/225/builds/108/steps/2/logs/stdio - 3: autoreconf ( success ) Logs: - stdio: https://builder.sourceware.org/buildbot/#builders/225/builds/108/steps/3/logs/stdio - 4: configure ( success ) Logs: - stdio: https://builder.sourceware.org/buildbot/#builders/225/builds/108/steps/4/logs/stdio - config.log: https://builder.sourceware.org/buildbot/#builders/225/builds/108/steps/4/logs/config_log - 5: get version ( success ) Logs: - stdio: https://builder.sourceware.org/buildbot/#builders/225/builds/108/steps/5/logs/stdio - property changes: https://builder.sourceware.org/buildbot/#builders/225/builds/108/steps/5/logs/property_changes - 6: make ( warnings ) Logs: - stdio: https://builder.sourceware.org/buildbot/#builders/225/builds/108/steps/6/logs/stdio - warnings (3): https://builder.sourceware.org/buildbot/#builders/225/builds/108/steps/6/logs/warnings__3_ - 7: make check ( success ) Logs: - stdio: https://builder.sourceware.org/buildbot/#builders/225/builds/108/steps/7/logs/stdio - test-suite.log: https://builder.sourceware.org/buildbot/#builders/225/builds/108/steps/7/logs/test-suite_log - 8: prep ( success ) Logs: - stdio: https://builder.sourceware.org/buildbot/#builders/225/builds/108/steps/8/logs/stdio - 9: build bunsen.cpio.gz ( success ) Logs: - stdio: https://builder.sourceware.org/buildbot/#builders/225/builds/108/steps/9/logs/stdio - 10: fetch bunsen.cpio.gz ( success ) Logs: - stdio: https://builder.sourceware.org/buildbot/#builders/225/builds/108/steps/10/logs/stdio - 11: unpack bunsen.cpio.gz ( success ) Logs: - stdio: https://builder.sourceware.org/buildbot/#builders/225/builds/108/steps/11/logs/stdio - 12: pass .bunsen.source.gitname ( success ) Logs: - stdio: https://builder.sourceware.org/buildbot/#builders/225/builds/108/steps/12/logs/stdio - 13: pass .bunsen.source.gitdescribe ( success ) Logs: - stdio: https://builder.sourceware.org/buildbot/#builders/225/builds/108/steps/13/logs/stdio - 14: pass .bunsen.source.gitbranch ( success ) Logs: - stdio: https://builder.sourceware.org/buildbot/#builders/225/builds/108/steps/14/logs/stdio - 15: pass .bunsen.source.gitrepo ( success ) Logs: - stdio: https://builder.sourceware.org/buildbot/#builders/225/builds/108/steps/15/logs/stdio - 16: upload to bunsen ( success ) Logs: - stdio: https://builder.sourceware.org/buildbot/#builders/225/builds/108/steps/16/logs/stdio - 17: clean up ( success ) Logs: - stdio: https://builder.sourceware.org/buildbot/#builders/225/builds/108/steps/17/logs/stdio - 18: make distclean ( success ) Logs: - stdio: https://builder.sourceware.org/buildbot/#builders/225/builds/108/steps/18/logs/stdio
Re: [PATCH] Fix thread-safety for elfutils
Hi Mark, Thank you for taking the time to review the patch and for your thoughtful feedback. I really appreciate your attention to detail and effort in splitting the commit into smaller, more manageable parts. John and I went over some of your comments and concerns yesterday, and I will be working on addressing them ASAP. I have a quick query regarding how you'd prefer to handle these changes. Would you like me to implement some of the recommended modifications and commit them (if possible), or would you prefer that I simply leave comments so you can make the necessary adjustments? I noticed that the split-up branch resides on what appears to be your personal space at https://code.wildebeest.org/git/user/mjw/elfutils/log/?h=thread-safety. I'm uncertain whether I have the necessary access for making commits there. Best regards, Heather McIntyre On Sat, Oct 14, 2023 at 10:40 AM Mark Wielaard wrote: > 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 most
