Re: [RFC 0/3] [gdb/symtab] Fix data-races in gdb.base/index-cache.exp

[Tom de Vries <tdevries@suse.de>]

On 7/28/23 10:56, Tom de Vries via Gdb-patches wrote:
> When building gdb with -fsanitize=thread, we run into a data race in
> gdb.base/index-cache.exp.
> 

I've submitted a v2 here ( 
https://sourceware.org/pipermail/gdb-patches/2023-August/201270.html ).

> Fixing this leads us to another, and another, so there are three patches, each
> fixing a data race.
> 
> The first two patches implement the approach mentioned in PR30392 comment 2:
> ...
> The reader probably should capture the necessarily globals
> on the main thread and stash them until the index has been
> written.
> ...
> 
> The 3rd patch cannot be fixed with this approach, so it uses the std::atomic
> approach:
> ...
> -  unsigned int queued : 1;
> +  std::atomic<bool> queued;
> ...
> 
> The 3rd patch is the reason this is an RFC.
> 

In v2 I'm no longer using std::atomic<bool>, instead I'm using 
packed<bool, 1>.

> I spent some time convincing myself that this is not a benign data race.  I
> started with reading [1], and got convinced by "2.5 Disjoint bit manipulation"
> in [2].  Also [3] looked interesting, but haven't read it in full.
> 
> As for fixing it, the std::atomic approach works.  I checked that the struct
> size is not increased.  It gives me an almost clean test-suite run (still
> running into PR30680), though I'm running into a lot of timeouts, so things
> still may be hidden there.
> 

I now also ran on a fast server with lots of memory, and the timeouts do 
not happen there.

> Alternatively, if I limit the fix to:
> ...
> -  unsigned int queued : 1;
> +  bool queued;
> ...
> I run into 19 more reported data races in the test-case.  None of them look
> as evidence that this fix on queued is insufficient.
> 
> Looking at the first one (see appendix for more detail), there's a race
> between:
> - the main thread calling find_main_name, because of calling main_language in
>    set_initial_language, and
> - the worker thread calling find_main_name, because of calling main_name in
>    write_cooked_index.
> At least 14 other of the 19 reported data races look like instances of the
> first one.
> 
> This looks like a real problem to me, but with the proposed fix it no longer
> triggers, although I fail to see how it fixes it.
> 

I managed to trigger data races by running with taskset -c 0.  I ended 
up fixing data races until the combination of make-check-all.sh and 
taskset -c 0 no longer triggered any.  Indeed one of the patches in v2 
does address the find_main_name issue.

> Which leaves me with the questions:
> - is the std::atomic approach too blunt, and are we consequently hiding
>    problems, and
> - should we address the find_main_name race in some form or another.
> 
I no longer have these questions, so the v2 no longer is an RFC.

Thanks,
- Tom

> Tested on x86_64-linux, with and without -fsanitize=thread.
> 
> PR symtab/30392
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30392
> 
> References:
> [1] https://hacks.mozilla.org/2021/04/eliminating-data-races-in-firefox-a-technical-report/
> [2] https://www.usenix.org/legacy/event/hotpar11/tech/final_files/Boehm.pdf
> [3] https://bartoszmilewski.com/2020/08/11/benign-data-races-considered-harmful/
> 
> Appendix:
> ...
> (gdb) file build/gdb/testsuite/outputs/gdb.base/index-cache/index-cache
> Reading symbols from build/gdb/testsuite/outputs/gdb.base/index-cache/index-cache...
> ==================
> WARNING: ThreadSanitizer: data race (pid=1569)
>    Read of size 1 at 0x7b4400097d08 by thread T12:
>      #0 bfd_get_section_limit_octets bfd.h:2376 (gdb+0x149b613)
>      #1 bfd_get_section_contents bfd/section.c:1571 (gdb+0x149cbe4)
>      #2 gdb_bfd_scan_elf_dyntag(int, bfd*, unsigned long*, unsigned long*) gdb/solib.c:1601 (gdb+0xed8fc8)
>      #3 elf_locate_base gdb/solib-svr4.c:705 (gdb+0xec29aa)
>      #4 svr4_iterate_over_objfiles_in_search_order gdb/solib-svr4.c:3430 (gdb+0xeca65b)
>      #5 gdbarch_iterate_over_objfiles_in_search_order(gdbarch*, gdb::function_view<bool (objfile*)>, objfile*) gdb/gdbarch.c:5041 (gdb+0x537cad)
>      #6 find_main_name gdb/symtab.c:6270 (gdb+0xf744a3)
>      #7 main_name() gdb/symtab.c:6299 (gdb+0xf74531)
>      #8 write_cooked_index gdb/dwarf2/index-write.c:1131 (gdb+0x830af4)
>      #9 write_gdbindex gdb/dwarf2/index-write.c:1256 (gdb+0x83134b)
>      #10 write_dwarf_index(dwarf2_per_bfd*, char const*, char const*, char const*, dw_index_kind) gdb/dwarf2/index-write.c:1484 (gdb+0x83232f)
>      #11 index_cache::store(dwarf2_per_bfd*, index_cache_store_context*) gdb/dwarf2/index-cache.c:177 (gdb+0x82d62b)
>      #12 cooked_index::maybe_write_index(dwarf2_per_bfd*) gdb/dwarf2/cooked-index.c:640 (gdb+0x7f1bf7)
>      #13 operator() gdb/dwarf2/cooked-index.c:470 (gdb+0x7f0f40)
>      #14 _M_invoke /usr/include/c++/7/bits/std_function.h:316 (gdb+0x7f2909)
>      #15 std::function<void ()>::operator()() const /usr/include/c++/7/bits/std_function.h:706 (gdb+0x700952)
>      #16 void std::__invoke_impl<void, std::function<void ()>&>(std::__invoke_other, std::function<void ()>&) /usr/include/c++/7/bits/invoke.h:60 (gdb+0x7381a0)
>      #17 std::__invoke_result<std::function<void ()>&>::type std::__invoke<std::function<void ()>&>(std::function<void ()>&) /usr/include/c++/7/bits/invoke.h:95 (gdb+0x737e91)
>      #18 std::__future_base::_Task_state<std::function<void ()>, std::allocator<int>, void ()>::_M_run()::{lambda()#1}::operator()() const /usr/include/c++/7/future:1421 (gdb+0x737b59)
>      #19 std::__future_base::_Task_setter<std::unique_ptr<std::__future_base::_Result<void>, std::__future_base::_Result_base::_Deleter>, std::__future_base::_Task_state<std::function<void ()>, std::allocator<int>, void ()>::_M_run()::{lambda()#1}, void>::operator()() const /usr/include/c++/7/future:1362 (gdb+0x738660)
>      #20 std::_Function_handler<std::unique_ptr<std::__future_base::_Result_base, std::__future_base::_Result_base::_Deleter> (), std::__future_base::_Task_setter<std::unique_ptr<std::__future_base::_Result<void>, std::__future_base::_Result_base::_Deleter>, std::__future_base::_Task_state<std::function<void ()>, std::allocator<int>, void ()>::_M_run()::{lambda()#1}, void> >::_M_invoke(std::_Any_data const&) /usr/include/c++/7/bits/std_function.h:302 (gdb+0x73825c)
>      #21 std::function<std::unique_ptr<std::__future_base::_Result_base, std::__future_base::_Result_base::_Deleter> ()>::operator()() const /usr/include/c++/7/bits/std_function.h:706 (gdb+0x733623)
>      #22 std::__future_base::_State_baseV2::_M_do_set(std::function<std::unique_ptr<std::__future_base::_Result_base, std::__future_base::_Result_base::_Deleter> ()>*, bool*) /usr/include/c++/7/future:561 (gdb+0x732bdf)
>      #23 void std::__invoke_impl<void, void (std::__future_base::_State_baseV2::*)(std::function<std::unique_ptr<std::__future_base::_Result_base, std::__future_base::_Result_base::_Deleter> ()>*, bool*), std::__future_base::_State_baseV2*, std::function<std::unique_ptr<std::__future_base::_Result_base, std::__future_base::_Result_base::_Deleter> ()>*, bool*>(std::__invoke_memfun_deref, void (std::__future_base::_State_baseV2::*&&)(std::function<std::unique_ptr<std::__future_base::_Result_base, std::__future_base::_Result_base::_Deleter> ()>*, bool*), std::__future_base::_State_baseV2*&&, std::function<std::unique_ptr<std::__future_base::_Result_base, std::__future_base::_Result_base::_Deleter> ()>*&&, bool*&&) /usr/include/c++/7/bits/invoke.h:73 (gdb+0x734c4f)
>      #24 std::__invoke_result<void (std::__future_base::_State_baseV2::*)(std::function<std::unique_ptr<std::__future_base::_Result_base, std::__future_base::_Result_base::_Deleter> ()>*, bool*), std::__future_base::_State_baseV2*, std::function<std::unique_ptr<std::__future_base::_Result_base, std::__future_base::_Result_base::_Deleter> ()>*, bool*>::type std::__invoke<void (std::__future_base::_State_baseV2::*)(std::function<std::unique_ptr<std::__future_base::_Result_base, std::__future_base::_Result_base::_Deleter> ()>*, bool*), std::__future_base::_State_baseV2*, std::function<std::unique_ptr<std::__future_base::_Result_base, std::__future_base::_Result_base::_Deleter> ()>*, bool*>(void (std::__future_base::_State_baseV2::*&&)(std::function<std::unique_ptr<std::__future_base::_Result_base, std::__future_base::_Result_base::_Deleter> ()>*, bool*), std::__future_base::_State_baseV2*&&, std::function<std::unique_ptr<std::__future_base::_Result_base, std::__future_base::_Result_base::_Deleter> ()>*&&, bool*&&) /usr/include/c++/7/bits/invoke.h:95 (gdb+0x733bc5)
>      #25 std::call_once<void (std::__future_base::_State_baseV2::*)(std::function<std::unique_ptr<std::__future_base::_Result_base, std::__future_base::_Result_base::_Deleter> ()>*, bool*), std::__future_base::_State_baseV2*, std::function<std::unique_ptr<std::__future_base::_Result_base, std::__future_base::_Result_base::_Deleter> ()>*, bool*>(std::once_flag&, void (std::__future_base::_State_baseV2::*&&)(std::function<std::unique_ptr<std::__future_base::_Result_base, std::__future_base::_Result_base::_Deleter> ()>*, bool*), std::__future_base::_State_baseV2*&&, std::function<std::unique_ptr<std::__future_base::_Result_base, std::__future_base::_Result_base::_Deleter> ()>*&&, bool*&&)::{lambda()#1}::operator()() const /usr/include/c++/7/mutex:672 (gdb+0x73300d)
>      #26 std::call_once<void (std::__future_base::_State_baseV2::*)(std::function<std::unique_ptr<std::__future_base::_Result_base, std::__future_base::_Result_base::_Deleter> ()>*, bool*), std::__future_base::_State_baseV2*, std::function<std::unique_ptr<std::__future_base::_Result_base, std::__future_base::_Result_base::_Deleter> ()>*, bool*>(std::once_flag&, void (std::__future_base::_State_baseV2::*&&)(std::function<std::unique_ptr<std::__future_base::_Result_base, std::__future_base::_Result_base::_Deleter> ()>*, bool*), std::__future_base::_State_baseV2*&&, std::function<std::unique_ptr<std::__future_base::_Result_base, std::__future_base::_Result_base::_Deleter> ()>*&&, bool*&&)::{lambda()#2}::operator()() const /usr/include/c++/7/mutex:677 (gdb+0x7330b2)
>      #27 std::call_once<void (std::__future_base::_State_baseV2::*)(std::function<std::unique_ptr<std::__future_base::_Result_base, std::__future_base::_Result_base::_Deleter> ()>*, bool*), std::__future_base::_State_baseV2*, std::function<std::unique_ptr<std::__future_base::_Result_base, std::__future_base::_Result_base::_Deleter> ()>*, bool*>(std::once_flag&, void (std::__future_base::_State_baseV2::*&&)(std::function<std::unique_ptr<std::__future_base::_Result_base, std::__future_base::_Result_base::_Deleter> ()>*, bool*), std::__future_base::_State_baseV2*&&, std::function<std::unique_ptr<std::__future_base::_Result_base, std::__future_base::_Result_base::_Deleter> ()>*&&, bool*&&)::{lambda()#2}::_FUN() /usr/include/c++/7/mutex:677 (gdb+0x7330f2)
>      #28 pthread_once <null> (libtsan.so.0+0x4457c)
>      #29 __gthread_once /usr/include/c++/7/x86_64-suse-linux/bits/gthr-default.h:699 (gdb+0x72f5dd)
>      #30 void std::call_once<void (std::__future_base::_State_baseV2::*)(std::function<std::unique_ptr<std::__future_base::_Result_base, std::__future_base::_Result_base::_Deleter> ()>*, bool*), std::__future_base::_State_baseV2*, std::function<std::unique_ptr<std::__future_base::_Result_base, std::__future_base::_Result_base::_Deleter> ()>*, bool*>(std::once_flag&, void (std::__future_base::_State_baseV2::*&&)(std::function<std::unique_ptr<std::__future_base::_Result_base, std::__future_base::_Result_base::_Deleter> ()>*, bool*), std::__future_base::_State_baseV2*&&, std::function<std::unique_ptr<std::__future_base::_Result_base, std::__future_base::_Result_base::_Deleter> ()>*&&, bool*&&) /usr/include/c++/7/mutex:684 (gdb+0x733224)
>      #31 std::__future_base::_State_baseV2::_M_set_result(std::function<std::unique_ptr<std::__future_base::_Result_base, std::__future_base::_Result_base::_Deleter> ()>, bool) /usr/include/c++/7/future:401 (gdb+0x732852)
>      #32 std::__future_base::_Task_state<std::function<void ()>, std::allocator<int>, void ()>::_M_run() /usr/include/c++/7/future:1423 (gdb+0x737bef)
>      #33 std::packaged_task<void ()>::operator()() /usr/include/c++/7/future:1556 (gdb+0x1daca1c)
>      #34 gdb::thread_pool::thread_function() gdbsupport/thread-pool.cc:242 (gdb+0x1dac33e)
>      #35 void std::__invoke_impl<void, void (gdb::thread_pool::*)(), gdb::thread_pool*>(std::__invoke_memfun_deref, void (gdb::thread_pool::*&&)(), gdb::thread_pool*&&) /usr/include/c++/7/bits/invoke.h:73 (gdb+0x1dad3ed)
>      #36 std::__invoke_result<void (gdb::thread_pool::*)(), gdb::thread_pool*>::type std::__invoke<void (gdb::thread_pool::*)(), gdb::thread_pool*>(void (gdb::thread_pool::*&&)(), gdb::thread_pool*&&) /usr/include/c++/7/bits/invoke.h:95 (gdb+0x1dac81e)
>      #37 decltype (__invoke((_S_declval<0ul>)(), (_S_declval<1ul>)())) std::thread::_Invoker<std::tuple<void (gdb::thread_pool::*)(), gdb::thread_pool*> >::_M_invoke<0ul, 1ul>(std::_Index_tuple<0ul, 1ul>) /usr/include/c++/7/thread:234 (gdb+0x1dafb50)
>      #38 std::thread::_Invoker<std::tuple<void (gdb::thread_pool::*)(), gdb::thread_pool*> >::operator()() /usr/include/c++/7/thread:243 (gdb+0x1dafadb)
>      #39 std::thread::_State_impl<std::thread::_Invoker<std::tuple<void (gdb::thread_pool::*)(), gdb::thread_pool*> > >::_M_run() /usr/include/c++/7/thread:186 (gdb+0x1dafa90)
>      #40 <null> <null> (libstdc++.so.6+0xdcac2)
> 
>    Previous write of size 4 at 0x7b4400097d08 by main thread:
>      #0 bfd_open_file bfd/cache.c:584 (gdb+0x148bc90)
>      #1 bfd_cache_lookup_worker bfd/cache.c:261 (gdb+0x148b228)
>      #2 cache_bseek bfd/cache.c:289 (gdb+0x148b422)
>      #3 bfd_seek bfd/bfdio.c:459 (gdb+0x1489d2f)
>      #4 _bfd_generic_get_section_contents bfd/libbfd.c:1069 (gdb+0x14978a2)
>      #5 bfd_get_section_contents bfd/section.c:1606 (gdb+0x149cd7a)
>      #6 gdb_bfd_scan_elf_dyntag(int, bfd*, unsigned long*, unsigned long*) gdb/solib.c:1601 (gdb+0xed8fc8)
>      #7 elf_locate_base gdb/solib-svr4.c:705 (gdb+0xec29aa)
>      #8 svr4_iterate_over_objfiles_in_search_order gdb/solib-svr4.c:3430 (gdb+0xeca65b)
>      #9 gdbarch_iterate_over_objfiles_in_search_order(gdbarch*, gdb::function_view<bool (objfile*)>, objfile*) gdb/gdbarch.c:5041 (gdb+0x537cad)
>      #10 find_main_name gdb/symtab.c:6270 (gdb+0xf744a3)
>      #11 main_language() gdb/symtab.c:6313 (gdb+0xf74597)
>      #12 set_initial_language() gdb/symfile.c:1700 (gdb+0xf4295a)
>      #13 symbol_file_add_main_1 gdb/symfile.c:1212 (gdb+0xf40f28)
>      #14 symbol_file_command(char const*, int) gdb/symfile.c:1681 (gdb+0xf428cf)
>      #15 file_command gdb/exec.c:554 (gdb+0x94f849)
>      #16 do_simple_func gdb/cli/cli-decode.c:95 (gdb+0x6d9528)
>      #17 cmd_func(cmd_list_element*, char const*, int) gdb/cli/cli-decode.c:2735 (gdb+0x6e0f69)
>      #18 execute_command(char const*, int) gdb/top.c:575 (gdb+0xff313a)
>      #19 command_handler(char const*) gdb/event-top.c:552 (gdb+0x94aedc)
>      #20 command_line_handler(std::unique_ptr<char, gdb::xfree_deleter<char> >&&) gdb/event-top.c:788 (gdb+0x94b599)
>      #21 tui_command_line_handler gdb/tui/tui-interp.c:104 (gdb+0x103489a)
>      #22 gdb_rl_callback_handler gdb/event-top.c:259 (gdb+0x94a481)
>      #23 rl_callback_read_char readline/readline/callback.c:290 (gdb+0x11bdf5b)
>      #24 gdb_rl_callback_read_char_wrapper_noexcept gdb/event-top.c:195 (gdb+0x94a280)
>      #25 gdb_rl_callback_read_char_wrapper gdb/event-top.c:234 (gdb+0x94a341)
>      #26 stdin_event_handler gdb/ui.c:155 (gdb+0x1074b3e)
>      #27 handle_file_event gdbsupport/event-loop.cc:573 (gdb+0x1d9536e)
>      #28 gdb_wait_for_event gdbsupport/event-loop.cc:694 (gdb+0x1d95aa6)
>      #29 gdb_do_one_event(int) gdbsupport/event-loop.cc:264 (gdb+0x1d93e92)
>      #30 start_event_loop gdb/main.c:412 (gdb+0xb5a472)
>      #31 captured_command_loop gdb/main.c:476 (gdb+0xb5a661)
>      #32 captured_main gdb/main.c:1320 (gdb+0xb5c7e1)
>      #33 gdb_main(captured_main_args*) gdb/main.c:1339 (gdb+0xb5c890)
>      #34 main gdb/gdb.c:32 (gdb+0x416776)
>    ...
> SUMMARY: ThreadSanitizer: data race bfd.h:2376 in bfd_get_section_limit_octets
> ...
> 
> Tom de Vries (3):
>    [gdb/symtab] Fix data race in index_cache::enable
>    [gdb/symtab] Fix data race in bfd_open_file
>    [gdb/symtab] Fix race on per_cu->queued
> 
>   gdb/dwarf2/cooked-index.c |  8 ++++--
>   gdb/dwarf2/cooked-index.h |  3 ++
>   gdb/dwarf2/index-cache.c  | 58 +++++++++++++++++++++++++++++----------
>   gdb/dwarf2/index-cache.h  | 20 +++++++++++++-
>   gdb/dwarf2/read.h         |  2 +-
>   5 files changed, 72 insertions(+), 19 deletions(-)
> 
> 
> base-commit: 29c108c9610640439daa5244a573348b7c47d994