Re: [PATCH 17/23] Multi-target support

[Tom Tromey <tom@tromey.com>]

>>>>> "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.

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?

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.

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.

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.

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.

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.

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.

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?

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.

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

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?

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.

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".

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.

Tom