[PATCH 1/2] gdb: lock complaint_mutex in clear_complaints

[PATCH 1/2] gdb: lock complaint_mutex in clear_complaints

[simon.marchi]

From: Simon Marchi <simon.marchi@polymtl.ca>

If I add a dummy complaint like

    complaint (_("I am a complaint"));

in cooked_index_worker_debug_info::process_unit, and then load a file
under a GDB built with ThreadSanitizer, I get this (trimmed for
readability):

    WARNING: ThreadSanitizer: data race (pid=497507)
      Read of size 4 at 0x7208000004c8 by thread T4 (mutexes: write M0):
        #0 std::pair<__gnu_debug::_Safe_iterator<__gnu_cxx::__normal_iterator<std::pair<char const*, int>*, std::__cxx1998::vector<std::pair<char const*, int>, std::allocator<std::pair<char const*, int> > > >, std::__debug::vector<std::pair<char const*, int>, std::allocator<std::pair<char const*, int> > >, std::random_access_iterator_tag>, bool> ankerl::unordered_dense::v4_8_0::detail::table<char const*, int, ankerl::unordered_dense::v4_8_0::hash<char const*, void>, std::equal_to<char const*>, std::allocator<std::pair<char const*, int> >, ankerl::unordered_dense::v4_8_0::bucket_type::standard, ankerl::unordered_dense::v4_8_0::detail::default_container_t, false>::do_try_emplace<char const* const&>(char const* const&) /home/simark/src/binutils-gdb/gdb/../gdbsupport/unordered_dense/unordered_dense.h:1227 (gdb+0xf0fd75) (BuildId: 5d682ab96882c738940aae3c2c67270d969f113f)
        #1 std::pair<__gnu_debug::_Safe_iterator<__gnu_cxx::__normal_iterator<std::pair<char const*, int>*, std::__cxx1998::vector<std::pair<char const*, int>, std::allocator<std::pair<char const*, int> > > >, std::__debug::vector<std::pair<char const*, int>, std::allocator<std::pair<char const*, int> > >, std::random_access_iterator_tag>, bool> ankerl::unordered_dense::v4_8_0::detail::table<char const*, int, ankerl::unordered_dense::v4_8_0::hash<char const*, void>, std::equal_to<char const*>, std::allocator<std::pair<char const*, int> >, ankerl::unordered_dense::v4_8_0::bucket_type::standard, ankerl::unordered_dense::v4_8_0::detail::default_container_t, false>::try_emplace<, int, true>(char const* const&) /home/simark/src/binutils-gdb/gdb/../gdbsupport/unordered_dense/unordered_dense.h:1701 (gdb+0xf0ec50) (BuildId: 5d682ab96882c738940aae3c2c67270d969f113f)
        #2 int& ankerl::unordered_dense::v4_8_0::detail::table<char const*, int, ankerl::unordered_dense::v4_8_0::hash<char const*, void>, std::equal_to<char const*>, std::allocator<std::pair<char const*, int> >, ankerl::unordered_dense::v4_8_0::bucket_type::standard, ankerl::unordered_dense::v4_8_0::detail::default_container_t, false>::operator[]<int, true>(char const* const&) /home/simark/src/binutils-gdb/gdb/../gdbsupport/unordered_dense/unordered_dense.h:1926 (gdb+0xf0e3e2) (BuildId: 5d682ab96882c738940aae3c2c67270d969f113f)
        #3 complaint_internal(char const*, ...) /home/simark/src/binutils-gdb/gdb/complaints.c:50 (gdb+0xf0abc6) (BuildId: 5d682ab96882c738940aae3c2c67270d969f113f)
        #4 cooked_index_worker_debug_info::process_unit(dwarf2_per_cu*, dwarf2_per_objfile*, cooked_index_worker_result*) /home/simark/src/binutils-gdb/gdb/dwarf2/read.c:3162 (gdb+0x11d741c) (BuildId: 5d682ab96882c738940aae3c2c67270d969f113f)
        #5 cooked_index_worker_debug_info::parallel_indexing_worker::process_one(dwarf2_per_cu&)::{lambda()#1}::operator()() const /home/simark/src/binutils-gdb/gdb/dwarf2/read.c:3068 (gdb+0x122b195) (BuildId: 5d682ab96882c738940aae3c2c67270d969f113f)
        #6 void cooked_index_worker_result::catch_error<cooked_index_worker_debug_info::parallel_indexing_worker::process_one(dwarf2_per_cu&)::{lambda()#1}>(cooked_index_worker_debug_info::parallel_indexing_worker::process_one(dwarf2_per_cu&)::{lambda()#1}&&) /home/simark/src/binutils-gdb/gdb/dwarf2/cooked-index-worker.h:122 (gdb+0x1236454) (BuildId: 5d682ab96882c738940aae3c2c67270d969f113f)
        #7 cooked_index_worker_debug_info::parallel_indexing_worker::process_one(dwarf2_per_cu&) /home/simark/src/binutils-gdb/gdb/dwarf2/read.c:3066 (gdb+0x122b22d) (BuildId: 5d682ab96882c738940aae3c2c67270d969f113f)
        #8 cooked_index_worker_debug_info::parallel_indexing_worker::operator()(iterator_range<std::unique_ptr<dwarf2_per_cu, dwarf2_per_cu_deleter>*>) /home/simark/src/binutils-gdb/gdb/dwarf2/read.c:3060 (gdb+0x122b0a3) (BuildId: 5d682ab96882c738940aae3c2c67270d969f113f)
        ...

      Previous write of size 8 at 0x7208000004c8 by main thread:
        #0 __tsan_memset <null> (libtsan.so.2+0x95e66) (BuildId: b5c99e8ceaf9098eb9a01fcfcc35ece8603116df)
        #1 ankerl::unordered_dense::v4_8_0::detail::table<char const*, int, ankerl::unordered_dense::v4_8_0::hash<char const*, void>, std::equal_to<char const*>, std::allocator<std::pair<char const*, int> >, ankerl::unordered_dense::v4_8_0::bucket_type::standard, ankerl::unordered_dense::v4_8_0::detail::default_container_t, false>::clear_buckets() /home/simark/src/binutils-gdb/gdb/../gdbsupport/unordered_dense/unordered_dense.h:1116 (gdb+0xf0edd6) (BuildId: 5d682ab96882c738940aae3c2c67270d969f113f)
        #2 ankerl::unordered_dense::v4_8_0::detail::table<char const*, int, ankerl::unordered_dense::v4_8_0::hash<char const*, void>, std::equal_to<char const*>, std::allocator<std::pair<char const*, int> >, ankerl::unordered_dense::v4_8_0::bucket_type::standard, ankerl::unordered_dense::v4_8_0::detail::default_container_t, false>::clear() /home/simark/src/binutils-gdb/gdb/../gdbsupport/unordered_dense/unordered_dense.h:1494 (gdb+0xf0e479) (BuildId: 5d682ab96882c738940aae3c2c67270d969f113f)
        #3 clear_complaints() /home/simark/src/binutils-gdb/gdb/complaints.c:74 (gdb+0xf0adbf) (BuildId: 5d682ab96882c738940aae3c2c67270d969f113f)
        #4 finish_new_objfile /home/simark/src/binutils-gdb/gdb/symfile.c:986 (gdb+0x1a2c65d) (BuildId: 5d682ab96882c738940aae3c2c67270d969f113f)
        #5 symbol_file_add_with_addrs /home/simark/src/binutils-gdb/gdb/symfile.c:1111 (gdb+0x1a2cdde) (BuildId: 5d682ab96882c738940aae3c2c67270d969f113f)
        #6 symbol_file_add_from_bfd(gdb::ref_ptr<bfd, gdb_bfd_ref_policy> const&, char const*, enum_flags<symfile_add_flag>, std::__debug::vector<other_sections, std::allocator<other_sections> >*, enum_flags<objfile_flag>, objfile*) /home/simark/src/binutils-gdb/gdb/symfile.c:1148 (gdb+0x1a2cfab) (BuildId: 5d682ab96882c738940aae3c2c67270d969f113f)
        #7 symbol_file_add(char const*, enum_flags<symfile_add_flag>, std::__debug::vector<other_sections, std::allocator<other_sections> >*, enum_flags<objfile_flag>) /home/simark/src/binutils-gdb/gdb/symfile.c:1161 (gdb+0x1a2d03a) (BuildId: 5d682ab96882c738940aae3c2c67270d969f113f)
        #8 symbol_file_add_main_1 /home/simark/src/binutils-gdb/gdb/symfile.c:1185 (gdb+0x1a2d1a5) (BuildId: 5d682ab96882c738940aae3c2c67270d969f113f)
        #9 symbol_file_command(char const*, int) /home/simark/src/binutils-gdb/gdb/symfile.c:1615 (gdb+0x1a2ed10) (BuildId: 5d682ab96882c738940aae3c2c67270d969f113f)
        #10 file_command /home/simark/src/binutils-gdb/gdb/exec.c:580 (gdb+0x1305356) (BuildId: 5d682ab96882c738940aae3c2c67270d969f113f)
        ...

      Location is heap block of size 32 at 0x7208000004c0 allocated by main thread:
        ...
        #11 __static_initialization_and_destruction_0 /home/simark/src/binutils-gdb/gdb/complaints.c:31 (gdb+0xf0df04) (BuildId: 5d682ab96882c738940aae3c2c67270d969f113f)
        ...

      Mutex M0 (0x558908cdbc20) created at:
        #0 pthread_mutex_lock <null> (libtsan.so.2+0x60b5a) (BuildId: b5c99e8ceaf9098eb9a01fcfcc35ece8603116df)
        #1 __gthread_mutex_lock(pthread_mutex_t*) /usr/include/c++/16.1.1/x86_64-pc-linux-gnu/bits/gthr-default.h:795 (gdb+0xf0dfa6) (BuildId: 5d682ab96882c738940aae3c2c67270d969f113f)
        #2 std::mutex::lock() /usr/include/c++/16.1.1/bits/std_mutex.h:116 (gdb+0xf0dfa6)
        #3 std::lock_guard<std::mutex>::lock_guard(std::mutex&) /usr/include/c++/16.1.1/bits/std_mutex.h:276 (gdb+0xf0e1f0) (BuildId: 5d682ab96882c738940aae3c2c67270d969f113f)
        #4 complaint_internal(char const*, ...) /home/simark/src/binutils-gdb/gdb/complaints.c:49 (gdb+0xf0abad) (BuildId: 5d682ab96882c738940aae3c2c67270d969f113f)
        #5 cooked_index_worker_debug_info::process_unit(dwarf2_per_cu*, dwarf2_per_objfile*, cooked_index_worker_result*) /home/simark/src/binutils-gdb/gdb/dwarf2/read.c:3162 (gdb+0x11d741c) (BuildId: 5d682ab96882c738940aae3c2c67270d969f113f)
        #6 cooked_index_worker_debug_info::parallel_indexing_worker::process_one(dwarf2_per_cu&)::{lambda()#1}::operator()() const /home/simark/src/binutils-gdb/gdb/dwarf2/read.c:3068 (gdb+0x122b195) (BuildId: 5d682ab96882c738940aae3c2c67270d969f113f)
        #7 void cooked_index_worker_result::catch_error<cooked_index_worker_debug_info::parallel_indexing_worker::process_one(dwarf2_per_cu&)::{lambda()#1}>(cooked_index_worker_debug_info::parallel_indexing_worker::process_one(dwarf2_per_cu&)::{lambda()#1}&&) /home/simark/src/binutils-gdb/gdb/dwarf2/cooked-index-worker.h:122 (gdb+0x1236454) (BuildId: 5d682ab96882c738940aae3c2c67270d969f113f)
        #8 cooked_index_worker_debug_info::parallel_indexing_worker::process_one(dwarf2_per_cu&) /home/simark/src/binutils-gdb/gdb/dwarf2/read.c:3066 (gdb+0x122b22d) (BuildId: 5d682ab96882c738940aae3c2c67270d969f113f)
        #9 cooked_index_worker_debug_info::parallel_indexing_worker::operator()(iterator_range<std::unique_ptr<dwarf2_per_cu, dwarf2_per_cu_deleter>*>) /home/simark/src/binutils-gdb/gdb/dwarf2/read.c:3060 (gdb+0x122b0a3) (BuildId: 5d682ab96882c738940aae3c2c67270d969f113f)
        ...

This points to clear_complaints touching the global counters map in the
main thread without holding a lock, while a background thread touched
the map in a worker thread while holding the lock complaint_mutex.

Fix this by holding the complaint_mutex lock in clear_complaints.

Change-Id: Id2fe442486bcdf57156aacdc9253055702c07600
---
 gdb/complaints.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/gdb/complaints.c b/gdb/complaints.c
index 4a2318a7f805..ab6e2049685c 100644
--- a/gdb/complaints.c
+++ b/gdb/complaints.c
@@ -71,6 +71,7 @@ complaint_internal (const char *fmt, ...)
 void
 clear_complaints ()
 {
+  gdb::lock_guard<gdb::mutex> guard (complaint_mutex);
   counters.clear ();
 }
 

base-commit: 2bf7c41602d9da7dac7616aabb78da32c99dcc83
-- 
2.54.0

[PATCH 2/2] gdb: remove complaint_interceptor::g_complaint_interceptor

[simon.marchi]

From: Simon Marchi <simon.marchi@polymtl.ca>

The thread_local g_complaint_interceptor pointer is unnecessary.  The
complaint_interceptor constructor registers itself as the warning hook
via m_saved_warning_hook (this), so when complaint_internal dispatches
through the warning hook, it lands in complaint_interceptor::warn with
'this' already pointing at the registered interceptor.  Inside warn,
g_complaint_interceptor and 'this' always refer to the same object.

Replace g_complaint_interceptor->m_complaints with m_complaints in
complaint_interceptor::warn and remove g_complaint_interceptor.

Change-Id: I75565a5f2c0e51363f36be0e3544210c10bb5491
---
 gdb/complaints.c | 9 ++-------
 gdb/complaints.h | 7 -------
 2 files changed, 2 insertions(+), 14 deletions(-)

diff --git a/gdb/complaints.c b/gdb/complaints.c
index ab6e2049685c..e3d68a869c99 100644
--- a/gdb/complaints.c
+++ b/gdb/complaints.c
@@ -77,13 +77,8 @@ clear_complaints ()
 
 /* See complaints.h.  */
 
-thread_local complaint_interceptor *complaint_interceptor::g_complaint_interceptor;
-
-/* See complaints.h.  */
-
 complaint_interceptor::complaint_interceptor ()
-  : m_saved_complaint_interceptor (&g_complaint_interceptor, this),
-    m_saved_warning_hook (this)
+  : m_saved_warning_hook (this)
 {
 }
 
@@ -122,7 +117,7 @@ void
 complaint_interceptor::warn (const char *fmt, va_list args)
 {
   gdb::lock_guard<gdb::mutex> guard (complaint_mutex);
-  g_complaint_interceptor->m_complaints.insert (string_vprintf (fmt, args));
+  m_complaints.insert (string_vprintf (fmt, args));
 }
 
 static void
diff --git a/gdb/complaints.h b/gdb/complaints.h
index c607194e265e..8f5cf24c1c9b 100644
--- a/gdb/complaints.h
+++ b/gdb/complaints.h
@@ -20,7 +20,6 @@
 #ifndef GDB_COMPLAINTS_H
 #define GDB_COMPLAINTS_H
 
-#include "gdbsupport/scoped_restore.h"
 #include "gdbsupport/unordered_set.h"
 
 /* Helper for complaint.  */
@@ -89,17 +88,11 @@ class complaint_interceptor final : public warning_hook_handler_type
   /* The issued complaints.  */
   complaint_collection m_complaints;
 
-  /* The saved value of g_complaint_interceptor.  */
-  scoped_restore_tmpl<complaint_interceptor *> m_saved_complaint_interceptor;
-
   /* A helper function that is used by the 'complaint' implementation
      to issue a complaint.  */
   void warn (const char *, va_list) override
     ATTRIBUTE_PRINTF (2, 0);
 
-  /* This object.  Used by the static callback function.  */
-  static thread_local complaint_interceptor *g_complaint_interceptor;
-
   /* Object to initialise the warning hook.  */
   scoped_restore_warning_hook m_saved_warning_hook;
 };
-- 
2.54.0

Re: [PATCH 1/2] gdb: lock complaint_mutex in clear_complaints

[Tom Tromey]

>>>>> "Simon" == simon marchi <simon.marchi@polymtl.ca> writes:

Simon> This points to clear_complaints touching the global counters map in the
Simon> main thread without holding a lock, while a background thread touched
Simon> the map in a worker thread while holding the lock complaint_mutex.

Simon> Fix this by holding the complaint_mutex lock in clear_complaints.

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

Tom

Re: [PATCH 2/2] gdb: remove complaint_interceptor::g_complaint_interceptor

[Tom Tromey]

>>>>> "Simon" == simon marchi <simon.marchi@polymtl.ca> writes:

Simon> From: Simon Marchi <simon.marchi@polymtl.ca>
Simon> The thread_local g_complaint_interceptor pointer is unnecessary.  The
Simon> complaint_interceptor constructor registers itself as the warning hook
Simon> via m_saved_warning_hook (this), so when complaint_internal dispatches
Simon> through the warning hook, it lands in complaint_interceptor::warn with
Simon> 'this' already pointing at the registered interceptor.  Inside warn,
Simon> g_complaint_interceptor and 'this' always refer to the same object.

Simon> Replace g_complaint_interceptor->m_complaints with m_complaints in
Simon> complaint_interceptor::warn and remove g_complaint_interceptor.

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

I don't remember the code too well but I do wonder if we should just
have each set of complaints be completely independent and emit them all
at some spot guaranteed to be on the main thread.  Like, instead of
'complaint' function, only expose a method on some object.

OTOH I don't think we should spend any more time on complaints than is
strictly necessary.

Tom

Re: [PATCH 2/2] gdb: remove complaint_interceptor::g_complaint_interceptor

[Simon Marchi]

On 5/21/26 10:34 AM, Tom Tromey wrote:
>>>>>> "Simon" == simon marchi <simon.marchi@polymtl.ca> writes:
> 
> Simon> From: Simon Marchi <simon.marchi@polymtl.ca>
> Simon> The thread_local g_complaint_interceptor pointer is unnecessary.  The
> Simon> complaint_interceptor constructor registers itself as the warning hook
> Simon> via m_saved_warning_hook (this), so when complaint_internal dispatches
> Simon> through the warning hook, it lands in complaint_interceptor::warn with
> Simon> 'this' already pointing at the registered interceptor.  Inside warn,
> Simon> g_complaint_interceptor and 'this' always refer to the same object.
> 
> Simon> Replace g_complaint_interceptor->m_complaints with m_complaints in
> Simon> complaint_interceptor::warn and remove g_complaint_interceptor.
> 
> Ok.
> Approved-By: Tom Tromey <tom@tromey.com>

Thanks, pushed both patches.

> I don't remember the code too well but I do wonder if we should just
> have each set of complaints be completely independent and emit them all
> at some spot guaranteed to be on the main thread.  Like, instead of
> 'complaint' function, only expose a method on some object.

I'm afraid I don't understand your suggestion.

> OTOH I don't think we should spend any more time on complaints than is
> strictly necessary.

Agreed.

Simon