Re: [PATCH 04/16 v2] Determine supported extended-remote features

["Breazeal, Don" <donb@codesourcery.com>]

On 10/15/2014 9:17 AM, Pedro Alves wrote:
> On 08/21/2014 01:29 AM, Don Breazeal wrote:
>> This patch implements a mechanism for GDB to ask gdbserver what
>> extended-mode features are enabled.
>>
>> The problems that are solved include:
>>
>> 1) A mechanism other than the qSupported RSP packet is needed for
>>    gdbserver to inform GDB about the list of extended mode features that
>>    are supported.
> 
> Sorry, this doesn't really make sense to me.
> 
>>    
>>    In the existing implementation most of the information about
>>    supported gdbserver features is sent to GDB in response to a
>>    qSupported RSP message.  This exchange occurs prior to GDB
>>    sending the "!" packet to enable extended mode.  So as-is this
>>    message doesn't work for features that are dependent on extended-
>>    mode.
> 
> The differences between extended and non-extended modes are very
> few, and I'd rather have fewer -- eventually merge them -- not
> more.
> 
> I don't understand what motivated this.  What's wrong with
> just reporting the features as supported in qSupported, and
> then have GDB decide whether to make use the features or not?
> 
>>    Also, the qSupported exchange is intended to inform GDB about which
>>    packets are supported, and most of the extended mode features do not
>>    have associated packets.  There are some features included in the
>>    qSupported response that do not have associated RSP messages, but the
>>    code comments make it clear that this practice is not acceptable for new
>>    packets.
> 
> Which comments?
> 
>>
>> 2) A mechanism is needed to enable extended mode features when GDB
>>    tells gdbserver to use extended mode.
>>    
>>    The existing implementation checks for ptrace extended events
>>    (just PTRACE_O_TRACELONE) and enables them immediately after checking.
>>    This is done when the first event occurs as the program is loaded, which
>>    can occur before GDB has connected to gdbserver and told it whether or
>>    not to use extended mode.
> 
> 
> Thanks,
> Pedro Alves
> 
Hi Pedro,
Thanks for looking at this.  I should have made the rationale for
the new qExtendedFeatures packet more clear in my original
submission.  I'll try to answer all of your questions in the
explanation below.

I had started out trying to report the features as
supported/unsupported using qSupported, following the
implementation of PACKET_multiprocess_feature as a model.
I abandoned that approach when I saw these comments in
remote.c_initialize_remote:

        /* Ideally all configs would have a command associated.  Some
           still don't though.  */
          [---snip---]
          case PACKET_multiprocess_feature:
          [---snip---]
            /* Additions to this list need to be well justified:
               pre-existing packets are OK; new packets are not.  */

I interpreted this to mean that the qSupported query was intended
to be used only to enable/disable packets that mapped to commands,
and that defining new packets that represented something else was
discouraged.

In the end, the qExtendedFeatures approach seemed better to me because
 - it didn't violate the directive in the comment above
 - it was less disruptive to existing code, and didn't affect native
 - it could potentially be used to eliminate some special-case code
   that used packets that had no associated command, e.g. the
   multiprocess feature could be implemented using qExtendedFeatures
   instead of PACKET_multiprocess_feature.

Implementing the qSupported approach required:
1) sending "!" before sending qSupported so that gdbserver knew it
   was in extended mode when responding to qSupported

2) splitting nat/linux-ptrace.c:linux_enable_event_reporting into two
   functions, one to check if extended events were supported at
   startup time, and one to enable any supported extended events
   where linux_enable_event_reporting is called today.  This allowed
   gdbserver to know what extended events were supported when
   responding to qSupported.  Note that this affects the native
   implementation.

3) defining a new packet for each of the extended-mode events

In refreshing my memory about this I wrote up some detailed notes on
it.  Let me know if you want to see the gruesome details.

Do you think the qSupported approach is preferable despite the
comment?  Did I misinterpret it?

Thanks
--Don