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

Breazeal, Don
2014-10-21T23:23:56Z

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