[PATCH v2 07/29] Thread options & clone events (core + remote)

[Simon Marchi]

On 2022-07-13 18:24, Pedro Alves wrote:
> A previous patch taught GDB about a new TARGET_WAITKIND_THREAD_CLONED
> event kind, and made the Linux target report clone events.
> 
> A following patch will teach Linux GDBserver to do the same thing.
> 
> However, for remote debugging, it wouldn't be ideal for GDBserver to
> report every clone event to GDB, when GDB only cares about such events
> in some specific situations.  Reporting clone events all the time
> would be potentially chatty.  We don't enable thread create/exit
> events all the time for the same reason.  Instead we have the
> QThreadEvents packet.  QThreadEvents is target-wide, though.
> 
> This patch makes GDB instead explicitly request that the target
> reports clone events or not, on a per-thread basis.
> 
> In order to be able to do that with GDBserver, we need a new remote
> protocol feature.  Since a following patch will want to enable thread
> exit events on per-thread basis too, the packet introduced here is
> more generic than just for clone events.  It lets you enable/disable a
> set of options at once, modelled on Linux ptrace's PTRACE_SETOPTIONS.
> 
> IOW, this commit introduces a new QThreadOptions packet, that lets you
> specify a set of per-thread event options you want to enable.  The
> packet accepts a list of options/thread-id pairs, similarly to vCont,
> processed left to right, with the options field being a number
> interpreted as a bit mask of options.  The only option defined in this
> commit is GDB_TO_CLONE (0x1), which ask the remote target to report

It took me a while to understand that "TO" means "Thread Options".
Since this ends up in the documentation, I think it wouldn't hurt to be
clear and use GDB_THREAD_OPTION_CLONE.

> clone events.  Another patch later in the series will introduce
> another option.
> 
> For example, this packet sets option "1" (clone events) on thread
> p1000.2345:
> 
>   QThreadOptions;1:p1000.2345
> 
> and this clears options for all threads of process 1000, and then sets
> option "1" (clone events) on thread p1000.2345:
> 
>   QThreadOptions;0:p1000.-1;1:p1000.2345
> 
> This clears options of all threads of all processes:
> 
>   QThreadOptions;0
> 
> The target reports the set of supported options by including
> "QThreadOptions=<supported options>" in its qSupported response.
> 
> infrun is then tweaked to enable GDB_TO_CLONE when stepping over a
> breakpoint.
> 
> Unlike PTRACE_SETOPTIONS, fork/vfork/clone children do NOT inherit
> their parent's thread options.  This is so that GDB can send e.g.,
> "QThreadOptions;0;1:TID" without worrying about threads it doesn't
> know about yet.

Just wondering (the behavior you chose is fine with me), couldn't we use
the same strategy as we use for resumption, where each layer is
responsible of "hiding" processes and threads it has not yet reported?
This would mean that even is a wildcard selector is used, GDBserver
would not apply the options to an unreported child thread.

> @@ -892,6 +904,114 @@ handle_general_set (char *own_buf)
>        return;
>      }
>  
> +  if (startswith (own_buf, "QThreadOptions;"))
> +    {
> +      const char *p = own_buf + strlen ("QThreadOptions");
> +
> +      gdb_thread_options supported_options;
> +      if (!target_supports_set_thread_options (&supported_options))
> +	{
> +	  /* Something went wrong -- we don't support options, but GDB
> +	     sent the packet anyway.  */
> +	  write_enn (own_buf);
> +	  return;
> +	}
> +
> +      /* We could store the options directly in thread->thread_options
> +	 without this map, but that would mean that a QThreadOptions
> +	 packet with a wildcard like "QThreadOptions;0;3:TID" would
> +	 result in the debug logs showing:
> +
> +	   [options for TID are now 0x0]
> +	   [options for TID are now 0x3]
> +
> +	 It's nicer if we only print the final options for each TID,
> +	 and if we only print about it if the options changed compared
> +	 to the options that were previously set on the thread.  */
> +      std::unordered_map<thread_info *, gdb_thread_options> set_options;
> +
> +      while (*p != '\0')
> +	{
> +	  if (p[0] != ';')
> +	    {
> +	      write_enn (own_buf);
> +	      return;
> +	    }
> +	  p++;
> +
> +	  /* Read the options.  */
> +
> +	  gdb_thread_options options = parse_gdb_thread_options (&p);
> +
> +	  if ((options & ~supported_options) != 0)
> +	    {
> +	      /* GDB asked for an unknown or unsupported option, so
> +		 error out.  */
> +	      std::string err
> +		= string_printf ("E.Unknown option requested: %s\n",
> +				 hex_string (options));
> +	      strcpy (own_buf, err.c_str ());
> +	      return;
> +	    }
> +
> +	  ptid_t ptid;
> +
> +	  if (p[0] == ';' || p[0] == '\0')
> +	    ptid = minus_one_ptid;
> +	  else if (p[0] == ':')
> +	    {
> +	      const char *q;
> +
> +	      ptid = read_ptid (p + 1, &q);
> +
> +	      if (p == q)
> +		{
> +		  write_enn (own_buf);
> +		  return;
> +		}
> +	      p = q;
> +	      if (p[0] != ';' && p[0] != '\0')
> +		{
> +		  write_enn (own_buf);
> +		  return;
> +		}
> +	    }
> +	  else
> +	    {
> +	      write_enn (own_buf);
> +	      return;
> +	    }
> +
> +	  /* Convert PID.-1 => PID.0 for ptid.matches.  */
> +	  if (ptid != minus_one_ptid && ptid.lwp () == -1)

The "ptid != minus_one_ptid" part seems unnecessary.  If the second part
of the condition is going to match, then for sure ptid is not going to
be equal to (-1,0,0).

> +	    ptid = ptid_t (ptid.pid ());
> +
> +	  for_each_thread ([&] (thread_info *thread)
> +	    {
> +	      if (ptid_of (thread).matches (ptid))
> +		set_options[thread] = options;
> +	    });
> +	}
> +
> +      for (const auto &iter : set_options)
> +	{
> +	  thread_info *thread = iter.first;
> +	  gdb_thread_options options = iter.second;
> +
> +	  if (debug_threads && thread->thread_options != options)
> +	    {
> +	      debug_printf ("[options for %s are now 0x%x]\n",
> +			    target_pid_to_str (ptid_of (thread)).c_str (),
> +			    (unsigned) options);

This should probably use threads_debug_printf.

IWBN to have a small function that formats `options` as an "or" of the
enabled bits, it would make logs easier to read.  For example if the two
options are enabled, it would show up as "GDB_TO_CLONE | GDB_TO_EXIT".

> diff --git a/gdbserver/target.h b/gdbserver/target.h
> index 6c536a30778..33142363a02 100644
> --- a/gdbserver/target.h
> +++ b/gdbserver/target.h
> @@ -277,6 +277,12 @@ class process_stratum_target
>    /* Returns true if vfork events are supported.  */
>    virtual bool supports_vfork_events ();
>  
> +  /* Returns true if the target supports setting thread options.  If
> +     options are supported, write into SUPPORTED_OPTIONS the set of
> +     supported options.  */
> +  virtual bool supports_set_thread_options
> +    (gdb_thread_options *supported_options);

I know I'm the one who used to dislike non-const references, but I have
accepted it now.  Therefore, this should probably be a reference, if we
don't intend to be able to pass nullptr.  Or, have the target return an
optional<gdb_thread_options>?

Simon