public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
From: Andrew Burgess <aburgess@redhat.com>
To: gdb-patches@sourceware.org
Cc: Andrew Burgess <aburgess@redhat.com>,
	Richard Bunt <richard.bunt@linaro.org>,
	Tom Tromey <tom@tromey.com>
Subject: [PUSHED] gdb: fix libstdc++ assert caused by invalid use of std::clamp
Date: Wed,  6 Dec 2023 11:26:33 +0000	[thread overview]
Message-ID: <5a22e042e41db962cd6a79cd59cab46cbbe58a98.1701861952.git.aburgess@redhat.com> (raw)
In-Reply-To: <20231124161321.572511-3-richard.bunt@linaro.org>

After this commit:

  commit 33ae45434d0ab1f7de365b9140ad4e4ffc34b8a2
  Date:   Mon Dec 4 14:23:17 2023 +0000

      gdb: Enable early init of thread pool size

I am now seeing this assert from libstdc++:

  /usr/include/c++/9/bits/stl_algo.h:3715: constexpr const _Tp& std::clamp(const _Tp&, const _Tp&, const _Tp&) [with _Tp = int]: Assertion '!(__hi < __lo)' failed.

This may only be visible because I compile with:

  -D_GLIBCXX_DEBUG=1 -D_GLIBCXX_DEBUG_PEDANTIC=1

but I haven't checked.  The issue the assert is highlighting is real,
and is caused by this block of code:

  if (n_threads < 0)
    {
      const int hardware_threads = std::thread::hardware_concurrency ();
      /* Testing in #29959 indicates that parallel efficiency drops between
         n_threads=5 to 8.  Therefore, clamp the default value to 8 to avoid an
         excessive number of threads in the pool on many-core systems.  */
      const int throttle = 8;
      n_threads = std::clamp (hardware_threads, hardware_threads, throttle);
    }

The arguments to std::clamp are VALUE, LOW, HIGH, but in the above, if
we have more than 8 hardware threads available the LOW will be greater
than the HIGH, which is triggering the assert I see above.

I believe std::clamp is the wrong tool to use here.  Instead std::min
would be a better choice; we want the smaller value of
HARDWARE_THREADS or THROTTLE.  If h/w threads is 2, then we want 2,
but if h/w threads is 16 we want 8, this is what std::min gives us.

After this commit, I no longer see the assert.
---
 gdb/maint.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/gdb/maint.c b/gdb/maint.c
index 2e6754c9558..68b70bf403d 100644
--- a/gdb/maint.c
+++ b/gdb/maint.c
@@ -855,11 +855,12 @@ update_thread_pool_size ()
   if (n_threads < 0)
     {
       const int hardware_threads = std::thread::hardware_concurrency ();
-      /* Testing in #29959 indicates that parallel efficiency drops between
-	 n_threads=5 to 8.  Therefore, clamp the default value to 8 to avoid an
-	 excessive number of threads in the pool on many-core systems.  */
-      const int throttle = 8;
-      n_threads = std::clamp (hardware_threads, hardware_threads, throttle);
+      /* Testing in PR gdb/29959 indicates that parallel efficiency drops
+	 between n_threads=5 to 8.  Therefore, use no more than 8 threads
+	 to avoid an excessive number of threads in the pool on many-core
+	 systems.  */
+      const int max_thread_count = 8;
+      n_threads = std::min (hardware_threads, max_thread_count);
     }
 
   gdb::thread_pool::g_thread_pool->set_thread_count (n_threads);

base-commit: b17ef9dcd8d16eedf4e60565cd7701698b5a0b6b
-- 
2.25.4


  reply	other threads:[~2023-12-06 11:28 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-11-24 16:13 [PATCH v4] gdb: Enable early init of thread pool size Richard Bunt
2023-11-24 16:13 ` [PATCH v4 1/2] gdb: install CLI uiout while processing early init files Richard Bunt
2023-11-24 16:13 ` [PATCH v4 2/2] gdb: Enable early init of thread pool size Richard Bunt
2023-12-06 11:26   ` Andrew Burgess [this message]
2023-11-27 20:23 ` [PATCH v4] " Tom Tromey
2023-12-05  7:56   ` Tom de Vries
2023-12-05  9:35     ` Richard Bunt
2023-12-05 13:10     ` Richard Bunt
2023-12-05 14:39       ` Tom Tromey

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=5a22e042e41db962cd6a79cd59cab46cbbe58a98.1701861952.git.aburgess@redhat.com \
    --to=aburgess@redhat.com \
    --cc=gdb-patches@sourceware.org \
    --cc=richard.bunt@linaro.org \
    --cc=tom@tromey.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).