Re: [PATCH v4 8/8] gdb: Document qIsAddressTagged packet

[Gustavo Romero <gustavo.romero@linaro.org>]

Hi Eli,

Thanks a lot for the review. I just have one question.

On 4/16/24 11:34 AM, Eli Zaretskii wrote:
>> From: Gustavo Romero <gustavo.romero@linaro.org>
>> Cc: luis.machado@arm.com,
>> 	thiago.bauermann@linaro.org,
>> 	eliz@gnu.org,
>> 	tom@tromey.com,
>> 	gustavo.romero@linaro.org
>> Date: Tue, 16 Apr 2024 14:07:28 +0000
>>
>> This commit documents the qIsAddressTagged packet.
> 
> Thanks.
> 
>> diff --git a/gdb/NEWS b/gdb/NEWS
>> index feb3a37393a..1693a7a15f8 100644
>> --- a/gdb/NEWS
>> +++ b/gdb/NEWS
>> @@ -192,6 +192,16 @@ QThreadOptions in qSupported
>>     QThreadOptions packet, and the qSupported response can contain the
>>     set of thread options the remote stub supports.
>>   
>> +qIsAddressTagged
>> +  This new packet allows GDB to query the stub about a given address to check
>> +  if it is tagged or not.  Many memory tagging-related GDB commands need to
>> +  perform this check before they read/write the allocation tag related to an
>> +  address.  Currently, however, this is done through a 'vFile' request to read
>> +  the file /proc/<PID>/smaps and check if the address is in a region reported
>> +  as memory tagged.  Since not all targets have a notion of what the smaps
>> +  file is about, this new packet provides a more generic way to perform such
>> +  a check.
>> +
>>   *** Changes in GDB 14
> 
> This part is okay.
> 
>> +@item qIsAddressTagged:@var{address}
>> +@cindex check if a given address is in a memory tagged region.
>                                                                  ^
> Index entries should not end in a period.
> 
>> +Check if address @var{address} is in a memory tagged region; if it is, it's
>> +said to be tagged.  The target is responsible for checking it, as this is
> 
> I suggest to use @dfn{tagged}, since this is new terminology.  It will
> then look nicer in print (and will be quoted in Info).
> 
>> +@item @var{01}
>> +Address @var{address} is tagged.
>> +
>> +@item @var{00}
>> +Address @var{address} is not tagged.
> 
> I think 01 and 00 should not be in @var here, since they are literal
> strings.  The @samp markup of the @table will do here.
> 
>> +@item E @var{nn}
>> +An error occurred.  This means that address could not be checked for some
>> +reason.
> 
> Here "nn" is the error value, right?  If so, I suggest to say
> 
>    @item E @var{nn}
>    An error occurred whose code is @var{nn}.

Do you mean remove the "This means that address could not be checked for some reason."
text completely or just s/An error occurred/An error occurred whose code is @var{nn}/?

I'd like to keep the text because the error codes actually don't tell much about
the reason of the error -- usually -- in this context, so I'd like to inform users
that in any case the address could not be checked if an error (any error) occurs.
For example, in the gdbserver we can find comments like:

gdbserver/remote-utils.cc-1020-void
gdbserver/remote-utils.cc-1021-write_enn (char *buf)
gdbserver/remote-utils.cc-1022-{
gdbserver/remote-utils.cc:1023:  /* Some day, we should define the meanings of the error codes... */
gdbserver/remote-utils.cc-1024-  buf[0] = 'E';
gdbserver/remote-utils.cc-1025-  buf[1] = '0';
gdbserver/remote-utils.cc-1026-  buf[2] = '1';
gdbserver/remote-utils.cc-1027-  buf[3] = '\0';
gdbserver/remote-utils.cc-1028-}


Cheers,
Gustavo