[PATCH v4] gdb: Enable early init of thread pool size

[PATCH v4] gdb: Enable early init of thread pool size

[Richard Bunt]

Updates in v4.

Clamped the default number of worker threads to eight. I'm ok with this as GDB
can always be configured to use a higher number of work threads if the use case
demands it.

I have included Andrew's patch in the set as they'll need to be pushed
together.  It has the following adjustments over the original version

* Commit message nit: nullput -> nullptr

* Rebased to trunk and swapped to std:make_unique.

* I'm not quite sure on the etiquette here but: Added tested by git trailer.

As always, happy to work on any further comments.

[PATCH v4 1/2] gdb: install CLI uiout while processing early init files

[Richard Bunt]

From: Andrew Burgess <aburgess@redhat.com>

The next commit wants to use a 'show' command within an early
initialisation file, despite these commands not being in the list of
acceptable commands for use within an early initialisation file.

The problem we run into is that the early initialisation files are
processed before GDB has installed the top level interpreter.  The
interpreter is responsible to installing the default uiout (accessed
through current_uiout), and as a result code that depends on
uiout (e.g. 'show' commands) will end up dereferencing a nullptr, and
crashing GDB.

I did consider moving the interpreter installation before the early
initialisation, and this would work fine except for the new DAP
interpreter, which relies on having Python available during its
initialisation.  Which means we can't install the interpreter until
after Python has been initialised, and the early initialisation
handling has to occur before Python is setup -- that's the whole point
of this feature (to allow customisation of how Python is setup).

So, what I propose is that early within captured_main_1, we install a
temporary cli_ui_out as the current_uiout.  This will remain in place
until the top-level interpreter is installed, at which point the
temporary will be replaced.

What this means is that current_uiout will no longer be nullptr,
instead, any commands within an early initialisation file that trigger
output, will perform that output in a CLI style.

I propose that we don't update the documentation for early
initialisation files, we leave the user advice as being only 'set' and
'source' commands are acceptable.  But now, if a user does try a
'show' command, then instead of crashing, GDB will do something
predictable.

I've not added a test in this commit.  The next commit relies on this
patch and will serve as a test.

Tested-By: Richard Bunt <richard.bunt@linaro.org>

---
 gdb/main.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/gdb/main.c b/gdb/main.c
index 486c1ffe071..eb11d6f2e46 100644
--- a/gdb/main.c
+++ b/gdb/main.c
@@ -56,6 +56,7 @@
 #include "gdbsupport/alt-stack.h"
 #include "observable.h"
 #include "serial.h"
+#include "cli-out.h"
 
 /* The selected interpreter.  */
 std::string interpreter_p;
@@ -688,6 +689,16 @@ captured_main_1 (struct captured_main_args *context)
   gdb_stdtargerr = gdb_stderr;
   gdb_stdtargin = gdb_stdin;
 
+  /* Put a CLI based uiout in place early.  If the early initialization
+     files trigger any I/O then it isn't hard to reach parts of GDB that
+     assume current_uiout is not nullptr.  Maybe we should just install the
+     CLI interpreter initially, then switch to the application requested
+     interpreter later?  But that would (potentially) result in an
+     interpreter being instantiated "just in case".  For now this feels
+     like the least effort way to protect GDB from crashing.  */
+  auto temp_uiout = std::make_unique<cli_ui_out> (gdb_stdout);
+  current_uiout = temp_uiout.get ();
+
   if (bfd_init () != BFD_INIT_MAGIC)
     error (_("fatal error: libbfd ABI mismatch"));
 
@@ -1142,6 +1153,10 @@ captured_main_1 (struct captured_main_args *context)
      look at things by now.  Initialize the default interpreter.  */
   set_top_level_interpreter (interpreter_p.c_str ());
 
+  /* The interpreter should have installed the real uiout by now.  */
+  gdb_assert (current_uiout != temp_uiout.get ());
+  temp_uiout = nullptr;
+
   if (!quiet)
     {
       /* Print all the junk at the top, with trailing "..." if we are
-- 
2.32.0

[PATCH v4 2/2] gdb: Enable early init of thread pool size

[Richard Bunt]

This commit enables the early initialization commands (92e4e97a9f5) to
modify the number of threads used by gdb's thread pool.

The motivation here is to prevent gdb from spawning a detrimental number
of threads on many-core systems under environments with restrictive
ulimits.

With gdb before this commit, the thread pool takes the following sizes:

1. Thread pool size is initialized to 0.

2. After the maintenance commands are defined, the thread pool size is
set to the number of system cores (if it has not already been set).

3. Using early initialization commands, the thread pool size can be
changed using "maint set worker-threads".

4. After the first prompt, the thread pool size can be changed as in the
previous step.

Therefore after step 2. gdb has potentially launched hundreds of threads
on a many-core system.

After this change, step 2 and 3 are reversed so there is an opportunity
to set the required number of threads without needing to default to the
number of system cores first.

There does exist a configure option (added in 261b07488b9) to disable
multithreading, but this does not allow for an already deployed gdb to
be configured.

Additionally, the default number of worker threads is clamped at eight
to control the number of worker threads spawned on many-core systems.
This value was chosen as testing recorded on bugzilla issue 29959
indicates that parallel efficiency drops past this point.

GDB built with GCC 13.

No test suite regressions detected. Compilers: GCC, ACfL, Intel, Intel
LLVM, NVHPC; Platforms: x86_64, aarch64.

The scenario that interests me the most involves preventing GDB from
spawning any worker threads at all. This was tested by counting the
number of clones observed by strace:

    strace -e clone,clone3 gdb/gdb -q \
    --early-init-eval-command="maint set worker-threads 0" \
    -ex q ./gdb/gdb |& grep --count clone

The new test relies on "gdb: install CLI uiout while processing early
init files" developed by Andrew Burgess. This patch will need pushing
prior to this change.

The clamping was tested on machines with both 16 cores and a single
core.  "maint show worker-threads" correctly reported eight and one
respectively.

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

---
 gdb/main.c                                 |  4 +++
 gdb/maint.c                                | 18 ++++++----
 gdb/maint.h                                |  4 +++
 gdb/testsuite/gdb.base/early-init-file.exp | 40 ++++++++++++++++++++++
 gdbsupport/thread-pool.cc                  |  4 +++
 gdbsupport/thread-pool.h                   |  1 +
 6 files changed, 65 insertions(+), 6 deletions(-)

diff --git a/gdb/main.c b/gdb/main.c
index eb11d6f2e46..688db7655a9 100644
--- a/gdb/main.c
+++ b/gdb/main.c
@@ -1058,6 +1058,10 @@ captured_main_1 (struct captured_main_args *context)
   execute_cmdargs (&cmdarg_vec, CMDARG_EARLYINIT_FILE,
 		   CMDARG_EARLYINIT_COMMAND, &ret);
 
+  /* Set the thread pool size here, so the size can be influenced by the
+     early initialization commands.  */
+  update_thread_pool_size ();
+
   /* Initialize the extension languages.  */
   ext_lang_initialization ();
 
diff --git a/gdb/maint.c b/gdb/maint.c
index c1154d0a54e..2e6754c9558 100644
--- a/gdb/maint.c
+++ b/gdb/maint.c
@@ -844,15 +844,23 @@ maintenance_set_profile_cmd (const char *args, int from_tty,
 
 static int n_worker_threads = -1;
 
-/* Update the thread pool for the desired number of threads.  */
-static void
+/* See maint.h.  */
+
+void
 update_thread_pool_size ()
 {
 #if CXX_STD_THREAD
   int n_threads = n_worker_threads;
 
   if (n_threads < 0)
-    n_threads = std::thread::hardware_concurrency ();
+    {
+      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);
+    }
 
   gdb::thread_pool::g_thread_pool->set_thread_count (n_threads);
 #endif
@@ -874,7 +882,7 @@ maintenance_show_worker_threads (struct ui_file *file, int from_tty,
   if (n_worker_threads == -1)
     {
       gdb_printf (file, _("The number of worker threads GDB "
-			  "can use is unlimited (currently %zu).\n"),
+			  "can use is the default (currently %zu).\n"),
 		  gdb::thread_pool::g_thread_pool->thread_count ());
       return;
     }
@@ -1474,6 +1482,4 @@ such as demangling symbol names."),
 					     maintenance_selftest_option_defs,
 					     &set_selftest_cmdlist,
 					     &show_selftest_cmdlist);
-
-  update_thread_pool_size ();
 }
diff --git a/gdb/maint.h b/gdb/maint.h
index 1741d132567..2ec2493d43d 100644
--- a/gdb/maint.h
+++ b/gdb/maint.h
@@ -26,6 +26,10 @@ extern void set_per_command_time (int);
 
 extern void set_per_command_space (int);
 
+/* Update the thread pool for the desired number of threads.  */
+
+extern void update_thread_pool_size ();
+
 /* Records a run time and space usage to be used as a base for
    reporting elapsed time or change in space.  */
 
diff --git a/gdb/testsuite/gdb.base/early-init-file.exp b/gdb/testsuite/gdb.base/early-init-file.exp
index 6426da8dacf..d09fd145cbf 100644
--- a/gdb/testsuite/gdb.base/early-init-file.exp
+++ b/gdb/testsuite/gdb.base/early-init-file.exp
@@ -99,6 +99,25 @@ proc check_gdb_startups_up_quietly { message } {
     }
 }
 
+# Restart GDB and check that the size of the thread pool has not been
+# adjusted to match the number of machine cores at early init time.
+proc check_gdb_maint_show { message } {
+    global gdb_prompt
+    global gdb_sanitizer_msg_re
+    gdb_exit
+    gdb_spawn
+    set setshowprefix "The number of worker threads GDB can use is"
+    set unset "$setshowprefix the default \\\(currently 0\\\)."
+    set final "$setshowprefix 1."
+    # Output when CXX_STD_THREAD is undefined.
+    set off "$setshowprefix 0."
+    gdb_test_multiple "" $message {
+	-re "^(${gdb_sanitizer_msg_re})?($unset|$off)\r\n($final|$off)\r\n$gdb_prompt $" {
+	    pass $gdb_test_name
+	}
+    }
+}
+
 with_ansi_styling_terminal {
 
     # Start GDB and confirm that the version string is styled.
@@ -164,4 +183,25 @@ with_ansi_styling_terminal {
 	check_gdb_startups_up_quietly \
 	    "check GDB starts quietly using XDG_CONFIG_HOME"
     }
+
+    # Create fake home directory for the thread pool size check.
+    set dirs [setup_home_directories "maint-show" \
+		  [multi_line_input \
+		       "set startup-quietly on" \
+		       "maint show worker-threads" \
+		       "maint set worker-threads 1" \
+		       "maint show worker-threads"]]
+
+    set home_dir [lindex $dirs 0]
+
+    # Now arrange to use the fake home directory startup file.
+    save_vars { INTERNAL_GDBFLAGS env(HOME) env(XDG_CONFIG_HOME) } {
+	set INTERNAL_GDBFLAGS [string map {"-nx" "" "-q" ""} $INTERNAL_GDBFLAGS]
+
+	# Now test GDB when using the HOME directory.
+	set env(HOME) $home_dir
+	unset -nocomplain env(XDG_CONFIG_HOME)
+	check_gdb_maint_show \
+	    "check early init of thread pool size"
+    }
 }
diff --git a/gdbsupport/thread-pool.cc b/gdbsupport/thread-pool.cc
index bbe043dc0a3..0b11213729f 100644
--- a/gdbsupport/thread-pool.cc
+++ b/gdbsupport/thread-pool.cc
@@ -154,6 +154,7 @@ thread_pool::set_thread_count (size_t num_threads)
 {
 #if CXX_STD_THREAD
   std::lock_guard<std::mutex> guard (m_tasks_mutex);
+  m_sized_at_least_once = true;
 
   /* If the new size is larger, start some new threads.  */
   if (m_thread_count < num_threads)
@@ -197,6 +198,9 @@ thread_pool::set_thread_count (size_t num_threads)
 void
 thread_pool::do_post_task (std::packaged_task<void ()> &&func)
 {
+  /* This assert is here to check that no tasks are posted to the pool between
+     its initialization and sizing.  */
+  gdb_assert (m_sized_at_least_once);
   std::packaged_task<void ()> t (std::move (func));
 
   if (m_thread_count != 0)
diff --git a/gdbsupport/thread-pool.h b/gdbsupport/thread-pool.h
index d5e1dc7fce1..6959414476f 100644
--- a/gdbsupport/thread-pool.h
+++ b/gdbsupport/thread-pool.h
@@ -204,6 +204,7 @@ class thread_pool
      between the main thread and the worker threads.  */
   std::condition_variable m_tasks_cv;
   std::mutex m_tasks_mutex;
+  bool m_sized_at_least_once = false;
 #endif /* CXX_STD_THREAD */
 };
 
-- 
2.32.0

[PUSHED] gdb: fix libstdc++ assert caused by invalid use of std::clamp

[Andrew Burgess]

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

Re: [PATCH v4] gdb: Enable early init of thread pool size

[Tom Tromey]

>>>>> "Richard" == Richard Bunt <richard.bunt@linaro.org> writes:

Richard> Updates in v4.
Richard> Clamped the default number of worker threads to eight. I'm ok with this as GDB
Richard> can always be configured to use a higher number of work threads if the use case
Richard> demands it.

I think these are fine.  Thank you for doing this.
Approved-By: Tom Tromey <tom@tromey.com>

Tom

Re: [PATCH v4] gdb: Enable early init of thread pool size

[Tom de Vries]

On 11/27/23 21:23, Tom Tromey wrote:
>>>>>> "Richard" == Richard Bunt <richard.bunt@linaro.org> writes:
> 
> Richard> Updates in v4.
> Richard> Clamped the default number of worker threads to eight. I'm ok with this as GDB
> Richard> can always be configured to use a higher number of work threads if the use case
> Richard> demands it.
> 
> I think these are fine.  Thank you for doing this.
> Approved-By: Tom Tromey <tom@tromey.com>

I think this caused:
...
FAIL: gdb.gdb/index-file.exp: maintenance show worker-threads
ERROR: tcl error sourcing 
/data/vries/gdb/src/gdb/testsuite/gdb.gdb/index-file.exp.
ERROR: invalid bareword "UNKNOWN"
in expression "UNKNOWN / 2";
should be "$UNKNOWN" or "{UNKNOWN}" or "UNKNOWN(...)" or ...
     (parsing expression "UNKNOWN / 2")
     invoked from within
"expr $worker_threads / 2"
     invoked from within
"if { $worker_threads > 1 } {
     # Start GDB, but don't load a file yet.
     clean_restart

     # Adjust the number of threads to use.
     set reduced..."
     (file "/data/vries/gdb/src/gdb/testsuite/gdb.gdb/index-file.exp" 
line 127)
     invoked from within
"source /data/vries/gdb/src/gdb/testsuite/gdb.gdb/index-file.exp"
     ("uplevel" body line 1)
     invoked from within
"uplevel #0 source /data/vries/gdb/src/gdb/testsuite/gdb.gdb/index-file.exp"
     invoked from within
"catch "uplevel #0 source $test_file_name""
...

Thanks,
- Tom

Re: [PATCH v4] gdb: Enable early init of thread pool size

[Richard Bunt]

On 05/12/2023 07:56, Tom de Vries wrote:
> On 11/27/23 21:23, Tom Tromey wrote:
>>>>>>> "Richard" == Richard Bunt <richard.bunt@linaro.org> writes:
>>
>> Richard> Updates in v4.
>> Richard> Clamped the default number of worker threads to eight. I'm ok 
>> with this as GDB
>> Richard> can always be configured to use a higher number of work 
>> threads if the use case
>> Richard> demands it.
>>
>> I think these are fine.  Thank you for doing this.
>> Approved-By: Tom Tromey <tom@tromey.com>
> 
> I think this caused:
> ...
> FAIL: gdb.gdb/index-file.exp: maintenance show worker-threads
> ERROR: tcl error sourcing 
> /data/vries/gdb/src/gdb/testsuite/gdb.gdb/index-file.exp.
> ERROR: invalid bareword "UNKNOWN"
> in expression "UNKNOWN / 2";
> should be "$UNKNOWN" or "{UNKNOWN}" or "UNKNOWN(...)" or ...
>      (parsing expression "UNKNOWN / 2")
>      invoked from within
> "expr $worker_threads / 2"
>      invoked from within
> "if { $worker_threads > 1 } {
>      # Start GDB, but don't load a file yet.
>      clean_restart
> 
>      # Adjust the number of threads to use.
>      set reduced..."
>      (file "/data/vries/gdb/src/gdb/testsuite/gdb.gdb/index-file.exp" 
> line 127)
>      invoked from within
> "source /data/vries/gdb/src/gdb/testsuite/gdb.gdb/index-file.exp"
>      ("uplevel" body line 1)
>      invoked from within
> "uplevel #0 source 
> /data/vries/gdb/src/gdb/testsuite/gdb.gdb/index-file.exp"
>      invoked from within
> "catch "uplevel #0 source $test_file_name""
> ...
> 
> Thanks,
> - Tom

Thanks for flagging.

I'm looking in to the issue.

Rich.

[PATCH v4] gdb: Enable early init of thread pool size

[Richard Bunt]

Sorry about this. Fix OK for trunk?

Commit 33ae45434d0 updated the text reported by GDB when showing the
number of work threads. However, it neglected to update the assertions
using this text, which caused index-file.exp to fail. This commit
corrects this omission.

Tested index-file.exp is fixed on my local machine.

---
 gdb/testsuite/lib/gdb.exp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index d0990dcfe0e..7e478ab9b60 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -10031,7 +10031,7 @@ proc is_target_non_stop { {testname ""} } {
 proc gdb_get_worker_threads { {testname ""} } {
     set worker_threads "UNKNOWN"
     gdb_test_multiple "maintenance show worker-threads" $testname {
-	-wrap -re "^The number of worker threads GDB can use is unlimited \\(currently ($::decimal)\\)\\." {
+	-wrap -re "^The number of worker threads GDB can use is the default \\(currently ($::decimal)\\)\\." {
 	    set worker_threads $expect_out(1,string)
 	}
 	-wrap -re "^The number of worker threads GDB can use is ($::decimal)\\." {
-- 
2.32.0

Re: [PATCH v4] gdb: Enable early init of thread pool size

[Tom Tromey]

>>>>> "Richard" == Richard Bunt <richard.bunt@linaro.org> writes:

Richard> Sorry about this. Fix OK for trunk?
Richard> Commit 33ae45434d0 updated the text reported by GDB when showing the
Richard> number of work threads. However, it neglected to update the assertions
Richard> using this text, which caused index-file.exp to fail. This commit
Richard> corrects this omission.

Richard> Tested index-file.exp is fixed on my local machine.

Looks good.  Thank you.
Approved-By: Tom Tromey <tom@tromey.com>

Tom