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

[PATCH v2] 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.

In order to implement a test case, a bug fix was needed to enable
set/show commands to output information at early initialization time.
The early initiailzation commands are run before any interpreters are
available and printing resulted in a segmentation fault. The fix causes
gdb_stdout to be used directly. A side effect of this is that, any
output from set/show commands called at early intialization time will
not be formatted by an interpreter. In the same way the version string
resultant from the following command will not be decorated with MI
stream records.

    gdb -i=mi --version

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

---
 gdb/cli/cli-setshow.c                      |  2 +-
 gdb/main.c                                 |  4 +++
 gdb/maint.c                                |  7 ++--
 gdb/maint.h                                |  4 +++
 gdb/testsuite/gdb.base/early-init-file.exp | 40 ++++++++++++++++++++++
 gdbsupport/thread-pool.cc                  |  4 +++
 gdbsupport/thread-pool.h                   |  1 +
 7 files changed, 57 insertions(+), 5 deletions(-)

diff --git a/gdb/cli/cli-setshow.c b/gdb/cli/cli-setshow.c
index c7bbac1666d..7bc919e7b20 100644
--- a/gdb/cli/cli-setshow.c
+++ b/gdb/cli/cli-setshow.c
@@ -661,7 +661,7 @@ do_show_command (const char *arg, int from_tty, struct cmd_list_element *c)
   /* FIXME: cagney/2005-02-10: There should be MI and CLI specific
      versions of code to print the value out.  */
 
-  if (uiout->is_mi_like_p ())
+  if (uiout && uiout->is_mi_like_p ())
     uiout->field_string ("value", val);
   else
     {
diff --git a/gdb/main.c b/gdb/main.c
index 2da39f89a90..6930c5e1549 100644
--- a/gdb/main.c
+++ b/gdb/main.c
@@ -1047,6 +1047,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 f91184c2005..d567eb2aa9b 100644
--- a/gdb/maint.c
+++ b/gdb/maint.c
@@ -844,8 +844,9 @@ 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
@@ -1474,6 +1475,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..996d8d38b04 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 unlimited \\\(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 1c871ed378f..a08afc37009 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 cb8696e1fa4..3d578e9c241 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

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

[Richard Bunt]

Polite ping.

On 11/10/2023 16:02, Richard Bunt wrote:
> 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.
> 
> In order to implement a test case, a bug fix was needed to enable
> set/show commands to output information at early initialization time.
> The early initiailzation commands are run before any interpreters are
> available and printing resulted in a segmentation fault. The fix causes
> gdb_stdout to be used directly. A side effect of this is that, any
> output from set/show commands called at early intialization time will
> not be formatted by an interpreter. In the same way the version string
> resultant from the following command will not be decorated with MI
> stream records.
> 
>      gdb -i=mi --version
> 
> 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
> 
> ---
>   gdb/cli/cli-setshow.c                      |  2 +-
>   gdb/main.c                                 |  4 +++
>   gdb/maint.c                                |  7 ++--
>   gdb/maint.h                                |  4 +++
>   gdb/testsuite/gdb.base/early-init-file.exp | 40 ++++++++++++++++++++++
>   gdbsupport/thread-pool.cc                  |  4 +++
>   gdbsupport/thread-pool.h                   |  1 +
>   7 files changed, 57 insertions(+), 5 deletions(-)
> 
> diff --git a/gdb/cli/cli-setshow.c b/gdb/cli/cli-setshow.c
> index c7bbac1666d..7bc919e7b20 100644
> --- a/gdb/cli/cli-setshow.c
> +++ b/gdb/cli/cli-setshow.c
> @@ -661,7 +661,7 @@ do_show_command (const char *arg, int from_tty, struct cmd_list_element *c)
>     /* FIXME: cagney/2005-02-10: There should be MI and CLI specific
>        versions of code to print the value out.  */
>   
> -  if (uiout->is_mi_like_p ())
> +  if (uiout && uiout->is_mi_like_p ())
>       uiout->field_string ("value", val);
>     else
>       {
> diff --git a/gdb/main.c b/gdb/main.c
> index 2da39f89a90..6930c5e1549 100644
> --- a/gdb/main.c
> +++ b/gdb/main.c
> @@ -1047,6 +1047,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 f91184c2005..d567eb2aa9b 100644
> --- a/gdb/maint.c
> +++ b/gdb/maint.c
> @@ -844,8 +844,9 @@ 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
> @@ -1474,6 +1475,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..996d8d38b04 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 unlimited \\\(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 1c871ed378f..a08afc37009 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 cb8696e1fa4..3d578e9c241 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 */
>   };
>   

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

[Andrew Burgess]

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

> 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.
>
> In order to implement a test case, a bug fix was needed to enable
> set/show commands to output information at early initialization time.
> The early initiailzation commands are run before any interpreters are
> available and printing resulted in a segmentation fault. The fix causes
> gdb_stdout to be used directly. A side effect of this is that, any
> output from set/show commands called at early intialization time will
> not be formatted by an interpreter. In the same way the version string
> resultant from the following command will not be decorated with MI
> stream records.
>
>     gdb -i=mi --version
>
> 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
>
> ---
>  gdb/cli/cli-setshow.c                      |  2 +-
>  gdb/main.c                                 |  4 +++
>  gdb/maint.c                                |  7 ++--
>  gdb/maint.h                                |  4 +++
>  gdb/testsuite/gdb.base/early-init-file.exp | 40 ++++++++++++++++++++++
>  gdbsupport/thread-pool.cc                  |  4 +++
>  gdbsupport/thread-pool.h                   |  1 +
>  7 files changed, 57 insertions(+), 5 deletions(-)
>
> diff --git a/gdb/cli/cli-setshow.c b/gdb/cli/cli-setshow.c
> index c7bbac1666d..7bc919e7b20 100644
> --- a/gdb/cli/cli-setshow.c
> +++ b/gdb/cli/cli-setshow.c
> @@ -661,7 +661,7 @@ do_show_command (const char *arg, int from_tty, struct cmd_list_element *c)
>    /* FIXME: cagney/2005-02-10: There should be MI and CLI specific
>       versions of code to print the value out.  */
>  
> -  if (uiout->is_mi_like_p ())
> +  if (uiout && uiout->is_mi_like_p ())
>      uiout->field_string ("value", val);
>    else
>      {

I'm not keen on this change.  It feels like a bit of a hack.

Instead, I'd like to propose that we consider addressing this back at
the source -- but putting a CLI uiout object in place so that uiout will
be defined.

I had a play to see what such a patch would look like, and I'd like to
propose the patch below.

What do you (and others I guess) think?

Thanks,
Andrew

---

commit 01abfe9073cec666f566c63d2b9ce21666237e75
Author: Andrew Burgess <aburgess@redhat.com>
Date:   Thu Oct 26 17:28:27 2023 +0100

    gdb: install CLI uiout while processing early init files
    
    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 nullput,
    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.

diff --git a/gdb/main.c b/gdb/main.c
index 2da39f89a90..10f07be9259 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 = gdb::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

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

[Richard Bunt]

Hi Andrew,

Thank you for taking the time to look at this patch.

On 27/10/2023 11:22, Andrew Burgess wrote:
> Richard Bunt <richard.bunt@linaro.org> writes:
> 
>> 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.
>>
>> In order to implement a test case, a bug fix was needed to enable
>> set/show commands to output information at early initialization time.
>> The early initiailzation commands are run before any interpreters are
>> available and printing resulted in a segmentation fault. The fix causes
>> gdb_stdout to be used directly. A side effect of this is that, any
>> output from set/show commands called at early intialization time will
>> not be formatted by an interpreter. In the same way the version string
>> resultant from the following command will not be decorated with MI
>> stream records.
>>
>>      gdb -i=mi --version
>>
>> 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
>>
>> ---
>>   gdb/cli/cli-setshow.c                      |  2 +-
>>   gdb/main.c                                 |  4 +++
>>   gdb/maint.c                                |  7 ++--
>>   gdb/maint.h                                |  4 +++
>>   gdb/testsuite/gdb.base/early-init-file.exp | 40 ++++++++++++++++++++++
>>   gdbsupport/thread-pool.cc                  |  4 +++
>>   gdbsupport/thread-pool.h                   |  1 +
>>   7 files changed, 57 insertions(+), 5 deletions(-)
>>
>> diff --git a/gdb/cli/cli-setshow.c b/gdb/cli/cli-setshow.c
>> index c7bbac1666d..7bc919e7b20 100644
>> --- a/gdb/cli/cli-setshow.c
>> +++ b/gdb/cli/cli-setshow.c
>> @@ -661,7 +661,7 @@ do_show_command (const char *arg, int from_tty, struct cmd_list_element *c)
>>     /* FIXME: cagney/2005-02-10: There should be MI and CLI specific
>>        versions of code to print the value out.  */
>>   
>> -  if (uiout->is_mi_like_p ())
>> +  if (uiout && uiout->is_mi_like_p ())
>>       uiout->field_string ("value", val);
>>     else
>>       {
> 
> I'm not keen on this change.  It feels like a bit of a hack.
> 
> Instead, I'd like to propose that we consider addressing this back at
> the source -- but putting a CLI uiout object in place so that uiout will
> be defined.
> 
> I had a play to see what such a patch would look like, and I'd like to
> propose the patch below.
> 
> What do you (and others I guess) think?

Your proposal is definitely better. As you say, it addresses the problem 
rather than the symptom.

> 
> Thanks,
> Andrew
> 
> ---
> 
> commit 01abfe9073cec666f566c63d2b9ce21666237e75
> Author: Andrew Burgess <aburgess@redhat.com>
> Date:   Thu Oct 26 17:28:27 2023 +0100
> 
>      gdb: install CLI uiout while processing early init files
>      
>      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.

I am in favour of working toward lifting this restriction, at least in 
practice, as it allows further automated testing at early initialisation 
time.

>      
>      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 nullput,
>      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.
> 
> diff --git a/gdb/main.c b/gdb/main.c
> index 2da39f89a90..10f07be9259 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 = gdb::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
> 

Many thanks,

Rich