diff mbox series

[v2,2/6,gdb/symtab] Fix data race on bfd::{cacheable,format}

Message ID 20230802095305.3668-3-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-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 warning Patch failed to apply
linaro-tcwg-bot/tcwg_gdb_build--master-arm warning Patch failed to apply
linaro-tcwg-bot/tcwg_gdb_check--master-arm warning Patch failed to apply

Commit Message

Tom de Vries Aug. 2, 2023, 9:53 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...
==================
WARNING: ThreadSanitizer: data race (pid=12261)
  Write of size 4 at 0x7b4400097d08 by main thread:
    #0 bfd_open_file bfd/cache.c:584 (gdb+0x148bb92)
    #1 bfd_cache_lookup_worker bfd/cache.c:261 (gdb+0x148b12a)
    #2 cache_bseek bfd/cache.c:289 (gdb+0x148b324)
    #3 bfd_seek bfd/bfdio.c:459 (gdb+0x1489c31)
    #4 _bfd_generic_get_section_contents bfd/libbfd.c:1069 (gdb+0x14977a4)
    #5 bfd_get_section_contents bfd/section.c:1606 (gdb+0x149cc7c)
    #6 gdb_bfd_scan_elf_dyntag(int, bfd*, unsigned long*, unsigned long*) gdb/solib.c:1601 (gdb+0xed8eca)
    #7 elf_locate_base gdb/solib-svr4.c:705 (gdb+0xec28ac)
    #8 svr4_iterate_over_objfiles_in_search_order gdb/solib-svr4.c:3430 (gdb+0xeca55d)
    #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+0xf743a5)
    #11 main_language() gdb/symtab.c:6313 (gdb+0xf74499)
    #12 set_initial_language() gdb/symfile.c:1700 (gdb+0xf4285c)
    #13 symbol_file_add_main_1 gdb/symfile.c:1212 (gdb+0xf40e2a)
    #14 symbol_file_command(char const*, int) gdb/symfile.c:1681 (gdb+0xf427d1)
    #15 file_command gdb/exec.c:554 (gdb+0x94f74b)
    #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+0xff303c)
    #19 command_handler(char const*) gdb/event-top.c:552 (gdb+0x94adde)
    #20 command_line_handler(std::unique_ptr<char, gdb::xfree_deleter<char> >&&) gdb/event-top.c:788 (gdb+0x94b49b)
    #21 tui_command_line_handler gdb/tui/tui-interp.c:104 (gdb+0x103479c)
    #22 gdb_rl_callback_handler gdb/event-top.c:259 (gdb+0x94a383)
    #23 rl_callback_read_char readline/readline/callback.c:290 (gdb+0x11bde5d)
    #24 gdb_rl_callback_read_char_wrapper_noexcept gdb/event-top.c:195 (gdb+0x94a182)
    #25 gdb_rl_callback_read_char_wrapper gdb/event-top.c:234 (gdb+0x94a243)
    #26 stdin_event_handler gdb/ui.c:155 (gdb+0x1074a40)
    #27 handle_file_event gdbsupport/event-loop.cc:573 (gdb+0x1d94f02)
    #28 gdb_wait_for_event gdbsupport/event-loop.cc:694 (gdb+0x1d9563a)
    #29 gdb_do_one_event(int) gdbsupport/event-loop.cc:264 (gdb+0x1d93a26)
    #30 start_event_loop gdb/main.c:412 (gdb+0xb5a374)
    #31 captured_command_loop gdb/main.c:476 (gdb+0xb5a563)
    #32 captured_main gdb/main.c:1320 (gdb+0xb5c6e3)
    #33 gdb_main(captured_main_args*) gdb/main.c:1339 (gdb+0xb5c792)
    #34 main gdb/gdb.c:32 (gdb+0x416776)

  Previous read of size 1 at 0x7b4400097d08 by thread T12:
    #0 bfd_check_format_matches bfd/format.c:323 (gdb+0x1492db4)
    #1 bfd_check_format bfd/format.c:94 (gdb+0x1492104)
    #2 build_id_bfd_get(bfd*) gdb/build-id.c:42 (gdb+0x6648f7)
    #3 index_cache::store(dwarf2_per_bfd*, index_cache_store_context*) gdb/dwarf2/index-cache.c:110 (gdb+0x82d205)
    #4 cooked_index::maybe_write_index(dwarf2_per_bfd*) gdb/dwarf2/cooked-index.c:640 (gdb+0x7f1bf1)
    #5 operator() gdb/dwarf2/cooked-index.c:470 (gdb+0x7f0f40)
    #6 _M_invoke /usr/include/c++/7/bits/std_function.h:316 (gdb+0x7f28f7)
    #7 std::function<void ()>::operator()() const /usr/include/c++/7/bits/std_function.h:706 (gdb+0x700952)
    #8 void std::__invoke_impl<void, std::function<void ()>&>(std::__invoke_other, std::function<void ()>&) /usr/include/c++/7/bits/invoke.h:60 (gdb+0x7381a0)
    #9 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)
    #10 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)
    #11 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)
    #12 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)
    #13 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)
    #14 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)
    #15 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)
    #16 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)
    #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()#1}::operator()() const /usr/include/c++/7/mutex:672 (gdb+0x73300d)
    #18 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)
    #19 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)
    #20 pthread_once <null> (libtsan.so.0+0x4457c)
    #21 __gthread_once /usr/include/c++/7/x86_64-suse-linux/bits/gthr-default.h:699 (gdb+0x72f5dd)
    #22 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)
    #23 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)
    #24 std::__future_base::_Task_state<std::function<void ()>, std::allocator<int>, void ()>::_M_run() /usr/include/c++/7/future:1423 (gdb+0x737bef)
    #25 std::packaged_task<void ()>::operator()() /usr/include/c++/7/future:1556 (gdb+0x1dac5b0)
    #26 gdb::thread_pool::thread_function() gdbsupport/thread-pool.cc:242 (gdb+0x1dabed2)
    #27 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+0x1dacf81)
    #28 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+0x1dac3b2)
    #29 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+0x1daf6e4)
    #30 std::thread::_Invoker<std::tuple<void (gdb::thread_pool::*)(), gdb::thread_pool*> >::operator()() /usr/include/c++/7/thread:243 (gdb+0x1daf66f)
    #31 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+0x1daf624)
    #32 <null> <null> (libstdc++.so.6+0xdcac2)
  ...
SUMMARY: ThreadSanitizer: data race bfd/cache.c:584 in bfd_open_file
...

The race happens when issuing the "file $exec" command.

The race is between:
- a worker thread getting the build id while writing the index cache, and in
  the process reading bfd::format, and
- the main thread calling find_main_name, and in the process setting
  bfd::cacheable.

The two bitfields bfd::cacheable and bfd::format share the same bitfield
container.

Fix this by capturing the build id in the main thread, and using the captured
value in the worker thread.

Likewise for the dwz build id, which likely suffers from the same issue.

While we're at it, also move the creation of the cache directory to
the index_cache_store_context constructor, to:
- make sure there's no race between subsequent file commands, and
- issue any related warning or error messages 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 |  2 +-
 gdb/dwarf2/index-cache.c  | 52 +++++++++++++++++++++++++--------------
 gdb/dwarf2/index-cache.h  |  9 ++++++-
 3 files changed, 43 insertions(+), 20 deletions(-)

Comments

Tom Tromey Aug. 2, 2023, 7:32 p.m. UTC | #1 
>>>>> "Tom" == Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> writes:

Tom> With gdb build with -fsanitize=thread and test-case gdb.base/index-cache.exp I
Tom> run into:

Thanks again.

Tom> -index_cache_store_context::index_cache_store_context (const index_cache &ic)
Tom> +index_cache_store_context::index_cache_store_context (const index_cache &ic,
Tom> +						      dwarf2_per_bfd *per_bfd)

Ok, I see why explicit isn't needed :)

This seems fine to me.
Approved-By: Tom Tromey <tom@tromey.com>


Tom
Tom de Vries Aug. 4, 2023, 12:09 a.m. UTC | #2 
On 8/2/23 21:32, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Tom> With gdb build with -fsanitize=thread and test-case gdb.base/index-cache.exp I
> Tom> run into:
> 
> Thanks again.
> 
> Tom> -index_cache_store_context::index_cache_store_context (const index_cache &ic)
> Tom> +index_cache_store_context::index_cache_store_context (const index_cache &ic,
> Tom> +						      dwarf2_per_bfd *per_bfd)
> 
> Ok, I see why explicit isn't needed :)
> 

In the v3 version, I've added explicit in the first patch, and then 
remove it in the second ( 
https://sourceware.org/pipermail/gdb-patches/2023-August/201338.html ).

Thanks,
- Tom

> This seems fine to me.
> Approved-By: Tom Tromey <tom@tromey.com>
> 
> 
> Tom
Tom Tromey Aug. 4, 2023, 3:57 p.m. UTC | #3 
Tom> In the v3 version, I've added explicit in the first patch, and then
Tom> remove it in the second (
Tom> https://sourceware.org/pipermail/gdb-patches/2023-August/201338.html
Tom> ).

Sorry about that.  IMO it's fine to just leave it out in a situation
like this, and push back on the review comment in that case as well.

Tom
diff mbox series

Patch

diff --git a/gdb/dwarf2/cooked-index.c b/gdb/dwarf2/cooked-index.c
index af6d129218d..b5718ae5529 100644
--- a/gdb/dwarf2/cooked-index.c
+++ b/gdb/dwarf2/cooked-index.c
@@ -460,7 +460,7 @@  cooked_index::cooked_index (vec_type &&vec)
 void
 cooked_index::start_writing_index (dwarf2_per_bfd *per_bfd)
 {
-  struct index_cache_store_context ctx (global_index_cache);
+  struct index_cache_store_context ctx (global_index_cache, per_bfd);
 
   /* This must be set after all the finalization tasks have been
      started, because it may call 'wait'.  */
diff --git a/gdb/dwarf2/index-cache.c b/gdb/dwarf2/index-cache.c
index 39109626d65..5281020d18a 100644
--- a/gdb/dwarf2/index-cache.c
+++ b/gdb/dwarf2/index-cache.c
@@ -88,18 +88,11 @@  index_cache::disable ()
 
 /* See dwarf-index-cache.h.  */
 
-index_cache_store_context::index_cache_store_context (const index_cache &ic)
+index_cache_store_context::index_cache_store_context (const index_cache &ic,
+						      dwarf2_per_bfd *per_bfd)
 {
   m_enabled = ic.enabled ();
-}
-
-/* See dwarf-index-cache.h.  */
-
-void
-index_cache::store (dwarf2_per_bfd *per_bfd,
-		    const struct index_cache_store_context &ctx)
-{
-  if (!ctx.m_enabled)
+  if (!m_enabled)
     return;
 
   /* Get build id of objfile.  */
@@ -108,15 +101,13 @@  index_cache::store (dwarf2_per_bfd *per_bfd,
     {
       index_cache_debug ("objfile %s has no build id",
 			 bfd_get_filename (per_bfd->obfd));
+      m_enabled = false;
       return;
     }
-
-  std::string build_id_str = build_id_to_string (build_id);
+  build_id_str = build_id_to_string (build_id);
 
   /* Get build id of dwz file, if present.  */
-  gdb::optional<std::string> dwz_build_id_str;
   const dwz_file *dwz = dwarf2_get_dwz_file (per_bfd);
-  const char *dwz_build_id_ptr = NULL;
 
   if (dwz != nullptr)
     {
@@ -126,36 +117,61 @@  index_cache::store (dwarf2_per_bfd *per_bfd,
 	{
 	  index_cache_debug ("dwz objfile %s has no build id",
 			     dwz->filename ());
+	  m_enabled = false;
 	  return;
 	}
 
       dwz_build_id_str = build_id_to_string (dwz_build_id);
-      dwz_build_id_ptr = dwz_build_id_str->c_str ();
     }
 
-  if (m_dir.empty ())
+  if (ic.m_dir.empty ())
     {
       warning (_("The index cache directory name is empty, skipping store."));
+      m_enabled = false;
       return;
     }
 
   try
     {
       /* Try to create the containing directory.  */
-      if (!mkdir_recursive (m_dir.c_str ()))
+      if (!mkdir_recursive (ic.m_dir.c_str ()))
 	{
 	  warning (_("index cache: could not make cache directory: %s"),
 		   safe_strerror (errno));
+	  m_enabled = false;
 	  return;
 	}
+    }
+  catch (const gdb_exception_error &except)
+    {
+      index_cache_debug ("couldn't store index cache for objfile %s: %s",
+			 bfd_get_filename (per_bfd->obfd), except.what ());
+      m_enabled = false;
+    }
+}
+
+/* See dwarf-index-cache.h.  */
 
+void
+index_cache::store (dwarf2_per_bfd *per_bfd,
+		    const struct index_cache_store_context &ctx)
+{
+  if (!ctx.m_enabled)
+    return;
+
+  const char *dwz_build_id_ptr = (ctx.dwz_build_id_str.has_value ()
+				  ? ctx.dwz_build_id_str->c_str ()
+				  : nullptr);
+
+  try
+    {
       index_cache_debug ("writing index cache for objfile %s",
 			 bfd_get_filename (per_bfd->obfd));
 
       /* Write the index itself to the directory, using the build id as the
 	 filename.  */
       write_dwarf_index (per_bfd, m_dir.c_str (),
-			 build_id_str.c_str (), dwz_build_id_ptr,
+			 ctx.build_id_str.c_str (), dwz_build_id_ptr,
 			 dw_index_kind::GDB_INDEX);
     }
   catch (const gdb_exception_error &except)
diff --git a/gdb/dwarf2/index-cache.h b/gdb/dwarf2/index-cache.h
index 7f3c63198d5..6e224f47f93 100644
--- a/gdb/dwarf2/index-cache.h
+++ b/gdb/dwarf2/index-cache.h
@@ -42,17 +42,24 @@  struct index_cache_store_context
 {
   friend class index_cache;
 
-  index_cache_store_context(const index_cache &ic);
+  index_cache_store_context(const index_cache &ic, dwarf2_per_bfd *);
 
 private:
   /* Captured value of enabled ().  */
   bool m_enabled;
+
+  /* Captured value of build id.  */
+  std::string build_id_str;
+
+  /* Captured value of dwz build id.  */
+  gdb::optional<std::string> dwz_build_id_str;
 };
 
 /* Class to manage the access to the DWARF index cache.  */
 
 class index_cache
 {
+  friend struct index_cache_store_context;
 public:
   /* Change the directory used to save/load index files.  */
   void set_directory (std::string dir);