From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp.polymtl.ca (smtp.polymtl.ca [132.207.4.11]) by sourceware.org (Postfix) with ESMTPS id 926F63858CDA for ; Fri, 28 Jul 2023 18:27:15 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 926F63858CDA Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=polymtl.ca Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=polymtl.ca Received: from simark.ca (simark.ca [158.69.221.121]) (authenticated bits=0) by smtp.polymtl.ca (8.14.7/8.14.7) with ESMTP id 36SIR9jv032472 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Fri, 28 Jul 2023 14:27:14 -0400 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp.polymtl.ca 36SIR9jv032472 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=polymtl.ca; s=default; t=1690568834; bh=nXkGuxlvatB4vEypcS61BOS1WtDHSnYc4WCvs1w9BV0=; h=Date:Subject:To:References:From:In-Reply-To:From; b=WhQD7NbLAG4+3MpRMZ8YncJ+gMUqzRDp/EhveI3xjRavgv1AAtUBp7RXdZVi542NS iB0jRLHOEHZOOsBof5vEvQjQX+Y0OobOWhIYacjhPTjsz17dlhUPg7yvLbHp/+5CxD 7leDGgxJBQIE9p+J00LWtOHr7tfjGZ+Xu1kfIsM0= Received: from [10.0.0.11] (modemcable238.237-201-24.mc.videotron.ca [24.201.237.238]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id C03C21E00F; Fri, 28 Jul 2023 14:27:09 -0400 (EDT) Message-ID: <43fcfbd2-115e-8458-8808-acdee98a2c83@polymtl.ca> Date: Fri, 28 Jul 2023 14:27:09 -0400 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [RFC 1/3] [gdb/symtab] Fix data race in index_cache::enable Content-Language: en-US To: Tom de Vries , gdb-patches@sourceware.org References: <20230728085645.3746-1-tdevries@suse.de> <20230728085645.3746-2-tdevries@suse.de> From: Simon Marchi In-Reply-To: <20230728085645.3746-2-tdevries@suse.de> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Fri, 28 Jul 2023 18:27:09 +0000 X-Spam-Status: No, score=-3031.6 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_PASS,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE,WEIRD_PORT autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On 2023-07-28 04:56, Tom de Vries via Gdb-patches wrote: > With gdb build with -fsanitize=3Dthread and test-case gdb.base/index-ca= che.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/i= ndex-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 > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > WARNING: ThreadSanitizer: data race (pid=3D32248) > 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 (gd= b+0x82d9af) > #2 bool setting::set(bool const&) gdb/command.h:353 (gdb+0x6f= e5f2) > #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 >&&) 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+0x11= bdd3f) > #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+0x94= a125) > #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+0x1d939= 08) > #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) >=20 > Previous read of size 1 at 0x00000321f540 by thread T12: > #0 index_cache::enabled() const gdb/dwarf2/index-cache.h:48 (gdb+0x= 82e1a6) > #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/cook= ed-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+0x7f28= 5b) > #5 std::function::operator()() const /usr/include/c++/7/bi= ts/std_function.h:706 (gdb+0x700952) > #6 void std::__invoke_impl&>(std::__in= voke_other, std::function&) /usr/include/c++/7/bits/invoke.h:60 = (gdb+0x7381a0) > #7 std::__invoke_result&>::type std::__invok= e&>(std::function&) /usr/include/c++/7/bi= ts/invoke.h:95 (gdb+0x737e91) > #8 std::__future_base::_Task_state, std::all= ocator, void ()>::_M_run()::{lambda()#1}::operator()() const /usr/in= clude/c++/7/future:1421 (gdb+0x737b59) > #9 std::__future_base::_Task_setter, std::__future_base::_Result_base::_Deleter>, std::__f= uture_base::_Task_state, std::allocator, void= ()>::_M_run()::{lambda()#1}, void>::operator()() const /usr/include/c++/= 7/future:1362 (gdb+0x738660) > #10 std::_Function_handler (), std::__future_b= ase::_Task_setter, std:= :__future_base::_Result_base::_Deleter>, std::__future_base::_Task_state<= std::function, std::allocator, void ()>::_M_run()::{lambda(= )#1}, void> >::_M_invoke(std::_Any_data const&) /usr/include/c++/7/bits/s= td_function.h:302 (gdb+0x73825c) > #11 std::function ()>::operator()() const /usr= /include/c++/7/bits/std_function.h:706 (gdb+0x733623) > #12 std::__future_base::_State_baseV2::_M_do_set(std::function ()>*, bool*) /usr/include/c++/7/future:561 (gdb+0x732bdf= ) > #13 void std::__invoke_impl ()>*, bool*), std::__future= _base::_State_baseV2*, std::function ()>*, bool*>(st= d::__invoke_memfun_deref, void (std::__future_base::_State_baseV2::*&&)(s= td::function ()>*, bool*), std::__future_base::_Stat= e_baseV2*&&, std::function ()>*&&, bool*&&) /usr/inc= lude/c++/7/bits/invoke.h:73 (gdb+0x734c4f) > #14 std::__invoke_result ()>*, bool*), std::__future_base::_S= tate_baseV2*, std::function ()>*, bool*>::type std::= __invoke ()>*, bool*), std::__future_base::_State_baseV2*, std::fun= ction ()>*, bool*>(void (std::__future_base::_State_= baseV2::*&&)(std::function ()>*, bool*), std::__futu= re_base::_State_baseV2*&&, std::function ()>*&&, boo= l*&&) /usr/include/c++/7/bits/invoke.h:95 (gdb+0x733bc5) > #15 std::call_once ()>*, bool*), std::__future_base::_State_b= aseV2*, std::function ()>*, bool*>(std::once_flag&, = void (std::__future_base::_State_baseV2::*&&)(std::function ()>*, bool*), std::__future_base::_State_baseV2*&&, std::function= ()>*&&, bool*&&)::{lambda()#1}::operator()() const = /usr/include/c++/7/mutex:672 (gdb+0x73300d) > #16 std::call_once ()>*, bool*), std::__future_base::_State_b= aseV2*, std::function ()>*, bool*>(std::once_flag&, = void (std::__future_base::_State_baseV2::*&&)(std::function ()>*, bool*), std::__future_base::_State_baseV2*&&, std::function= ()>*&&, bool*&&)::{lambda()#2}::operator()() const = /usr/include/c++/7/mutex:677 (gdb+0x7330b2) > #17 std::call_once ()>*, bool*), std::__future_base::_State_b= aseV2*, std::function ()>*, bool*>(std::once_flag&, = void (std::__future_base::_State_baseV2::*&&)(std::function ()>*, bool*), std::__future_base::_State_baseV2*&&, std::function= ()>*&&, bool*&&)::{lambda()#2}::_FUN() /usr/include= /c++/7/mutex:677 (gdb+0x7330f2) > #18 pthread_once (libtsan.so.0+0x4457c) > #19 __gthread_once /usr/include/c++/7/x86_64-suse-linux/bits/gthr-d= efault.h:699 (gdb+0x72f5dd) > #20 void std::call_once ()>*, bool*), std::__future_base::_St= ate_baseV2*, std::function ()>*, bool*>(std::once_fl= ag&, void (std::__future_base::_State_baseV2::*&&)(std::function ()>*, bool*), std::__future_base::_State_baseV2*&&, std::fun= ction ()>*&&, bool*&&) /usr/include/c++/7/mutex:684 = (gdb+0x733224) > #21 std::__future_base::_State_baseV2::_M_set_result(std::function<= std::unique_ptr ()>, bool) /usr/include/c++/7/future:401 (gdb+0x7328= 52) > #22 std::__future_base::_Task_state, std::al= locator, void ()>::_M_run() /usr/include/c++/7/future:1423 (gdb+0x73= 7bef) > #23 std::packaged_task::operator()() /usr/include/c++/7/fu= ture:1556 (gdb+0x1dac492) > #24 gdb::thread_pool::thread_function() gdbsupport/thread-pool.cc:2= 42 (gdb+0x1dabdb4) > #25 void std::__invoke_impl(std::__invoke_memfun_deref, void (gdb::thread_pool::*&&)(= ), gdb::thread_pool*&&) /usr/include/c++/7/bits/invoke.h:73 (gdb+0x1dace6= 3) > #26 std::__invoke_result::type std::__invoke(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>)())) s= td::thread::_Invoker >::_M_invoke<0ul, 1ul>(std::_Index_tuple<0ul, 1ul>) /usr/include/= c++/7/thread:234 (gdb+0x1daf5c6) > #28 std::thread::_Invoker >::operator()() /usr/include/c++/7/thread:243 (gdb+0x1= daf551) > #29 std::thread::_State_impl > >::_M_run() /usr/include/c+= +/7/thread:186 (gdb+0x1daf506) > #30 (libstdc++.so.6+0xdcac2) >=20 > Location is global 'global_index_cache' of size 48 at 0x00000321f520 = (gdb+0x00000321f540) > ... > SUMMARY: ThreadSanitizer: data race gdb/dwarf2/index-cache.c:76 in inde= x_cache::enable() > ... >=20 > 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 a= n > index-cache entry for $exec needs to be written > (due to command "file $exec") >=20 > Fix this by capturing the value of index_cache::m_enabled during the fi= le > command. For completeness: in 30392, I initially suggested making m_enabled an std::atomic. To which you responded: By making it a std::atomic, 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