[PING] [PATCH v4] gdb/gcore: interrupt all threads before generating the corefile

[Lancelot SIX <Lancelot.Six@amd.com>]

Kindly pinging,

Best,
Lancelot.

On 06/12/2022 14:12, Lancelot SIX wrote:
> Hi,
> 
> Here is a V4 for https://sourceware.org/pipermail/gdb-patches/2022-November/193861.html
> 
> Noticeable changes from V3:
> 
> - Updated NEWS and documentation following Andrew's recommendations.
> - Fixed compatibility issues in the testcase.
> - Followed Andrew's recommendations on the gcore_command changes.
> 
> All feedbacks are welcome.
> 
> Best,
> Lancelot.
> 
> ---
> 
> In non-stop mode, if the user tries to generate a core dump (using the
> gcore command) while some threads are running, a non-helpful error
> message is shown.
> 
> Lets consider the following session as an example (debugging the test
> program included in this patch):
> 
>      (gdb) set non-stop on
>      (gdb) b 37
>      (gdb) r
>      Thread 1 "gcore-nonstop" hit Breakpoint 1, main () at gcore-nonstop.c:39
>      (gdb) info thread
>         Id   Target Id                                          Frame
>       * 1    Thread 0x7ffff7d7a740 (LWP 431838) "gcore-nonstop" main () at gcore-nonstop.c:39
>         2    Thread 0x7ffff7d79640 (LWP 431841) "gcore-nonstop" (running)
>      (gdb) gcore
>      Couldn't get registers: No such process.
> 
> The reported error ("No such process") does not help the user understand
> what happens.  This is due to the fact that we cannot access the
> registers of a running thread.  Even if we ignore this error, generating
> a core dump while any thread might update memory would most likely
> result in a core file with an inconsistent view of the process' memory.
> 
> To solve this, this patch proposes to change the gcore command so it
> first stops all running threads (from the current inferior) before
> generating the corefile, and then resumes them in their previous state.
> 
> To achieve this, this patch exposes the restart_threads function in infrun.h
> (used to be local to infrun.c).  We also allow the first parameter
> (event_thread) to be nullptr as it is possible that the gcore command is
> called while all threads are running, in which case we want all threads
> to be restarted at the end of the procedure.
> 
> When testing this patch against gdbserver, it appears that using
> stop_all_threads / restart_threads was not compatible with all-stop
> targets.  For those targets, we need to call target_stop_and_wait /
> target_resume.  The gcore_command has code to handle both
> configurations.  I could not use a RAII-like object to have a cleaner
> way to restore the state at the end as the restore procedure could
> throw.  Instead, the procedure keeps track of which method was used to
> interrupt threads so the appropriate method can be used to restore their
> state.
> 
> Tested on x86_64 on navite GDB and the native-extended-gdbserver board.
> ---
>   gdb/NEWS                                 |  6 ++
>   gdb/doc/gdb.texinfo                      |  5 ++
>   gdb/gcore.c                              | 30 +++++++++
>   gdb/infrun.c                             | 16 ++---
>   gdb/infrun.h                             |  9 +++
>   gdb/testsuite/gdb.base/gcore-nonstop.c   | 44 ++++++++++++++
>   gdb/testsuite/gdb.base/gcore-nonstop.exp | 77 ++++++++++++++++++++++++
>   7 files changed, 176 insertions(+), 11 deletions(-)
>   create mode 100644 gdb/testsuite/gdb.base/gcore-nonstop.c
>   create mode 100644 gdb/testsuite/gdb.base/gcore-nonstop.exp
> 
> diff --git a/gdb/NEWS b/gdb/NEWS
> index c4ccfcc9e32..ab9c1dca1c6 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -155,6 +155,12 @@ set style tui-current-position [on|off]
>   
>   * Changed commands
>   
> +gcore
> +generate-core-file
> +  GDB now ensures that all threads of the current inferior are stopped
> +  before generating a core dump.  At the end of the command, threads are
> +  restored to their previous state.
> +
>   document user-defined
>     It is now possible to document user-defined aliases.
>     When a user-defined alias is documented, the help and apropos commands
> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index 5b566669975..afe4e0a2799 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -13654,6 +13654,11 @@ Produce a core dump of the inferior process.  The optional argument
>   specified, the file name defaults to @file{core.@var{pid}}, where
>   @var{pid} is the inferior process ID.
>   
> +@value{GDBN} ensures that all threads of the current inferior are
> +stopped while generating the core dump.  If any of the inferior's threads
> +are running when executing this command, @value{GDBN} stops the threads
> +and resumes them when the command is done.
> +
>   Note that this command is implemented only for some systems (as of
>   this writing, @sc{gnu}/Linux, FreeBSD, Solaris, and S390).
>   
> diff --git a/gdb/gcore.c b/gdb/gcore.c
> index 672bdf78736..8e17af5ee59 100644
> --- a/gdb/gcore.c
> +++ b/gdb/gcore.c
> @@ -34,6 +34,7 @@
>   #include "regset.h"
>   #include "gdb_bfd.h"
>   #include "readline/tilde.h"
> +#include "infrun.h"
>   #include <algorithm>
>   #include "gdbsupport/gdb_unlinker.h"
>   #include "gdbsupport/byte-vector.h"
> @@ -131,6 +132,28 @@ gcore_command (const char *args, int from_tty)
>     if (!target_has_execution ())
>       noprocess ();
>   
> +  scoped_restore_current_thread restore_current_thread;
> +  scoped_disable_commit_resumed disable_commit_resume ("generating coredump");
> +  struct inferior *inf = current_inferior ();
> +  scoped_finish_thread_state finish_state (inf->process_target (),
> +					   ptid_t (inf->pid));
> +
> +  bool all_stop_was_running = false;
> +  if (exists_non_stop_target ())
> +    stop_all_threads ("generating coredump", inf);
> +  else
> +    {
> +      all_stop_was_running = any_thread_of_inferior (inf)->executing ();
> +
> +      if (all_stop_was_running)
> +	{
> +	  if (!may_stop)
> +	    error (_("Cannot stop the target to generate the core dump."));
> +
> +	  target_stop_and_wait (ptid_t (inf->pid));
> +	}
> +    }
> +
>     if (args && *args)
>       corefilename.reset (tilde_expand (args));
>     else
> @@ -161,6 +184,13 @@ gcore_command (const char *args, int from_tty)
>       }
>   
>     gdb_printf ("Saved corefile %s\n", corefilename.get ());
> +
> +  if (exists_non_stop_target ())
> +    restart_threads (nullptr, inf);
> +  else if (all_stop_was_running)
> +    target_resume (ptid_t (inf->pid), 0, GDB_SIGNAL_0);
> +
> +  disable_commit_resume.reset_and_commit ();
>   }
>   
>   static enum bfd_architecture
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index c67458b30b6..9a4759d2d3c 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -96,9 +96,6 @@ static void resume (gdb_signal sig);
>   
>   static void wait_for_inferior (inferior *inf);
>   
> -static void restart_threads (struct thread_info *event_thread,
> -			     inferior *inf = nullptr);
> -
>   static bool start_step_over (void);
>   
>   static bool step_over_info_valid_p (void);
> @@ -5857,18 +5854,15 @@ handle_inferior_event (struct execution_control_state *ecs)
>       }
>   }
>   
> -/* Restart threads back to what they were trying to do back when we
> -   paused them (because of an in-line step-over or vfork, for example).
> -   The EVENT_THREAD thread is ignored (not restarted).
> -
> -   If INF is non-nullptr, only resume threads from INF.  */
> +/* See infrun.h.  */
>   
> -static void
> +void
>   restart_threads (struct thread_info *event_thread, inferior *inf)
>   {
>     INFRUN_SCOPED_DEBUG_START_END ("event_thread=%s, inf=%d",
> -				 event_thread->ptid.to_string ().c_str (),
> -				 inf != nullptr ? inf->num : -1);
> +				 (event_thread != nullptr
> +				  ? event_thread->ptid.to_string ().c_str ()
> +				  : "None"), inf != nullptr ? inf->num : -1);
>   
>     gdb_assert (!step_over_info_valid_p ());
>   
> diff --git a/gdb/infrun.h b/gdb/infrun.h
> index c711b9b21cc..4cd98ec06c5 100644
> --- a/gdb/infrun.h
> +++ b/gdb/infrun.h
> @@ -175,6 +175,15 @@ extern void nullify_last_target_wait_ptid ();
>      all threads of all inferiors.  */
>   extern void stop_all_threads (const char *reason, inferior *inf = nullptr);
>   
> +/* Restart threads back to what they were trying to do back when we
> +   paused them (because of an in-line step-over or vfork, for example).
> +   The EVENT_THREAD thread, if non-nullptr, is ignored (not restarted).
> +
> +   If INF is non-nullptr, only resume threads from INF.  */
> +
> +extern void restart_threads (struct thread_info *event_thread,
> +			     inferior *inf = nullptr);
> +
>   extern void prepare_for_detach (void);
>   
>   extern void fetch_inferior_event ();
> diff --git a/gdb/testsuite/gdb.base/gcore-nonstop.c b/gdb/testsuite/gdb.base/gcore-nonstop.c
> new file mode 100644
> index 00000000000..690d3289873
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/gcore-nonstop.c
> @@ -0,0 +1,44 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 2022 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/>.  */
> +
> +#include <pthread.h>
> +
> +static pthread_barrier_t barrier;
> +
> +static void *
> +worker_func (void *ignored)
> +{
> +  pthread_barrier_wait (&barrier);
> +  return NULL;
> +}
> +
> +int
> +main (void)
> +{
> +  pthread_t worker_thread;
> +  pthread_barrier_init (&barrier, NULL, 2);
> +
> +  pthread_create (&worker_thread, NULL, worker_func, NULL);
> +
> +  /* Break here.  */
> +
> +  pthread_barrier_wait (&barrier);
> +  pthread_join (worker_thread, NULL);
> +  pthread_barrier_destroy (&barrier);
> +
> +  return 0;
> +}
> diff --git a/gdb/testsuite/gdb.base/gcore-nonstop.exp b/gdb/testsuite/gdb.base/gcore-nonstop.exp
> new file mode 100644
> index 00000000000..4a240eb0872
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/gcore-nonstop.exp
> @@ -0,0 +1,77 @@
> +# Copyright 2022 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/>.
> +
> +# This testcase checks that when in non-stop mode with some threads running
> +# the gcore command can interrupt all threads, generate a core dump and
> +# restart threads as required.
> +
> +standard_testfile
> +
> +if { [prepare_for_testing "failed to prepare" \
> +	${testfile} ${srcfile} {pthreads debug}] } {
> +    return
> +}
> +
> +save_vars { GDBFLAGS } {
> +    append GDBFLAGS " -ex \"set non-stop on\""
> +    clean_restart ${binfile}
> +}
> +
> +set lineno [gdb_get_line_number "Break here"]
> +if { ![runto $lineno] } {
> +    return
> +}
> +
> +# We should be stopped in thread 1 while thread 2 is running.
> +gdb_test_sequence "info threads" "info threads" {
> +    {Id\s+Target Id\s+Frame}
> +    {\*\s+1[^\n]*\n}
> +    {\s+2\s+[^\n]*\(running\)[^\n]*\n}
> +}
> +
> +set th1_pc ""
> +gdb_test_multiple "p/x \$pc" "fetch thread 1 PC" {
> +  -wrap -re "$::decimal = ($::hex)" {
> +    set th1_pc $expect_out(1,string)
> +    pass $gdb_test_name
> +  }
> +}
> +
> +set corefile [standard_output_file "corefile"]
> +if {![gdb_gcore_cmd $corefile "generate corefile"]} {
> +  # gdb_gcore_cmd issues a "UNSUPPORTED".
> +  return
> +}
> +
> +# After the core file is generated, thread 2 should be back running
> +# and thread 1 should still be selected.
> +gdb_test_sequence "info threads" "correct thread selection after gcore" {
> +    {Id\s+Target Id\s+Frame}
> +    {\*\s+1[^\n]*\n}
> +    {\s+2\s+[^\n]*\(running\)[^\n]*\n}
> +}
> +
> +# Thread 1 is at the same stop it was before calling the gcore command.
> +gdb_test "p/x \$pc" "\\\$$::decimal = $th1_pc" "thread 1 unchanged"
> +
> +clean_restart $binfile
> +gdb_test "core-file $corefile" "Core was generated by.*" "load corefile"
> +
> +# The core file has the 2 threads.
> +gdb_test_sequence "info threads" "threads in corefile" {
> +    {Id\s+Target Id\s+Frame}
> +    {\s+1\s+Thread[^\n]*\n}
> +    {\s+2\s+Thread[^\n]*\n}
> +}
> 
> base-commit: c8ea5e409b02cf7fa848e44af74b2e8246ad03f1