diff mbox series

[RFC,1/3,gdb/symtab] Fix data race in index_cache::enable

Message ID 20230728085645.3746-2-tdevries@suse.de
State Superseded
Headers
Series Fix data-races in gdb.base/index-cache.exp |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gdb_check--master-arm success Testing passed
linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_gdb_build--master-arm success Testing passed

Commit Message

Tom de Vries July 28, 2023, 8:56 a.m. UTC 
  With gdb build with -fsanitize=thread and test-case gdb.base/index-cache.exp I
run into:
...
(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...
(gdb) show index-cache enabled
The index cache is off.
(gdb) PASS: gdb.base/index-cache.exp: test_basic_stuff: index-cache is disabled by default
set index-cache enabled on
==================
WARNING: ThreadSanitizer: data race (pid=32248)
  Write of size 1 at 0x00000321f540 by main thread:
    #0 index_cache::enable() gdb/dwarf2/index-cache.c:76 (gdb+0x82cfdd)
    #1 set_index_cache_enabled_command gdb/dwarf2/index-cache.c:270 (gdb+0x82d9af)
    #2 bool setting::set<bool>(bool const&) gdb/command.h:353 (gdb+0x6fe5f2)
    #3 do_set_command(char const*, int, cmd_list_element*) gdb/cli/cli-setshow.c:414 (gdb+0x6fcd21)
    #4 execute_command(char const*, int) gdb/top.c:567 (gdb+0xff2e64)
    #5 command_handler(char const*) gdb/event-top.c:552 (gdb+0x94acc0)
    #6 command_line_handler(std::unique_ptr<char, gdb::xfree_deleter<char> >&&) gdb/event-top.c:788 (gdb+0x94b37d)
    #7 tui_command_line_handler gdb/tui/tui-interp.c:104 (gdb+0x103467e)
    #8 gdb_rl_callback_handler gdb/event-top.c:259 (gdb+0x94a265)
    #9 rl_callback_read_char readline/readline/callback.c:290 (gdb+0x11bdd3f)
    #10 gdb_rl_callback_read_char_wrapper_noexcept gdb/event-top.c:195 (gdb+0x94a064)
    #11 gdb_rl_callback_read_char_wrapper gdb/event-top.c:234 (gdb+0x94a125)
    #12 stdin_event_handler gdb/ui.c:155 (gdb+0x1074922)
    #13 handle_file_event gdbsupport/event-loop.cc:573 (gdb+0x1d94de4)
    #14 gdb_wait_for_event gdbsupport/event-loop.cc:694 (gdb+0x1d9551c)
    #15 gdb_do_one_event(int) gdbsupport/event-loop.cc:264 (gdb+0x1d93908)
    #16 start_event_loop gdb/main.c:412 (gdb+0xb5a256)
    #17 captured_command_loop gdb/main.c:476 (gdb+0xb5a445)
    #18 captured_main gdb/main.c:1320 (gdb+0xb5c5c5)
    #19 gdb_main(captured_main_args*) gdb/main.c:1339 (gdb+0xb5c674)
    #20 main gdb/gdb.c:32 (gdb+0x416776)

  Previous read of size 1 at 0x00000321f540 by thread T12:
    #0 index_cache::enabled() const gdb/dwarf2/index-cache.h:48 (gdb+0x82e1a6)
    #1 index_cache::store(dwarf2_per_bfd*) gdb/dwarf2/index-cache.c:94 (gdb+0x82d0bc)
    #2 cooked_index::maybe_write_index(dwarf2_per_bfd*) gdb/dwarf2/cooked-index.c:638 (gdb+0x7f1b97)
    #3 operator() gdb/dwarf2/cooked-index.c:468 (gdb+0x7f0f24)
    #4 _M_invoke /usr/include/c++/7/bits/std_function.h:316 (gdb+0x7f285b)
    #5 std::function<void ()>::operator()() const /usr/include/c++/7/bits/std_function.h:706 (gdb+0x700952)
    #6 void std::__invoke_impl<void, std::function<void ()>&>(std::__invoke_other, std::function<void ()>&) /usr/include/c++/7/bits/invoke.h:60 (gdb+0x7381a0)
    #7 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)
    #8 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)
    #9 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)
    #10 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)
    #11 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)
    #12 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)
    #13 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)
    #14 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)
    #15 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)
    #16 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)
    #17 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)
    #18 pthread_once <null> (libtsan.so.0+0x4457c)
    #19 __gthread_once /usr/include/c++/7/x86_64-suse-linux/bits/gthr-default.h:699 (gdb+0x72f5dd)
    #20 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)
    #21 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)
    #22 std::__future_base::_Task_state<std::function<void ()>, std::allocator<int>, void ()>::_M_run() /usr/include/c++/7/future:1423 (gdb+0x737bef)
    #23 std::packaged_task<void ()>::operator()() /usr/include/c++/7/future:1556 (gdb+0x1dac492)
    #24 gdb::thread_pool::thread_function() gdbsupport/thread-pool.cc:242 (gdb+0x1dabdb4)
    #25 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+0x1dace63)
    #26 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+0x1dac294)
    #27 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+0x1daf5c6)
    #28 std::thread::_Invoker<std::tuple<void (gdb::thread_pool::*)(), gdb::thread_pool*> >::operator()() /usr/include/c++/7/thread:243 (gdb+0x1daf551)
    #29 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+0x1daf506)
    #30 <null> <null> (libstdc++.so.6+0xdcac2)

  Location is global 'global_index_cache' of size 48 at 0x00000321f520 (gdb+0x00000321f540)
  ...
SUMMARY: ThreadSanitizer: data race gdb/dwarf2/index-cache.c:76 in index_cache::enable()
...

The race is between:
- the main thread setting index_cache::m_enabled
  (due to command "set index-cache enabled on")
- a worker thread reading index_cache::m_enabled to determine whether an
  index-cache entry for $exec needs to be written
  (due to command "file $exec")

Fix this by capturing the value of index_cache::m_enabled during the file
command.

Tested on x86_64-linux.

PR symtab/30392
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30392
---
 gdb/dwarf2/cooked-index.c |  8 ++++++--
 gdb/dwarf2/cooked-index.h |  3 +++
 gdb/dwarf2/index-cache.c  | 16 ++++++++++++++--
 gdb/dwarf2/index-cache.h  | 14 +++++++++++++-
 4 files changed, 36 insertions(+), 5 deletions(-)

Comments

Simon Marchi July 28, 2023, 6:27 p.m. UTC | #1 
On 2023-07-28 04:56, Tom de Vries via Gdb-patches wrote:
> With gdb build with -fsanitize=thread and test-case gdb.base/index-cache.exp I
> run into:
> ...
> (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...
> (gdb) show index-cache enabled
> The index cache is off.
> (gdb) PASS: gdb.base/index-cache.exp: test_basic_stuff: index-cache is disabled by default
> set index-cache enabled on
> ==================
> WARNING: ThreadSanitizer: data race (pid=32248)
>   Write of size 1 at 0x00000321f540 by main thread:
>     #0 index_cache::enable() gdb/dwarf2/index-cache.c:76 (gdb+0x82cfdd)
>     #1 set_index_cache_enabled_command gdb/dwarf2/index-cache.c:270 (gdb+0x82d9af)
>     #2 bool setting::set<bool>(bool const&) gdb/command.h:353 (gdb+0x6fe5f2)
>     #3 do_set_command(char const*, int, cmd_list_element*) gdb/cli/cli-setshow.c:414 (gdb+0x6fcd21)
>     #4 execute_command(char const*, int) gdb/top.c:567 (gdb+0xff2e64)
>     #5 command_handler(char const*) gdb/event-top.c:552 (gdb+0x94acc0)
>     #6 command_line_handler(std::unique_ptr<char, gdb::xfree_deleter<char> >&&) gdb/event-top.c:788 (gdb+0x94b37d)
>     #7 tui_command_line_handler gdb/tui/tui-interp.c:104 (gdb+0x103467e)
>     #8 gdb_rl_callback_handler gdb/event-top.c:259 (gdb+0x94a265)
>     #9 rl_callback_read_char readline/readline/callback.c:290 (gdb+0x11bdd3f)
>     #10 gdb_rl_callback_read_char_wrapper_noexcept gdb/event-top.c:195 (gdb+0x94a064)
>     #11 gdb_rl_callback_read_char_wrapper gdb/event-top.c:234 (gdb+0x94a125)
>     #12 stdin_event_handler gdb/ui.c:155 (gdb+0x1074922)
>     #13 handle_file_event gdbsupport/event-loop.cc:573 (gdb+0x1d94de4)
>     #14 gdb_wait_for_event gdbsupport/event-loop.cc:694 (gdb+0x1d9551c)
>     #15 gdb_do_one_event(int) gdbsupport/event-loop.cc:264 (gdb+0x1d93908)
>     #16 start_event_loop gdb/main.c:412 (gdb+0xb5a256)
>     #17 captured_command_loop gdb/main.c:476 (gdb+0xb5a445)
>     #18 captured_main gdb/main.c:1320 (gdb+0xb5c5c5)
>     #19 gdb_main(captured_main_args*) gdb/main.c:1339 (gdb+0xb5c674)
>     #20 main gdb/gdb.c:32 (gdb+0x416776)
> 
>   Previous read of size 1 at 0x00000321f540 by thread T12:
>     #0 index_cache::enabled() const gdb/dwarf2/index-cache.h:48 (gdb+0x82e1a6)
>     #1 index_cache::store(dwarf2_per_bfd*) gdb/dwarf2/index-cache.c:94 (gdb+0x82d0bc)
>     #2 cooked_index::maybe_write_index(dwarf2_per_bfd*) gdb/dwarf2/cooked-index.c:638 (gdb+0x7f1b97)
>     #3 operator() gdb/dwarf2/cooked-index.c:468 (gdb+0x7f0f24)
>     #4 _M_invoke /usr/include/c++/7/bits/std_function.h:316 (gdb+0x7f285b)
>     #5 std::function<void ()>::operator()() const /usr/include/c++/7/bits/std_function.h:706 (gdb+0x700952)
>     #6 void std::__invoke_impl<void, std::function<void ()>&>(std::__invoke_other, std::function<void ()>&) /usr/include/c++/7/bits/invoke.h:60 (gdb+0x7381a0)
>     #7 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)
>     #8 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)
>     #9 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)
>     #10 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)
>     #11 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)
>     #12 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)
>     #13 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)
>     #14 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)
>     #15 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)
>     #16 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)
>     #17 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)
>     #18 pthread_once <null> (libtsan.so.0+0x4457c)
>     #19 __gthread_once /usr/include/c++/7/x86_64-suse-linux/bits/gthr-default.h:699 (gdb+0x72f5dd)
>     #20 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)
>     #21 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)
>     #22 std::__future_base::_Task_state<std::function<void ()>, std::allocator<int>, void ()>::_M_run() /usr/include/c++/7/future:1423 (gdb+0x737bef)
>     #23 std::packaged_task<void ()>::operator()() /usr/include/c++/7/future:1556 (gdb+0x1dac492)
>     #24 gdb::thread_pool::thread_function() gdbsupport/thread-pool.cc:242 (gdb+0x1dabdb4)
>     #25 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+0x1dace63)
>     #26 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+0x1dac294)
>     #27 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+0x1daf5c6)
>     #28 std::thread::_Invoker<std::tuple<void (gdb::thread_pool::*)(), gdb::thread_pool*> >::operator()() /usr/include/c++/7/thread:243 (gdb+0x1daf551)
>     #29 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+0x1daf506)
>     #30 <null> <null> (libstdc++.so.6+0xdcac2)
> 
>   Location is global 'global_index_cache' of size 48 at 0x00000321f520 (gdb+0x00000321f540)
>   ...
> SUMMARY: ThreadSanitizer: data race gdb/dwarf2/index-cache.c:76 in index_cache::enable()
> ...
> 
> The race is between:
> - the main thread setting index_cache::m_enabled
>   (due to command "set index-cache enabled on")
> - a worker thread reading index_cache::m_enabled to determine whether an
>   index-cache entry for $exec needs to be written
>   (due to command "file $exec")
> 
> Fix this by capturing the value of index_cache::m_enabled during the file
> command.

For completeness: in 30392, I initially suggested making m_enabled an
std::atomic.  To which you responded:

    By making it a std::atomic<bool>, we make this non-determinism
    defined behaviour, so tsan stops complaining, but we want
    deterministic behaviour instead.

I agree with that.  Capturing the value at the time of the command makes
sense.

One comment on the implementation.  Do we really need to save an
index_cache_store_context object in cooked_index?  Could we:

 - make get_store_context return it by value
 - std::move it to the thread (or just copy it, if it's not expected to
   be large) into the worker thread's closure (in
   cooked_index::start_writing_index)
 - pass it down by const-ref to whatever needs it

Simon
Tom de Vries Aug. 2, 2023, 10:40 a.m. UTC | #2 
On 7/28/23 20:27, Simon Marchi wrote:

Hi Simon,

thanks for the review.

>> The race is between:
>> - the main thread setting index_cache::m_enabled
>>    (due to command "set index-cache enabled on")
>> - a worker thread reading index_cache::m_enabled to determine whether an
>>    index-cache entry for $exec needs to be written
>>    (due to command "file $exec")
>>
>> Fix this by capturing the value of index_cache::m_enabled during the file
>> command.
> For completeness: in 30392, I initially suggested making m_enabled an
> std::atomic.  To which you responded:
> 
>      By making it a std::atomic<bool>, we make this non-determinism
>      defined behaviour, so tsan stops complaining, but we want
>      deterministic behaviour instead.
> 
> I agree with that.  Capturing the value at the time of the command makes
> sense.
> 

Ack, thanks for making that explicit.

> One comment on the implementation.  Do we really need to save an
> index_cache_store_context object in cooked_index?  Could we:
> 
>   - make get_store_context return it by value
>   - std::move it to the thread (or just copy it, if it's not expected to
>     be large) into the worker thread's closure (in
>     cooked_index::start_writing_index)
>   - pass it down by const-ref to whatever needs it

I've implemented this approach in v2 ( 
https://sourceware.org/pipermail/gdb-patches/2023-August/201273.html ).

I also realized that get_store_context is basically a constructor, so 
I've remodeled things in this way as well.

Thanks,
- Tom
diff mbox series

Patch

diff --git a/gdb/dwarf2/cooked-index.c b/gdb/dwarf2/cooked-index.c
index 25635d9b72e..b580e7e25a9 100644
--- a/gdb/dwarf2/cooked-index.c
+++ b/gdb/dwarf2/cooked-index.c
@@ -444,7 +444,7 @@  cooked_index_shard::wait (bool allow_quit) const
 }
 
 cooked_index::cooked_index (vec_type &&vec)
-  : m_vector (std::move (vec))
+  : m_vector (std::move (vec)), m_index_cache_store_context (nullptr)
 {
   for (auto &idx : m_vector)
     idx->finalize ();
@@ -460,6 +460,8 @@  cooked_index::cooked_index (vec_type &&vec)
 void
 cooked_index::start_writing_index (dwarf2_per_bfd *per_bfd)
 {
+  m_index_cache_store_context = global_index_cache.get_store_context ();
+
   /* This must be set after all the finalization tasks have been
      started, because it may call 'wait'.  */
   m_write_future
@@ -635,7 +637,9 @@  cooked_index::maybe_write_index (dwarf2_per_bfd *per_bfd)
   wait ();
 
   /* (maybe) store an index in the cache.  */
-  global_index_cache.store (per_bfd);
+  global_index_cache.store (per_bfd, m_index_cache_store_context);
+  delete m_index_cache_store_context;
+  m_index_cache_store_context = nullptr;
 }
 
 /* Wait for all the index cache entries to be written before gdb
diff --git a/gdb/dwarf2/cooked-index.h b/gdb/dwarf2/cooked-index.h
index 0d6f3e5aa0e..6f61cd45f67 100644
--- a/gdb/dwarf2/cooked-index.h
+++ b/gdb/dwarf2/cooked-index.h
@@ -443,6 +443,9 @@  class cooked_index : public dwarf_scanner_base
 
   /* A future that tracks when the 'index_write' method is done.  */
   gdb::future<void> m_write_future;
+
+  /* The context to pass to global_index_cache.store.  */
+  struct index_cache_store_context *m_index_cache_store_context;
 };
 
 #endif /* GDB_DWARF2_COOKED_INDEX_H */
diff --git a/gdb/dwarf2/index-cache.c b/gdb/dwarf2/index-cache.c
index 79ab706ee9d..3bce83ffcd0 100644
--- a/gdb/dwarf2/index-cache.c
+++ b/gdb/dwarf2/index-cache.c
@@ -88,10 +88,22 @@  index_cache::disable ()
 
 /* See dwarf-index-cache.h.  */
 
+struct index_cache_store_context *
+index_cache::get_store_context ()
+{
+  struct index_cache_store_context *res
+    = new struct index_cache_store_context;
+
+  res->enabled = enabled ();
+  return res;
+}
+
+/* See dwarf-index-cache.h.  */
+
 void
-index_cache::store (dwarf2_per_bfd *per_bfd)
+index_cache::store (dwarf2_per_bfd *per_bfd, struct index_cache_store_context *ctx)
 {
-  if (!enabled ())
+  if (!ctx->enabled)
     return;
 
   /* Get build id of objfile.  */
diff --git a/gdb/dwarf2/index-cache.h b/gdb/dwarf2/index-cache.h
index 1efff17049f..217720b46d9 100644
--- a/gdb/dwarf2/index-cache.h
+++ b/gdb/dwarf2/index-cache.h
@@ -34,6 +34,15 @@  struct index_cache_resource
   virtual ~index_cache_resource () = 0;
 };
 
+/* Information to be captured in the main thread using get_store_context, and
+   to be used by worker threads during store ().  */
+
+struct index_cache_store_context
+{
+  /* Captured value of enabled ().  */
+  bool enabled;
+};
+
 /* Class to manage the access to the DWARF index cache.  */
 
 class index_cache
@@ -54,8 +63,11 @@  class index_cache
   /* Disable the cache.  */
   void disable ();
 
+  /* Get index_cache_store_context.  */
+  struct index_cache_store_context *get_store_context ();
+
   /* Store an index for the specified object file in the cache.  */
-  void store (dwarf2_per_bfd *per_bfd);
+  void store (dwarf2_per_bfd *per_bfd, struct index_cache_store_context *);
 
   /* Look for an index file matching BUILD_ID.  If found, return the contents
      as an array_view and store the underlying resources (allocated memory,