[PATCH v3 0/7] Add another way to check tagged addresses on remote targets

[PATCH v3 0/7] Add another way to check tagged addresses on remote targets

[Gustavo Romero]

This series introduces a new method to check for memory tagged addresses
on remote targets.

A new remote packet, qMemTagAddrCheck, is introduced along with a
feature associated with it, 'memory-tagging-check-add.' GDB will only
use the new packet to query if an address is tagged when the
'memory-tagging-check-add+' feature is advertised.

This new mechanism allows for checking memory tagged addresses in an
OS-agnostic way, which is necessary when debugging targets that do not
support '/proc/<PID>/smaps', as the current method of reading the smaps
contents fails in such cases.

Since v1:
 - Fixed build error "no match for ‘operator!=’ (operand types are ‘packet_result’ and ‘packet_status’)"
   reported by Linaro CI bot, caused by a last-minute rebase
 - Added instructions on how to test the series on a remote target using
   QEMU gdbstub (-g option) -- see below
 
Since v2:
 - Changed packet name to qMemTagCheckAddr for consistence
 - Documented the new packet in gdb.texinfo and NEWS
 - Changed target hook name to is_address_tagged
 - Fixed several GNU Style nonconformities
 - Split commit that adds the target hook and the qMemTagCheckAddr in
   two commits
 - Tested fallback mechanism using gdbserver (use of vFile requests
   instead of qMemTagCheckAddr)
 - Fixed off-by-one error
 - Changed targe hook signature to take gdbarch as an argument for
   better modularity
 
----

This series can be tested with the 'mte_t' binary found in:
https://people.linaro.org/~gustavo.romero/gdb, using the GDB
'memory-tag print-allocation-tag' command to show the allocation
tag for array pointer 'a'. To download mte_t:

$ wget https://people.linaro.org/~gustavo.romero/gdb/mte_t
$ chmod +x ./mte_t

... or build it from source:

$ wget https://people.linaro.org/~gustavo.romero/gdb/mte_t.c
$ gcc -march=armv8.5-a+memtag -static -g3 -O0 mte_t.c -o mte_t

For example, testing the address check for the aarch64_linux_nat.c
target:

gromero@arm64:~/code$ ~/git/binutils-gdb_remote/build/gdb/gdb -q ./mte_t
Reading symbols from ./mte_t...
(gdb) run
Starting program: /home/gromero/code/mte_t 
a[] address is 0xfffff7ffc000
a[0] = 1 a[1] = 2
0x100fffff7ffc000
a[0] = 3 a[1] = 2
Expecting SIGSEGV...

Program received signal SIGSEGV, Segmentation fault
Memory tag violation
Fault address unavailable.
0x0000000000418658 in write ()
(gdb) bt
#0  0x0000000000418658 in write ()
#1  0x000000000040a3bc in _IO_new_file_write ()
#2  0x0000000000409574 in new_do_write ()
#3  0x000000000040ae20 in _IO_new_do_write ()
#4  0x000000000040b55c in _IO_new_file_overflow ()
#5  0x0000000000407414 in puts ()
#6  0x000000000040088c in main () at mte_t.c:119
(gdb) frame 6
#6  0x000000000040088c in main () at mte_t.c:119
119	            printf("...haven't got one\n");
(gdb) memory-tag print-logical-tag a
$1 = 0x1
(gdb) memory-tag print-allocation-tag &a[16]
$2 = 0x0
(gdb)  # Tag mismatch
(gdb) 


Testing address check on a core file:

gromero@arm64:~/code$ ulimit -c unlimited
gromero@arm64:~/code$ ./mte_t
a[] address is 0xffffb3bcc000
a[0] = 1 a[1] = 2
0x900ffffb3bcc000
a[0] = 3 a[1] = 2
Expecting SIGSEGV...
Segmentation fault (core dumped)
gromero@arm64:~/code$ ~/git/binutils-gdb_remote/build/gdb/gdb -q ./mte_t ./core
Reading symbols from ./mte_t...
[New LWP 256036]
Core was generated by `./mte_t'.
Program terminated with signal SIGSEGV, Segmentation fault
Memory tag violation
Fault address unavailable.
#0  0x0000000000418658 in write ()
(gdb) bt
#0  0x0000000000418658 in write ()
#1  0x000000000040a3bc in _IO_new_file_write ()
#2  0x0000000000409574 in new_do_write ()
#3  0x000000000040ae20 in _IO_new_do_write ()
#4  0x000000000040b55c in _IO_new_file_overflow ()
#5  0x0000000000407414 in puts ()
#6  0x000000000040088c in main () at mte_t.c:119
(gdb) frame 6
#6  0x000000000040088c in main () at mte_t.c:119
119	            printf("...haven't got one\n");
(gdb) memory-tag print-logical-tag a
$1 = 0x9
(gdb) memory-tag print-allocation-tag &a[16]
$2 = 0x0
(gdb) # Tag mismatch 
(gdb) 


Finally, testing the new packet on a remote target using QEMU gdbstub
which supports the new 'memory-tagging-check-add+' feature (WIP).

Clone and build QEMU:

$ git clone --depth=1 --single-branch -b mte https://github.com/gromero/qemu.git
$ mkdir qemu/build && cd qemu/build
$ ../configure --target-list=aarch64-linux-user --disable-docs
$ make -j
$ wget https://people.linaro.org/~gustavo.romero/gdb/mte_t
$ chmod +x ./mte_t
$ ./qemu-aarch64 -g 1234 ./mte_t

... and connect to QEMU gdbstub from GDB:

gromero@amd:~/git/binutils-gdb/build$ ./gdb/gdb -q
(gdb) target remote localhost:1234 
Remote debugging using localhost:1234
Reading /tmp/qemu/build/mte_t from remote target...
warning: File transfers from remote targets can be slow. Use "set sysroot" to access files locally instead.
Reading /tmp/qemu/build/mte_t from remote target...
Reading symbols from target:/tmp/qemu/build/mte_t...
(gdb) c
Continuing.

Program received signal SIGSEGV, Segmentation fault
Memory tag violation
Fault address unavailable.
0x0000000000407290 in puts ()
(gdb) bt
#0  0x0000000000407290 in puts ()
#1  0x000000000040088c in main () at mte_t.c:119
(gdb) frame 1 
#1  0x000000000040088c in main () at mte_t.c:119
119	
(gdb) memory-tag print-allocation-tag a
$1 = 0x2
(gdb) set debug remote on
(gdb) memory-tag print-allocation-tag a
[remote] Sending packet: $qMemTagAddrCheck:200400000802000#1f
[remote] Received Ack
[remote] Packet received: 01
[remote] Sending packet: $qMemTags:400000802000,1:1#6f
[remote] Received Ack
[remote] Packet received: m02
$2 = 0x2
(gdb) 


Also, below is a test of the fallback mechanism using the gdbserver,
which must use vFile requests instead of the new packet:

In one terminal:

gromero@arm64:~/git/binutils-gdb_remote/build$ ./gdbserver/gdbserver localhost:1234 /home/gromero/code/mte_t

... in another terminal:

gromero@arm64:~/git/binutils-gdb_remote/build$ gdb/gdb -q 
(gdb) target remote localhost:1234 
Remote debugging using localhost:1234
Reading /home/gromero/code/mte_t from remote target...
warning: File transfers from remote targets can be slow. Use "set sysroot" to access files locally instead.
Reading /home/gromero/code/mte_t from remote target...
Reading symbols from target:/home/gromero/code/mte_t...
Reading /home/gromero/.local/lib/debug/.build-id/a1/fb8db7731a11f85efa2ae80005bdb590796021.debug from remote target...
Reading /usr/lib/debug/.build-id/a1/fb8db7731a11f85efa2ae80005bdb590796021.debug from remote target...
0x0000000000400580 in _start ()
(gdb) b 103
Breakpoint 1 at 0x400818: file mte_t.c, line 103.
(gdb) c
Continuing.

Breakpoint 1, main () at mte_t.c:103
103	            set_tag(a);
(gdb) n
105	            printf("%p\n", a);
(gdb) set debug remote on 
(gdb) memory-tag print-allocation-tag a 
[remote] Sending packet: $m400948,4#06
[remote] Packet received: 3f030094
[remote] Sending packet: $m400944,4#02
[remote] Packet received: 60003fd6
[remote] Sending packet: $m400948,4#06
[remote] Packet received: 3f030094
[remote] Sending packet: $vFile:setfs:0#bf
[remote] Packet received: F0
[remote] Sending packet: $vFile:open:2f70726f632f3236353634362f736d617073,0,1c0#b0
[remote] Packet received: F8
[remote] remote_hostio_pread: readahead cache miss 28
[remote] Sending packet: $vFile:pread:8,2001f,0#5f
[remote] Packet received: Feaa;00400000-0047e000 r-xp 00000000 fe:02 5663492                            /home/gromero/code/mte_t\nSize:                504 kB\nKernelPageSize:        4 kB\nMMUPageSize:           4 kB\nRss:                 440 kB\nPss:                 440 kB\nPss_Dirty:            12 kB\nShared_Clean:          0 kB\nShared_Dirty:          0 kB\nPrivate_Clean:       428 kB\nPrivate_Dirty:        12 kB\nReferenced:          440 kB\nAnonymous:            12 kB\nKSM:                   0 kB\nLazyFree:              0 kB\nAnonHugePages:    [3247 bytes omitted]
[remote] remote_hostio_pread: readahead cache miss 29
[remote] Sending packet: $vFile:pread:8,2001f,eaa#56
[remote] Packet received: Fb96;fffff7ffc000-fffff7ffd000 rw-p 00000000 00:00 0 \nSize:                  4 kB\nKernelPageSize:        4 kB\nMMUPageSize:           4 kB\nRss:                   4 kB\nPss:                   4 kB\nPss_Dirty:             4 kB\nShared_Clean:          0 kB\nShared_Dirty:          0 kB\nPrivate_Clean:         0 kB\nPrivate_Dirty:         4 kB\nReferenced:            4 kB\nAnonymous:             4 kB\nKSM:                   0 kB\nLazyFree:              0 kB\nAnonHugePages:         0 kB\nShmemPmdMapped:        0 kB\nFilePmdMap [2459 bytes omitted]
[remote] remote_hostio_pread: readahead cache miss 30
[remote] Sending packet: $vFile:pread:8,2001f,1a40#25
[remote] Packet received: F0;
[remote] Sending packet: $vFile:close:8#b8
[remote] Packet received: F0
[remote] Sending packet: $qMemTags:fffff7ffc000,1:1#15
[remote] Packet received: m0e
$1 = 0xe
(gdb)


Cheers,
Gustavo

Gustavo Romero (7):
  gdb: aarch64: Remove MTE address checking from get_memtag
  gdb: aarch64: Move MTE address check out of set_memtag
  gdb: aarch64: Remove MTE address checking from memtag_matches_p
  gdb: Use passed gdbarch instead of calling current_inferior
  gdb: Introduce is_address_tagged target hook
  gdb: Add qMemTagAddrCheck packet
  gdb: Document qMemTagCheckAddr packet

 gdb/NEWS                  |  9 ++++++
 gdb/aarch64-linux-nat.c   |  8 +++++
 gdb/aarch64-linux-tdep.c  | 22 +++-----------
 gdb/arch-utils.c          |  2 +-
 gdb/arch-utils.h          |  2 +-
 gdb/corelow.c             |  8 +++++
 gdb/doc/gdb.texinfo       | 50 +++++++++++++++++++++++++++++--
 gdb/gdbarch-gen.h         |  4 +--
 gdb/gdbarch.c             |  2 +-
 gdb/gdbarch_components.py |  2 +-
 gdb/printcmd.c            | 32 ++++++++++++--------
 gdb/remote.c              | 62 +++++++++++++++++++++++++++++++++++++++
 gdb/target-delegates.c    | 30 +++++++++++++++++++
 gdb/target.c              |  6 ++++
 gdb/target.h              |  6 ++++
 15 files changed, 206 insertions(+), 39 deletions(-)

-- 
2.34.1

[PATCH v3 1/7] gdb: aarch64: Remove MTE address checking from get_memtag

[Gustavo Romero]

This commit removes aarch64_linux_tagged_address_p from
aarch64_linux_get_memtag. aarch64_linux_tagged_address_p checks if an
address is tagged (MTE) or not.

The check is redundant because aarch64_linux_get_memtag is always called
from the upper layers (i.e. from printcmd.c via gdbarch hook
gdbarch_get_memtag) after either gdbarch_tagged_address_p (that already
points to aarch64_linux_tagged_address_p) has been called or
after should_validate_memtags (that calls gdbarch_tagged_address_p at
the end) has been called, so the address is already checked. Hence:

a) in print_command_1, aarch64_linux_get_memtag (via gdbarch_get_memtag
hook) is called but only after should_validate_memtags, which calls
gdbarch_tagged_address_p;

b) in do_examine, aarch64_linux_get_memtag is also called only after
gdbarch_tagged_address_p is directly called;

c) in memory_tag_check_command, gdbarch_get_memtag is called -- tags
matching or not -- after the initial check via direct call to
gdbarch_tagged_address_p;

d) in memory_tag_print_tag_command, address is checked directly via
gdbarch_tagged_address_p before gdbarch_get_memtag is called.

Also, because after this change the address checking only happens at the
upper layer it now allows the address checking to be specialized easily
per target, via a target hook.

Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
---
 gdb/aarch64-linux-tdep.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/gdb/aarch64-linux-tdep.c b/gdb/aarch64-linux-tdep.c
index 0b9784f38e4..50055ac3f48 100644
--- a/gdb/aarch64-linux-tdep.c
+++ b/gdb/aarch64-linux-tdep.c
@@ -2575,10 +2575,6 @@ aarch64_linux_get_memtag (struct gdbarch *gdbarch, struct value *address,
     tag = aarch64_mte_get_ltag (addr);
   else
     {
-      /* Make sure we are dealing with a tagged address to begin with.  */
-      if (!aarch64_linux_tagged_address_p (gdbarch, address))
-	return nullptr;
-
       /* Remove the top byte.  */
       addr = gdbarch_remove_non_address_bits (gdbarch, addr);
       std::optional<CORE_ADDR> atag = aarch64_mte_get_atag (addr);
-- 
2.34.1

[PATCH v3 2/7] gdb: aarch64: Move MTE address check out of set_memtag

[Gustavo Romero]

Move MTE address check out of set_memtag and add this check to the
upper layer, before set_memtag is called. This is a preparation for
using a target hook instead of a gdbarch hook MTE address checks.

Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
---
 gdb/aarch64-linux-tdep.c | 4 ----
 gdb/printcmd.c           | 5 +++++
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/gdb/aarch64-linux-tdep.c b/gdb/aarch64-linux-tdep.c
index 50055ac3f48..8e6e63d4dcb 100644
--- a/gdb/aarch64-linux-tdep.c
+++ b/gdb/aarch64-linux-tdep.c
@@ -2525,10 +2525,6 @@ aarch64_linux_set_memtags (struct gdbarch *gdbarch, struct value *address,
       /* Remove the top byte.  */
       addr = gdbarch_remove_non_address_bits (gdbarch, addr);
 
-      /* Make sure we are dealing with a tagged address to begin with.  */
-      if (!aarch64_linux_tagged_address_p (gdbarch, address))
-	return false;
-
       /* With G being the number of tag granules and N the number of tags
 	 passed in, we can have the following cases:
 
diff --git a/gdb/printcmd.c b/gdb/printcmd.c
index cb0d32aa4bc..774e3ec74ae 100644
--- a/gdb/printcmd.c
+++ b/gdb/printcmd.c
@@ -3127,6 +3127,11 @@ memory_tag_set_allocation_tag_command (const char *args, int from_tty)
   /* Parse the input.  */
   parse_set_allocation_tag_input (args, &val, &length, tags);
 
+  /* If the address is not in a region memory mapped with a memory tagging
+     flag, it is no use trying to manipulate its allocation tag.  */
+  if (!gdbarch_tagged_address_p (current_inferior ()->arch (), val))
+    show_addr_not_tagged (value_as_address(val));
+
   if (!gdbarch_set_memtags (current_inferior ()->arch (), val, length, tags,
 			    memtag_type::allocation))
     gdb_printf (_("Could not update the allocation tag(s).\n"));
-- 
2.34.1

[PATCH v3 3/7] gdb: aarch64: Remove MTE address checking from memtag_matches_p

[Gustavo Romero]

This commit removes aarch64_linux_tagged_address_p from
aarch64_linux_memtag_matches_p. aarch64_linux_tagged_address_p checks if
an address is tagged (MTE) or not.

The check is redundant because aarch64_linux_memtag_matches_p is always
called from the upper layers (i.e. from printcmd.c via gdbarch hook
gdbarch_memtag_matches_p) after either gdbarch_tagged_address_p (that
already points to aarch64_linux_tagged_address_p) has been called or
after should_validate_memtags (that calls gdbarch_tagged_address_p at
the end) has been called, so the address is already checked. Hence:

a) in print_command_1, gdbarch_memtag_matches_p is called only after
should_validate_memtags is called, which checks the address at its end;

b) in memory_tag_check_command, gdbarch_memtag_matches_p is called only
after gdbarch_tagged_address_p is called directly.

Also, because after this change the address checking only happens at the
upper layer it now allows the address checking to be specialized easily
per target, via a target hook.

Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
---
 gdb/aarch64-linux-tdep.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/gdb/aarch64-linux-tdep.c b/gdb/aarch64-linux-tdep.c
index 8e6e63d4dcb..fc60e602748 100644
--- a/gdb/aarch64-linux-tdep.c
+++ b/gdb/aarch64-linux-tdep.c
@@ -2476,10 +2476,6 @@ aarch64_linux_memtag_matches_p (struct gdbarch *gdbarch,
 {
   gdb_assert (address != nullptr);
 
-  /* Make sure we are dealing with a tagged address to begin with.  */
-  if (!aarch64_linux_tagged_address_p (gdbarch, address))
-    return true;
-
   CORE_ADDR addr = value_as_address (address);
 
   /* Fetch the allocation tag for ADDRESS.  */
-- 
2.34.1

[PATCH v3 4/7] gdb: Use passed gdbarch instead of calling current_inferior

[Gustavo Romero]

In do_examine function, use passed gdbarch when checking if an address
is tagged instead of calling current_inferior()->arch() to make the code
more localized and help modularity by not calling a current_* function,
which disguises the use of a global state/variable. There is no change
in the code behavior.

Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
---
 gdb/printcmd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/printcmd.c b/gdb/printcmd.c
index 774e3ec74ae..62fbcb98cfb 100644
--- a/gdb/printcmd.c
+++ b/gdb/printcmd.c
@@ -1132,7 +1132,7 @@ do_examine (struct format_data fmt, struct gdbarch *gdbarch, CORE_ADDR addr)
 	    = value_from_ulongest (builtin_type (gdbarch)->builtin_data_ptr,
 				   tag_laddr);
 
-	  if (gdbarch_tagged_address_p (current_inferior ()->arch  (), v_addr))
+	  if (gdbarch_tagged_address_p (gdbarch, v_addr))
 	    {
 	      /* Fetch the allocation tag.  */
 	      struct value *tag
-- 
2.34.1

[PATCH v3 5/7] gdb: Introduce is_address_tagged target hook

[Gustavo Romero]

This commit introduces a new target hook, target_is_address_tagged,
which is used instead of the gdbarch_tagged_address_p gdbarch hook in
the upper layer (printcmd.c).

This change allows the memory tagging address checking to be specialized
easily per target in the future. Since target_is_address_tagged
continues to use the gdbarch_tagged_address_p hook there is no change
in behavior for the targets using the new target hook (the remote.c,
aarch64-linux-nat.c, and corelow.c targets).

This change enables easy specialization of memory tagging address
check per target in the future. As target_is_address_tagged continues
to utilize the gdbarch_tagged_address_p hook, there is no change in
behavior for all the targets that use the new target hook (i.e., the
remote.c, aarch64-linux-nat.c, and corelow.c targets).

Just the gdbarch_tagged_address_p signature is changed for convenience,
since target_is_address_tagged takes the address to be checked as a
CORE_ADDR type.

Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
---
 gdb/aarch64-linux-nat.c   |  8 ++++++++
 gdb/aarch64-linux-tdep.c  | 10 ++++------
 gdb/arch-utils.c          |  2 +-
 gdb/arch-utils.h          |  2 +-
 gdb/corelow.c             |  8 ++++++++
 gdb/gdbarch-gen.h         |  4 ++--
 gdb/gdbarch.c             |  2 +-
 gdb/gdbarch_components.py |  2 +-
 gdb/printcmd.c            | 31 +++++++++++++++++--------------
 gdb/remote.c              | 10 ++++++++++
 gdb/target-delegates.c    | 30 ++++++++++++++++++++++++++++++
 gdb/target.c              |  6 ++++++
 gdb/target.h              |  6 ++++++
 13 files changed, 95 insertions(+), 26 deletions(-)

diff --git a/gdb/aarch64-linux-nat.c b/gdb/aarch64-linux-nat.c
index 3face34ce79..19c099832a5 100644
--- a/gdb/aarch64-linux-nat.c
+++ b/gdb/aarch64-linux-nat.c
@@ -110,6 +110,8 @@ class aarch64_linux_nat_target final
   /* Write allocation tags to memory via PTRACE.  */
   bool store_memtags (CORE_ADDR address, size_t len,
 		      const gdb::byte_vector &tags, int type) override;
+  /* Check if an address is tagged.  */
+  bool is_address_tagged (gdbarch *gdbarch, CORE_ADDR address) override;
 };
 
 static aarch64_linux_nat_target the_aarch64_linux_nat_target;
@@ -1071,6 +1073,12 @@ aarch64_linux_nat_target::store_memtags (CORE_ADDR address, size_t len,
   return false;
 }
 
+bool
+aarch64_linux_nat_target::is_address_tagged (gdbarch *gdbarch, CORE_ADDR address)
+{
+  return gdbarch_tagged_address_p (gdbarch, address);
+}
+
 void _initialize_aarch64_linux_nat ();
 void
 _initialize_aarch64_linux_nat ()
diff --git a/gdb/aarch64-linux-tdep.c b/gdb/aarch64-linux-tdep.c
index fc60e602748..2a47c3f0845 100644
--- a/gdb/aarch64-linux-tdep.c
+++ b/gdb/aarch64-linux-tdep.c
@@ -2451,17 +2451,15 @@ aarch64_mte_get_atag (CORE_ADDR address)
 /* Implement the tagged_address_p gdbarch method.  */
 
 static bool
-aarch64_linux_tagged_address_p (struct gdbarch *gdbarch, struct value *address)
+aarch64_linux_tagged_address_p (struct gdbarch *gdbarch, CORE_ADDR address)
 {
-  gdb_assert (address != nullptr);
-
-  CORE_ADDR addr = value_as_address (address);
+  gdb_assert (address);
 
   /* Remove the top byte for the memory range check.  */
-  addr = gdbarch_remove_non_address_bits (gdbarch, addr);
+  address = gdbarch_remove_non_address_bits (gdbarch, address);
 
   /* Check if the page that contains ADDRESS is mapped with PROT_MTE.  */
-  if (!linux_address_in_memtag_page (addr))
+  if (!linux_address_in_memtag_page (address))
     return false;
 
   /* We have a valid tag in the top byte of the 64-bit address.  */
diff --git a/gdb/arch-utils.c b/gdb/arch-utils.c
index 456bfe971ff..cb149c36bc9 100644
--- a/gdb/arch-utils.c
+++ b/gdb/arch-utils.c
@@ -102,7 +102,7 @@ default_memtag_to_string (struct gdbarch *gdbarch, struct value *tag)
 /* See arch-utils.h */
 
 bool
-default_tagged_address_p (struct gdbarch *gdbarch, struct value *address)
+default_tagged_address_p (struct gdbarch *gdbarch, CORE_ADDR address)
 {
   /* By default, assume the address is untagged.  */
   return false;
diff --git a/gdb/arch-utils.h b/gdb/arch-utils.h
index 2dcd8f6dc53..467be40c688 100644
--- a/gdb/arch-utils.h
+++ b/gdb/arch-utils.h
@@ -141,7 +141,7 @@ extern std::string default_memtag_to_string (struct gdbarch *gdbarch,
 					     struct value *tag);
 
 /* Default implementation of gdbarch_tagged_address_p.  */
-bool default_tagged_address_p (struct gdbarch *gdbarch, struct value *address);
+bool default_tagged_address_p (struct gdbarch *gdbarch, CORE_ADDR address);
 
 /* Default implementation of gdbarch_memtag_matches_p.  */
 extern bool default_memtag_matches_p (struct gdbarch *gdbarch,
diff --git a/gdb/corelow.c b/gdb/corelow.c
index f4e8273d962..b13d0124471 100644
--- a/gdb/corelow.c
+++ b/gdb/corelow.c
@@ -109,6 +109,8 @@ class core_target final : public process_stratum_target
   bool fetch_memtags (CORE_ADDR address, size_t len,
 		      gdb::byte_vector &tags, int type) override;
 
+  bool is_address_tagged (gdbarch *gdbarch, CORE_ADDR address) override;
+
   x86_xsave_layout fetch_x86_xsave_layout () override;
 
   /* A few helpers.  */
@@ -1410,6 +1412,12 @@ core_target::fetch_memtags (CORE_ADDR address, size_t len,
   return false;
 }
 
+bool
+core_target::is_address_tagged (gdbarch *gdbarch, CORE_ADDR address)
+{
+  return gdbarch_tagged_address_p (gdbarch, address);
+}
+
 /* Implementation of the "fetch_x86_xsave_layout" target_ops method.  */
 
 x86_xsave_layout
diff --git a/gdb/gdbarch-gen.h b/gdb/gdbarch-gen.h
index ebcff80bb9e..63fab26987f 100644
--- a/gdb/gdbarch-gen.h
+++ b/gdb/gdbarch-gen.h
@@ -707,8 +707,8 @@ extern void set_gdbarch_memtag_to_string (struct gdbarch *gdbarch, gdbarch_memta
 /* Return true if ADDRESS contains a tag and false otherwise.  ADDRESS
    must be either a pointer or a reference type. */
 
-typedef bool (gdbarch_tagged_address_p_ftype) (struct gdbarch *gdbarch, struct value *address);
-extern bool gdbarch_tagged_address_p (struct gdbarch *gdbarch, struct value *address);
+typedef bool (gdbarch_tagged_address_p_ftype) (struct gdbarch *gdbarch, CORE_ADDR address);
+extern bool gdbarch_tagged_address_p (struct gdbarch *gdbarch, CORE_ADDR address);
 extern void set_gdbarch_tagged_address_p (struct gdbarch *gdbarch, gdbarch_tagged_address_p_ftype *tagged_address_p);
 
 /* Return true if the tag from ADDRESS matches the memory tag for that
diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
index 9319571deba..2d92f604c49 100644
--- a/gdb/gdbarch.c
+++ b/gdb/gdbarch.c
@@ -3232,7 +3232,7 @@ set_gdbarch_memtag_to_string (struct gdbarch *gdbarch,
 }
 
 bool
-gdbarch_tagged_address_p (struct gdbarch *gdbarch, struct value *address)
+gdbarch_tagged_address_p (struct gdbarch *gdbarch, CORE_ADDR address)
 {
   gdb_assert (gdbarch != NULL);
   gdb_assert (gdbarch->tagged_address_p != NULL);
diff --git a/gdb/gdbarch_components.py b/gdb/gdbarch_components.py
index 7d913ade621..24e979431b6 100644
--- a/gdb/gdbarch_components.py
+++ b/gdb/gdbarch_components.py
@@ -1267,7 +1267,7 @@ must be either a pointer or a reference type.
 """,
     type="bool",
     name="tagged_address_p",
-    params=[("struct value *", "address")],
+    params=[("CORE_ADDR", "address")],
     predefault="default_tagged_address_p",
     invalid=False,
 )
diff --git a/gdb/printcmd.c b/gdb/printcmd.c
index 62fbcb98cfb..24eac7c8421 100644
--- a/gdb/printcmd.c
+++ b/gdb/printcmd.c
@@ -1132,7 +1132,7 @@ do_examine (struct format_data fmt, struct gdbarch *gdbarch, CORE_ADDR addr)
 	    = value_from_ulongest (builtin_type (gdbarch)->builtin_data_ptr,
 				   tag_laddr);
 
-	  if (gdbarch_tagged_address_p (gdbarch, v_addr))
+	  if (target_is_address_tagged (gdbarch, value_as_address (v_addr)))
 	    {
 	      /* Fetch the allocation tag.  */
 	      struct value *tag
@@ -1268,7 +1268,7 @@ print_value (value *val, const value_print_options &opts)
 /* Returns true if memory tags should be validated.  False otherwise.  */
 
 static bool
-should_validate_memtags (struct value *value)
+should_validate_memtags (gdbarch *gdbarch, struct value *value)
 {
   gdb_assert (value != nullptr && value->type () != nullptr);
 
@@ -1289,7 +1289,7 @@ should_validate_memtags (struct value *value)
     return false;
 
   /* We do.  Check whether it includes any tags.  */
-  return gdbarch_tagged_address_p (current_inferior ()->arch  (), value);
+  return target_is_address_tagged (gdbarch, value_as_address (value));
 }
 
 /* Helper for parsing arguments for print_command_1.  */
@@ -1346,7 +1346,7 @@ print_command_1 (const char *args, int voidprint)
 	    {
 	      gdbarch *arch = current_inferior ()->arch ();
 
-	      if (should_validate_memtags (val)
+	      if (should_validate_memtags (arch, val)
 		  && !gdbarch_memtag_matches_p (arch, val))
 		{
 		  /* Fetch the logical tag.  */
@@ -2946,9 +2946,10 @@ memory_tag_print_tag_command (const char *args, enum memtag_type tag_type)
      flag, it is no use trying to access/manipulate its allocation tag.
 
      It is OK to manipulate the logical tag though.  */
+  CORE_ADDR addr = value_as_address (val);
   if (tag_type == memtag_type::allocation
-      && !gdbarch_tagged_address_p (arch, val))
-    show_addr_not_tagged (value_as_address (val));
+      && !target_is_address_tagged (arch, addr))
+    show_addr_not_tagged (addr);
 
   value *tag_value = gdbarch_get_memtag (arch, val, tag_type);
   std::string tag = gdbarch_memtag_to_string (arch, tag_value);
@@ -3104,8 +3105,9 @@ parse_set_allocation_tag_input (const char *args, struct value **val,
 
   /* If the address is not in a region memory mapped with a memory tagging
      flag, it is no use trying to access/manipulate its allocation tag.  */
-  if (!gdbarch_tagged_address_p (current_inferior ()->arch (), *val))
-    show_addr_not_tagged (value_as_address (*val));
+  CORE_ADDR addr = value_as_address (*val);
+  if (!target_is_address_tagged (current_inferior ()->arch (), addr))
+    show_addr_not_tagged (addr);
 }
 
 /* Implement the "memory-tag set-allocation-tag" command.
@@ -3129,8 +3131,9 @@ memory_tag_set_allocation_tag_command (const char *args, int from_tty)
 
   /* If the address is not in a region memory mapped with a memory tagging
      flag, it is no use trying to manipulate its allocation tag.  */
-  if (!gdbarch_tagged_address_p (current_inferior ()->arch (), val))
-    show_addr_not_tagged (value_as_address(val));
+  CORE_ADDR addr = value_as_address (val);
+  if (!target_is_address_tagged (current_inferior ()-> arch(), addr))
+    show_addr_not_tagged (addr);
 
   if (!gdbarch_set_memtags (current_inferior ()->arch (), val, length, tags,
 			    memtag_type::allocation))
@@ -3157,12 +3160,12 @@ memory_tag_check_command (const char *args, int from_tty)
   struct value *val = process_print_command_args (args, &print_opts, true);
   gdbarch *arch = current_inferior ()->arch ();
 
+  CORE_ADDR addr = value_as_address (val);
+
   /* If the address is not in a region memory mapped with a memory tagging
      flag, it is no use trying to access/manipulate its allocation tag.  */
-  if (!gdbarch_tagged_address_p (arch, val))
-    show_addr_not_tagged (value_as_address (val));
-
-  CORE_ADDR addr = value_as_address (val);
+  if (!target_is_address_tagged (current_inferior ()->arch (), addr))
+    show_addr_not_tagged (addr);
 
   /* Check if the tag is valid.  */
   if (!gdbarch_memtag_matches_p (arch, val))
diff --git a/gdb/remote.c b/gdb/remote.c
index e278711df7b..9717db55e27 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -1084,6 +1084,8 @@ class remote_target : public process_stratum_target
   bool store_memtags (CORE_ADDR address, size_t len,
 		      const gdb::byte_vector &tags, int type) override;
 
+  bool is_address_tagged (gdbarch *gdbarch, CORE_ADDR address) override;
+
 public: /* Remote specific methods.  */
 
   void remote_download_command_source (int num, ULONGEST addr,
@@ -15573,6 +15575,14 @@ remote_target::store_memtags (CORE_ADDR address, size_t len,
   return packet_check_result (rs->buf).status () == PACKET_OK;
 }
 
+/* Implement the "is_address_tagged" target_ops method.  */
+
+bool
+remote_target::is_address_tagged (gdbarch *gdbarch, CORE_ADDR address)
+{
+  return gdbarch_tagged_address_p (gdbarch, address);
+}
+
 /* Return true if remote target T is non-stop.  */
 
 bool
diff --git a/gdb/target-delegates.c b/gdb/target-delegates.c
index 59ea70458ad..e322bbbe481 100644
--- a/gdb/target-delegates.c
+++ b/gdb/target-delegates.c
@@ -197,6 +197,7 @@ struct dummy_target : public target_ops
   bool supports_memory_tagging () override;
   bool fetch_memtags (CORE_ADDR arg0, size_t arg1, gdb::byte_vector &arg2, int arg3) override;
   bool store_memtags (CORE_ADDR arg0, size_t arg1, const gdb::byte_vector &arg2, int arg3) override;
+  bool is_address_tagged (gdbarch *arg0, CORE_ADDR arg1) override;
   x86_xsave_layout fetch_x86_xsave_layout () override;
 };
 
@@ -373,6 +374,7 @@ struct debug_target : public target_ops
   bool supports_memory_tagging () override;
   bool fetch_memtags (CORE_ADDR arg0, size_t arg1, gdb::byte_vector &arg2, int arg3) override;
   bool store_memtags (CORE_ADDR arg0, size_t arg1, const gdb::byte_vector &arg2, int arg3) override;
+  bool is_address_tagged (gdbarch *arg0, CORE_ADDR arg1) override;
   x86_xsave_layout fetch_x86_xsave_layout () override;
 };
 
@@ -4562,6 +4564,34 @@ debug_target::store_memtags (CORE_ADDR arg0, size_t arg1, const gdb::byte_vector
   return result;
 }
 
+bool
+target_ops::is_address_tagged (gdbarch *arg0, CORE_ADDR arg1)
+{
+  return this->beneath ()->is_address_tagged (arg0, arg1);
+}
+
+bool
+dummy_target::is_address_tagged (gdbarch *arg0, CORE_ADDR arg1)
+{
+  tcomplain ();
+}
+
+bool
+debug_target::is_address_tagged (gdbarch *arg0, CORE_ADDR arg1)
+{
+  gdb_printf (gdb_stdlog, "-> %s->is_address_tagged (...)\n", this->beneath ()->shortname ());
+  bool result
+    = this->beneath ()->is_address_tagged (arg0, arg1);
+  gdb_printf (gdb_stdlog, "<- %s->is_address_tagged (", this->beneath ()->shortname ());
+  target_debug_print_gdbarch_p (arg0);
+  gdb_puts (", ", gdb_stdlog);
+  target_debug_print_CORE_ADDR (arg1);
+  gdb_puts (") = ", gdb_stdlog);
+  target_debug_print_bool (result);
+  gdb_puts ("\n", gdb_stdlog);
+  return result;
+}
+
 x86_xsave_layout
 target_ops::fetch_x86_xsave_layout ()
 {
diff --git a/gdb/target.c b/gdb/target.c
index 107a84b3ca1..5c3c1a57dbd 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -796,6 +796,12 @@ target_store_memtags (CORE_ADDR address, size_t len,
   return current_inferior ()->top_target ()->store_memtags (address, len, tags, type);
 }
 
+bool
+target_is_address_tagged (gdbarch *gdbarch, CORE_ADDR address)
+{
+  return current_inferior ()->top_target ()->is_address_tagged (gdbarch, address);
+}
+
 x86_xsave_layout
 target_fetch_x86_xsave_layout ()
 {
diff --git a/gdb/target.h b/gdb/target.h
index c9eaff16346..486a0a687b0 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -1334,6 +1334,10 @@ struct target_ops
 				const gdb::byte_vector &tags, int type)
       TARGET_DEFAULT_NORETURN (tcomplain ());
 
+    /* Returns true if ADDRESS is tagged, otherwise returns false.  */
+    virtual bool is_address_tagged (gdbarch *gdbarch, CORE_ADDR address)
+      TARGET_DEFAULT_NORETURN (tcomplain ());
+
     /* Return the x86 XSAVE extended state area layout.  */
     virtual x86_xsave_layout fetch_x86_xsave_layout ()
       TARGET_DEFAULT_RETURN (x86_xsave_layout ());
@@ -2317,6 +2321,8 @@ extern bool target_fetch_memtags (CORE_ADDR address, size_t len,
 extern bool target_store_memtags (CORE_ADDR address, size_t len,
 				  const gdb::byte_vector &tags, int type);
 
+extern bool target_is_address_tagged (gdbarch *gdbarch, CORE_ADDR address);
+
 extern x86_xsave_layout target_fetch_x86_xsave_layout ();
 
 /* Command logging facility.  */
-- 
2.34.1

[PATCH v3 6/7] gdb: Add qMemTagAddrCheck packet

[Gustavo Romero]

This commit adds a new packet, qMemTagAddrCheck, allowing GDB remote
targets to use it to query the stub if a given address is tagged.

It also adds a new GDB remote feature, 'memory-tagging-check-addr',
which must be advertised by the GDB stub to inform it can reply to
memory tagging address checks via the new qMemTagCheckAddr packet.

Currently, the memory tagging address check is done via a read query,
where the contents of /proc/<PID>/smaps is read and the flags are
inspected for memory tagging-related flags that indicate the address is
in a memory tagged region.

This is not ideal, for example, for QEMU stub and other cases, such as
on bare-metal, where there is no notion of an OS file like 'smaps.'
Hence, the qMemTagCheckAddr packet allows checking addresses in an
OS-agnostic way.

The is_address_tagged target hook in remote.c uses the qMemTagCheckAddr
packet to check an address if the stub advertises the
'memory-tagging-check-add+' feature, otherwise it falls back to using
the current mechanism, which reads the contents of /proc/<PID>/smaps via
vFile requests.

Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
---
 gdb/remote.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 53 insertions(+), 1 deletion(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index 9717db55e27..94ac8520740 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -337,6 +337,9 @@ enum {
      packets and the tag violation stop replies.  */
   PACKET_memory_tagging_feature,
 
+  /* Support checking if an address is tagged via qMemTagCheckAddr packet.  */
+  PACKET_memory_tagging_check_addr_feature,
+
   PACKET_MAX
 };
 
@@ -758,6 +761,10 @@ struct remote_features
   bool remote_memory_tagging_p () const
   { return packet_support (PACKET_memory_tagging_feature) == PACKET_ENABLE; }
 
+  bool remote_memory_tagging_check_addr_p () const
+  { return packet_support (PACKET_memory_tagging_check_addr_feature) ==
+			   PACKET_ENABLE; }
+
   /* Reset all packets back to "unknown support".  Called when opening a
      new connection to a remote target.  */
   void reset_all_packet_configs_support ();
@@ -5764,6 +5771,8 @@ static const struct protocol_feature remote_protocol_features[] = {
   { "no-resumed", PACKET_DISABLE, remote_supported_packet, PACKET_no_resumed },
   { "memory-tagging", PACKET_DISABLE, remote_supported_packet,
     PACKET_memory_tagging_feature },
+  { "memory-tagging-check-addr", PACKET_DISABLE, remote_supported_packet,
+    PACKET_memory_tagging_check_addr_feature },
 };
 
 static char *remote_support_xml;
@@ -5875,6 +5884,10 @@ remote_target::remote_query_supported ()
 	  != AUTO_BOOLEAN_FALSE)
 	remote_query_supported_append (&q, "memory-tagging+");
 
+      if (m_features.packet_set_cmd_state (PACKET_memory_tagging_check_addr_feature)
+	  != AUTO_BOOLEAN_FALSE)
+	remote_query_supported_append (&q, "memory-tagging-check-addr+");
+
       /* Keep this one last to work around a gdbserver <= 7.10 bug in
 	 the qSupported:xmlRegisters=i386 handling.  */
       if (remote_support_xml != NULL
@@ -15534,6 +15547,21 @@ create_store_memtags_request (gdb::char_vector &packet, CORE_ADDR address,
   strcpy (packet.data (), request.c_str ());
 }
 
+static void
+create_is_address_tagged_request (gdb::char_vector &packet, CORE_ADDR address)
+{
+  int addr_size;
+  std::string request;
+
+  addr_size = gdbarch_addr_bit (current_inferior ()->arch()) / 8;
+  request = string_printf ("qMemTagCheckAddr:%s", phex_nz (address, addr_size));
+
+  if (packet.size () < request.length () + 1)
+    error (_("Contents too big for packet qMemTagCheckAddr."));
+
+  strcpy (packet.data (), request.c_str ());
+}
+
 /* Implement the "fetch_memtags" target_ops method.  */
 
 bool
@@ -15580,7 +15608,27 @@ remote_target::store_memtags (CORE_ADDR address, size_t len,
 bool
 remote_target::is_address_tagged (gdbarch *gdbarch, CORE_ADDR address)
 {
-  return gdbarch_tagged_address_p (gdbarch, address);
+  struct remote_state *rs = get_remote_state ();
+
+  if (!m_features.remote_memory_tagging_check_addr_p ())
+    /* Fallback to arch-specific method of checking whether an address is
+       tagged.  */
+    return gdbarch_tagged_address_p (gdbarch, address);
+
+  create_is_address_tagged_request (rs->buf, address);
+
+  putpkt (rs->buf);
+  getpkt (&rs->buf);
+
+  /* Check if reply is OK.  */
+  if (packet_check_result (rs->buf).status () != PACKET_OK)
+    return false;
+
+  gdb_byte tagged_addr;
+  /* Convert only 2 hex digits, i.e. 1 byte in hex format.  */
+  hex2bin (rs->buf.data (), &tagged_addr , 1);
+
+  return tagged_addr != 0;
 }
 
 /* Return true if remote target T is non-stop.  */
@@ -16066,6 +16114,10 @@ Show the maximum size of the address (in bits) in a memory packet."), NULL,
   add_packet_config_cmd (PACKET_memory_tagging_feature,
 			 "memory-tagging-feature", "memory-tagging-feature", 0);
 
+  add_packet_config_cmd (PACKET_memory_tagging_check_addr_feature,
+			 "memory-tagging-check-addr-feature",
+			 "memory-tagging-check-addr-feature", 0);
+
   /* Assert that we've registered "set remote foo-packet" commands
      for all packet configs.  */
   {
-- 
2.34.1

[PATCH v3 7/7] gdb: Document qMemTagCheckAddr packet

[Gustavo Romero]

This commit documents the 'qMemTagCheckAddr' packet and the associated
feature 'memory-tagging-check-addr' that informs if this packet is
supported by the target.

Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
---
 gdb/NEWS            |  9 ++++++++
 gdb/doc/gdb.texinfo | 50 ++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 56 insertions(+), 3 deletions(-)

diff --git a/gdb/NEWS b/gdb/NEWS
index feb3a37393a..1158715f41c 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -192,6 +192,15 @@ QThreadOptions in qSupported
   QThreadOptions packet, and the qSupported response can contain the
   set of thread options the remote stub supports.
 
+qMemTagCheckAddr
+  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..afd57ff874c 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -44093,6 +44093,37 @@ 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 qMemTagCheckAddr:@var{address}
+@anchor {qMemTagCheckAddr}
+@cindex check if a given address is in a memory tagged region.
+@cindex @samp{qMemTagCheckAddr} packet
+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 are in a 2 hex digits format.
+
+@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{qMemTagCheckAddr} is not supported by the
+stub.  This situation should not occur because @value{GDBN} will only send this
+packet if the stub has advertised support for memory tagging check via the
+@samp{memory-tagging-check-addr} feature in @samp{qSupported}.
+@end table
+
 @item QMemTags:@var{start address},@var{length}:@var{type}:@var{tag bytes}
 @anchor{QMemTags}
 @cindex store memory tags
@@ -44914,6 +44945,11 @@ These are the currently defined stub features and their properties:
 @tab @samp{-}
 @tab No
 
+@item @samp{memory-tagging-check-addr}
+@tab No
+@tab @samp{-}
+@tab No
+
 @end multitable
 
 These are the currently defined stub features, in more detail:
@@ -45141,9 +45177,17 @@ 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{memory-tagging-check-addr} feature (see below) is not supported by the
+stub. Access to the @file{/proc/@var{pid}/smaps} file is done via @samp{vFile}
+requests.
+
+@item memory-tagging-check-addr
+The remote stub supports and implements the required memory tagging address
+check functionality and understands the @samp{qMemTagCheckAddr}
+(@pxref{qMemTagCheckAddr}) packet, which is used for address checking instead
+of accessing the @file{/proc/@var{pid}/smaps} file.
 
 @end table
 
-- 
2.34.1

Re: [PATCH v3 7/7] gdb: Document qMemTagCheckAddr packet

[Eli Zaretskii]

> From: Gustavo Romero <gustavo.romero@linaro.org>
> Cc: luis.machado@arm.com, thiago.bauermann@linaro.org,
>  gustavo.romero@linaro.org
> Date: Thu,  4 Apr 2024 06:48:19 +0000
> 
> This commit documents the 'qMemTagCheckAddr' packet and the associated
> feature 'memory-tagging-check-addr' that informs if this packet is
> supported by the target.
> 
> Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
> ---
>  gdb/NEWS            |  9 ++++++++
>  gdb/doc/gdb.texinfo | 50 ++++++++++++++++++++++++++++++++++++++++++---
>  2 files changed, 56 insertions(+), 3 deletions(-)

Thanks.

> diff --git a/gdb/NEWS b/gdb/NEWS
> index feb3a37393a..1158715f41c 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -192,6 +192,15 @@ QThreadOptions in qSupported
>    QThreadOptions packet, and the qSupported response can contain the
>    set of thread options the remote stub supports.
>  
> +qMemTagCheckAddr
> +  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.

Please reformat this text so that each line is shorter than 78
columns.  Also, please leave two spaces between sentences, per our
conventions.

> +@item qMemTagCheckAddr:@var{address}
> +@anchor {qMemTagCheckAddr}
> +@cindex check if a given address is in a memory tagged region.
> +@cindex @samp{qMemTagCheckAddr} packet
> +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.

Likewise here: please make the lines shorter.

Also, the @cindex lines should come _before_ the @item line, not after
it, so that the index entry records the location before the @item.

> +Reply:
> +@table @samp
> +Replies are in a 2 hex digits format.

This should be before @table, like this:

  Replies to this packet should all be in two hex digit format, as
  follows:

  @item @var{01}

> +stub. Access to the @file{/proc/@var{pid}/smaps} file is done via @samp{vFile}
       ^^
Two spaces between sentences, please.

Reviewed-By: Eli Zaretskii <eliz@gnu.org>

Re: [PATCH v3 1/7] gdb: aarch64: Remove MTE address checking from get_memtag

[Luis Machado]

On 4/4/24 07:48, Gustavo Romero wrote:
> This commit removes aarch64_linux_tagged_address_p from
> aarch64_linux_get_memtag. aarch64_linux_tagged_address_p checks if an
> address is tagged (MTE) or not.
> 
> The check is redundant because aarch64_linux_get_memtag is always called
> from the upper layers (i.e. from printcmd.c via gdbarch hook
> gdbarch_get_memtag) after either gdbarch_tagged_address_p (that already
> points to aarch64_linux_tagged_address_p) has been called or
> after should_validate_memtags (that calls gdbarch_tagged_address_p at
> the end) has been called, so the address is already checked. Hence:
> 
> a) in print_command_1, aarch64_linux_get_memtag (via gdbarch_get_memtag
> hook) is called but only after should_validate_memtags, which calls
> gdbarch_tagged_address_p;
> 
> b) in do_examine, aarch64_linux_get_memtag is also called only after
> gdbarch_tagged_address_p is directly called;
> 
> c) in memory_tag_check_command, gdbarch_get_memtag is called -- tags
> matching or not -- after the initial check via direct call to
> gdbarch_tagged_address_p;
> 
> d) in memory_tag_print_tag_command, address is checked directly via
> gdbarch_tagged_address_p before gdbarch_get_memtag is called.
> 
> Also, because after this change the address checking only happens at the
> upper layer it now allows the address checking to be specialized easily
> per target, via a target hook.
> 
> Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
> ---
>  gdb/aarch64-linux-tdep.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/gdb/aarch64-linux-tdep.c b/gdb/aarch64-linux-tdep.c
> index 0b9784f38e4..50055ac3f48 100644
> --- a/gdb/aarch64-linux-tdep.c
> +++ b/gdb/aarch64-linux-tdep.c
> @@ -2575,10 +2575,6 @@ aarch64_linux_get_memtag (struct gdbarch *gdbarch, struct value *address,
>      tag = aarch64_mte_get_ltag (addr);
>    else
>      {
> -      /* Make sure we are dealing with a tagged address to begin with.  */
> -      if (!aarch64_linux_tagged_address_p (gdbarch, address))
> -	return nullptr;
> -
>        /* Remove the top byte.  */
>        addr = gdbarch_remove_non_address_bits (gdbarch, addr);
>        std::optional<CORE_ADDR> atag = aarch64_mte_get_atag (addr);

Thanks. This is OK and can go in separately, or alongside the rest of the series if you wish.

Approved-By: Luis Machado <luis.machado@arm.com>
Tested-By: Luis Machado <luis.machado@arm.com>

Re: [PATCH v3 2/7] gdb: aarch64: Move MTE address check out of set_memtag

[Luis Machado]

On 4/4/24 07:48, Gustavo Romero wrote:
> Move MTE address check out of set_memtag and add this check to the
> upper layer, before set_memtag is called. This is a preparation for
> using a target hook instead of a gdbarch hook MTE address checks.
> 
> Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
> ---
>  gdb/aarch64-linux-tdep.c | 4 ----
>  gdb/printcmd.c           | 5 +++++
>  2 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/gdb/aarch64-linux-tdep.c b/gdb/aarch64-linux-tdep.c
> index 50055ac3f48..8e6e63d4dcb 100644
> --- a/gdb/aarch64-linux-tdep.c
> +++ b/gdb/aarch64-linux-tdep.c
> @@ -2525,10 +2525,6 @@ aarch64_linux_set_memtags (struct gdbarch *gdbarch, struct value *address,
>        /* Remove the top byte.  */
>        addr = gdbarch_remove_non_address_bits (gdbarch, addr);
>  
> -      /* Make sure we are dealing with a tagged address to begin with.  */
> -      if (!aarch64_linux_tagged_address_p (gdbarch, address))
> -	return false;
> -
>        /* With G being the number of tag granules and N the number of tags
>  	 passed in, we can have the following cases:
>  
> diff --git a/gdb/printcmd.c b/gdb/printcmd.c
> index cb0d32aa4bc..774e3ec74ae 100644
> --- a/gdb/printcmd.c
> +++ b/gdb/printcmd.c
> @@ -3127,6 +3127,11 @@ memory_tag_set_allocation_tag_command (const char *args, int from_tty)
>    /* Parse the input.  */
>    parse_set_allocation_tag_input (args, &val, &length, tags);
>  
> +  /* If the address is not in a region memory mapped with a memory tagging

A nit:

s/memory mapped/memory-mapped ?

> +     flag, it is no use trying to manipulate its allocation tag.  */
> +  if (!gdbarch_tagged_address_p (current_inferior ()->arch (), val))
> +    show_addr_not_tagged (value_as_address(val));
> +

Looks like memory_tag_set_allocation_tag_command calls parse_set_allocation_tag_input,
and the latter has the exact same check as you are adding here. Is this then redundant?


>    if (!gdbarch_set_memtags (current_inferior ()->arch (), val, length, tags,
>  			    memtag_type::allocation))
>      gdb_printf (_("Could not update the allocation tag(s).\n"));

Re: [PATCH v3 3/7] gdb: aarch64: Remove MTE address checking from memtag_matches_p

[Luis Machado]

On 4/4/24 07:48, Gustavo Romero wrote:
> This commit removes aarch64_linux_tagged_address_p from
> aarch64_linux_memtag_matches_p. aarch64_linux_tagged_address_p checks if
> an address is tagged (MTE) or not.
> 
> The check is redundant because aarch64_linux_memtag_matches_p is always
> called from the upper layers (i.e. from printcmd.c via gdbarch hook
> gdbarch_memtag_matches_p) after either gdbarch_tagged_address_p (that
> already points to aarch64_linux_tagged_address_p) has been called or
> after should_validate_memtags (that calls gdbarch_tagged_address_p at
> the end) has been called, so the address is already checked. Hence:
> 
> a) in print_command_1, gdbarch_memtag_matches_p is called only after
> should_validate_memtags is called, which checks the address at its end;
> 
> b) in memory_tag_check_command, gdbarch_memtag_matches_p is called only
> after gdbarch_tagged_address_p is called directly.
> 
> Also, because after this change the address checking only happens at the
> upper layer it now allows the address checking to be specialized easily
> per target, via a target hook.
> 
> Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
> ---
>  gdb/aarch64-linux-tdep.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/gdb/aarch64-linux-tdep.c b/gdb/aarch64-linux-tdep.c
> index 8e6e63d4dcb..fc60e602748 100644
> --- a/gdb/aarch64-linux-tdep.c
> +++ b/gdb/aarch64-linux-tdep.c
> @@ -2476,10 +2476,6 @@ aarch64_linux_memtag_matches_p (struct gdbarch *gdbarch,
>  {
>    gdb_assert (address != nullptr);
>  
> -  /* Make sure we are dealing with a tagged address to begin with.  */
> -  if (!aarch64_linux_tagged_address_p (gdbarch, address))
> -    return true;
> -
>    CORE_ADDR addr = value_as_address (address);
>  
>    /* Fetch the allocation tag for ADDRESS.  */

Thanks. This is OK and can go in separately, or alongside the rest of the series if you wish.

Approved-By: Luis Machado <luis.machado@arm.com>
Tested-By: Luis Machado <luis.machado@arm.com>

Re: [PATCH v3 4/7] gdb: Use passed gdbarch instead of calling current_inferior

[Luis Machado]

On 4/4/24 07:48, Gustavo Romero wrote:
> In do_examine function, use passed gdbarch when checking if an address
> is tagged instead of calling current_inferior()->arch() to make the code
> more localized and help modularity by not calling a current_* function,
> which disguises the use of a global state/variable. There is no change
> in the code behavior.
> 
> Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
> ---
>  gdb/printcmd.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/gdb/printcmd.c b/gdb/printcmd.c
> index 774e3ec74ae..62fbcb98cfb 100644
> --- a/gdb/printcmd.c
> +++ b/gdb/printcmd.c
> @@ -1132,7 +1132,7 @@ do_examine (struct format_data fmt, struct gdbarch *gdbarch, CORE_ADDR addr)
>  	    = value_from_ulongest (builtin_type (gdbarch)->builtin_data_ptr,
>  				   tag_laddr);
>  
> -	  if (gdbarch_tagged_address_p (current_inferior ()->arch  (), v_addr))
> +	  if (gdbarch_tagged_address_p (gdbarch, v_addr))
>  	    {
>  	      /* Fetch the allocation tag.  */
>  	      struct value *tag

Thanks. This is OK and can go in separately, or alongside the rest of the series if you wish.

Approved-By: Luis Machado <luis.machado@arm.com>
Tested-By: Luis Machado <luis.machado@arm.com>

Re: [PATCH v3 5/7] gdb: Introduce is_address_tagged target hook

[Luis Machado]

On 4/4/24 07:48, Gustavo Romero wrote:
> This commit introduces a new target hook, target_is_address_tagged,
> which is used instead of the gdbarch_tagged_address_p gdbarch hook in
> the upper layer (printcmd.c).
> 
> This change allows the memory tagging address checking to be specialized
> easily per target in the future. Since target_is_address_tagged
> continues to use the gdbarch_tagged_address_p hook there is no change
> in behavior for the targets using the new target hook (the remote.c,
> aarch64-linux-nat.c, and corelow.c targets).

The above block...

> 
> This change enables easy specialization of memory tagging address
> check per target in the future. As target_is_address_tagged continues
> to utilize the gdbarch_tagged_address_p hook, there is no change in
> behavior for all the targets that use the new target hook (i.e., the
> remote.c, aarch64-linux-nat.c, and corelow.c targets).

... seems to be somewhat duplicated above in the commit message.

Also, as general rule, we usually make updates clear at the top of the
commit message. for instace:

Updates in v3:

- Something something.
- Fixed breakage.

And then those update blocks don't get pushed when the series is
approved (sometimes some do push it).

> 
> Just the gdbarch_tagged_address_p signature is changed for convenience,
> since target_is_address_tagged takes the address to be checked as a
> CORE_ADDR type.
> 
> Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
> ---
>  gdb/aarch64-linux-nat.c   |  8 ++++++++
>  gdb/aarch64-linux-tdep.c  | 10 ++++------
>  gdb/arch-utils.c          |  2 +-
>  gdb/arch-utils.h          |  2 +-
>  gdb/corelow.c             |  8 ++++++++
>  gdb/gdbarch-gen.h         |  4 ++--
>  gdb/gdbarch.c             |  2 +-
>  gdb/gdbarch_components.py |  2 +-
>  gdb/printcmd.c            | 31 +++++++++++++++++--------------
>  gdb/remote.c              | 10 ++++++++++
>  gdb/target-delegates.c    | 30 ++++++++++++++++++++++++++++++
>  gdb/target.c              |  6 ++++++
>  gdb/target.h              |  6 ++++++
>  13 files changed, 95 insertions(+), 26 deletions(-)
> 
> diff --git a/gdb/aarch64-linux-nat.c b/gdb/aarch64-linux-nat.c
> index 3face34ce79..19c099832a5 100644
> --- a/gdb/aarch64-linux-nat.c
> +++ b/gdb/aarch64-linux-nat.c
> @@ -110,6 +110,8 @@ class aarch64_linux_nat_target final
>    /* Write allocation tags to memory via PTRACE.  */
>    bool store_memtags (CORE_ADDR address, size_t len,
>  		      const gdb::byte_vector &tags, int type) override;
> +  /* Check if an address is tagged.  */
> +  bool is_address_tagged (gdbarch *gdbarch, CORE_ADDR address) override;
>  };
>  
>  static aarch64_linux_nat_target the_aarch64_linux_nat_target;
> @@ -1071,6 +1073,12 @@ aarch64_linux_nat_target::store_memtags (CORE_ADDR address, size_t len,
>    return false;
>  }
>  
> +bool
> +aarch64_linux_nat_target::is_address_tagged (gdbarch *gdbarch, CORE_ADDR address)
> +{
> +  return gdbarch_tagged_address_p (gdbarch, address);

I'd add a comment here on why we take a detour going to the linux-tdep layer
to read the smaps file, because we don't have a better way to get that information.

In the future, if this check is ever made available through PTRACE, one can come
here and drop the smaps path.

> +}
> +
>  void _initialize_aarch64_linux_nat ();
>  void
>  _initialize_aarch64_linux_nat ()
> diff --git a/gdb/aarch64-linux-tdep.c b/gdb/aarch64-linux-tdep.c
> index fc60e602748..2a47c3f0845 100644
> --- a/gdb/aarch64-linux-tdep.c
> +++ b/gdb/aarch64-linux-tdep.c
> @@ -2451,17 +2451,15 @@ aarch64_mte_get_atag (CORE_ADDR address)
>  /* Implement the tagged_address_p gdbarch method.  */
>  
>  static bool
> -aarch64_linux_tagged_address_p (struct gdbarch *gdbarch, struct value *address)
> +aarch64_linux_tagged_address_p (struct gdbarch *gdbarch, CORE_ADDR address)
>  {
> -  gdb_assert (address != nullptr);
> -
> -  CORE_ADDR addr = value_as_address (address);
> +  gdb_assert (address);

No need to assert for a non-zero address. We used to assert that we had a valid pointer,
but since we're no longer dealing with a pointer, we don't have to worry about it.

Plus, 0x0 is a valid address (at least for bare-metal. We could run into an assertion if
the user explicitly tries to check for tags in a 0x0 address. See below:

(gdb) memory-tag check 0x0
../../../repos/binutils-gdb/gdb/aarch64-linux-tdep.c:2456: internal-error: aarch64_linux_tagged_address_p: Assertion `address' failed.
A problem internal to GDB has been detected,
further debugging may prove unreliable.
----- Backtrace -----
0xaaaac69ccf97 _Z22gdb_internal_backtracev
0xaaaac6e443e7 _ZL17internal_vproblemP16internal_problemPKciS2_St9__va_list
0xaaaac6e4464f _Z15internal_verrorPKciS0_St9__va_list
0xaaaac734a8b3 _Z18internal_error_locPKciS0_z
0xaaaac68dc407 _ZL30aarch64_linux_tagged_address_pP7gdbarchm
0xaaaac6c849ab _ZL24memory_tag_check_commandPKci
0xaaaac69fb8ab _Z8cmd_funcP16cmd_list_elementPKci
0xaaaac6de5ed3 _Z15execute_commandPKci
0xaaaac6adc203 _Z15command_handlerPKc
0xaaaac6add45f _Z20command_line_handlerOSt10unique_ptrIcN3gdb13xfree_deleterIcEEE
0xaaaac6adccd7 _ZL23gdb_rl_callback_handlerPc
0xaaaac6ea7eeb rl_callback_read_char
0xaaaac6adbc7b _ZL42gdb_rl_callback_read_char_wrapper_noexceptv
0xaaaac6adcb3b _ZL33gdb_rl_callback_read_char_wrapperPv
0xaaaac6e1ef5f _ZL19stdin_event_handleriPv
0xaaaac734b29f _ZL18gdb_wait_for_eventi.part.0
0xaaaac734bc7f _Z16gdb_do_one_eventi
0xaaaac6bdd977 _ZL21captured_command_loopv
0xaaaac6bdf6bb _Z8gdb_mainP18captured_main_args
0xaaaac68cee47 main

>  
>    /* Remove the top byte for the memory range check.  */
> -  addr = gdbarch_remove_non_address_bits (gdbarch, addr);
> +  address = gdbarch_remove_non_address_bits (gdbarch, address);
>  
>    /* Check if the page that contains ADDRESS is mapped with PROT_MTE.  */
> -  if (!linux_address_in_memtag_page (addr))
> +  if (!linux_address_in_memtag_page (address))
>      return false;
>  
>    /* We have a valid tag in the top byte of the 64-bit address.  */
> diff --git a/gdb/arch-utils.c b/gdb/arch-utils.c
> index 456bfe971ff..cb149c36bc9 100644
> --- a/gdb/arch-utils.c
> +++ b/gdb/arch-utils.c
> @@ -102,7 +102,7 @@ default_memtag_to_string (struct gdbarch *gdbarch, struct value *tag)
>  /* See arch-utils.h */
>  
>  bool
> -default_tagged_address_p (struct gdbarch *gdbarch, struct value *address)
> +default_tagged_address_p (struct gdbarch *gdbarch, CORE_ADDR address)
>  {
>    /* By default, assume the address is untagged.  */
>    return false;
> diff --git a/gdb/arch-utils.h b/gdb/arch-utils.h
> index 2dcd8f6dc53..467be40c688 100644
> --- a/gdb/arch-utils.h
> +++ b/gdb/arch-utils.h
> @@ -141,7 +141,7 @@ extern std::string default_memtag_to_string (struct gdbarch *gdbarch,
>  					     struct value *tag);
>  
>  /* Default implementation of gdbarch_tagged_address_p.  */
> -bool default_tagged_address_p (struct gdbarch *gdbarch, struct value *address);
> +bool default_tagged_address_p (struct gdbarch *gdbarch, CORE_ADDR address);
>  
>  /* Default implementation of gdbarch_memtag_matches_p.  */
>  extern bool default_memtag_matches_p (struct gdbarch *gdbarch,
> diff --git a/gdb/corelow.c b/gdb/corelow.c
> index f4e8273d962..b13d0124471 100644
> --- a/gdb/corelow.c
> +++ b/gdb/corelow.c
> @@ -109,6 +109,8 @@ class core_target final : public process_stratum_target
>    bool fetch_memtags (CORE_ADDR address, size_t len,
>  		      gdb::byte_vector &tags, int type) override;
>  
> +  bool is_address_tagged (gdbarch *gdbarch, CORE_ADDR address) override;
> +

Maybe add a comment to this function stating what we're looking for:

/* If the architecture supports it, check if ADDRESS is within a memory range
   mapped with tags.  For example,  MTE tags for AArch64.  */

Or some other variation of the above.

The reason being that the corelow target is a bit special because it is generic, hence
why we see an override for a x86 target hook in there as well.

>    x86_xsave_layout fetch_x86_xsave_layout () override;
>  
>    /* A few helpers.  */
> @@ -1410,6 +1412,12 @@ core_target::fetch_memtags (CORE_ADDR address, size_t len,
>    return false;
>  }
>  
> +bool
> +core_target::is_address_tagged (gdbarch *gdbarch, CORE_ADDR address)
> +{
> +  return gdbarch_tagged_address_p (gdbarch, address);
> +}
> +
>  /* Implementation of the "fetch_x86_xsave_layout" target_ops method.  */
>  
>  x86_xsave_layout
> diff --git a/gdb/gdbarch-gen.h b/gdb/gdbarch-gen.h
> index ebcff80bb9e..63fab26987f 100644
> --- a/gdb/gdbarch-gen.h
> +++ b/gdb/gdbarch-gen.h
> @@ -707,8 +707,8 @@ extern void set_gdbarch_memtag_to_string (struct gdbarch *gdbarch, gdbarch_memta
>  /* Return true if ADDRESS contains a tag and false otherwise.  ADDRESS
>     must be either a pointer or a reference type. */
>  
> -typedef bool (gdbarch_tagged_address_p_ftype) (struct gdbarch *gdbarch, struct value *address);
> -extern bool gdbarch_tagged_address_p (struct gdbarch *gdbarch, struct value *address);
> +typedef bool (gdbarch_tagged_address_p_ftype) (struct gdbarch *gdbarch, CORE_ADDR address);
> +extern bool gdbarch_tagged_address_p (struct gdbarch *gdbarch, CORE_ADDR address);
>  extern void set_gdbarch_tagged_address_p (struct gdbarch *gdbarch, gdbarch_tagged_address_p_ftype *tagged_address_p);
>  
>  /* Return true if the tag from ADDRESS matches the memory tag for that
> diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
> index 9319571deba..2d92f604c49 100644
> --- a/gdb/gdbarch.c
> +++ b/gdb/gdbarch.c
> @@ -3232,7 +3232,7 @@ set_gdbarch_memtag_to_string (struct gdbarch *gdbarch,
>  }
>  
>  bool
> -gdbarch_tagged_address_p (struct gdbarch *gdbarch, struct value *address)
> +gdbarch_tagged_address_p (struct gdbarch *gdbarch, CORE_ADDR address)
>  {
>    gdb_assert (gdbarch != NULL);
>    gdb_assert (gdbarch->tagged_address_p != NULL);
> diff --git a/gdb/gdbarch_components.py b/gdb/gdbarch_components.py
> index 7d913ade621..24e979431b6 100644
> --- a/gdb/gdbarch_components.py
> +++ b/gdb/gdbarch_components.py
> @@ -1267,7 +1267,7 @@ must be either a pointer or a reference type.
>  """,
>      type="bool",
>      name="tagged_address_p",
> -    params=[("struct value *", "address")],
> +    params=[("CORE_ADDR", "address")],
>      predefault="default_tagged_address_p",
>      invalid=False,
>  )
> diff --git a/gdb/printcmd.c b/gdb/printcmd.c
> index 62fbcb98cfb..24eac7c8421 100644
> --- a/gdb/printcmd.c
> +++ b/gdb/printcmd.c
> @@ -1132,7 +1132,7 @@ do_examine (struct format_data fmt, struct gdbarch *gdbarch, CORE_ADDR addr)
>  	    = value_from_ulongest (builtin_type (gdbarch)->builtin_data_ptr,
>  				   tag_laddr);
>  
> -	  if (gdbarch_tagged_address_p (gdbarch, v_addr))
> +	  if (target_is_address_tagged (gdbarch, value_as_address (v_addr)))
>  	    {
>  	      /* Fetch the allocation tag.  */
>  	      struct value *tag
> @@ -1268,7 +1268,7 @@ print_value (value *val, const value_print_options &opts)
>  /* Returns true if memory tags should be validated.  False otherwise.  */
>  
>  static bool
> -should_validate_memtags (struct value *value)
> +should_validate_memtags (gdbarch *gdbarch, struct value *value)
>  {
>    gdb_assert (value != nullptr && value->type () != nullptr);
>  
> @@ -1289,7 +1289,7 @@ should_validate_memtags (struct value *value)
>      return false;
>  
>    /* We do.  Check whether it includes any tags.  */
> -  return gdbarch_tagged_address_p (current_inferior ()->arch  (), value);
> +  return target_is_address_tagged (gdbarch, value_as_address (value));
>  }
>  
>  /* Helper for parsing arguments for print_command_1.  */
> @@ -1346,7 +1346,7 @@ print_command_1 (const char *args, int voidprint)
>  	    {
>  	      gdbarch *arch = current_inferior ()->arch ();
>  
> -	      if (should_validate_memtags (val)
> +	      if (should_validate_memtags (arch, val)
>  		  && !gdbarch_memtag_matches_p (arch, val))
>  		{
>  		  /* Fetch the logical tag.  */
> @@ -2946,9 +2946,10 @@ memory_tag_print_tag_command (const char *args, enum memtag_type tag_type)
>       flag, it is no use trying to access/manipulate its allocation tag.
>  
>       It is OK to manipulate the logical tag though.  */
> +  CORE_ADDR addr = value_as_address (val);
>    if (tag_type == memtag_type::allocation
> -      && !gdbarch_tagged_address_p (arch, val))
> -    show_addr_not_tagged (value_as_address (val));
> +      && !target_is_address_tagged (arch, addr))
> +    show_addr_not_tagged (addr);
>  
>    value *tag_value = gdbarch_get_memtag (arch, val, tag_type);
>    std::string tag = gdbarch_memtag_to_string (arch, tag_value);
> @@ -3104,8 +3105,9 @@ parse_set_allocation_tag_input (const char *args, struct value **val,
>  
>    /* If the address is not in a region memory mapped with a memory tagging
>       flag, it is no use trying to access/manipulate its allocation tag.  */
> -  if (!gdbarch_tagged_address_p (current_inferior ()->arch (), *val))
> -    show_addr_not_tagged (value_as_address (*val));
> +  CORE_ADDR addr = value_as_address (*val);
> +  if (!target_is_address_tagged (current_inferior ()->arch (), addr))
> +    show_addr_not_tagged (addr);
>  }
>  
>  /* Implement the "memory-tag set-allocation-tag" command.
> @@ -3129,8 +3131,9 @@ memory_tag_set_allocation_tag_command (const char *args, int from_tty)
>  
>    /* If the address is not in a region memory mapped with a memory tagging
>       flag, it is no use trying to manipulate its allocation tag.  */
> -  if (!gdbarch_tagged_address_p (current_inferior ()->arch (), val))
> -    show_addr_not_tagged (value_as_address(val));
> +  CORE_ADDR addr = value_as_address (val);
> +  if (!target_is_address_tagged (current_inferior ()-> arch(), addr))
> +    show_addr_not_tagged (addr);
>  
>    if (!gdbarch_set_memtags (current_inferior ()->arch (), val, length, tags,
>  			    memtag_type::allocation))
> @@ -3157,12 +3160,12 @@ memory_tag_check_command (const char *args, int from_tty)
>    struct value *val = process_print_command_args (args, &print_opts, true);
>    gdbarch *arch = current_inferior ()->arch ();
>  
> +  CORE_ADDR addr = value_as_address (val);
> +
>    /* If the address is not in a region memory mapped with a memory tagging
>       flag, it is no use trying to access/manipulate its allocation tag.  */
> -  if (!gdbarch_tagged_address_p (arch, val))
> -    show_addr_not_tagged (value_as_address (val));
> -
> -  CORE_ADDR addr = value_as_address (val);
> +  if (!target_is_address_tagged (current_inferior ()->arch (), addr))
> +    show_addr_not_tagged (addr);

I noticed the above checks happen at least 3 times in the code. Maybe a future
cleanup possibility to split the check into a separate function.

>  
>    /* Check if the tag is valid.  */
>    if (!gdbarch_memtag_matches_p (arch, val))
> diff --git a/gdb/remote.c b/gdb/remote.c
> index e278711df7b..9717db55e27 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -1084,6 +1084,8 @@ class remote_target : public process_stratum_target
>    bool store_memtags (CORE_ADDR address, size_t len,
>  		      const gdb::byte_vector &tags, int type) override;
>  
> +  bool is_address_tagged (gdbarch *gdbarch, CORE_ADDR address) override;
> +
>  public: /* Remote specific methods.  */
>  
>    void remote_download_command_source (int num, ULONGEST addr,
> @@ -15573,6 +15575,14 @@ remote_target::store_memtags (CORE_ADDR address, size_t len,
>    return packet_check_result (rs->buf).status () == PACKET_OK;
>  }
>  
> +/* Implement the "is_address_tagged" target_ops method.  */
> +
> +bool
> +remote_target::is_address_tagged (gdbarch *gdbarch, CORE_ADDR address)
> +{
> +  return gdbarch_tagged_address_p (gdbarch, address);
> +}
> +
>  /* Return true if remote target T is non-stop.  */
>  
>  bool
> diff --git a/gdb/target-delegates.c b/gdb/target-delegates.c
> index 59ea70458ad..e322bbbe481 100644
> --- a/gdb/target-delegates.c
> +++ b/gdb/target-delegates.c
> @@ -197,6 +197,7 @@ struct dummy_target : public target_ops
>    bool supports_memory_tagging () override;
>    bool fetch_memtags (CORE_ADDR arg0, size_t arg1, gdb::byte_vector &arg2, int arg3) override;
>    bool store_memtags (CORE_ADDR arg0, size_t arg1, const gdb::byte_vector &arg2, int arg3) override;
> +  bool is_address_tagged (gdbarch *arg0, CORE_ADDR arg1) override;
>    x86_xsave_layout fetch_x86_xsave_layout () override;
>  };
>  
> @@ -373,6 +374,7 @@ struct debug_target : public target_ops
>    bool supports_memory_tagging () override;
>    bool fetch_memtags (CORE_ADDR arg0, size_t arg1, gdb::byte_vector &arg2, int arg3) override;
>    bool store_memtags (CORE_ADDR arg0, size_t arg1, const gdb::byte_vector &arg2, int arg3) override;
> +  bool is_address_tagged (gdbarch *arg0, CORE_ADDR arg1) override;
>    x86_xsave_layout fetch_x86_xsave_layout () override;
>  };
>  
> @@ -4562,6 +4564,34 @@ debug_target::store_memtags (CORE_ADDR arg0, size_t arg1, const gdb::byte_vector
>    return result;
>  }
>  
> +bool
> +target_ops::is_address_tagged (gdbarch *arg0, CORE_ADDR arg1)
> +{
> +  return this->beneath ()->is_address_tagged (arg0, arg1);
> +}
> +
> +bool
> +dummy_target::is_address_tagged (gdbarch *arg0, CORE_ADDR arg1)
> +{
> +  tcomplain ();
> +}
> +
> +bool
> +debug_target::is_address_tagged (gdbarch *arg0, CORE_ADDR arg1)
> +{
> +  gdb_printf (gdb_stdlog, "-> %s->is_address_tagged (...)\n", this->beneath ()->shortname ());
> +  bool result
> +    = this->beneath ()->is_address_tagged (arg0, arg1);
> +  gdb_printf (gdb_stdlog, "<- %s->is_address_tagged (", this->beneath ()->shortname ());
> +  target_debug_print_gdbarch_p (arg0);
> +  gdb_puts (", ", gdb_stdlog);
> +  target_debug_print_CORE_ADDR (arg1);
> +  gdb_puts (") = ", gdb_stdlog);
> +  target_debug_print_bool (result);
> +  gdb_puts ("\n", gdb_stdlog);
> +  return result;
> +}
> +
>  x86_xsave_layout
>  target_ops::fetch_x86_xsave_layout ()
>  {
> diff --git a/gdb/target.c b/gdb/target.c
> index 107a84b3ca1..5c3c1a57dbd 100644
> --- a/gdb/target.c
> +++ b/gdb/target.c
> @@ -796,6 +796,12 @@ target_store_memtags (CORE_ADDR address, size_t len,
>    return current_inferior ()->top_target ()->store_memtags (address, len, tags, type);
>  }
>  
> +bool
> +target_is_address_tagged (gdbarch *gdbarch, CORE_ADDR address)
> +{
> +  return current_inferior ()->top_target ()->is_address_tagged (gdbarch, address);
> +}
> +
>  x86_xsave_layout
>  target_fetch_x86_xsave_layout ()
>  {
> diff --git a/gdb/target.h b/gdb/target.h
> index c9eaff16346..486a0a687b0 100644
> --- a/gdb/target.h
> +++ b/gdb/target.h
> @@ -1334,6 +1334,10 @@ struct target_ops
>  				const gdb::byte_vector &tags, int type)
>        TARGET_DEFAULT_NORETURN (tcomplain ());
>  
> +    /* Returns true if ADDRESS is tagged, otherwise returns false.  */
> +    virtual bool is_address_tagged (gdbarch *gdbarch, CORE_ADDR address)
> +      TARGET_DEFAULT_NORETURN (tcomplain ());
> +

Should the default for is_address_tagged by to return false, meaning the address is never
tagged?

>      /* Return the x86 XSAVE extended state area layout.  */
>      virtual x86_xsave_layout fetch_x86_xsave_layout ()
>        TARGET_DEFAULT_RETURN (x86_xsave_layout ());
> @@ -2317,6 +2321,8 @@ extern bool target_fetch_memtags (CORE_ADDR address, size_t len,
>  extern bool target_store_memtags (CORE_ADDR address, size_t len,
>  				  const gdb::byte_vector &tags, int type);
>  
> +extern bool target_is_address_tagged (gdbarch *gdbarch, CORE_ADDR address);
> +
>  extern x86_xsave_layout target_fetch_x86_xsave_layout ();
>  
>  /* Command logging facility.  */

Re: [PATCH v3 5/7] gdb: Introduce is_address_tagged target hook

[Gustavo Romero]

Hi Luis,

On 4/4/24 12:45 PM, Luis Machado wrote:
> On 4/4/24 07:48, Gustavo Romero wrote:
>> This commit introduces a new target hook, target_is_address_tagged,
>> which is used instead of the gdbarch_tagged_address_p gdbarch hook in
>> the upper layer (printcmd.c).
>>
>> This change allows the memory tagging address checking to be specialized
>> easily per target in the future. Since target_is_address_tagged
>> continues to use the gdbarch_tagged_address_p hook there is no change
>> in behavior for the targets using the new target hook (the remote.c,
>> aarch64-linux-nat.c, and corelow.c targets).
> 
> The above block...
> 
>>
>> This change enables easy specialization of memory tagging address
>> check per target in the future. As target_is_address_tagged continues
>> to utilize the gdbarch_tagged_address_p hook, there is no change in
>> behavior for all the targets that use the new target hook (i.e., the
>> remote.c, aarch64-linux-nat.c, and corelow.c targets).
> 
> ... seems to be somewhat duplicated above in the commit message.

Right, it's duplicated. I'll drop the first block in v4. Thanks.


> Also, as general rule, we usually make updates clear at the top of the
> commit message. for instace:
> 
> Updates in v3:
> 
> - Something something.
> - Fixed breakage.
> 
> And then those update blocks don't get pushed when the series is
> approved (sometimes some do push it).

hmm, k, I'll put the updates at the top next, however I put them always in the
cover letter and never in the commit messages. Are you recommending putting
them per commit?"


Cheers,
Gustavo

Re: [PATCH v3 6/7] gdb: Add qMemTagAddrCheck packet

[Luis Machado]

On 4/4/24 07:48, Gustavo Romero wrote:
> This commit adds a new packet, qMemTagAddrCheck, allowing GDB remote
> targets to use it to query the stub if a given address is tagged.

I'm not a big fan of the packet name, but naming those things is sometimes hard.

So here goes my attempt. Based on the target hook, how about qAddressIsTagged?

> 
> It also adds a new GDB remote feature, 'memory-tagging-check-addr',

And the above could be adjusted to "memory-tagging-address-check+"?

Just trying to make things a bit more clear and less abbreviated (guilty of
doing that myself sometimes!).

> which must be advertised by the GDB stub to inform it can reply to
> memory tagging address checks via the new qMemTagCheckAddr packet.
> 
> Currently, the memory tagging address check is done via a read query,
> where the contents of /proc/<PID>/smaps is read and the flags are
> inspected for memory tagging-related flags that indicate the address is
> in a memory tagged region.
> 
> This is not ideal, for example, for QEMU stub and other cases, such as
> on bare-metal, where there is no notion of an OS file like 'smaps.'
> Hence, the qMemTagCheckAddr packet allows checking addresses in an
> OS-agnostic way.
> 
> The is_address_tagged target hook in remote.c uses the qMemTagCheckAddr
> packet to check an address if the stub advertises the
> 'memory-tagging-check-add+' feature, otherwise it falls back to using
> the current mechanism, which reads the contents of /proc/<PID>/smaps via
> vFile requests.
> 
> Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
> ---
>  gdb/remote.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 53 insertions(+), 1 deletion(-)
> 
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 9717db55e27..94ac8520740 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -337,6 +337,9 @@ enum {
>       packets and the tag violation stop replies.  */
>    PACKET_memory_tagging_feature,
>  
> +  /* Support checking if an address is tagged via qMemTagCheckAddr packet.  */
> +  PACKET_memory_tagging_check_addr_feature,
> +
>    PACKET_MAX
>  };
>  
> @@ -758,6 +761,10 @@ struct remote_features
>    bool remote_memory_tagging_p () const
>    { return packet_support (PACKET_memory_tagging_feature) == PACKET_ENABLE; }
>  
> +  bool remote_memory_tagging_check_addr_p () const
> +  { return packet_support (PACKET_memory_tagging_check_addr_feature) ==
> +			   PACKET_ENABLE; }
> +
>    /* Reset all packets back to "unknown support".  Called when opening a
>       new connection to a remote target.  */
>    void reset_all_packet_configs_support ();
> @@ -5764,6 +5771,8 @@ static const struct protocol_feature remote_protocol_features[] = {
>    { "no-resumed", PACKET_DISABLE, remote_supported_packet, PACKET_no_resumed },
>    { "memory-tagging", PACKET_DISABLE, remote_supported_packet,
>      PACKET_memory_tagging_feature },
> +  { "memory-tagging-check-addr", PACKET_DISABLE, remote_supported_packet,
> +    PACKET_memory_tagging_check_addr_feature },
>  };
>  
>  static char *remote_support_xml;
> @@ -5875,6 +5884,10 @@ remote_target::remote_query_supported ()
>  	  != AUTO_BOOLEAN_FALSE)
>  	remote_query_supported_append (&q, "memory-tagging+");
>  
> +      if (m_features.packet_set_cmd_state (PACKET_memory_tagging_check_addr_feature)
> +	  != AUTO_BOOLEAN_FALSE)
> +	remote_query_supported_append (&q, "memory-tagging-check-addr+");
> +
>        /* Keep this one last to work around a gdbserver <= 7.10 bug in
>  	 the qSupported:xmlRegisters=i386 handling.  */
>        if (remote_support_xml != NULL
> @@ -15534,6 +15547,21 @@ create_store_memtags_request (gdb::char_vector &packet, CORE_ADDR address,
>    strcpy (packet.data (), request.c_str ());
>  }
>  
> +static void
> +create_is_address_tagged_request (gdb::char_vector &packet, CORE_ADDR address)
> +{
> +  int addr_size;
> +  std::string request;
> +
> +  addr_size = gdbarch_addr_bit (current_inferior ()->arch()) / 8;

You should add another argument to create_is_address_tagged_request to pass in the gdbarch, then
you can use it when invoking gdbarch_addr_bit.

> +  request = string_printf ("qMemTagCheckAddr:%s", phex_nz (address, addr_size));
> +
> +  if (packet.size () < request.length () + 1)
> +    error (_("Contents too big for packet qMemTagCheckAddr."));
> +
> +  strcpy (packet.data (), request.c_str ());
> +}
> +
>  /* Implement the "fetch_memtags" target_ops method.  */
>  
>  bool
> @@ -15580,7 +15608,27 @@ remote_target::store_memtags (CORE_ADDR address, size_t len,
>  bool
>  remote_target::is_address_tagged (gdbarch *gdbarch, CORE_ADDR address)
>  {
> -  return gdbarch_tagged_address_p (gdbarch, address);
> +  struct remote_state *rs = get_remote_state ();
> +
> +  if (!m_features.remote_memory_tagging_check_addr_p ())
> +    /* Fallback to arch-specific method of checking whether an address is
> +       tagged.  */
> +    return gdbarch_tagged_address_p (gdbarch, address);
> +
> +  create_is_address_tagged_request (rs->buf, address);

Here you can pass the gdbarch you've already obtained from the incoming argument
of is_address_tagged.

> +
> +  putpkt (rs->buf);
> +  getpkt (&rs->buf);
> +
> +  /* Check if reply is OK.  */
> +  if (packet_check_result (rs->buf).status () != PACKET_OK)
> +    return false;
> +
> +  gdb_byte tagged_addr;
> +  /* Convert only 2 hex digits, i.e. 1 byte in hex format.  */
> +  hex2bin (rs->buf.data (), &tagged_addr , 1);
> +
> +  return tagged_addr != 0;

The current code assumes any reply != 0 means we have a tagged address. I think we
need to make that a bit more strict/explicit (in the documentation as well).

If we get a reply that is not 00 or 01, should we error out or issue a complaint?

If we plan on extending the packet to account for other return values, we should make
that known in the code and in the documentationm abd take appropriate action. That way
we avoid causing confusion for remote debugging stub implementors.

>  }>  
>  /* Return true if remote target T is non-stop.  */
> @@ -16066,6 +16114,10 @@ Show the maximum size of the address (in bits) in a memory packet."), NULL,
>    add_packet_config_cmd (PACKET_memory_tagging_feature,
>  			 "memory-tagging-feature", "memory-tagging-feature", 0);
>  
> +  add_packet_config_cmd (PACKET_memory_tagging_check_addr_feature,
> +			 "memory-tagging-check-addr-feature",
> +			 "memory-tagging-check-addr-feature", 0);
> +
>    /* Assert that we've registered "set remote foo-packet" commands
>       for all packet configs.  */
>    {

To make sure the new packet is exercised correctly when the gdb testsuite is executed,
it is worth adding some self tests. See remote.c:test_memory_tagging_functions for an
example of how it is done.

Basically we want to validate that gdb always handles things correctly and fails gracefully.

For example, we should handle the following replies:

- EXX (error)
- 00/01 (the expected return values)
- Any other numeric combinations of any arbitrary length
- Any other mix of numeric/non-numeric data of any arbitrary length

Pending the items above, the code looks OK to me.

Also, I've validated things on my end as well, and MTE still works fine for
native/core/remote (gdbserver).

Re: [PATCH v3 5/7] gdb: Introduce is_address_tagged target hook

[Luis Machado]

On 4/4/24 17:12, Gustavo Romero wrote:
> Hi Luis,
> 
> On 4/4/24 12:45 PM, Luis Machado wrote:
>> On 4/4/24 07:48, Gustavo Romero wrote:
>>> This commit introduces a new target hook, target_is_address_tagged,
>>> which is used instead of the gdbarch_tagged_address_p gdbarch hook in
>>> the upper layer (printcmd.c).
>>>
>>> This change allows the memory tagging address checking to be specialized
>>> easily per target in the future. Since target_is_address_tagged
>>> continues to use the gdbarch_tagged_address_p hook there is no change
>>> in behavior for the targets using the new target hook (the remote.c,
>>> aarch64-linux-nat.c, and corelow.c targets).
>>
>> The above block...
>>
>>>
>>> This change enables easy specialization of memory tagging address
>>> check per target in the future. As target_is_address_tagged continues
>>> to utilize the gdbarch_tagged_address_p hook, there is no change in
>>> behavior for all the targets that use the new target hook (i.e., the
>>> remote.c, aarch64-linux-nat.c, and corelow.c targets).
>>
>> ... seems to be somewhat duplicated above in the commit message.
> 
> Right, it's duplicated. I'll drop the first block in v4. Thanks.
> 
> 
>> Also, as general rule, we usually make updates clear at the top of the
>> commit message. for instace:
>>
>> Updates in v3:
>>
>> - Something something.
>> - Fixed breakage.
>>
>> And then those update blocks don't get pushed when the series is
>> approved (sometimes some do push it).
> 
> hmm, k, I'll put the updates at the top next, however I put them always in the
> cover letter and never in the commit messages. Are you recommending putting
> them per commit?"

Yes, we usually add them per-patch in the series. As one edits the patches in a
series (after reviews), they can add the updates one by one and git send-email
the series again.

You don't have to do it for this series though. Just a heads-up.

Re: [PATCH v3 2/7] gdb: aarch64: Move MTE address check out of set_memtag

[Gustavo Romero]

Hi Luis,

On 4/4/24 11:17 AM, Luis Machado wrote:
> On 4/4/24 07:48, Gustavo Romero wrote:
>> Move MTE address check out of set_memtag and add this check to the
>> upper layer, before set_memtag is called. This is a preparation for
>> using a target hook instead of a gdbarch hook MTE address checks.
>>
>> Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
>> ---
>>   gdb/aarch64-linux-tdep.c | 4 ----
>>   gdb/printcmd.c           | 5 +++++
>>   2 files changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/gdb/aarch64-linux-tdep.c b/gdb/aarch64-linux-tdep.c
>> index 50055ac3f48..8e6e63d4dcb 100644
>> --- a/gdb/aarch64-linux-tdep.c
>> +++ b/gdb/aarch64-linux-tdep.c
>> @@ -2525,10 +2525,6 @@ aarch64_linux_set_memtags (struct gdbarch *gdbarch, struct value *address,
>>         /* Remove the top byte.  */
>>         addr = gdbarch_remove_non_address_bits (gdbarch, addr);
>>   
>> -      /* Make sure we are dealing with a tagged address to begin with.  */
>> -      if (!aarch64_linux_tagged_address_p (gdbarch, address))
>> -	return false;
>> -
>>         /* With G being the number of tag granules and N the number of tags
>>   	 passed in, we can have the following cases:
>>   
>> diff --git a/gdb/printcmd.c b/gdb/printcmd.c
>> index cb0d32aa4bc..774e3ec74ae 100644
>> --- a/gdb/printcmd.c
>> +++ b/gdb/printcmd.c
>> @@ -3127,6 +3127,11 @@ memory_tag_set_allocation_tag_command (const char *args, int from_tty)
>>     /* Parse the input.  */
>>     parse_set_allocation_tag_input (args, &val, &length, tags);
>>   
>> +  /* If the address is not in a region memory mapped with a memory tagging
> 
> A nit:
> 
> s/memory mapped/memory-mapped ?
> 
>> +     flag, it is no use trying to manipulate its allocation tag.  */
>> +  if (!gdbarch_tagged_address_p (current_inferior ()->arch (), val))
>> +    show_addr_not_tagged (value_as_address(val));
>> +
> 
> Looks like memory_tag_set_allocation_tag_command calls parse_set_allocation_tag_input,
> and the latter has the exact same check as you are adding here. Is this then redundant?

hmm, right, it is (and was) redundant. I'm removing the check from parse_set_allocation_tag_input.


Thanks,
Gustavo

Re: [PATCH v3 7/7] gdb: Document qMemTagCheckAddr packet

[Tom Tromey]

>>>>> "Gustavo" == Gustavo Romero <gustavo.romero@linaro.org> writes:

Gustavo> +@item @w{}
Gustavo> +An empty reply indicates that @samp{qMemTagCheckAddr} is not supported by the
Gustavo> +stub.  This situation should not occur because @value{GDBN} will only send this
Gustavo> +packet if the stub has advertised support for memory tagging check via the
Gustavo> +@samp{memory-tagging-check-addr} feature in @samp{qSupported}.

Is querying really needed in this case?
Like, if there is some user feature that requires knowing whether this
work before ever trying it, then I guess that would be a good
justification.  In other cases, it seems to me that simply trying to use
a packet is better than a qSupported response; or at least I don't know
why it wouldn't be.

Tom

Re: [PATCH v3 5/7] gdb: Introduce is_address_tagged target hook

[Gustavo Romero]

Hi Luis,

On 4/4/24 12:45 PM, Luis Machado wrote:
> On 4/4/24 07:48, Gustavo Romero wrote:
>> This commit introduces a new target hook, target_is_address_tagged,
>> which is used instead of the gdbarch_tagged_address_p gdbarch hook in
>> the upper layer (printcmd.c).
>>
>> This change allows the memory tagging address checking to be specialized
>> easily per target in the future. Since target_is_address_tagged
>> continues to use the gdbarch_tagged_address_p hook there is no change
>> in behavior for the targets using the new target hook (the remote.c,
>> aarch64-linux-nat.c, and corelow.c targets).
> 
> The above block...
> 
>>
>> This change enables easy specialization of memory tagging address
>> check per target in the future. As target_is_address_tagged continues
>> to utilize the gdbarch_tagged_address_p hook, there is no change in
>> behavior for all the targets that use the new target hook (i.e., the
>> remote.c, aarch64-linux-nat.c, and corelow.c targets).
> 
> ... seems to be somewhat duplicated above in the commit message.
> 
> Also, as general rule, we usually make updates clear at the top of the
> commit message. for instace:
> 
> Updates in v3:
> 
> - Something something.
> - Fixed breakage.
> 
> And then those update blocks don't get pushed when the series is
> approved (sometimes some do push it).
> 
>>
>> Just the gdbarch_tagged_address_p signature is changed for convenience,
>> since target_is_address_tagged takes the address to be checked as a
>> CORE_ADDR type.
>>
>> Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
>> ---
>>   gdb/aarch64-linux-nat.c   |  8 ++++++++
>>   gdb/aarch64-linux-tdep.c  | 10 ++++------
>>   gdb/arch-utils.c          |  2 +-
>>   gdb/arch-utils.h          |  2 +-
>>   gdb/corelow.c             |  8 ++++++++
>>   gdb/gdbarch-gen.h         |  4 ++--
>>   gdb/gdbarch.c             |  2 +-
>>   gdb/gdbarch_components.py |  2 +-
>>   gdb/printcmd.c            | 31 +++++++++++++++++--------------
>>   gdb/remote.c              | 10 ++++++++++
>>   gdb/target-delegates.c    | 30 ++++++++++++++++++++++++++++++
>>   gdb/target.c              |  6 ++++++
>>   gdb/target.h              |  6 ++++++
>>   13 files changed, 95 insertions(+), 26 deletions(-)
>>
>> diff --git a/gdb/aarch64-linux-nat.c b/gdb/aarch64-linux-nat.c
>> index 3face34ce79..19c099832a5 100644
>> --- a/gdb/aarch64-linux-nat.c
>> +++ b/gdb/aarch64-linux-nat.c
>> @@ -110,6 +110,8 @@ class aarch64_linux_nat_target final
>>     /* Write allocation tags to memory via PTRACE.  */
>>     bool store_memtags (CORE_ADDR address, size_t len,
>>   		      const gdb::byte_vector &tags, int type) override;
>> +  /* Check if an address is tagged.  */
>> +  bool is_address_tagged (gdbarch *gdbarch, CORE_ADDR address) override;
>>   };
>>   
>>   static aarch64_linux_nat_target the_aarch64_linux_nat_target;
>> @@ -1071,6 +1073,12 @@ aarch64_linux_nat_target::store_memtags (CORE_ADDR address, size_t len,
>>     return false;
>>   }
>>   
>> +bool
>> +aarch64_linux_nat_target::is_address_tagged (gdbarch *gdbarch, CORE_ADDR address)
>> +{
>> +  return gdbarch_tagged_address_p (gdbarch, address);
> 
> I'd add a comment here on why we take a detour going to the linux-tdep layer
> to read the smaps file, because we don't have a better way to get that information.
> 
> In the future, if this check is ever made available through PTRACE, one can come
> here and drop the smaps path.
> 
>> +}
>> +
>>   void _initialize_aarch64_linux_nat ();
>>   void
>>   _initialize_aarch64_linux_nat ()
>> diff --git a/gdb/aarch64-linux-tdep.c b/gdb/aarch64-linux-tdep.c
>> index fc60e602748..2a47c3f0845 100644
>> --- a/gdb/aarch64-linux-tdep.c
>> +++ b/gdb/aarch64-linux-tdep.c
>> @@ -2451,17 +2451,15 @@ aarch64_mte_get_atag (CORE_ADDR address)
>>   /* Implement the tagged_address_p gdbarch method.  */
>>   
>>   static bool
>> -aarch64_linux_tagged_address_p (struct gdbarch *gdbarch, struct value *address)
>> +aarch64_linux_tagged_address_p (struct gdbarch *gdbarch, CORE_ADDR address)
>>   {
>> -  gdb_assert (address != nullptr);
>> -
>> -  CORE_ADDR addr = value_as_address (address);
>> +  gdb_assert (address);
> 
> No need to assert for a non-zero address. We used to assert that we had a valid pointer,
> but since we're no longer dealing with a pointer, we don't have to worry about it.
> 
> Plus, 0x0 is a valid address (at least for bare-metal. We could run into an assertion if
> the user explicitly tries to check for tags in a 0x0 address. See below:
> 
> (gdb) memory-tag check 0x0
> ../../../repos/binutils-gdb/gdb/aarch64-linux-tdep.c:2456: internal-error: aarch64_linux_tagged_address_p: Assertion `address' failed.
> A problem internal to GDB has been detected,
> further debugging may prove unreliable.
> ----- Backtrace -----
> 0xaaaac69ccf97 _Z22gdb_internal_backtracev
> 0xaaaac6e443e7 _ZL17internal_vproblemP16internal_problemPKciS2_St9__va_list
> 0xaaaac6e4464f _Z15internal_verrorPKciS0_St9__va_list
> 0xaaaac734a8b3 _Z18internal_error_locPKciS0_z
> 0xaaaac68dc407 _ZL30aarch64_linux_tagged_address_pP7gdbarchm
> 0xaaaac6c849ab _ZL24memory_tag_check_commandPKci
> 0xaaaac69fb8ab _Z8cmd_funcP16cmd_list_elementPKci
> 0xaaaac6de5ed3 _Z15execute_commandPKci
> 0xaaaac6adc203 _Z15command_handlerPKc
> 0xaaaac6add45f _Z20command_line_handlerOSt10unique_ptrIcN3gdb13xfree_deleterIcEEE
> 0xaaaac6adccd7 _ZL23gdb_rl_callback_handlerPc
> 0xaaaac6ea7eeb rl_callback_read_char
> 0xaaaac6adbc7b _ZL42gdb_rl_callback_read_char_wrapper_noexceptv
> 0xaaaac6adcb3b _ZL33gdb_rl_callback_read_char_wrapperPv
> 0xaaaac6e1ef5f _ZL19stdin_event_handleriPv
> 0xaaaac734b29f _ZL18gdb_wait_for_eventi.part.0
> 0xaaaac734bc7f _Z16gdb_do_one_eventi
> 0xaaaac6bdd977 _ZL21captured_command_loopv
> 0xaaaac6bdf6bb _Z8gdb_mainP18captured_main_args
> 0xaaaac68cee47 main
> 
>>   
>>     /* Remove the top byte for the memory range check.  */
>> -  addr = gdbarch_remove_non_address_bits (gdbarch, addr);
>> +  address = gdbarch_remove_non_address_bits (gdbarch, address);
>>   
>>     /* Check if the page that contains ADDRESS is mapped with PROT_MTE.  */
>> -  if (!linux_address_in_memtag_page (addr))
>> +  if (!linux_address_in_memtag_page (address))
>>       return false;
>>   
>>     /* We have a valid tag in the top byte of the 64-bit address.  */
>> diff --git a/gdb/arch-utils.c b/gdb/arch-utils.c
>> index 456bfe971ff..cb149c36bc9 100644
>> --- a/gdb/arch-utils.c
>> +++ b/gdb/arch-utils.c
>> @@ -102,7 +102,7 @@ default_memtag_to_string (struct gdbarch *gdbarch, struct value *tag)
>>   /* See arch-utils.h */
>>   
>>   bool
>> -default_tagged_address_p (struct gdbarch *gdbarch, struct value *address)
>> +default_tagged_address_p (struct gdbarch *gdbarch, CORE_ADDR address)
>>   {
>>     /* By default, assume the address is untagged.  */
>>     return false;
>> diff --git a/gdb/arch-utils.h b/gdb/arch-utils.h
>> index 2dcd8f6dc53..467be40c688 100644
>> --- a/gdb/arch-utils.h
>> +++ b/gdb/arch-utils.h
>> @@ -141,7 +141,7 @@ extern std::string default_memtag_to_string (struct gdbarch *gdbarch,
>>   					     struct value *tag);
>>   
>>   /* Default implementation of gdbarch_tagged_address_p.  */
>> -bool default_tagged_address_p (struct gdbarch *gdbarch, struct value *address);
>> +bool default_tagged_address_p (struct gdbarch *gdbarch, CORE_ADDR address);
>>   
>>   /* Default implementation of gdbarch_memtag_matches_p.  */
>>   extern bool default_memtag_matches_p (struct gdbarch *gdbarch,
>> diff --git a/gdb/corelow.c b/gdb/corelow.c
>> index f4e8273d962..b13d0124471 100644
>> --- a/gdb/corelow.c
>> +++ b/gdb/corelow.c
>> @@ -109,6 +109,8 @@ class core_target final : public process_stratum_target
>>     bool fetch_memtags (CORE_ADDR address, size_t len,
>>   		      gdb::byte_vector &tags, int type) override;
>>   
>> +  bool is_address_tagged (gdbarch *gdbarch, CORE_ADDR address) override;
>> +
> 
> Maybe add a comment to this function stating what we're looking for:
> 
> /* If the architecture supports it, check if ADDRESS is within a memory range
>     mapped with tags.  For example,  MTE tags for AArch64.  */
> 
> Or some other variation of the above.
> 
> The reason being that the corelow target is a bit special because it is generic, hence
> why we see an override for a x86 target hook in there as well.
> 
>>     x86_xsave_layout fetch_x86_xsave_layout () override;
>>   
>>     /* A few helpers.  */
>> @@ -1410,6 +1412,12 @@ core_target::fetch_memtags (CORE_ADDR address, size_t len,
>>     return false;
>>   }
>>   
>> +bool
>> +core_target::is_address_tagged (gdbarch *gdbarch, CORE_ADDR address)
>> +{
>> +  return gdbarch_tagged_address_p (gdbarch, address);
>> +}
>> +
>>   /* Implementation of the "fetch_x86_xsave_layout" target_ops method.  */
>>   
>>   x86_xsave_layout
>> diff --git a/gdb/gdbarch-gen.h b/gdb/gdbarch-gen.h
>> index ebcff80bb9e..63fab26987f 100644
>> --- a/gdb/gdbarch-gen.h
>> +++ b/gdb/gdbarch-gen.h
>> @@ -707,8 +707,8 @@ extern void set_gdbarch_memtag_to_string (struct gdbarch *gdbarch, gdbarch_memta
>>   /* Return true if ADDRESS contains a tag and false otherwise.  ADDRESS
>>      must be either a pointer or a reference type. */
>>   
>> -typedef bool (gdbarch_tagged_address_p_ftype) (struct gdbarch *gdbarch, struct value *address);
>> -extern bool gdbarch_tagged_address_p (struct gdbarch *gdbarch, struct value *address);
>> +typedef bool (gdbarch_tagged_address_p_ftype) (struct gdbarch *gdbarch, CORE_ADDR address);
>> +extern bool gdbarch_tagged_address_p (struct gdbarch *gdbarch, CORE_ADDR address);
>>   extern void set_gdbarch_tagged_address_p (struct gdbarch *gdbarch, gdbarch_tagged_address_p_ftype *tagged_address_p);
>>   
>>   /* Return true if the tag from ADDRESS matches the memory tag for that
>> diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
>> index 9319571deba..2d92f604c49 100644
>> --- a/gdb/gdbarch.c
>> +++ b/gdb/gdbarch.c
>> @@ -3232,7 +3232,7 @@ set_gdbarch_memtag_to_string (struct gdbarch *gdbarch,
>>   }
>>   
>>   bool
>> -gdbarch_tagged_address_p (struct gdbarch *gdbarch, struct value *address)
>> +gdbarch_tagged_address_p (struct gdbarch *gdbarch, CORE_ADDR address)
>>   {
>>     gdb_assert (gdbarch != NULL);
>>     gdb_assert (gdbarch->tagged_address_p != NULL);
>> diff --git a/gdb/gdbarch_components.py b/gdb/gdbarch_components.py
>> index 7d913ade621..24e979431b6 100644
>> --- a/gdb/gdbarch_components.py
>> +++ b/gdb/gdbarch_components.py
>> @@ -1267,7 +1267,7 @@ must be either a pointer or a reference type.
>>   """,
>>       type="bool",
>>       name="tagged_address_p",
>> -    params=[("struct value *", "address")],
>> +    params=[("CORE_ADDR", "address")],
>>       predefault="default_tagged_address_p",
>>       invalid=False,
>>   )
>> diff --git a/gdb/printcmd.c b/gdb/printcmd.c
>> index 62fbcb98cfb..24eac7c8421 100644
>> --- a/gdb/printcmd.c
>> +++ b/gdb/printcmd.c
>> @@ -1132,7 +1132,7 @@ do_examine (struct format_data fmt, struct gdbarch *gdbarch, CORE_ADDR addr)
>>   	    = value_from_ulongest (builtin_type (gdbarch)->builtin_data_ptr,
>>   				   tag_laddr);
>>   
>> -	  if (gdbarch_tagged_address_p (gdbarch, v_addr))
>> +	  if (target_is_address_tagged (gdbarch, value_as_address (v_addr)))
>>   	    {
>>   	      /* Fetch the allocation tag.  */
>>   	      struct value *tag
>> @@ -1268,7 +1268,7 @@ print_value (value *val, const value_print_options &opts)
>>   /* Returns true if memory tags should be validated.  False otherwise.  */
>>   
>>   static bool
>> -should_validate_memtags (struct value *value)
>> +should_validate_memtags (gdbarch *gdbarch, struct value *value)
>>   {
>>     gdb_assert (value != nullptr && value->type () != nullptr);
>>   
>> @@ -1289,7 +1289,7 @@ should_validate_memtags (struct value *value)
>>       return false;
>>   
>>     /* We do.  Check whether it includes any tags.  */
>> -  return gdbarch_tagged_address_p (current_inferior ()->arch  (), value);
>> +  return target_is_address_tagged (gdbarch, value_as_address (value));
>>   }
>>   
>>   /* Helper for parsing arguments for print_command_1.  */
>> @@ -1346,7 +1346,7 @@ print_command_1 (const char *args, int voidprint)
>>   	    {
>>   	      gdbarch *arch = current_inferior ()->arch ();
>>   
>> -	      if (should_validate_memtags (val)
>> +	      if (should_validate_memtags (arch, val)
>>   		  && !gdbarch_memtag_matches_p (arch, val))
>>   		{
>>   		  /* Fetch the logical tag.  */
>> @@ -2946,9 +2946,10 @@ memory_tag_print_tag_command (const char *args, enum memtag_type tag_type)
>>        flag, it is no use trying to access/manipulate its allocation tag.
>>   
>>        It is OK to manipulate the logical tag though.  */
>> +  CORE_ADDR addr = value_as_address (val);
>>     if (tag_type == memtag_type::allocation
>> -      && !gdbarch_tagged_address_p (arch, val))
>> -    show_addr_not_tagged (value_as_address (val));
>> +      && !target_is_address_tagged (arch, addr))
>> +    show_addr_not_tagged (addr);
>>   
>>     value *tag_value = gdbarch_get_memtag (arch, val, tag_type);
>>     std::string tag = gdbarch_memtag_to_string (arch, tag_value);
>> @@ -3104,8 +3105,9 @@ parse_set_allocation_tag_input (const char *args, struct value **val,
>>   
>>     /* If the address is not in a region memory mapped with a memory tagging
>>        flag, it is no use trying to access/manipulate its allocation tag.  */
>> -  if (!gdbarch_tagged_address_p (current_inferior ()->arch (), *val))
>> -    show_addr_not_tagged (value_as_address (*val));
>> +  CORE_ADDR addr = value_as_address (*val);
>> +  if (!target_is_address_tagged (current_inferior ()->arch (), addr))
>> +    show_addr_not_tagged (addr);
>>   }
>>   
>>   /* Implement the "memory-tag set-allocation-tag" command.
>> @@ -3129,8 +3131,9 @@ memory_tag_set_allocation_tag_command (const char *args, int from_tty)
>>   
>>     /* If the address is not in a region memory mapped with a memory tagging
>>        flag, it is no use trying to manipulate its allocation tag.  */
>> -  if (!gdbarch_tagged_address_p (current_inferior ()->arch (), val))
>> -    show_addr_not_tagged (value_as_address(val));
>> +  CORE_ADDR addr = value_as_address (val);
>> +  if (!target_is_address_tagged (current_inferior ()-> arch(), addr))
>> +    show_addr_not_tagged (addr);
>>   
>>     if (!gdbarch_set_memtags (current_inferior ()->arch (), val, length, tags,
>>   			    memtag_type::allocation))
>> @@ -3157,12 +3160,12 @@ memory_tag_check_command (const char *args, int from_tty)
>>     struct value *val = process_print_command_args (args, &print_opts, true);
>>     gdbarch *arch = current_inferior ()->arch ();
>>   
>> +  CORE_ADDR addr = value_as_address (val);
>> +
>>     /* If the address is not in a region memory mapped with a memory tagging
>>        flag, it is no use trying to access/manipulate its allocation tag.  */
>> -  if (!gdbarch_tagged_address_p (arch, val))
>> -    show_addr_not_tagged (value_as_address (val));
>> -
>> -  CORE_ADDR addr = value_as_address (val);
>> +  if (!target_is_address_tagged (current_inferior ()->arch (), addr))
>> +    show_addr_not_tagged (addr);
> 
> I noticed the above checks happen at least 3 times in the code. Maybe a future
> cleanup possibility to split the check into a separate function.
> 
>>   
>>     /* Check if the tag is valid.  */
>>     if (!gdbarch_memtag_matches_p (arch, val))
>> diff --git a/gdb/remote.c b/gdb/remote.c
>> index e278711df7b..9717db55e27 100644
>> --- a/gdb/remote.c
>> +++ b/gdb/remote.c
>> @@ -1084,6 +1084,8 @@ class remote_target : public process_stratum_target
>>     bool store_memtags (CORE_ADDR address, size_t len,
>>   		      const gdb::byte_vector &tags, int type) override;
>>   
>> +  bool is_address_tagged (gdbarch *gdbarch, CORE_ADDR address) override;
>> +
>>   public: /* Remote specific methods.  */
>>   
>>     void remote_download_command_source (int num, ULONGEST addr,
>> @@ -15573,6 +15575,14 @@ remote_target::store_memtags (CORE_ADDR address, size_t len,
>>     return packet_check_result (rs->buf).status () == PACKET_OK;
>>   }
>>   
>> +/* Implement the "is_address_tagged" target_ops method.  */
>> +
>> +bool
>> +remote_target::is_address_tagged (gdbarch *gdbarch, CORE_ADDR address)
>> +{
>> +  return gdbarch_tagged_address_p (gdbarch, address);
>> +}
>> +
>>   /* Return true if remote target T is non-stop.  */
>>   
>>   bool
>> diff --git a/gdb/target-delegates.c b/gdb/target-delegates.c
>> index 59ea70458ad..e322bbbe481 100644
>> --- a/gdb/target-delegates.c
>> +++ b/gdb/target-delegates.c
>> @@ -197,6 +197,7 @@ struct dummy_target : public target_ops
>>     bool supports_memory_tagging () override;
>>     bool fetch_memtags (CORE_ADDR arg0, size_t arg1, gdb::byte_vector &arg2, int arg3) override;
>>     bool store_memtags (CORE_ADDR arg0, size_t arg1, const gdb::byte_vector &arg2, int arg3) override;
>> +  bool is_address_tagged (gdbarch *arg0, CORE_ADDR arg1) override;
>>     x86_xsave_layout fetch_x86_xsave_layout () override;
>>   };
>>   
>> @@ -373,6 +374,7 @@ struct debug_target : public target_ops
>>     bool supports_memory_tagging () override;
>>     bool fetch_memtags (CORE_ADDR arg0, size_t arg1, gdb::byte_vector &arg2, int arg3) override;
>>     bool store_memtags (CORE_ADDR arg0, size_t arg1, const gdb::byte_vector &arg2, int arg3) override;
>> +  bool is_address_tagged (gdbarch *arg0, CORE_ADDR arg1) override;
>>     x86_xsave_layout fetch_x86_xsave_layout () override;
>>   };
>>   
>> @@ -4562,6 +4564,34 @@ debug_target::store_memtags (CORE_ADDR arg0, size_t arg1, const gdb::byte_vector
>>     return result;
>>   }
>>   
>> +bool
>> +target_ops::is_address_tagged (gdbarch *arg0, CORE_ADDR arg1)
>> +{
>> +  return this->beneath ()->is_address_tagged (arg0, arg1);
>> +}
>> +
>> +bool
>> +dummy_target::is_address_tagged (gdbarch *arg0, CORE_ADDR arg1)
>> +{
>> +  tcomplain ();
>> +}
>> +
>> +bool
>> +debug_target::is_address_tagged (gdbarch *arg0, CORE_ADDR arg1)
>> +{
>> +  gdb_printf (gdb_stdlog, "-> %s->is_address_tagged (...)\n", this->beneath ()->shortname ());
>> +  bool result
>> +    = this->beneath ()->is_address_tagged (arg0, arg1);
>> +  gdb_printf (gdb_stdlog, "<- %s->is_address_tagged (", this->beneath ()->shortname ());
>> +  target_debug_print_gdbarch_p (arg0);
>> +  gdb_puts (", ", gdb_stdlog);
>> +  target_debug_print_CORE_ADDR (arg1);
>> +  gdb_puts (") = ", gdb_stdlog);
>> +  target_debug_print_bool (result);
>> +  gdb_puts ("\n", gdb_stdlog);
>> +  return result;
>> +}
>> +
>>   x86_xsave_layout
>>   target_ops::fetch_x86_xsave_layout ()
>>   {
>> diff --git a/gdb/target.c b/gdb/target.c
>> index 107a84b3ca1..5c3c1a57dbd 100644
>> --- a/gdb/target.c
>> +++ b/gdb/target.c
>> @@ -796,6 +796,12 @@ target_store_memtags (CORE_ADDR address, size_t len,
>>     return current_inferior ()->top_target ()->store_memtags (address, len, tags, type);
>>   }
>>   
>> +bool
>> +target_is_address_tagged (gdbarch *gdbarch, CORE_ADDR address)
>> +{
>> +  return current_inferior ()->top_target ()->is_address_tagged (gdbarch, address);
>> +}
>> +
>>   x86_xsave_layout
>>   target_fetch_x86_xsave_layout ()
>>   {
>> diff --git a/gdb/target.h b/gdb/target.h
>> index c9eaff16346..486a0a687b0 100644
>> --- a/gdb/target.h
>> +++ b/gdb/target.h
>> @@ -1334,6 +1334,10 @@ struct target_ops
>>   				const gdb::byte_vector &tags, int type)
>>         TARGET_DEFAULT_NORETURN (tcomplain ());
>>   
>> +    /* Returns true if ADDRESS is tagged, otherwise returns false.  */
>> +    virtual bool is_address_tagged (gdbarch *gdbarch, CORE_ADDR address)
>> +      TARGET_DEFAULT_NORETURN (tcomplain ());
>> +
> 
> Should the default for is_address_tagged by to return false, meaning the address is never
> tagged?

No, I think tcomplain() is correct. This is the virtual (default) declaration, so
it will be called if the target layer does not implement/override this method in the
base class, and tcomplain() will correctly complain:

"You can't do that when your target is `%s`"

But this should be a rare condition, when GDB understands that the target supports
memory tagging and so will call target_is_address_tagged at some point but the
target layer does not implement the target hook.


Cheers,
Gustavo

Re: [PATCH v3 7/7] gdb: Document qMemTagCheckAddr packet

[Gustavo Romero]

Hi Tom,

On 4/8/24 4:37 PM, Tom Tromey wrote:
>>>>>> "Gustavo" == Gustavo Romero <gustavo.romero@linaro.org> writes:
> 
> Gustavo> +@item @w{}
> Gustavo> +An empty reply indicates that @samp{qMemTagCheckAddr} is not supported by the
> Gustavo> +stub.  This situation should not occur because @value{GDBN} will only send this
> Gustavo> +packet if the stub has advertised support for memory tagging check via the
> Gustavo> +@samp{memory-tagging-check-addr} feature in @samp{qSupported}.
> 
> Is querying really needed in this case?
> Like, if there is some user feature that requires knowing whether this
> work before ever trying it, then I guess that would be a good
> justification.  In other cases, it seems to me that simply trying to use
> a packet is better than a qSupported response; or at least I don't know
> why it wouldn't be.

That's right, I think we are in sync here. Luis communicated to me last week
(private conversation) about this possibility, hence for v4 we just try to
send the qMemTagCheckAddr packet, and if it fails (empty reply) there is a
fallback to the current code path, which reads the smaps. So I'm dropping
the memory-tagging-check-addr feature.

Thanks for review!


Cheers,
Gustavo

Re: [PATCH v3 7/7] gdb: Document qMemTagCheckAddr packet

[Tom Tromey]

>>>>> "Gustavo" == Gustavo Romero <gustavo.romero@linaro.org> writes:

>> Is querying really needed in this case?
>> Like, if there is some user feature that requires knowing whether this
>> work before ever trying it, then I guess that would be a good
>> justification.  In other cases, it seems to me that simply trying to use
>> a packet is better than a qSupported response; or at least I don't know
>> why it wouldn't be.

Gustavo> That's right, I think we are in sync here. Luis communicated to me last week
Gustavo> (private conversation) about this possibility, hence for v4 we just try to
Gustavo> send the qMemTagCheckAddr packet, and if it fails (empty reply) there is a
Gustavo> fallback to the current code path, which reads the smaps. So I'm dropping
Gustavo> the memory-tagging-check-addr feature.

Great, thank you.

TBH I am not sure if my view on the desirability of probing versus
qSupported is the correct one.  Like, there may be counterexamples I'm
unaware of.

Tom

Re: [PATCH v3 7/7] gdb: Document qMemTagCheckAddr packet

[Luis Machado]

On 4/9/24 16:33, Tom Tromey wrote:
>>>>>> "Gustavo" == Gustavo Romero <gustavo.romero@linaro.org> writes:
> 
>>> Is querying really needed in this case?
>>> Like, if there is some user feature that requires knowing whether this
>>> work before ever trying it, then I guess that would be a good
>>> justification.  In other cases, it seems to me that simply trying to use
>>> a packet is better than a qSupported response; or at least I don't know
>>> why it wouldn't be.
> 
> Gustavo> That's right, I think we are in sync here. Luis communicated to me last week
> Gustavo> (private conversation) about this possibility, hence for v4 we just try to
> Gustavo> send the qMemTagCheckAddr packet, and if it fails (empty reply) there is a
> Gustavo> fallback to the current code path, which reads the smaps. So I'm dropping
> Gustavo> the memory-tagging-check-addr feature.
> 
> Great, thank you.
> 
> TBH I am not sure if my view on the desirability of probing versus
> qSupported is the correct one.  Like, there may be counterexamples I'm
> unaware of.

We already have a qSupported string that reports if memory tagging is
supported by the remote (memory-tagging+). So before we send this new
packet, gdb can check if it makes sense to send it. If the target
supports memory tagging, then the answer is yes. Otherwise no.

If we didn't have "memory-tagging+", I think it would make sense to
have a feature string in place, because then a remote can support
the packet itself, but not necessarily support checking if a particular
address is within a tagged memory range.

So probing seems fine in this case. In the future gdbserver may implement
it as well. Until then, an empty reply from gdbserver will tell gdb this method
of querying is not supported.