[PATCH v2 0/6] [gdb/symtab] Fix data-races in gdb.base/index-cache.exp

public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed

* [PATCH v2 0/6] [gdb/symtab] Fix data-races in gdb.base/index-cache.exp
@ 2023-08-02  9:52 Tom de Vries
  2023-08-02  9:53 ` [PATCH v2 1/6] [gdb/symtab] Fix data race on index_cache::m_enabled Tom de Vries
                   ` (6 more replies)
  0 siblings, 7 replies; 19+ messages in thread
From: Tom de Vries @ 2023-08-02  9:52 UTC (permalink / raw)
  To: gdb-patches

When building gdb with -fsanitize=thread, we run into a data race in
gdb.base/index-cache.exp.

Fixing this leads us to another, and so on, so each patch addresses one
particular data race, with the exception of the last patch, which extends
the test-case a bit.

The last patch, when applied without the series runs into a segfault with
target board native-extended-gdbserver, filed as PR symtab/30712, but that
seems to be fixed by a previous commit in this series.  This is the reason for
which the patch is part of this series.

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 packed<bool, 1>
approach:
...
-  unsigned int queued : 1;
+  packed<bool, 1> queued;
...

There's one more patch like that, I checked using pahole that the struct size
is not increased.

I spent some time convincing myself that the data races on disjoint bitfields
are not benign.  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.

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/

Tom de Vries (6):
  [gdb/symtab] Fix data race on index_cache::m_enabled
  [gdb/symtab] Fix data race on bfd::{cacheable,format}
  [gdb/symtab] Fix race on dwarf2_per_cu_data::{queued,is_debug_type}
  [gdb/symtab] Fix data race on bfd_last_cache
  [gdb/symtab] Fix data race on
    dwarf2_per_cu_data::{m_header_read_in,is_debug_type}
  [gdb/testsuite] Extend gdb.base/index-cache.exp

 gdb/dwarf2/cooked-index.c              | 19 ++++++++---
 gdb/dwarf2/cooked-index.h              |  3 +-
 gdb/dwarf2/index-cache.c               | 46 ++++++++++++++++++++------
 gdb/dwarf2/index-cache.h               | 25 +++++++++++++-
 gdb/dwarf2/read.c                      |  8 ++---
 gdb/dwarf2/read.h                      | 26 +++++++--------
 gdb/testsuite/gdb.base/index-cache-2.c | 24 ++++++++++++++
 gdb/testsuite/gdb.base/index-cache.c   |  6 ++--
 gdb/testsuite/gdb.base/index-cache.exp | 22 ++++++++++--
 9 files changed, 141 insertions(+), 38 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/index-cache-2.c

base-commit: 69c37f53e20dc3e0b3c179b511ff786db6ae114e
-- 
2.35.3

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v2 1/6] [gdb/symtab] Fix data race on index_cache::m_enabled
  2023-08-02  9:52 [PATCH v2 0/6] [gdb/symtab] Fix data-races in gdb.base/index-cache.exp Tom de Vries
@ 2023-08-02  9:53 ` Tom de Vries
  2023-08-02 19:29   ` Tom Tromey
  2023-08-02  9:53 ` [PATCH v2 2/6] [gdb/symtab] Fix data race on bfd::{cacheable,format} Tom de Vries
                   ` (5 subsequent siblings)
  6 siblings, 1 reply; 19+ messages in thread
From: Tom de Vries @ 2023-08-02  9:53 UTC (permalink / raw)
  To: gdb-patches

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 happens when issuing a "file $exec" command followed by a
"set index-cache enabled on" command.

The race is between:
- 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"), and
- the main thread setting index_cache::m_enabled
  (due to command "set index-cache enabled on").

Fix this by capturing the value of index_cache::m_enabled in the main thread,
and using the captured value in the worker thread.

Tested on x86_64-linux.

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

diff --git a/gdb/dwarf2/cooked-index.c b/gdb/dwarf2/cooked-index.c
index 25635d9b72e..af6d129218d 100644
--- a/gdb/dwarf2/cooked-index.c
+++ b/gdb/dwarf2/cooked-index.c
@@ -460,12 +460,15 @@ 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);
+
   /* This must be set after all the finalization tasks have been
      started, because it may call 'wait'.  */
   m_write_future
-    = gdb::thread_pool::g_thread_pool->post_task ([this, per_bfd] ()
+    = gdb::thread_pool::g_thread_pool->post_task ([this, per_bfd,
+						   ctx = std::move (ctx)] ()
 	{
-	  maybe_write_index (per_bfd);
+	  maybe_write_index (per_bfd, ctx);
 	});
 }
 
@@ -629,13 +632,14 @@ cooked_index::dump (gdbarch *arch) const
 }
 
 void
-cooked_index::maybe_write_index (dwarf2_per_bfd *per_bfd)
+cooked_index::maybe_write_index (dwarf2_per_bfd *per_bfd,
+				 const struct index_cache_store_context &ctx)
 {
   /* Wait for finalization.  */
   wait ();
 
   /* (maybe) store an index in the cache.  */
-  global_index_cache.store (per_bfd);
+  global_index_cache.store (per_bfd, ctx);
 }
 
 /* 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..d26d7ad4b1d 100644
--- a/gdb/dwarf2/cooked-index.h
+++ b/gdb/dwarf2/cooked-index.h
@@ -435,7 +435,8 @@ class cooked_index : public dwarf_scanner_base
 private:
 
   /* Maybe write the index to the index cache.  */
-  void maybe_write_index (dwarf2_per_bfd *per_bfd);
+  void maybe_write_index (dwarf2_per_bfd *per_bfd,
+			  const struct index_cache_store_context &);
 
   /* The vector of cooked_index objects.  This is stored because the
      entries are stored on the obstacks in those objects.  */
diff --git a/gdb/dwarf2/index-cache.c b/gdb/dwarf2/index-cache.c
index 79ab706ee9d..39109626d65 100644
--- a/gdb/dwarf2/index-cache.c
+++ b/gdb/dwarf2/index-cache.c
@@ -88,10 +88,18 @@ index_cache::disable ()
 
 /* See dwarf-index-cache.h.  */
 
+index_cache_store_context::index_cache_store_context (const index_cache &ic)
+{
+  m_enabled = ic.enabled ();
+}
+
+/* See dwarf-index-cache.h.  */
+
 void
-index_cache::store (dwarf2_per_bfd *per_bfd)
+index_cache::store (dwarf2_per_bfd *per_bfd,
+		    const struct index_cache_store_context &ctx)
 {
-  if (!enabled ())
+  if (!ctx.m_enabled)
     return;
 
   /* Get build id of objfile.  */
diff --git a/gdb/dwarf2/index-cache.h b/gdb/dwarf2/index-cache.h
index 1efff17049f..7f3c63198d5 100644
--- a/gdb/dwarf2/index-cache.h
+++ b/gdb/dwarf2/index-cache.h
@@ -25,6 +25,7 @@
 #include "symfile.h"
 
 class dwarf2_per_bfd;
+class index_cache;
 
 /* Base of the classes used to hold the resources of the indices loaded from
    the cache (e.g. mmapped files).  */
@@ -34,6 +35,20 @@ struct index_cache_resource
   virtual ~index_cache_resource () = 0;
 };
 
+/* Information to be captured in the main thread, and to be used by worker
+   threads during store ().  */
+
+struct index_cache_store_context
+{
+  friend class index_cache;
+
+  index_cache_store_context(const index_cache &ic);
+
+private:
+  /* Captured value of enabled ().  */
+  bool m_enabled;
+};
+
 /* Class to manage the access to the DWARF index cache.  */
 
 class index_cache
@@ -55,7 +70,8 @@ class index_cache
   void disable ();
 
   /* 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,
+	      const 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,
-- 
2.35.3


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v2 2/6] [gdb/symtab] Fix data race on bfd::{cacheable,format}
  2023-08-02  9:52 [PATCH v2 0/6] [gdb/symtab] Fix data-races in gdb.base/index-cache.exp Tom de Vries
  2023-08-02  9:53 ` [PATCH v2 1/6] [gdb/symtab] Fix data race on index_cache::m_enabled Tom de Vries
@ 2023-08-02  9:53 ` Tom de Vries
  2023-08-02 19:32   ` Tom Tromey
  2023-08-02  9:53 ` [PATCH v2 3/6] [gdb/symtab] Fix race on dwarf2_per_cu_data::{queued,is_debug_type} Tom de Vries
                   ` (4 subsequent siblings)
  6 siblings, 1 reply; 19+ messages in thread
From: Tom de Vries @ 2023-08-02  9:53 UTC (permalink / raw)
  To: gdb-patches

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(-)

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);
-- 
2.35.3


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v2 3/6] [gdb/symtab] Fix race on dwarf2_per_cu_data::{queued,is_debug_type}
  2023-08-02  9:52 [PATCH v2 0/6] [gdb/symtab] Fix data-races in gdb.base/index-cache.exp Tom de Vries
  2023-08-02  9:53 ` [PATCH v2 1/6] [gdb/symtab] Fix data race on index_cache::m_enabled Tom de Vries
  2023-08-02  9:53 ` [PATCH v2 2/6] [gdb/symtab] Fix data race on bfd::{cacheable,format} Tom de Vries
@ 2023-08-02  9:53 ` Tom de Vries
  2023-08-02 19:34   ` [PATCH v2 3/6] [gdb/symtab] Fix race on dwarf2_per_cu_data::{queued, is_debug_type} Tom Tromey
  2023-08-02  9:53 ` [PATCH v2 4/6] [gdb/symtab] Fix data race on bfd_last_cache Tom de Vries
                   ` (3 subsequent siblings)
  6 siblings, 1 reply; 19+ messages in thread
From: Tom de Vries @ 2023-08-02  9:53 UTC (permalink / raw)
  To: gdb-patches

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=24296)
  Write of size 1 at 0x7b200000420d by main thread:
    #0 queue_comp_unit gdb/dwarf2/read.c:5564 (gdb+0x8939ce)
    #1 dw2_do_instantiate_symtab gdb/dwarf2/read.c:1754 (gdb+0x885b96)
    #2 dw2_instantiate_symtab gdb/dwarf2/read.c:1792 (gdb+0x885d86)
    #3 dw2_expand_symtabs_matching_one(dwarf2_per_cu_data*, dwarf2_per_objfile*, gdb::function_view<bool (char const*, bool)>, gdb::function_view<bool (compunit_symtab*)>) gdb/dwarf2/read.c:3042 (gdb+0x88ac77)
    #4 cooked_index_functions::expand_symtabs_matching(objfile*, gdb::function_view<bool (char const*, bool)>, lookup_name_info const*, gdb::function_view<bool (char const*)>, gdb::function_view<bool (compunit_symtab*)>, enum_flags<block_search_flag_values>, domain_enum, search_domain) gdb/dwarf2/read.c:16915 (gdb+0x8c1c8a)
    #5 objfile::lookup_symbol(block_enum, char const*, domain_enum) gdb/symfile-debug.c:288 (gdb+0xf389a1)
    #6 lookup_symbol_via_quick_fns gdb/symtab.c:2385 (gdb+0xf66403)
    #7 lookup_symbol_in_objfile gdb/symtab.c:2516 (gdb+0xf66a67)
    #8 operator() gdb/symtab.c:2562 (gdb+0xf66bbe)
    #9 operator() gdb/../gdbsupport/function-view.h:305 (gdb+0xf76ffd)
    #10 _FUN gdb/../gdbsupport/function-view.h:299 (gdb+0xf77054)
    #11 gdb::function_view<bool (objfile*)>::operator()(objfile*) const gdb/../gdbsupport/function-view.h:289 (gdb+0xc3f5e3)
    #12 svr4_iterate_over_objfiles_in_search_order gdb/solib-svr4.c:3455 (gdb+0xeca793)
    #13 gdbarch_iterate_over_objfiles_in_search_order(gdbarch*, gdb::function_view<bool (objfile*)>, objfile*) gdb/gdbarch.c:5041 (gdb+0x537cad)
    #14 lookup_global_or_static_symbol gdb/symtab.c:2559 (gdb+0xf66e47)
    #15 lookup_global_symbol(char const*, block const*, domain_enum) gdb/symtab.c:2615 (gdb+0xf670cc)
    #16 language_defn::lookup_symbol_nonlocal(char const*, block const*, domain_enum) const gdb/symtab.c:2447 (gdb+0xf666ba)
    #17 lookup_symbol_aux gdb/symtab.c:2123 (gdb+0xf655ff)
    #18 lookup_symbol_in_language(char const*, block const*, domain_enum, language, field_of_this_result*) gdb/symtab.c:1931 (gdb+0xf646f7)
    #19 set_initial_language() gdb/symfile.c:1708 (gdb+0xf429c0)
    #20 symbol_file_add_main_1 gdb/symfile.c:1212 (gdb+0xf40f54)
    #21 symbol_file_command(char const*, int) gdb/symfile.c:1681 (gdb+0xf428fb)
    #22 file_command gdb/exec.c:554 (gdb+0x94f875)
    #23 do_simple_func gdb/cli/cli-decode.c:95 (gdb+0x6d9528)
    #24 cmd_func(cmd_list_element*, char const*, int) gdb/cli/cli-decode.c:2735 (gdb+0x6e0f69)
    #25 execute_command(char const*, int) gdb/top.c:575 (gdb+0xff3166)
    #26 command_handler(char const*) gdb/event-top.c:552 (gdb+0x94af08)
    #27 command_line_handler(std::unique_ptr<char, gdb::xfree_deleter<char> >&&) gdb/event-top.c:788 (gdb+0x94b5c5)
    #28 tui_command_line_handler gdb/tui/tui-interp.c:104 (gdb+0x10348c6)
    #29 gdb_rl_callback_handler gdb/event-top.c:259 (gdb+0x94a4ad)
    #30 rl_callback_read_char readline/readline/callback.c:290 (gdb+0x11bdf87)
    #31 gdb_rl_callback_read_char_wrapper_noexcept gdb/event-top.c:195 (gdb+0x94a2ac)
    #32 gdb_rl_callback_read_char_wrapper gdb/event-top.c:234 (gdb+0x94a36d)
    #33 stdin_event_handler gdb/ui.c:155 (gdb+0x1074b6a)
    #34 handle_file_event gdbsupport/event-loop.cc:573 (gdb+0x1d9502c)
    #35 gdb_wait_for_event gdbsupport/event-loop.cc:694 (gdb+0x1d95764)
    #36 gdb_do_one_event(int) gdbsupport/event-loop.cc:264 (gdb+0x1d93b50)
    #37 start_event_loop gdb/main.c:412 (gdb+0xb5a49e)
    #38 captured_command_loop gdb/main.c:476 (gdb+0xb5a68d)
    #39 captured_main gdb/main.c:1320 (gdb+0xb5c80d)
    #40 gdb_main(captured_main_args*) gdb/main.c:1339 (gdb+0xb5c8bc)
    #41 main gdb/gdb.c:32 (gdb+0x416776)

  Previous read of size 1 at 0x7b200000420d by thread T12:
    #0 write_gdbindex gdb/dwarf2/index-write.c:1229 (gdb+0x8310c8)
    #1 write_dwarf_index(dwarf2_per_bfd*, char const*, char const*, char const*, dw_index_kind) gdb/dwarf2/index-write.c:1484 (gdb+0x83232f)
    #2 index_cache::store(dwarf2_per_bfd*, index_cache_store_context*) gdb/dwarf2/index-cache.c:177 (gdb+0x82d62b)
    #3 cooked_index::maybe_write_index(dwarf2_per_bfd*) gdb/dwarf2/cooked-index.c:640 (gdb+0x7f1bf7)
    #4 operator() gdb/dwarf2/cooked-index.c:470 (gdb+0x7f0f40)
    #5 _M_invoke /usr/include/c++/7/bits/std_function.h:316 (gdb+0x7f2909)
    #6 std::function<void ()>::operator()() const /usr/include/c++/7/bits/std_function.h:706 (gdb+0x700952)
    #7 void std::__invoke_impl<void, std::function<void ()>&>(std::__invoke_other, std::function<void ()>&) /usr/include/c++/7/bits/invoke.h:60 (gdb+0x7381a0)
    #8 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)
    #9 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)
    #10 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)
    #11 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)
    #12 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)
    #13 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)
    #14 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)
    #15 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)
    #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()#1}::operator()() const /usr/include/c++/7/mutex:672 (gdb+0x73300d)
    #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}::operator()() const /usr/include/c++/7/mutex:677 (gdb+0x7330b2)
    #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}::_FUN() /usr/include/c++/7/mutex:677 (gdb+0x7330f2)
    #19 pthread_once <null> (libtsan.so.0+0x4457c)
    #20 __gthread_once /usr/include/c++/7/x86_64-suse-linux/bits/gthr-default.h:699 (gdb+0x72f5dd)
    #21 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)
    #22 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)
    #23 std::__future_base::_Task_state<std::function<void ()>, std::allocator<int>, void ()>::_M_run() /usr/include/c++/7/future:1423 (gdb+0x737bef)
    #24 std::packaged_task<void ()>::operator()() /usr/include/c++/7/future:1556 (gdb+0x1dac6da)
    #25 gdb::thread_pool::thread_function() gdbsupport/thread-pool.cc:242 (gdb+0x1dabffc)
    #26 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+0x1dad0ab)
    #27 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+0x1dac4dc)
    #28 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+0x1daf80e)
    #29 std::thread::_Invoker<std::tuple<void (gdb::thread_pool::*)(), gdb::thread_pool*> >::operator()() /usr/include/c++/7/thread:243 (gdb+0x1daf799)
    #30 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+0x1daf74e)
    #31 <null> <null> (libstdc++.so.6+0xdcac2)
 ...
SUMMARY: ThreadSanitizer: data race gdb/dwarf2/read.c:5564 in queue_comp_unit
...

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

The race is between:
- a worker thread writing the index cache, and in the process reading
  dwarf2_per_cu_data::is_debug_type, and
- the main thread expanding the CU containing main, and in the process setting
  dwarf2_per_cu_data::queued.

The two bitfields dwarf2_per_cu_data::queue and
dwarf2_per_cu_data::is_debug_type share the same bitfield container.

Fix this by making dwarf2_per_cu_data::queued a packed<bool, 1>.

Tested on x86_64-linux.

PR symtab/30392
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30392
---
 gdb/dwarf2/read.h | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/gdb/dwarf2/read.h b/gdb/dwarf2/read.h
index a99299cbc6d..5fa276a7c1b 100644
--- a/gdb/dwarf2/read.h
+++ b/gdb/dwarf2/read.h
@@ -100,14 +100,14 @@ typedef std::unique_ptr<dwarf2_per_cu_data, dwarf2_per_cu_data_deleter>
 struct dwarf2_per_cu_data
 {
   dwarf2_per_cu_data ()
-    : queued (false),
-      is_debug_types (false),
+    : is_debug_types (false),
       is_dwz (false),
       reading_dwo_directly (false),
       tu_read (false),
       m_header_read_in (false),
       mark (false),
       files_read (false),
+      queued (false),
       scanned (false)
   {
   }
@@ -126,10 +126,6 @@ struct dwarf2_per_cu_data
   unsigned char m_dwarf_version = 0;
 
 public:
-  /* Flag indicating this compilation unit will be read in before
-     any of the current compilation units are processed.  */
-  unsigned int queued : 1;
-
   /* Non-zero if this CU is from .debug_types.
      Struct dwarf2_per_cu_data is contained in struct signatured_type iff
      this is non-zero.  */
@@ -176,6 +172,10 @@ struct dwarf2_per_cu_data
      .debug_aranges), then this flag is set.  */
   packed<bool, 1> addresses_seen = false;
 
+  /* Flag indicating this compilation unit will be read in before
+     any of the current compilation units are processed.  */
+  packed<bool, 1> queued;
+
 private:
   /* The unit type of this CU.  */
   std::atomic<packed<dwarf_unit_type, 1>> m_unit_type {(dwarf_unit_type)0};
-- 
2.35.3


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v2 4/6] [gdb/symtab] Fix data race on bfd_last_cache
  2023-08-02  9:52 [PATCH v2 0/6] [gdb/symtab] Fix data-races in gdb.base/index-cache.exp Tom de Vries
                   ` (2 preceding siblings ...)
  2023-08-02  9:53 ` [PATCH v2 3/6] [gdb/symtab] Fix race on dwarf2_per_cu_data::{queued,is_debug_type} Tom de Vries
@ 2023-08-02  9:53 ` Tom de Vries
  2023-08-02 19:37   ` Tom Tromey
  2023-08-02  9:53 ` [PATCH v2 5/6] [gdb/symtab] Fix data race on dwarf2_per_cu_data::{m_header_read_in,is_debug_type} Tom de Vries
                   ` (2 subsequent siblings)
  6 siblings, 1 reply; 19+ messages in thread
From: Tom de Vries @ 2023-08-02  9:53 UTC (permalink / raw)
  To: gdb-patches

With gdb build with -fsanitize=thread and test-case gdb.base/index-cache.exp
run using "taskset -c 0", 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=5420)
  Write of size 8 at 0x0000032569e0 by main thread:
    #0 snip bfd/cache.c:155 (gdb+0x148b709)
    #1 bfd_cache_delete bfd/cache.c:176 (gdb+0x148b7a8)
    #2 bfd_cache_close bfd/cache.c:537 (gdb+0x148c406)
    #3 bfd_cache_close_all bfd/cache.c:561 (gdb+0x148c44a)
    #4 symbol_file_add_with_addrs gdb/symfile.c:1132 (gdb+0xf412ad)
    #5 symbol_file_add_from_bfd(gdb::ref_ptr<bfd, gdb_bfd_ref_policy> const&, char const*, enum_flags<symfile_add_flag>, std::vector<other_sections, std::allocator<other_sections> >*, enum_flags<objfile_flag>, objfile*) gdb/symfile.c:1167 (gdb+0xf4141d)
    #6 symbol_file_add(char const*, enum_flags<symfile_add_flag>, std::vector<other_sections, std::allocator<other_sections> >*, enum_flags<objfile_flag>) gdb/symfile.c:1180 (gdb+0xf4149d)
    #7 symbol_file_add_main_1 gdb/symfile.c:1203 (gdb+0xf415a9)
    #8 symbol_file_command(char const*, int) gdb/symfile.c:1681 (gdb+0xf42f97)
    #9 file_command gdb/exec.c:554 (gdb+0x94ff11)
    #10 do_simple_func gdb/cli/cli-decode.c:95 (gdb+0x6d9528)
    #11 cmd_func(cmd_list_element*, char const*, int) gdb/cli/cli-decode.c:2735 (gdb+0x6e0f69)
    #12 execute_command(char const*, int) gdb/top.c:575 (gdb+0xff3784)
    #13 command_handler(char const*) gdb/event-top.c:552 (gdb+0x94b5a4)
    #14 command_line_handler(std::unique_ptr<char, gdb::xfree_deleter<char> >&&) gdb/event-top.c:788 (gdb+0x94bc61)
    #15 tui_command_line_handler gdb/tui/tui-interp.c:104 (gdb+0x1034ee4)
    #16 gdb_rl_callback_handler gdb/event-top.c:259 (gdb+0x94ab49)
    #17 rl_callback_read_char readline/readline/callback.c:290 (gdb+0x11be4d7)
    #18 gdb_rl_callback_read_char_wrapper_noexcept gdb/event-top.c:195 (gdb+0x94a948)
    #19 gdb_rl_callback_read_char_wrapper gdb/event-top.c:234 (gdb+0x94aa09)
    #20 stdin_event_handler gdb/ui.c:155 (gdb+0x1075188)
    #21 handle_file_event gdbsupport/event-loop.cc:573 (gdb+0x1d95b94)
    #22 gdb_wait_for_event gdbsupport/event-loop.cc:694 (gdb+0x1d962cc)
    #23 gdb_do_one_event(int) gdbsupport/event-loop.cc:217 (gdb+0x1d94573)
    #24 start_event_loop gdb/main.c:412 (gdb+0xb5ab3a)
    #25 captured_command_loop gdb/main.c:476 (gdb+0xb5ad29)
    #26 captured_main gdb/main.c:1320 (gdb+0xb5cea9)
    #27 gdb_main(captured_main_args*) gdb/main.c:1339 (gdb+0xb5cf58)
    #28 main gdb/gdb.c:32 (gdb+0x416776)

  Previous read of size 8 at 0x0000032569e0 by thread T12:
    #0 cache_bseek bfd/cache.c:289 (gdb+0x148bbf7)
    #1 bfd_seek bfd/bfdio.c:459 (gdb+0x148a554)
    #2 _bfd_generic_get_section_contents bfd/libbfd.c:1069 (gdb+0x14980c7)
    #3 bfd_get_section_contents bfd/section.c:1606 (gdb+0x149d59f)
    #4 section_table_xfer_memory_partial(unsigned char*, unsigned char const*, unsigned long, unsigned long, unsigned long*, std::vector<target_section, std::allocator<target_section> > const&, gdb::function_view<bool (target_section const*)>) gdb/exec.c:840 (gdb+0x951171)
    #5 exec_target::xfer_partial(target_object, char const*, unsigned char*, unsigned char const*, unsigned long, unsigned long, unsigned long*) gdb/exec.c:894 (gdb+0x951372)
    #6 raw_memory_xfer_partial(target_ops*, unsigned char*, unsigned char const*, unsigned long, long, unsigned long*) gdb/target.c:1476 (gdb+0xfade7a)
    #7 memory_xfer_partial_1 gdb/target.c:1607 (gdb+0xfae39a)
    #8 memory_xfer_partial gdb/target.c:1636 (gdb+0xfae46c)
    #9 target_xfer_partial(target_ops*, target_object, char const*, unsigned char*, unsigned char const*, unsigned long, unsigned long, unsigned long*) gdb/target.c:1693 (gdb+0xfae7f4)
    #10 target_read_partial gdb/target.c:1947 (gdb+0xfaf3e4)
    #11 target_read(target_ops*, target_object, char const*, unsigned char*, unsigned long, long) gdb/target.c:1986 (gdb+0xfaf54a)
    #12 target_read_memory(unsigned long, unsigned char*, long) gdb/target.c:1782 (gdb+0xfaecc6)
    #13 gdb_bfd_scan_elf_dyntag(int, bfd*, unsigned long*, unsigned long*) gdb/solib.c:1638 (gdb+0xed98cf)
    #14 elf_locate_base gdb/solib-svr4.c:743 (gdb+0xec32e5)
    #15 svr4_iterate_over_objfiles_in_search_order gdb/solib-svr4.c:3430 (gdb+0xecad23)
    #16 gdbarch_iterate_over_objfiles_in_search_order(gdbarch*, gdb::function_view<bool (objfile*)>, objfile*) gdb/gdbarch.c:5041 (gdb+0x537cad)
    #17 find_main_name gdb/symtab.c:6270 (gdb+0xf74b6b)
    #18 main_name() gdb/symtab.c:6299 (gdb+0xf74bf9)
    #19 write_cooked_index gdb/dwarf2/index-write.c:1131 (gdb+0x831044)
    #20 write_gdbindex gdb/dwarf2/index-write.c:1256 (gdb+0x83189b)
    #21 write_dwarf_index(dwarf2_per_bfd*, char const*, char const*, char const*, dw_index_kind) gdb/dwarf2/index-write.c:1484 (gdb+0x83287f)
    #22 index_cache::store(dwarf2_per_bfd*, index_cache_store_context const&) gdb/dwarf2/index-cache.c:173 (gdb+0x82db35)
    #23 cooked_index::maybe_write_index(dwarf2_per_bfd*, index_cache_store_context const&) gdb/dwarf2/cooked-index.c:642 (gdb+0x7f1d31)
    #24 operator() gdb/dwarf2/cooked-index.c:471 (gdb+0x7f0f31)
    #25 _M_invoke /usr/include/c++/7/bits/std_function.h:316 (gdb+0x7f29fb)
    #26 std::function<void ()>::operator()() const /usr/include/c++/7/bits/std_function.h:706 (gdb+0x700952)
    #27 void std::__invoke_impl<void, std::function<void ()>&>(std::__invoke_other, std::function<void ()>&) /usr/include/c++/7/bits/invoke.h:60 (gdb+0x7381a0)
    #28 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)
    #29 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)
    #30 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)
    #31 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)
    #32 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)
    #33 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)
    #34 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)
    #35 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)
    #36 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)
    #37 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)
    #38 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)
    #39 pthread_once <null> (libtsan.so.0+0x4457c)
    #40 __gthread_once /usr/include/c++/7/x86_64-suse-linux/bits/gthr-default.h:699 (gdb+0x72f5dd)
    #41 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)
    #42 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)
    #43 std::__future_base::_Task_state<std::function<void ()>, std::allocator<int>, void ()>::_M_run() /usr/include/c++/7/future:1423 (gdb+0x737bef)
    #44 std::packaged_task<void ()>::operator()() /usr/include/c++/7/future:1556 (gdb+0x1dad242)
    #45 gdb::thread_pool::thread_function() gdbsupport/thread-pool.cc:242 (gdb+0x1dacb64)
    #46 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+0x1dadc13)
    #47 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+0x1dad044)
    #48 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+0x1db0376)
    #49 std::thread::_Invoker<std::tuple<void (gdb::thread_pool::*)(), gdb::thread_pool*> >::operator()() /usr/include/c++/7/thread:243 (gdb+0x1db0301)
    #50 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+0x1db02b6)
    #51 <null> <null> (libstdc++.so.6+0xdcac2)

  Location is global 'bfd_last_cache' of size 8 at 0x0000032569e0 (gdb+0x0000032569e0)
  ...
SUMMARY: ThreadSanitizer: data race bfd/cache.c:155 in snip
...

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

The race is between:
- a worker thread calling main_name (), and in the process reading
  bfd_last_cache, and
- the main thread writing to bfd_last_cache.

Fix this by calling main_name () from the main thread.

Tested on x86_64-linux.

PR symtab/30392
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30392
---
 gdb/dwarf2/cooked-index.c | 7 +++++++
 gdb/dwarf2/read.c         | 8 ++++----
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/gdb/dwarf2/cooked-index.c b/gdb/dwarf2/cooked-index.c
index b5718ae5529..9f647cfd4a2 100644
--- a/gdb/dwarf2/cooked-index.c
+++ b/gdb/dwarf2/cooked-index.c
@@ -462,6 +462,13 @@ cooked_index::start_writing_index (dwarf2_per_bfd *per_bfd)
 {
   struct index_cache_store_context ctx (global_index_cache, per_bfd);
 
+  if (global_index_cache.enabled ())
+    {
+      /* Make sure find_main_name is called in the main thread rather than a
+	 worker thread, to avoid data races on bfd.  */
+      main_name ();
+    }
+
   /* This must be set after all the finalization tasks have been
      started, because it may call 'wait'.  */
   m_write_future
diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 61730f6481c..c37bacad3b1 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -5175,10 +5175,6 @@ dwarf2_build_psymtabs_hard (dwarf2_per_objfile *per_objfile)
   cooked_index *vec = new cooked_index (std::move (indexes));
   per_bfd->index_table.reset (vec);
 
-  /* Cannot start writing the index entry until after the
-     'index_table' member has been set.  */
-  vec->start_writing_index (per_bfd);
-
   const cooked_index_entry *main_entry = vec->get_main ();
   if (main_entry != nullptr)
     {
@@ -5193,6 +5189,10 @@ dwarf2_build_psymtabs_hard (dwarf2_per_objfile *per_objfile)
       set_objfile_main_name (objfile, full_name, lang);
     }
 
+  /* Cannot start writing the index entry until after the
+     'index_table' member has been set.  */
+  vec->start_writing_index (per_bfd);
+
   dwarf_read_debug_printf ("Done building psymtabs of %s",
 			   objfile_name (objfile));
 }
-- 
2.35.3


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v2 5/6] [gdb/symtab] Fix data race on dwarf2_per_cu_data::{m_header_read_in,is_debug_type}
  2023-08-02  9:52 [PATCH v2 0/6] [gdb/symtab] Fix data-races in gdb.base/index-cache.exp Tom de Vries
                   ` (3 preceding siblings ...)
  2023-08-02  9:53 ` [PATCH v2 4/6] [gdb/symtab] Fix data race on bfd_last_cache Tom de Vries
@ 2023-08-02  9:53 ` Tom de Vries
  2023-08-02 19:39   ` [PATCH v2 5/6] [gdb/symtab] Fix data race on dwarf2_per_cu_data::{m_header_read_in, is_debug_type} Tom Tromey
  2023-08-02  9:53 ` [PATCH v2 6/6] [gdb/testsuite] Extend gdb.base/index-cache.exp Tom de Vries
  2023-08-02 19:44 ` [PATCH v2 0/6] [gdb/symtab] Fix data-races in gdb.base/index-cache.exp Tom Tromey
  6 siblings, 1 reply; 19+ messages in thread
From: Tom de Vries @ 2023-08-02  9:53 UTC (permalink / raw)
  To: gdb-patches

With gdb build with -fsanitize=thread and test-case gdb.base/index-cache.exp
and target board debug-types, 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=9654)
  Write of size 1 at 0x7b200000420d by main thread:
    #0 dwarf2_per_cu_data::get_header() const gdb/dwarf2/read.c:21513 (gdb+0x8d1eee)
    #1 dwarf2_per_cu_data::addr_size() const gdb/dwarf2/read.c:21524 (gdb+0x8d1f4e)
    #2 dwarf2_cu::addr_type() const gdb/dwarf2/cu.c:112 (gdb+0x806327)
    #3 set_die_type gdb/dwarf2/read.c:21932 (gdb+0x8d3870)
    #4 read_base_type gdb/dwarf2/read.c:15448 (gdb+0x8bcacb)
    #5 read_type_die_1 gdb/dwarf2/read.c:19832 (gdb+0x8cc0a5)
    #6 read_type_die gdb/dwarf2/read.c:19767 (gdb+0x8cbe6d)
    #7 lookup_die_type gdb/dwarf2/read.c:19739 (gdb+0x8cbdc7)
    #8 die_type gdb/dwarf2/read.c:19593 (gdb+0x8cb68a)
    #9 read_subroutine_type gdb/dwarf2/read.c:14648 (gdb+0x8b998e)
    #10 read_type_die_1 gdb/dwarf2/read.c:19792 (gdb+0x8cbf2f)
    #11 read_type_die gdb/dwarf2/read.c:19767 (gdb+0x8cbe6d)
    #12 read_func_scope gdb/dwarf2/read.c:10154 (gdb+0x8a4f36)
    #13 process_die gdb/dwarf2/read.c:6667 (gdb+0x898daa)
    #14 read_file_scope gdb/dwarf2/read.c:7682 (gdb+0x89bad8)
    #15 process_die gdb/dwarf2/read.c:6654 (gdb+0x898ced)
    #16 process_full_comp_unit gdb/dwarf2/read.c:6418 (gdb+0x8981de)
    #17 process_queue gdb/dwarf2/read.c:5690 (gdb+0x894433)
    #18 dw2_do_instantiate_symtab gdb/dwarf2/read.c:1770 (gdb+0x88623a)
    #19 dw2_instantiate_symtab gdb/dwarf2/read.c:1792 (gdb+0x886300)
    #20 dw2_expand_symtabs_matching_one(dwarf2_per_cu_data*, dwarf2_per_objfile*, gdb::function_view<bool (char const*, bool)>, gdb::function_view<bool (compunit_symtab*)>) gdb/dwarf2/read.c:3042 (gdb+0x88b1f1)
    #21 cooked_index_functions::expand_symtabs_matching(objfile*, gdb::function_view<bool (char const*, bool)>, lookup_name_info const*, gdb::function_view<bool (char const*)>, gdb::function_view<bool (compunit_symtab*)>, enum_flags<block_search_flag_values>, domain_enum, search_domain) gdb/dwarf2/read.c:16917 (gdb+0x8c228e)
    #22 objfile::lookup_symbol(block_enum, char const*, domain_enum) gdb/symfile-debug.c:288 (gdb+0xf39055)
    #23 lookup_symbol_via_quick_fns gdb/symtab.c:2385 (gdb+0xf66ab7)
    #24 lookup_symbol_in_objfile gdb/symtab.c:2516 (gdb+0xf6711b)
    #25 operator() gdb/symtab.c:2562 (gdb+0xf67272)
    #26 operator() gdb/../gdbsupport/function-view.h:305 (gdb+0xf776b1)
    #27 _FUN gdb/../gdbsupport/function-view.h:299 (gdb+0xf77708)
    #28 gdb::function_view<bool (objfile*)>::operator()(objfile*) const gdb/../gdbsupport/function-view.h:289 (gdb+0xc3fc97)
    #29 svr4_iterate_over_objfiles_in_search_order gdb/solib-svr4.c:3455 (gdb+0xecae47)
    #30 gdbarch_iterate_over_objfiles_in_search_order(gdbarch*, gdb::function_view<bool (objfile*)>, objfile*) gdb/gdbarch.c:5041 (gdb+0x537cad)
    #31 lookup_global_or_static_symbol gdb/symtab.c:2559 (gdb+0xf674fb)
    #32 lookup_global_symbol(char const*, block const*, domain_enum) gdb/symtab.c:2615 (gdb+0xf67780)
    #33 language_defn::lookup_symbol_nonlocal(char const*, block const*, domain_enum) const gdb/symtab.c:2447 (gdb+0xf66d6e)
    #34 lookup_symbol_aux gdb/symtab.c:2123 (gdb+0xf65cb3)
    #35 lookup_symbol_in_language(char const*, block const*, domain_enum, language, field_of_this_result*) gdb/symtab.c:1931 (gdb+0xf64dab)
    #36 set_initial_language() gdb/symfile.c:1708 (gdb+0xf43074)
    #37 symbol_file_add_main_1 gdb/symfile.c:1212 (gdb+0xf41608)
    #38 symbol_file_command(char const*, int) gdb/symfile.c:1681 (gdb+0xf42faf)
    #39 file_command gdb/exec.c:554 (gdb+0x94ff29)
    #40 do_simple_func gdb/cli/cli-decode.c:95 (gdb+0x6d9528)
    #41 cmd_func(cmd_list_element*, char const*, int) gdb/cli/cli-decode.c:2735 (gdb+0x6e0f69)
    #42 execute_command(char const*, int) gdb/top.c:575 (gdb+0xff379c)
    #43 command_handler(char const*) gdb/event-top.c:552 (gdb+0x94b5bc)
    #44 command_line_handler(std::unique_ptr<char, gdb::xfree_deleter<char> >&&) gdb/event-top.c:788 (gdb+0x94bc79)
    #45 tui_command_line_handler gdb/tui/tui-interp.c:104 (gdb+0x1034efc)
    #46 gdb_rl_callback_handler gdb/event-top.c:259 (gdb+0x94ab61)
    #47 rl_callback_read_char readline/readline/callback.c:290 (gdb+0x11be4ef)
    #48 gdb_rl_callback_read_char_wrapper_noexcept gdb/event-top.c:195 (gdb+0x94a960)
    #49 gdb_rl_callback_read_char_wrapper gdb/event-top.c:234 (gdb+0x94aa21)
    #50 stdin_event_handler gdb/ui.c:155 (gdb+0x10751a0)
    #51 handle_file_event gdbsupport/event-loop.cc:573 (gdb+0x1d95bac)
    #52 gdb_wait_for_event gdbsupport/event-loop.cc:694 (gdb+0x1d962e4)
    #53 gdb_do_one_event(int) gdbsupport/event-loop.cc:264 (gdb+0x1d946d0)
    #54 start_event_loop gdb/main.c:412 (gdb+0xb5ab52)
    #55 captured_command_loop gdb/main.c:476 (gdb+0xb5ad41)
    #56 captured_main gdb/main.c:1320 (gdb+0xb5cec1)
    #57 gdb_main(captured_main_args*) gdb/main.c:1339 (gdb+0xb5cf70)
    #58 main gdb/gdb.c:32 (gdb+0x416776)

  Previous read of size 1 at 0x7b200000420d by thread T11:
    #0 write_gdbindex gdb/dwarf2/index-write.c:1229 (gdb+0x831630)
    #1 write_dwarf_index(dwarf2_per_bfd*, char const*, char const*, char const*, dw_index_kind) gdb/dwarf2/index-write.c:1484 (gdb+0x832897)
    #2 index_cache::store(dwarf2_per_bfd*, index_cache_store_context const&) gdb/dwarf2/index-cache.c:173 (gdb+0x82db8d)
    #3 cooked_index::maybe_write_index(dwarf2_per_bfd*, index_cache_store_context const&) gdb/dwarf2/cooked-index.c:645 (gdb+0x7f1d49)
    #4 operator() gdb/dwarf2/cooked-index.c:474 (gdb+0x7f0f31)
    #5 _M_invoke /usr/include/c++/7/bits/std_function.h:316 (gdb+0x7f2a13)
    #6 std::function<void ()>::operator()() const /usr/include/c++/7/bits/std_function.h:706 (gdb+0x700952)
    #7 void std::__invoke_impl<void, std::function<void ()>&>(std::__invoke_other, std::function<void ()>&) /usr/include/c++/7/bits/invoke.h:60 (gdb+0x7381a0)
    #8 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)
    #9 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)
    #10 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)
    #11 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)
    #12 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)
    #13 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)
    #14 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)
    #15 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)
    #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()#1}::operator()() const /usr/include/c++/7/mutex:672 (gdb+0x73300d)
    #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}::operator()() const /usr/include/c++/7/mutex:677 (gdb+0x7330b2)
    #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}::_FUN() /usr/include/c++/7/mutex:677 (gdb+0x7330f2)
    #19 pthread_once <null> (libtsan.so.0+0x4457c)
    #20 __gthread_once /usr/include/c++/7/x86_64-suse-linux/bits/gthr-default.h:699 (gdb+0x72f5dd)
    #21 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)
    #22 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)
    #23 std::__future_base::_Task_state<std::function<void ()>, std::allocator<int>, void ()>::_M_run() /usr/include/c++/7/future:1423 (gdb+0x737bef)
    #24 std::packaged_task<void ()>::operator()() /usr/include/c++/7/future:1556 (gdb+0x1dad25a)
    #25 gdb::thread_pool::thread_function() gdbsupport/thread-pool.cc:242 (gdb+0x1dacb7c)
    #26 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+0x1dadc2b)
    #27 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+0x1dad05c)
    #28 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+0x1db038e)
    #29 std::thread::_Invoker<std::tuple<void (gdb::thread_pool::*)(), gdb::thread_pool*> >::operator()() /usr/include/c++/7/thread:243 (gdb+0x1db0319)
    #30 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+0x1db02ce)
    #31 <null> <null> (libstdc++.so.6+0xdcac2)
  ...
SUMMARY: ThreadSanitizer: data race gdb/dwarf2/read.c:21513 in dwarf2_per_cu_data::get_header() const
...

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

The race is between:
- a worker thread writing the index cache, and in the process reading
   dwarf2_per_cu_data::is_debug_type, and
- the main thread writing to dwarf2_per_cu_data::m_header_read_in.

The two bitfields dwarf2_per_cu_data::m_header_read_in and
dwarf2_per_cu_data::is_debug_type share the same bitfield container.

Fix this by making dwarf2_per_cu_data::m_header_read_in a packed<bool, 1>.

Tested on x86_64-linux.

PR symtab/30392
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30392
---
 gdb/dwarf2/read.h | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/gdb/dwarf2/read.h b/gdb/dwarf2/read.h
index 5fa276a7c1b..5f7d2dcd895 100644
--- a/gdb/dwarf2/read.h
+++ b/gdb/dwarf2/read.h
@@ -104,10 +104,10 @@ struct dwarf2_per_cu_data
       is_dwz (false),
       reading_dwo_directly (false),
       tu_read (false),
-      m_header_read_in (false),
       mark (false),
       files_read (false),
       queued (false),
+      m_header_read_in (false),
       scanned (false)
   {
   }
@@ -150,12 +150,6 @@ struct dwarf2_per_cu_data
      This flag is only valid if is_debug_types is true.  */
   unsigned int tu_read : 1;
 
-  /* True if HEADER has been read in.
-
-     Don't access this field directly.  It should be private, but we can't make
-     it private at the moment.  */
-  mutable bool m_header_read_in : 1;
-
   /* A temporary mark bit used when iterating over all CUs in
      expand_symtabs_matching.  */
   unsigned int mark : 1;
@@ -176,6 +170,12 @@ struct dwarf2_per_cu_data
      any of the current compilation units are processed.  */
   packed<bool, 1> queued;
 
+  /* True if HEADER has been read in.
+
+     Don't access this field directly.  It should be private, but we can't make
+     it private at the moment.  */
+  mutable packed<bool, 1> m_header_read_in;
+
 private:
   /* The unit type of this CU.  */
   std::atomic<packed<dwarf_unit_type, 1>> m_unit_type {(dwarf_unit_type)0};
-- 
2.35.3


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v2 6/6] [gdb/testsuite] Extend gdb.base/index-cache.exp
  2023-08-02  9:52 [PATCH v2 0/6] [gdb/symtab] Fix data-races in gdb.base/index-cache.exp Tom de Vries
                   ` (4 preceding siblings ...)
  2023-08-02  9:53 ` [PATCH v2 5/6] [gdb/symtab] Fix data race on dwarf2_per_cu_data::{m_header_read_in,is_debug_type} Tom de Vries
@ 2023-08-02  9:53 ` Tom de Vries
  2023-08-02 19:41   ` Tom Tromey
  2023-08-02 19:44 ` [PATCH v2 0/6] [gdb/symtab] Fix data-races in gdb.base/index-cache.exp Tom Tromey
  6 siblings, 1 reply; 19+ messages in thread
From: Tom de Vries @ 2023-08-02  9:53 UTC (permalink / raw)
  To: gdb-patches

The test-case gdb.base/index-cache.exp uses only one source file, which
contains main.

While doing "file $exec", in set_initial_language a symbol lookup of "main" is
done, causing the symtab containing main to be expanded.

Handling of main is special, and a future optimization may skip the lookup and
expansion.

Reliably exercise:
- the lookup of main, expanding the symtab containing main, by doing
  "ptype main", and
- the lookup of another symbol, expanding a symtab not containing main, by:
  - adding another source file containing function foo, and
  - doing "ptype foo".

This triggered a segfault with target board native-extended-gdbserver, filed
as PR symtab/30712, but that seems to be fixed by a previous commit in this
series.

Tested on x86_64-linux.
---
 gdb/testsuite/gdb.base/index-cache-2.c | 24 ++++++++++++++++++++++++
 gdb/testsuite/gdb.base/index-cache.c   |  6 ++++--
 gdb/testsuite/gdb.base/index-cache.exp | 22 ++++++++++++++++++++--
 3 files changed, 48 insertions(+), 4 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/index-cache-2.c

diff --git a/gdb/testsuite/gdb.base/index-cache-2.c b/gdb/testsuite/gdb.base/index-cache-2.c
new file mode 100644
index 00000000000..d0c10780499
--- /dev/null
+++ b/gdb/testsuite/gdb.base/index-cache-2.c
@@ -0,0 +1,24 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2023 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+extern int foo (void);
+
+int
+foo (void)
+{
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.base/index-cache.c b/gdb/testsuite/gdb.base/index-cache.c
index 16521df96f5..fecba981b91 100644
--- a/gdb/testsuite/gdb.base/index-cache.c
+++ b/gdb/testsuite/gdb.base/index-cache.c
@@ -15,9 +15,11 @@
    You should have received a copy of the GNU General Public License
    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
+extern int foo (void);
+
 int
-main ()
+main (void)
 {
-  return 0;
+  return foo ();
 }
 
diff --git a/gdb/testsuite/gdb.base/index-cache.exp b/gdb/testsuite/gdb.base/index-cache.exp
index 9f2f53053cb..c26c4f94e65 100644
--- a/gdb/testsuite/gdb.base/index-cache.exp
+++ b/gdb/testsuite/gdb.base/index-cache.exp
@@ -16,9 +16,9 @@
 # This test checks that the index-cache feature generates the expected files at
 # the expected location.
 
-standard_testfile
+standard_testfile .c -2.c
 
-if { [build_executable "failed to prepare" $testfile $srcfile \
+if { [build_executable "failed to prepare" $testfile [list $srcfile $srcfile2] \
 	  {debug ldflags=-Wl,--build-id}] } {
     return
 }
@@ -147,6 +147,12 @@ proc_with_prefix test_cache_disabled { cache_dir test_prefix } {
 	    gdb_assert "$nfiles_created == 0" "no files were created"
 
 	    check_cache_stats 0 0
+
+	    # Trigger expansion of symtab containing main, if not already done.
+	    gdb_test "ptype main" "^type = int \\(void\\)"
+
+	    # Trigger expansion of symtab not containing main.
+	    gdb_test "ptype foo" "^type = int \\(void\\)"
 	}
     }
 }
@@ -192,6 +198,12 @@ proc_with_prefix test_cache_enabled_miss { cache_dir } {
 	} else {
 	    check_cache_stats 0 0
 	}
+
+	# Trigger expansion of symtab containing main, if not already done.
+	gdb_test "ptype main" "^type = int \\(void\\)"
+
+	# Trigger expansion of symtab not containing main.
+	gdb_test "ptype foo" "^type = int \\(void\\)"
     }
 }
 
@@ -221,6 +233,12 @@ proc_with_prefix test_cache_enabled_hit { cache_dir } {
 	} else {
 	    check_cache_stats 0 0
 	}
+
+	# Trigger expansion of symtab containing main, if not already done.
+	gdb_test "ptype main" "^type = int \\(void\\)"
+
+	# Trigger expansion of symtab not containing main.
+	gdb_test "ptype foo" "^type = int \\(void\\)"
     }
 }
 
-- 
2.35.3


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2 1/6] [gdb/symtab] Fix data race on index_cache::m_enabled
  2023-08-02  9:53 ` [PATCH v2 1/6] [gdb/symtab] Fix data race on index_cache::m_enabled Tom de Vries
@ 2023-08-02 19:29   ` Tom Tromey
  2023-08-04  0:08     ` Tom de Vries
  0 siblings, 1 reply; 19+ messages in thread
From: Tom Tromey @ 2023-08-02 19:29 UTC (permalink / raw)
  To: Tom de Vries via Gdb-patches; +Cc: Tom de Vries

>>>>> "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:
[race]

Tom> Fix this by capturing the value of index_cache::m_enabled in the main thread,
Tom> and using the captured value in the worker thread.

Thanks for the patch.  I have some stylistic nits but otherwise it seems
fine to me.

Tom> +  struct index_cache_store_context ctx (global_index_cache);

I think we tend to avoid the struct/class keyword in cases like this now.

Tom> +				 const struct index_cache_store_context &ctx)

... basically everywhere.

Tom> +index_cache_store_context::index_cache_store_context (const index_cache &ic)
Tom> +{
Tom> +  m_enabled = ic.enabled ();

It's better to use the initializer syntax like

  : m_enabled (ic.enabled ())

This can be in the header if possible (I didn't try).

Tom> +struct index_cache_store_context
Tom> +{
Tom> +  friend class index_cache;
Tom> +
Tom> +  index_cache_store_context(const index_cache &ic);

Single-argument constructors should be 'explicit' as a rule.

Tom

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2 2/6] [gdb/symtab] Fix data race on bfd::{cacheable,format}
  2023-08-02  9:53 ` [PATCH v2 2/6] [gdb/symtab] Fix data race on bfd::{cacheable,format} Tom de Vries
@ 2023-08-02 19:32   ` Tom Tromey
  2023-08-04  0:09     ` Tom de Vries
  0 siblings, 1 reply; 19+ messages in thread
From: Tom Tromey @ 2023-08-02 19:32 UTC (permalink / raw)
  To: Tom de Vries via Gdb-patches; +Cc: Tom de Vries

>>>>> "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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2 3/6] [gdb/symtab] Fix race on dwarf2_per_cu_data::{queued, is_debug_type}
  2023-08-02  9:53 ` [PATCH v2 3/6] [gdb/symtab] Fix race on dwarf2_per_cu_data::{queued,is_debug_type} Tom de Vries
@ 2023-08-02 19:34   ` Tom Tromey
  0 siblings, 0 replies; 19+ messages in thread
From: Tom Tromey @ 2023-08-02 19:34 UTC (permalink / raw)
  To: Tom de Vries via Gdb-patches; +Cc: Tom de Vries

>>>>> "Tom" == Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> writes:

Tom> The two bitfields dwarf2_per_cu_data::queue and
Tom> dwarf2_per_cu_data::is_debug_type share the same bitfield container.

Tom> Fix this by making dwarf2_per_cu_data::queued a packed<bool, 1>.

Approved-By: Tom Tromey <tom@tromey.com>

Tom

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2 4/6] [gdb/symtab] Fix data race on bfd_last_cache
  2023-08-02  9:53 ` [PATCH v2 4/6] [gdb/symtab] Fix data race on bfd_last_cache Tom de Vries
@ 2023-08-02 19:37   ` Tom Tromey
  2023-08-03 14:04     ` Tom de Vries
  0 siblings, 1 reply; 19+ messages in thread
From: Tom Tromey @ 2023-08-02 19:37 UTC (permalink / raw)
  To: Tom de Vries via Gdb-patches; +Cc: Tom de Vries

>>>>> "Tom" == Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> writes:

Tom> The race is between:
Tom> - a worker thread calling main_name (), and in the process reading
Tom>   bfd_last_cache, and
Tom> - the main thread writing to bfd_last_cache.

Tom> Fix this by calling main_name () from the main thread.

It seems to me that index-write.c should not be calling main_name.

main_name might iterate over objfiles, but writing when writing an index
for a specific objfile, only that objfile's notion of "main" should be
considered.

Instead maybe this code can just query the cooked index directly.

Tom

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2 5/6] [gdb/symtab] Fix data race on dwarf2_per_cu_data::{m_header_read_in, is_debug_type}
  2023-08-02  9:53 ` [PATCH v2 5/6] [gdb/symtab] Fix data race on dwarf2_per_cu_data::{m_header_read_in,is_debug_type} Tom de Vries
@ 2023-08-02 19:39   ` Tom Tromey
  0 siblings, 0 replies; 19+ messages in thread
From: Tom Tromey @ 2023-08-02 19:39 UTC (permalink / raw)
  To: Tom de Vries via Gdb-patches; +Cc: Tom de Vries

>>>>> "Tom" == Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> writes:

Tom> The race is between:
Tom> - a worker thread writing the index cache, and in the process reading
Tom>    dwarf2_per_cu_data::is_debug_type, and
Tom> - the main thread writing to dwarf2_per_cu_data::m_header_read_in.

Tom> The two bitfields dwarf2_per_cu_data::m_header_read_in and
Tom> dwarf2_per_cu_data::is_debug_type share the same bitfield container.

Tom> Fix this by making dwarf2_per_cu_data::m_header_read_in a packed<bool, 1>.

Thanks.  This is ok.

Approved-By: Tom Tromey <tom@tromey.com>

FWIW I once tried to clean up the madness surrounding m_header_read_in
and CU header reading in general.  Unfortunately I did not succeed... I
don't recall why but I think with DWO the header might have to be from
the DWO.  This is all kind of lame, because when doing the initial
cooked index scan, the headers have to be read, and so it seems like the
dwarf2_per_cu_data could be "more read-only" after that.

Tom

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2 6/6] [gdb/testsuite] Extend gdb.base/index-cache.exp
  2023-08-02  9:53 ` [PATCH v2 6/6] [gdb/testsuite] Extend gdb.base/index-cache.exp Tom de Vries
@ 2023-08-02 19:41   ` Tom Tromey
  0 siblings, 0 replies; 19+ messages in thread
From: Tom Tromey @ 2023-08-02 19:41 UTC (permalink / raw)
  To: Tom de Vries via Gdb-patches; +Cc: Tom de Vries

>>>>> "Tom" == Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> writes:

Tom> The test-case gdb.base/index-cache.exp uses only one source file, which
Tom> contains main.

Tom> While doing "file $exec", in set_initial_language a symbol lookup of "main" is
Tom> done, causing the symtab containing main to be expanded.

Tom> Handling of main is special, and a future optimization may skip the lookup and
Tom> expansion.

Tom> Reliably exercise:
Tom> - the lookup of main, expanding the symtab containing main, by doing
Tom>   "ptype main", and
Tom> - the lookup of another symbol, expanding a symtab not containing main, by:
Tom>   - adding another source file containing function foo, and
Tom>   - doing "ptype foo".

Thanks.

Approved-By: Tom Tromey <tom@tromey.com>

Tom> This triggered a segfault with target board native-extended-gdbserver, filed
Tom> as PR symtab/30712, but that seems to be fixed by a previous commit in this
Tom> series.

I guess something here needs needs a "Bug:" trailer, it's fine by me if
it is this patch.

Tom

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2 0/6] [gdb/symtab] Fix data-races in gdb.base/index-cache.exp
  2023-08-02  9:52 [PATCH v2 0/6] [gdb/symtab] Fix data-races in gdb.base/index-cache.exp Tom de Vries
                   ` (5 preceding siblings ...)
  2023-08-02  9:53 ` [PATCH v2 6/6] [gdb/testsuite] Extend gdb.base/index-cache.exp Tom de Vries
@ 2023-08-02 19:44 ` Tom Tromey
  2023-08-04  0:14   ` Tom de Vries
  6 siblings, 1 reply; 19+ messages in thread
From: Tom Tromey @ 2023-08-02 19:44 UTC (permalink / raw)
  To: Tom de Vries via Gdb-patches; +Cc: Tom de Vries

>>>>> "Tom" == Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> writes:

Thanks for this series.  I read through it and sent a few notes.

Tom> There's one more patch like that, I checked using pahole that the struct size
Tom> is not increased.

"ptype/o" is basically pahole FWIW.

I tend to think that in most cases, the size of objects doesn't really
matter.  I mean, obviously we don't want to bloat them unnecessarily,
but for something like this, I just wouldn't worry much... and if we did
care there's probably some other way we could shrink them than worrying
about packing.

In the olden days pretty much the only thing really worth worrying about
was partial symbols.  Now I guess it would be cooked_index_entry.

Tom

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2 4/6] [gdb/symtab] Fix data race on bfd_last_cache
  2023-08-02 19:37   ` Tom Tromey
@ 2023-08-03 14:04     ` Tom de Vries
  0 siblings, 0 replies; 19+ messages in thread
From: Tom de Vries @ 2023-08-03 14:04 UTC (permalink / raw)
  To: Tom Tromey, Tom de Vries via Gdb-patches

On 8/2/23 21:37, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Tom> The race is between:
> Tom> - a worker thread calling main_name (), and in the process reading
> Tom>   bfd_last_cache, and
> Tom> - the main thread writing to bfd_last_cache.
> 
> Tom> Fix this by calling main_name () from the main thread.
> 
> It seems to me that index-write.c should not be calling main_name.
> 
> main_name might iterate over objfiles, but writing when writing an index
> for a specific objfile, only that objfile's notion of "main" should be
> considered.
> 
> Instead maybe this code can just query the cooked index directly.

I looked into this, and while investigating came to the conclusion that 
the use of main_name () is unnecessary.  I've submitted a patch ( 
https://sourceware.org/pipermail/gdb-patches/2023-August/201322.html ) 
to remove it.

Thanks,
- Tom


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2 1/6] [gdb/symtab] Fix data race on index_cache::m_enabled
  2023-08-02 19:29   ` Tom Tromey
@ 2023-08-04  0:08     ` Tom de Vries
  0 siblings, 0 replies; 19+ messages in thread
From: Tom de Vries @ 2023-08-04  0:08 UTC (permalink / raw)
  To: Tom Tromey, Tom de Vries via Gdb-patches

On 8/2/23 21:29, 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:
> [race]
> 
> Tom> Fix this by capturing the value of index_cache::m_enabled in the main thread,
> Tom> and using the captured value in the worker thread.
> 
> Thanks for the patch.  I have some stylistic nits but otherwise it seems
> fine to me.
> 
> Tom> +  struct index_cache_store_context ctx (global_index_cache);
> 
> I think we tend to avoid the struct/class keyword in cases like this now.
> 
> Tom> +				 const struct index_cache_store_context &ctx)
> 
> ... basically everywhere.
> 
> Tom> +index_cache_store_context::index_cache_store_context (const index_cache &ic)
> Tom> +{
> Tom> +  m_enabled = ic.enabled ();
> 
> It's better to use the initializer syntax like
> 
>    : m_enabled (ic.enabled ())
> 
> This can be in the header if possible (I didn't try).
> 
> Tom> +struct index_cache_store_context
> Tom> +{
> Tom> +  friend class index_cache;
> Tom> +
> Tom> +  index_cache_store_context(const index_cache &ic);
> 
> Single-argument constructors should be 'explicit' as a rule.

Ack, fixed in a v3 version ( 
https://sourceware.org/pipermail/gdb-patches/2023-August/201339.html ).

Thanks,
- Tom


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2 2/6] [gdb/symtab] Fix data race on bfd::{cacheable,format}
  2023-08-02 19:32   ` Tom Tromey
@ 2023-08-04  0:09     ` Tom de Vries
  2023-08-04 15:57       ` Tom Tromey
  0 siblings, 1 reply; 19+ messages in thread
From: Tom de Vries @ 2023-08-04  0:09 UTC (permalink / raw)
  To: Tom Tromey, Tom de Vries via Gdb-patches

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


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2 0/6] [gdb/symtab] Fix data-races in gdb.base/index-cache.exp
  2023-08-02 19:44 ` [PATCH v2 0/6] [gdb/symtab] Fix data-races in gdb.base/index-cache.exp Tom Tromey
@ 2023-08-04  0:14   ` Tom de Vries
  0 siblings, 0 replies; 19+ messages in thread
From: Tom de Vries @ 2023-08-04  0:14 UTC (permalink / raw)
  To: Tom Tromey, Tom de Vries via Gdb-patches

On 8/2/23 21:44, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Thanks for this series.  I read through it and sent a few notes.
> 
> Tom> There's one more patch like that, I checked using pahole that the struct size
> Tom> is not increased.
> 
> "ptype/o" is basically pahole FWIW.
> 
> I tend to think that in most cases, the size of objects doesn't really
> matter.  I mean, obviously we don't want to bloat them unnecessarily,
> but for something like this, I just wouldn't worry much... and if we did
> care there's probably some other way we could shrink them than worrying
> about packing.
> 
> In the olden days pretty much the only thing really worth worrying about
> was partial symbols.  Now I guess it would be cooked_index_entry.

Ack.

I've submitted a v3, following up on comments and dropping the patch 
that's no longer required.

I'll commit tomorrow unless there are further comments.

Thanks,
- Tom


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2 2/6] [gdb/symtab] Fix data race on bfd::{cacheable,format}
  2023-08-04  0:09     ` Tom de Vries
@ 2023-08-04 15:57       ` Tom Tromey
  0 siblings, 0 replies; 19+ messages in thread
From: Tom Tromey @ 2023-08-04 15:57 UTC (permalink / raw)
  To: Tom de Vries; +Cc: Tom Tromey, Tom de Vries via Gdb-patches

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2023-08-04 15:57 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-08-02  9:52 [PATCH v2 0/6] [gdb/symtab] Fix data-races in gdb.base/index-cache.exp Tom de Vries
2023-08-02  9:53 ` [PATCH v2 1/6] [gdb/symtab] Fix data race on index_cache::m_enabled Tom de Vries
2023-08-02 19:29   ` Tom Tromey
2023-08-04  0:08     ` Tom de Vries
2023-08-02  9:53 ` [PATCH v2 2/6] [gdb/symtab] Fix data race on bfd::{cacheable,format} Tom de Vries
2023-08-02 19:32   ` Tom Tromey
2023-08-04  0:09     ` Tom de Vries
2023-08-04 15:57       ` Tom Tromey
2023-08-02  9:53 ` [PATCH v2 3/6] [gdb/symtab] Fix race on dwarf2_per_cu_data::{queued,is_debug_type} Tom de Vries
2023-08-02 19:34   ` [PATCH v2 3/6] [gdb/symtab] Fix race on dwarf2_per_cu_data::{queued, is_debug_type} Tom Tromey
2023-08-02  9:53 ` [PATCH v2 4/6] [gdb/symtab] Fix data race on bfd_last_cache Tom de Vries
2023-08-02 19:37   ` Tom Tromey
2023-08-03 14:04     ` Tom de Vries
2023-08-02  9:53 ` [PATCH v2 5/6] [gdb/symtab] Fix data race on dwarf2_per_cu_data::{m_header_read_in,is_debug_type} Tom de Vries
2023-08-02 19:39   ` [PATCH v2 5/6] [gdb/symtab] Fix data race on dwarf2_per_cu_data::{m_header_read_in, is_debug_type} Tom Tromey
2023-08-02  9:53 ` [PATCH v2 6/6] [gdb/testsuite] Extend gdb.base/index-cache.exp Tom de Vries
2023-08-02 19:41   ` Tom Tromey
2023-08-02 19:44 ` [PATCH v2 0/6] [gdb/symtab] Fix data-races in gdb.base/index-cache.exp Tom Tromey
2023-08-04  0:14   ` Tom de Vries

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).