diff mbox series

[v4,8/8] gdb: Document qIsAddressTagged packet

Message ID 20240416140728.198163-9-gustavo.romero@linaro.org
State Superseded
Headers show
Series Add another way to check tagged addresses on remote targets | expand

Commit Message

Gustavo Romero April 16, 2024, 2:07 p.m. UTC
This commit documents the qIsAddressTagged packet.

Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
Reviewed-by: Eli Zaretskii <eliz@gnu.org>
---
 gdb/NEWS            | 10 ++++++++++
 gdb/doc/gdb.texinfo | 37 ++++++++++++++++++++++++++++++++++---
 2 files changed, 44 insertions(+), 3 deletions(-)

Comments

Eli Zaretskii April 16, 2024, 2:34 p.m. UTC | #1
> 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}.

Reviewed-By: Eli Zaretskii <eliz@gnu.org>
Gustavo Romero April 16, 2024, 11:10 p.m. UTC | #2
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
Eli Zaretskii April 17, 2024, 12:09 p.m. UTC | #3
> Cc: gdb-patches@sourceware.org, luis.machado@arm.com,
>  thiago.bauermann@linaro.org, tom@tromey.com
> From: Gustavo Romero <gustavo.romero@linaro.org>
> Date: Tue, 16 Apr 2024 20:10:43 -0300
> 
> Hi Eli,
> 
> Thanks a lot for the review. I just have one question.
> 
> >> +@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}/?

The latter.
Gustavo Romero April 17, 2024, 6:21 p.m. UTC | #4
On 4/17/24 9:09 AM, Eli Zaretskii wrote:
>> Cc: gdb-patches@sourceware.org, luis.machado@arm.com,
>>   thiago.bauermann@linaro.org, tom@tromey.com
>> From: Gustavo Romero <gustavo.romero@linaro.org>
>> Date: Tue, 16 Apr 2024 20:10:43 -0300
>>
>> Hi Eli,
>>
>> Thanks a lot for the review. I just have one question.
>>
>>>> +@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}/?
> 
> The latter.

Thanks for the clarification!


Cheers,
Gustavo
diff mbox series

Patch

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
 
 * GDB now supports the AArch64 Scalable Matrix Extension 2 (SME2), which
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 82a617e9ad3..cb6033bbbee 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -44093,6 +44093,35 @@  although this should not happen given @value{GDBN} will only send this packet
 if the stub has advertised support for memory tagging via @samp{qSupported}.
 @end table
 
+@item qIsAddressTagged:@var{address}
+@cindex check if a given address is in a memory tagged region.
+@cindex @samp{qIsAddressTagged} packet
+@anchor {qIsAddressTagged}
+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
+architecture-specific.
+
+@var{address} is the address to be checked.
+
+Reply:
+@table @samp
+Replies to this packet should all be in two hex digit format, as follows:
+
+@item @var{01}
+Address @var{address} is tagged.
+
+@item @var{00}
+Address @var{address} is not tagged.
+
+@item E @var{nn}
+An error occurred.  This means that address could not be checked for some
+reason.
+
+@item @w{}
+An empty reply indicates that @samp{qIsAddressTagged} is not supported by the
+stub.
+@end table
+
 @item QMemTags:@var{start address},@var{length}:@var{type}:@var{tag bytes}
 @anchor{QMemTags}
 @cindex store memory tags
@@ -45141,9 +45170,11 @@  The remote stub supports and implements the required memory tagging
 functionality and understands the @samp{qMemTags} (@pxref{qMemTags}) and
 @samp{QMemTags} (@pxref{QMemTags}) packets.
 
-For AArch64 GNU/Linux systems, this feature also requires access to the
-@file{/proc/@var{pid}/smaps} file so memory mapping page flags can be inspected.
-This is done via the @samp{vFile} requests.
+For AArch64 GNU/Linux systems, this feature can require access to the
+@file{/proc/@var{pid}/smaps} file so memory mapping page flags can be
+inspected, if @samp{qIsAddressTagged} (@pxref{qIsAddressTagged}) packet
+is not supported by the stub.  Access to the @file{/proc/@var{pid}/smaps}
+file is done via @samp{vFile} requests.
 
 @end table