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

[Pedro Alves <pedro@palves.net>]

On 2022-07-21 4:14 a.m., Simon Marchi wrote:
> 
> 
> 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.

Done, in all affected patches, including commit logs / subjects.

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

If child threads inherit parent options, and then we apply that strategy, then we
have no way to clear the options of all threads, even those GDB doesn't know
about yet.  If threads don't inherit options, then their options start cleared,
and "QThreadOptions;0" thus results in all threads with options cleared, whether
we apply that strategy or not.

> 
>> @@ -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).

True.  Tweaked.

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

Done.

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

Done, in a separate patch.

> 
>> 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>?

I got rid of the boolean 'supports/doesn't support' distinction instead,
it's not really useful.  There's no good use for gdbserver supporting the packet
but then supporting no option.  Instead, we'll have this:

+  /* Returns the set of supported thread options.  */
+  virtual gdb_thread_options supported_thread_options ();

and the target can just return 0 to indicate no support.  The callers
end up changing like this:

diff --git c/gdbserver/server.cc w/gdbserver/server.cc
index 307290ed7de..871cfccd74e 100644
--- c/gdbserver/server.cc
+++ w/gdbserver/server.cc
@@ -908,11 +908,11 @@ handle_general_set (char *own_buf)
     {
       const char *p = own_buf + strlen ("QThreadOptions");
 
-      gdb_thread_options supported_options;
-      if (!target_supports_set_thread_options (&supported_options))
+      gdb_thread_options supported_options = target_supported_thread_options ();
+      if (supported_options == 0)
        {
-         /* Something went wrong -- we don't support options, but GDB
-            sent the packet anyway.  */
+         /* Something went wrong -- we don't support any option, but
+            GDB sent the packet anyway.  */
          write_enn (own_buf);
          return;
        }
@@ -2611,8 +2611,8 @@ handle_query (char *own_buf, int packet_len, int *new_packet_len_p)
 
       strcat (own_buf, ";vContSupported+");
 
-      gdb_thread_options supported_options;
-      if (target_supports_set_thread_options (&supported_options))
+      gdb_thread_options supported_options = target_supported_thread_options ();
+      if (supported_options != 0)
        {
          char *end_buf = own_buf + strlen (own_buf);
          sprintf (end_buf, ";QThreadOptions=%s",