Re: [PATCH v5 01/06] Identify remote fork event support

[Pedro Alves <palves at redhat dot com>]

On 03/26/2015 07:00 PM, Don Breazeal wrote:
> On 3/24/2015 3:57 AM, Pedro Alves wrote:
>> On 03/17/2015 08:56 PM, Don Breazeal wrote:
>>
>>> @@ -12508,7 +12524,8 @@ Show the maximum size of the address (in bits) in a memory packet."), NULL,
>>>    add_packet_config_cmd (&remote_protocol_packets[PACKET_hwbreak_feature],
>>>                           "hwbreak-feature", "hwbreak-feature", 0);
>>>  
>>> -  /* Assert that we've registered commands for all packet configs.  */
>>> +  /* Assert that we've registered "set remote foo-packet" commands
>>> +     for all packet configs.  */
>>>    {
>>>      int i;
>>>  
>>
>> This hunk could go immediately/separately as obvious.
> 
> Thanks, this has been pushed.
> 
>>> @@ -12527,6 +12544,8 @@ Show the maximum size of the address (in bits) in a memory packet."), NULL,
>>>  	  case PACKET_DisconnectedTracing_feature:
>>>  	  case PACKET_augmented_libraries_svr4_read_feature:
>>>  	  case PACKET_qCRC:
>>> +	  case PACKET_fork_event_feature:
>>> +	  case PACKET_vfork_event_feature:
>>>  	    /* Additions to this list need to be well justified:
>>>  	       pre-existing packets are OK; new packets are not.  */
>>
>> I think I mentioned this before: please do register commands for these
>> features.  As the comment says, new packets here are not OK without
>> good justification, and I can't think of a good justification.
>> Then you'll need to make sure remote_query_supported only includes the
>> feature in GDB's query if the feature wasn't force-disabled.  See
>> the new swbreak+ feature's handling in that function.
> 
> You did mention this before, but I misunderstood.  I've made the updates
> in this version.  Sorry for the repeat.

Sorry, I should probably have been clearer before then.

The "pre-existing packets are OK" comment just means that
when the assertion was added, those packets already existed.
It'd be great to just add the commands for those, and get
rid of that exceptions list...

The code looks good to me.  The docs still need a few tweaks:

> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -35812,6 +35812,18 @@ extensions unless the stub also reports that it supports them by
>  including @samp{multiprocess+} in its @samp{qSupported} reply.
>  @xref{multiprocess extensions}, for details.
>  
> +@item fork-events
> +This feature indicates whether @value{GDBN} supports fork event 
> +extensions to the remote protocol.  @value{GDBN} does not use such
> +extensions unless the stub also reports that it supports them by
> +including @samp{fork-events+} in its @samp{qSupported} reply.
> +
> +@item vfork-events
> +This feature indicates whether @value{GDBN} supports vfork event 
> +extensions to the remote protocol.  @value{GDBN} does not use such
> +extensions unless the stub also reports that it supports them by
> +including @samp{vfork-events+} in its @samp{qSupported} reply.
> +
>  @item xmlRegisters
>  This feature indicates that @value{GDBN} supports the XML target
>  description.  If the stub sees @samp{xmlRegisters=} with target
> diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
> index e4c5420..06ac673 100644

You'll need to update the tables below, where it reads:

"These are the currently defined stub features and their properties"

and:

"These are the currently defined stub features, in more detail"

and also, mention the new commands in:

"For each packet @var{name}, the command to enable or disable the
packet is @code{set remote @var{name}-packet}.  The available settings
are:"

Thanks,
Pedro Alves