[17/23] Multi-target support

From: Pedro Alves <palves@redhat.com>

On 9/11/19 6:11 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
> Pedro> This commit adds multi-target support to GDB.  What this means is that
> Pedro> with this commit, GDB can now be connected to different targets at the
> Pedro> same time.  E.g., you can debug a live native process and a core dump
> Pedro> at the same time, connect to multiple gdbservers, etc.
> 
> Awesome!
> 
> I read through the patch, but I can't really claim to understand how all
> the parts interrelate.  However your overview made sense to me, and I
> don't have any conceptual concerns; just the usual sort of thing that
> there are lurking bugs.  I guess my main fear is that this will
> introduce regressions... but on the other hand, I think it's important
> direction for gdb, so I'd rather err on the side of moving forward with
> it.
> 
> Pedro> To fix this,
> Pedro> this commit renames gdbserver's target_ops to process_stratum_target.
> Pedro> I think this makes sense.  There's no concept of target stack in
> Pedro> gdbserver, and gdbserver's target_ops really implements a
> Pedro> process_stratum-like target.
> 
> Makes sense.  Sometimes I dream about re-merging the target stacks,
> further shrinking the size of the gdbserver fork.

Yeah, that's been a long term goal of mine too.

> 
> Pedro>  - you can only resume more that one target at the same time if all
> Pedro>    targets support asynchronous debugging, and support non-stop mode.
> Pedro>    It should be possible to support mixed all-stop + non-stop
> Pedro>    backends, but that is left for another time.  This means that
> Pedro>    currently in order to do multi-target with gdbserver you need to
> Pedro>    issue "maint set target-non-stop on".  I would like to make that
> Pedro>    mode be the default, but we're not there yet.  Note that I'm
> Pedro>    talking about how the target backend works, only.  User-visible
> Pedro>    all-stop mode works just fine.
> 
> Pedro>  - as explained above, connecting to different remote servers at the
> Pedro>    same time is likely to produce bad results if they don't support the
> Pedro>    exact set of RSP features.
> 
> Are these limitations something that can be noticed when making the
> remote connection, or do they require the user to be careful?  What
> happens if the user forgets or just doesn't know?

I'm not seeing how to detect that easily&gracefully, unfortunately.  If the
user forgets, gdb might misbehave.  I'm hoping that the limitation
will be temporary, though.

> Pedro> -  thread_info *tp = find_thread_ptid (task_info->ptid);
> Pedro> +  thread_info *tp = find_thread_ptid (inf->process_target (), task_info->ptid);
> 
> There are a few spots in the patch that could use the overload that
> accepts an inferior, but instead call the process_target method
> directly.

Thanks.  I fixed that one.  I skimmed other calls and didn't
see any that was obvious it could be changed.

> 
> Pedro> +/* Calls target_commit_resume on all targets.  */
> Pedro> +
> Pedro> +static void
> Pedro> +commit_resume_all_targets ()
> Pedro> +{
> Pedro> +  scoped_restore_current_thread restore_thread;
> Pedro> +
> Pedro> +  for (inferior *inf : all_non_exited_inferiors ())
> Pedro> +    if (inf->has_execution ())
> Pedro> +      {
> Pedro> +	switch_to_inferior_no_thread (inf);
> Pedro> +	target_commit_resume ();
> Pedro> +      }
> 
> IIUC this can cause multiple calls to a given target's commit_resume
> method.  That seems fine (assuming you audited these implementations)
> but I suppose it would be good to document this somewhere.
> 
> Alternatively I guess gdb would need some kind of iterator that ensures
> it only visits each target once.

You're right.  I didn't worry about it because resumptions are
supposed to be idempotent, but, reducing number of redundant
vCont packets can only be a good thing.  I did not add an iterator,
instead fixed it locally.  Thinking that we may end up changing
the data inferior/process_stratum_target structures to include

> 
> Pedro> -	  event_ptid = wait_one (&ws);
> Pedro> +	  wait_one_event event = wait_one ();
> Pedro> +	  target_waitstatus &ws = event.ws;
> Pedro> +	  ptid_t &event_ptid = event.ptid;
> 
> I was wondering if these need to be references.  It seemed like maybe
> they could be const references but I couldn't immediately tell.
> 

Good point.  They can, if I constify save_waitstatus as well:

  static void
 -save_waitstatus (struct thread_info *tp, struct target_waitstatus *ws)
 +save_waitstatus (struct thread_info *tp, const target_waitstatus *ws)

But I decided to just get rid of them and adjust the code to refer
to event.ws / event.ptid directly.

> Pedro> -     For all-stop targets, we only step INFERIOR_PTID and continue others.  */
> Pedro> +      For all-stop targets, we only step INFERIOR_PTID and continue others.  */
> 
> This looks like extra indentation was added.

Thanks.

> 
> Pedro>  struct regcache *
> Pedro>  get_thread_regcache_for_ptid (ptid_t ptid)
> Pedro>  {
> Pedro> -  return get_thread_regcache (ptid);
> Pedro> +  /* This function doesn't take a process_stratum_target parameter
> Pedro> +     because it's a common/ routine implemented by both gdb and
> Pedro> +     gdbserver.  It always refers to a ptid of the current target. */
> 
> It's "gdbsupport/" now.

Indeed.  :-)  Fixed, and also fixed double space at the end.

> 
> Pedro>    /* The status of the stub support for the various vCont actions.  */
> Pedro>    vCont_action_support supports_vCont;
> Pedro> +  /* Whether vCont support was probed already.  */
> Pedro> +  bool supports_vCont_probed;
> 
> If it's the case that this is only a temporary measure, then I think
> this comment should mention that.

Fixed.

> 
> Pedro> @@ -6601,6 +6622,8 @@ remote_target::commit_resume ()
> Pedro>      }
>  
> Pedro>    vcont_builder.flush ();
> Pedro> +
> Pedro> +  target_async (1);
> Pedro>  }
> 
> I didn't understand this.  Like, is it always ok to do this?

I had to back this one out to remember why it was needed.
It is simply that we now loop over various targets, and nothing
is calling target_async on each of them to register them
on the event loop.  I've moved this to common code now
(in do_target_resume), where I think it's more obvious.

> 
> Pedro>  /* Callback for iterate_over_inferiors.  Gets rid of the given
> Pedro>     inferior.  */
>  
> Pedro> -static int
> Pedro> -dispose_inferior (struct inferior *inf, void *args)
> Pedro> +static void
> Pedro> +dispose_inferior (inferior *inf)
> Pedro>  {
> Pedro>    /* Not all killed inferiors can, or will ever be, removed from the
> Pedro>       inferior list.  Killed inferiors clearly don't need to be killed
> Pedro>       again, so, we're done.  */
> Pedro>    if (inf->pid == 0)
> Pedro> -    return 0;
> Pedro> +    return;
> 
> I think the comments here (both the intro comment and the one in the
> function) need to be updated, since it seems that just a single inferior
> is handled here now, and this is no longer a callback for
> iterate_over_inferiors.

Indeed.  I think it's better to just delete the function an inline
its body in the caller.

> 
> I didn't understand this change.  Why did it used to iterate but now
> does not?

So before there was only ever one target stack.  If you were e.g., connected
to the native target, and did "tar rem :9999", then we'd need to discard all
the native inferiors that were debugging.  Since now each inferior has its
own connection, we only discard the current inferior, all other inferiors
keep using their current connections.

> 
> Pedro>  target_pass_ctrlc (void)
> Pedro>  {
> Pedro> -  current_top_target ()->pass_ctrlc ();
> Pedro> +  /* Pass the Ctrl-C to the first inferior that has a thread
> Pedro> +     running.  */
> Pedro> +  for (inferior *inf : all_inferiors ())
> Pedro> +    {
> [...]
> Pedro> +	      return;
> 
> I didn't understand this.  It seemed to me that a C-c should maybe be
> sent to all running inferiors?

This is like what the kernel does -- The C-c is sent to one process
(the one in the foreground) and then when gdb intercepts the SIGINT, it
pauses all processes/threads.  If we sent SIGINT to all running inferiors,
then we'd end up presenting one SIGINT stop to the user for each
inferior.  Like:

* running *
Press C-c
Thread 1.1 stopped with SIGINT
(gdb) c
Thread 2.1 stopped with SIGINT
(gdb) c
Thread 3.1 stopped with SIGINT
(gdb) c
Thread 4.1 stopped with SIGINT
(gdb) c
* all running again *

The comment has a typo, which I fixed now.  It should read:

  /* Pass the Ctrl-C to the first target that has a thread
     running.  */

It doesn't matter which inferior is selected for a target,
as target_pass_ctrlc forwards the Ctrl-C to the process
in the foreground.

> 
> I don't actually know this area.  Maybe that's obvious now :)
> 
> Pedro>  /* See target.h.  */
> Pedro> @@ -3987,10 +4047,8 @@ set_write_memory_permission (const char *args, int from_tty,
> Pedro>  }
>  
> Pedro>  void
> Pedro> -initialize_targets (void)
> Pedro> +_initialize_target (void)
> 
> You might as well remove the 'void' when touching the line.

Done.

> 
> Pedro> -  explicit all_matching_threads_iterator (ptid_t filter_ptid);
> Pedro> +  explicit all_matching_threads_iterator (process_stratum_target *filter_target,
> Pedro> +					  ptid_t filter_ptid);
> 
> I guess this could drop the "explicit".

Indeed, done.

> 
> Pedro> -  explicit all_matching_threads_range (ptid_t filter_ptid)
> Pedro> -    : m_filter_ptid (filter_ptid)
> Pedro> +  explicit all_matching_threads_range (process_stratum_target *filter_target,
> Pedro> +				       ptid_t filter_ptid)
> 
> Here too.
> 
> Pedro> -  explicit all_non_exited_threads_range (ptid_t filter_ptid)
> Pedro> -    : m_filter_ptid (filter_ptid)
> Pedro> +  explicit all_non_exited_threads_range (process_stratum_target *filter_target,
> Pedro> +					 ptid_t filter_ptid)
> Pedro> +    : m_filter_target (filter_target), m_filter_ptid (filter_ptid)
> 
> And here.

All fixed.

Here's the patch that I'm folding into the main patch, FYI.

I've also went through a number of native targets and fixed/adjusted
them AIX, Solaris, Windows, etc.

From 53aacc25ea7e3856588e9ea9fe47a2e22bbedaf4 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Thu, 17 Oct 2019 02:48:50 +0100
Subject: [PATCH] Address Tromey's review & more

---
 gdb/ada-tasks.c           |  2 +-
 gdb/aix-thread.c          | 24 ++++++++++++++------
 gdb/gdbserver/lynx-low.c  |  2 +-
 gdb/gdbserver/nto-low.c   |  2 +-
 gdb/gdbserver/target.h    |  2 +-
 gdb/gdbserver/win32-low.c |  4 ++--
 gdb/inferior-iter.h       |  3 +--
 gdb/infrun.c              | 56 ++++++++++++++++++++++++++++-------------------
 gdb/nto-procfs.c          |  2 +-
 gdb/procfs.c              | 49 ++++++++++++++++++++++-------------------
 gdb/ravenscar-thread.c    |  5 ++---
 gdb/record-btrace.c       |  2 +-
 gdb/regcache.c            |  4 ++--
 gdb/remote.c              |  5 ++---
 gdb/sol-thread.c          | 28 +++++++++++++++++-------
 gdb/target.c              | 36 ++++++++----------------------
 gdb/thread-iter.h         | 12 +++++-----
 gdb/windows-nat.c         | 20 +++++++++--------
 18 files changed, 138 insertions(+), 120 deletions(-)

[17/23] Multi-target support

Commit Message

Patch