This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 04/16 v2] Determine supported extended-remote features
- From: Pedro Alves <palves at redhat dot com>
- To: "Breazeal, Don" <donb at codesourcery dot com>, gdb-patches at sourceware dot org
- Date: Wed, 22 Oct 2014 22:48:26 +0100
- Subject: Re: [PATCH 04/16 v2] Determine supported extended-remote features
- Authentication-results: sourceware.org; auth=none
- References: <1407434395-19089-1-git-send-email-donb at codesourcery dot com> <1408580964-27916-5-git-send-email-donb at codesourcery dot com> <543E9E09 dot 80009 at redhat dot com> <5446EB04 dot 7010208 at codesourcery dot com>
On 10/22/2014 12:23 AM, Breazeal, Don wrote:
> 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.
Oh, you're reading this backwards --- this block is making sure that
we don't forget to call add_packet_config_cmd whenever we add a
new feature/packet. The commands that is talking about are the
"set remote foo-packet" commands that add_packet_config_cmd registers.
Those are commands that allow force- enable/disabling a given
RSP packet/feature. That loop is allowing some exceptions for
some packets/features that already didn't have the corresponding
"set remote foo-packet" commands when that assertion was
first added:
/* Assert that we've registered commands for all packet configs. */
{
int i;
for (i = 0; i < PACKET_MAX; i++)
{
/* Ideally all configs would have a command associated. Some
still don't though. */
int excepted;
See the log of ca4f7f8be, the commit that added this.
We can certainly add new qSupported features that don't map
to regular user commands.
Guess this would make it clearer:
- /* Assert that we've registered commands for all packet configs. */
+ /* Assert that we've registered "set remote foo-packet" commands for all packet configs. */
> 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?
Yes, qSupported is preferable. I'm sorry about not foreseeing
this potential confusion when I wrote that comment.
Thanks,
Pedro Alves