Patchwork [v2,00/24] Multi-target support

login
register
mail settings
Submitter Philippe Waroquiers
Date Oct. 20, 2019, 11:41 a.m.
Message ID <0deb38a1bee181cdcd6689d69ea69913f136f979.camel@skynet.be>
Download mbox | patch
Permalink /patch/35185/
State New
Headers show

Comments

Philippe Waroquiers - Oct. 20, 2019, 11:41 a.m.
I played a little bit with the patch and valgrind
(I had to apply the patch with git am --3way, otherwise it failed to apply).
No problem encountered when playing with the result.
I also re-read the documentation (as I forgot about how this was all
working), and it was all pretty clear.

3 notes:
  * It might be interesting to one day add
     something like 'inferior apply all/list of inferiors SOMMECOMMAND'
  * when having 2 inferiors connected to 2 different valgrind gdbservers,
    I could continue both inferiors by using 'continue&',
    but I had to (artificially) issue 'interrupt' in the second inferior
    to have GDB accepting 'continue&'.  So, this might be the indication
    of a status 'running' which should be maintained per inferior,
    while it might now be maintained globally.
  * I was (again?) confused by add-inferior silently ignoring abbreviations
    (or more generally anything starting with - unless matching exactly
     an option).
    Waiting for option framework to be extended, the add-inferior command
    could use the below that accepts abbreviations and does not ignore
    wrong options.

Thanks

Philippe





On Thu, 2019-10-17 at 23:50 +0100, Pedro Alves wrote:
> Here's v2 of the multi-target patchset, which addresses all the review
> comments so far, I believe.  Patch 15 is new, so all following patches
> are shifted by one.
> 
> This time, I've adjusted the host-specific nat files to function API
> changes.  I tried to find spots that would need changes using grep.  I
> built the series on AIX, x86/SPARC Solaris, 64-bit Windows, x86
> GNU/Linux -m64/-m32, and Aarch64 GNU/Linux.  I'm currently running
> this through the buildbot, will have results tomorrow.  I don't expect
> any serious major issue, if any, as so far runs that completed seem
> OK.
> 
> Follows the intro blurb:
> 
> This series adds multi-target support to GDB.  What this means is that
> with this series, GDB can now be connected to different targets at the
> same time.  E.g., you can debug a live native process and a core dump
> at the same time, connect to multiple gdbservers, etc.
> 
> Patches 1 to 17 are preparatory patches.
> 
> Patch 18 is the actual multi-target patch.  This is the largest patch
> in the series.  It does a number of things at the same time, but
> they're all intertwined, so I gave up on splitting it further.
> 
> Patch 19 adds tests.  Split out because patch 18 is already too big as
> is.
> 
> Patch 22 does some user-visible changes, including a new command to
> list open target connections.  This is just the bare minimum I could
> think of that is necessary for multi-target work.  I'm sure we'll find
> other tweaks to other commands necessary.
> 
> Documentation is in patch 24.  Eli already reviewed and approved it.
> 
> I've pushed this to users/palves/multi-target-v2 on sourceware.
> 
> Pedro Alves (23):
>   Preserve selected thread in all-stop w/ background execution
>   Don't rely on inferior_ptid in record_full_wait
>   Make "show remote exec-file" inferior-aware
>   exceptions.c:print_flush: Remove obsolete check
>   Make target_ops::has_execution take an 'inferior *' instead of a
>     ptid_t
>   Don't check target is running in remote_target::mourn_inferior
>   Delete unnecessary code from kill_command
>   Introduce switch_to_inferior_no_thread
>   switch inferior/thread before calling target methods
>   Some get_last_target_status tweaks
>   tfile_target::close: trace_fd can't be -1
>   Use all_non_exited_inferiors in infrun.c
>   Delete exit_inferior_silent(int pid)
>   Tweak handling of remote errors in response to resumption packet
>   Fix reconnecting to a gdbserver already debugging multiple processes,
>     I
>   Fix reconnecting to a gdbserver already debugging multiple processes,
>     II
>   Multi-target support
>   Add multi-target tests
>   gdbarch-selftests.c: No longer error out if debugging something
>   Revert 'Remove unused struct serial::name field'
>   Add "info connections" command, "info inferiors" connection
>     number/string
>   Require always-non-stop for multi-target resumptions
>   Multi-target: NEWS and user manual
> 
> Tankut Baris Aktemur (1):
>   Avoid another inferior_ptid reference in gdb/remote.c
> 
>  gdb/doc/gdb.texinfo                                | 137 ++--
>  gdb/doc/guile.texi                                 |   4 +-
>  gdb/doc/python.texi                                |   6 +-
>  gdb/NEWS                                           |  29 +
>  gdb/Makefile.in                                    |   1 +
>  gdb/aarch64-linux-nat.c                            |   2 +-
>  gdb/ada-tasks.c                                    |   4 +-
>  gdb/aix-thread.c                                   |  24 +-
>  gdb/amd64-fbsd-tdep.c                              |   4 +-
>  gdb/amd64-linux-nat.c                              |   2 +-
>  gdb/break-catch-sig.c                              |   3 +-
>  gdb/break-catch-syscall.c                          |   3 +-
>  gdb/breakpoint.c                                   |  25 +-
>  gdb/bsd-uthread.c                                  |  20 +-
>  gdb/btrace.c                                       |   2 +-
>  gdb/corelow.c                                      |  10 +-
>  gdb/event-top.c                                    |  14 +-
>  gdb/exceptions.c                                   |   6 +-
>  gdb/exec.c                                         |  51 +-
>  gdb/exec.h                                         |   7 +
>  gdb/fbsd-tdep.c                                    |   3 +-
>  gdb/fork-child.c                                   |   7 +-
>  gdb/gdbarch-selftests.c                            |   5 -
>  gdb/gdbserver/fork-child.c                         |   3 +-
>  gdb/gdbserver/inferiors.c                          |   2 +-
>  gdb/gdbserver/linux-low.c                          |   2 +-
>  gdb/gdbserver/lynx-low.c                           |   2 +-
>  gdb/gdbserver/nto-low.c                            |   2 +-
>  gdb/gdbserver/remote-utils.c                       |   2 +-
>  gdb/gdbserver/target.c                             |   8 +-
>  gdb/gdbserver/target.h                             |  11 +-
>  gdb/gdbserver/win32-low.c                          |   4 +-
>  gdb/gdbsupport/common-gdbthread.h                  |   5 +-
>  gdb/gdbthread.h                                    | 133 ++--
>  gdb/i386-fbsd-tdep.c                               |   4 +-
>  gdb/i386-linux-nat.c                               |   2 +-
>  gdb/inf-child.c                                    |   2 +-
>  gdb/inf-ptrace.c                                   |   6 +-
>  gdb/infcall.c                                      |   3 +-
>  gdb/infcmd.c                                       | 129 ++--
>  gdb/inferior-iter.h                                |  76 ++-
>  gdb/inferior.c                                     | 156 +++--
>  gdb/inferior.h                                     |  71 +-
>  gdb/infrun.c                                       | 720 ++++++++++++++++-----
>  gdb/infrun.h                                       |  23 +-
>  gdb/inline-frame.c                                 |  51 +-
>  gdb/inline-frame.h                                 |  12 +-
>  gdb/linux-fork.c                                   |   5 +-
>  gdb/linux-nat.c                                    |  76 ++-
>  gdb/linux-nat.h                                    |   1 +
>  gdb/linux-tdep.c                                   |   3 +-
>  gdb/linux-thread-db.c                              | 112 ++--
>  gdb/mi/mi-interp.c                                 |  10 +-
>  gdb/mi/mi-main.c                                   |   6 +-
>  gdb/nat/fork-inferior.c                            |   8 +-
>  gdb/nat/fork-inferior.h                            |   5 +-
>  gdb/nto-procfs.c                                   |   2 +-
>  gdb/ppc-fbsd-tdep.c                                |   4 +-
>  gdb/proc-service.c                                 |  17 +-
>  gdb/process-stratum-target.c                       |  12 +-
>  gdb/process-stratum-target.h                       |  31 +-
>  gdb/procfs.c                                       |  49 +-
>  gdb/python/py-threadevent.c                        |   4 +-
>  gdb/ravenscar-thread.c                             |  15 +-
>  gdb/record-btrace.c                                |  41 +-
>  gdb/record-full.c                                  |  22 +-
>  gdb/regcache.c                                     | 162 +++--
>  gdb/regcache.h                                     |  30 +-
>  gdb/remote.c                                       | 307 +++++----
>  gdb/riscv-fbsd-tdep.c                              |   4 +-
>  gdb/serial.c                                       |   4 +
>  gdb/serial.h                                       |   1 +
>  gdb/sol-thread.c                                   |  28 +-
>  gdb/sol2-tdep.c                                    |   2 +-
>  gdb/solib-svr4.c                                   |   3 +-
>  gdb/target-connection.c                            | 159 +++++
>  gdb/target-connection.h                            |  40 ++
>  gdb/target-delegates.c                             |  27 +
>  gdb/target.c                                       | 192 +++---
>  gdb/target.h                                       |  41 +-
>  gdb/testsuite/gdb.base/fork-running-state.exp      |  17 +-
>  .../gdb.base/kill-detach-inferiors-cmd.exp         |   4 +-
>  gdb/testsuite/gdb.base/quit-live.exp               |   2 +-
>  gdb/testsuite/gdb.base/remote-exec-file.exp        |  46 ++
>  gdb/testsuite/gdb.guile/scm-progspace.exp          |   2 +-
>  gdb/testsuite/gdb.linespec/linespec.exp            |   2 +-
>  gdb/testsuite/gdb.mi/new-ui-mi-sync.exp            |   2 +-
>  .../gdb.mi/user-selected-context-sync.exp          |   2 +-
>  gdb/testsuite/gdb.multi/multi-target.c             | 100 +++
>  gdb/testsuite/gdb.multi/multi-target.exp           | 387 +++++++++++
>  gdb/testsuite/gdb.multi/remove-inferiors.exp       |   2 +-
>  gdb/testsuite/gdb.multi/tids-gid-reset.c           |  22 +
>  gdb/testsuite/gdb.multi/tids-gid-reset.exp         |  96 +++
>  gdb/testsuite/gdb.multi/watchpoint-multi.exp       |   2 +-
>  gdb/testsuite/gdb.python/py-inferior.exp           |   4 +-
>  .../gdb.server/connect-without-multi-process.exp   |   7 +-
>  .../gdb.server/extended-remote-restart.exp         |  22 +-
>  gdb/testsuite/gdb.threads/async.c                  |  70 ++
>  gdb/testsuite/gdb.threads/async.exp                |  98 +++
>  gdb/testsuite/gdb.threads/fork-plus-threads.exp    |   2 +-
>  .../forking-threads-plus-breakpoint.exp            |   2 +-
>  gdb/testsuite/gdb.trace/report.exp                 |   2 +-
>  gdb/testsuite/lib/gdbserver-support.exp            |   4 +
>  gdb/thread-iter.c                                  |  14 +-
>  gdb/thread-iter.h                                  |  25 +-
>  gdb/thread.c                                       | 230 ++++---
>  gdb/top.c                                          |  17 +-
>  gdb/tracectf.c                                     |   2 +-
>  gdb/tracefile-tfile.c                              |   5 +-
>  gdb/tracefile.h                                    |   2 +-
>  gdb/windows-nat.c                                  |  20 +-
>  111 files changed, 3360 insertions(+), 1073 deletions(-)
>  create mode 100644 gdb/target-connection.c
>  create mode 100644 gdb/target-connection.h
>  create mode 100644 gdb/testsuite/gdb.base/remote-exec-file.exp
>  create mode 100644 gdb/testsuite/gdb.multi/multi-target.c
>  create mode 100644 gdb/testsuite/gdb.multi/multi-target.exp
>  create mode 100644 gdb/testsuite/gdb.multi/tids-gid-reset.c
>  create mode 100644 gdb/testsuite/gdb.multi/tids-gid-reset.exp
>  create mode 100644 gdb/testsuite/gdb.threads/async.c
>  create mode 100644 gdb/testsuite/gdb.threads/async.exp
>
Pedro Alves - Oct. 29, 2019, 7:56 p.m.
On 10/20/19 12:41 PM, Philippe Waroquiers wrote:
> I played a little bit with the patch and valgrind
> (I had to apply the patch with git am --3way, otherwise it failed to apply).

Guess you missed that this was in the users/palves/multi-target-v2 branch.  :-/

> No problem encountered when playing with the result.
> I also re-read the documentation (as I forgot about how this was all
> working), and it was all pretty clear.

That's great to hear.

> 
> 3 notes:
>   * It might be interesting to one day add
>      something like 'inferior apply all/list of inferiors SOMMECOMMAND'

Agreed.

>   * when having 2 inferiors connected to 2 different valgrind gdbservers,
>     I could continue both inferiors by using 'continue&',
>     but I had to (artificially) issue 'interrupt' in the second inferior
>     to have GDB accepting 'continue&'.  So, this might be the indication
>     of a status 'running' which should be maintained per inferior,
>     while it might now be maintained globally.

I've tried to debug this a little, but I'd like to punt for now.  The error
I'm seeing first comes from a breakpoint re-set, where gdb tries to
read memory from the valgrind that is running.  Breakpoint re-sets currently
are too dumb and re-set all locations, instead of only adding/removing
the locations necessary.  And then that is running into the fact that
the remote protocol in the old all-stop mode (which is what valgrind supports)
is synchronous, gdb can't talk to the remote target until the target
reports a stop.

These sorts of issues is why I required that the remote backends
support non-stop mode for this initial pass.  I'd rather not have to fix
all these issues before landing the initial multi-target support, even
if we know there are issues we need to fix to make it cope better with
all-stop-only targets.

>   * I was (again?) confused by add-inferior silently ignoring abbreviations
>     (or more generally anything starting with - unless matching exactly
>      an option).
>     Waiting for option framework to be extended, the add-inferior command
>     could use the below that accepts abbreviations and does not ignore
>     wrong options.

Yes, that is fine with me.

Thanks,
Pedro Alves

Patch

diff --git a/gdb/inferior.c b/gdb/inferior.c
index 061cf5cebb..5d72780eb3 100644
--- a/gdb/inferior.c
+++ b/gdb/inferior.c
@@ -786,24 +786,23 @@  add_inferior_command (const char *args, int from_tty)
 
       for (char **argv = built_argv.get (); *argv != NULL; argv++)
 	{
-	  if (**argv == '-')
+	  int len = strlen (*argv);
+
+	  if (strncmp (*argv, "-copies", len) == 0)
 	    {
-	      if (strcmp (*argv, "-copies") == 0)
-		{
-		  ++argv;
-		  if (!*argv)
-		    error (_("No argument to -copies"));
-		  copies = parse_and_eval_long (*argv);
-		}
-	      else if (strcmp (*argv, "-no-connection") == 0)
-		no_connection = true;
-	      else if (strcmp (*argv, "-exec") == 0)
-		{
-		  ++argv;
-		  if (!*argv)
-		    error (_("No argument to -exec"));
-		  exec.reset (tilde_expand (*argv));
-		}
+	      ++argv;
+	      if (!*argv)
+		error (_("No argument to -copies"));
+	      copies = parse_and_eval_long (*argv);
+	    }
+	  else if (strncmp (*argv, "-no-connection", len) == 0)
+	    no_connection = true;
+	  else if (strncmp (*argv, "-exec", len) == 0)
+	    {
+	      ++argv;
+	      if (!*argv)
+		error (_("No argument to -exec"));
+	      exec.reset (tilde_expand (*argv));
 	    }
 	  else
 	    error (_("Invalid argument"));