[PATCH v4 0/8] Add another way to check tagged addresses on remote targets

[PATCH v4 0/8] 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. This new method is based on a new packet, qIsAddressTagged. 

GDB now sends first a qIsAddressTagged packet to the stub for checking an
address and if the stub sends an empty reply GDB attemps to use the current code
path as a fallback mechanism, so no change in stubs not supporting the new
packet is required. Stubs that support the new packet just need to implement
the check and reply accordingly to qIsAddressTagged queries.

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.

Updates in v2:
 * 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

Updates in v3:
 * 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

Updates in v4:
 * Changed packet name to qIsAddressTagged as per Luis's suggestion
 * Removed the need for memory-tagging-check-add+ feature in qSupport to
   use the qIsAddressTagged packet; now GDB first attempts to use the
   packet to check the address and if the stub returns empty the fallback
   mechanism (the current code path that reads smaps) is used
 * Fixed documentation as per Eli's review
 * Added unittests for qIsAddressTagged request and replies
 * Fixed "gdb: Introduce is_address_tagged target hook" commit message
 * Removed wrong assert in aarch64_linux_tagged_address_p that crashed
   GDB, for instance, on "memory-tag check 0x0", because 0x0 address is
   actually valid in this context
 * Added several comments in the code as per Luis's reviews

----

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 (8):
  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 qIsAddressTagged packet
  gdb/testsuite: Add unittest for qIsAddressTagged packet
  gdb: Document qIsAddressTagged packet

 gdb/NEWS                  |  10 ++++
 gdb/aarch64-linux-nat.c   |  15 ++++++
 gdb/aarch64-linux-tdep.c  |  22 ++-------
 gdb/arch-utils.c          |   2 +-
 gdb/arch-utils.h          |   2 +-
 gdb/corelow.c             |  10 ++++
 gdb/doc/gdb.texinfo       |  37 ++++++++++++--
 gdb/gdbarch-gen.h         |   4 +-
 gdb/gdbarch.c             |   2 +-
 gdb/gdbarch_components.py |   2 +-
 gdb/printcmd.c            |  32 ++++++------
 gdb/remote.c              | 101 ++++++++++++++++++++++++++++++++++++++
 gdb/target-delegates.c    |  30 +++++++++++
 gdb/target.c              |   6 +++
 gdb/target.h              |   6 +++
 15 files changed, 238 insertions(+), 43 deletions(-)

-- 
2.34.1

[PATCH v4 1/8] 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>
Reviewed-by: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
Approved-By: Luis Machado <luis.machado@arm.com>
Tested-By: Luis Machado <luis.machado@arm.com>
---
 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 v4 2/8] gdb: aarch64: Move MTE address check out of set_memtag

[Gustavo Romero]

Remove check in parse_set_allocation_tag_input as it is redundant:
currently the check happens at the end of parse_set_allocation_tag_input
and also in set_memtag (called after parse_set_allocation_tag_input).

After it, 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
on MTE address checks.

Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
---
 gdb/aarch64-linux-tdep.c |  4 ----
 gdb/printcmd.c           | 10 +++++-----
 2 files changed, 5 insertions(+), 9 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..5635f605314 100644
--- a/gdb/printcmd.c
+++ b/gdb/printcmd.c
@@ -3101,11 +3101,6 @@ parse_set_allocation_tag_input (const char *args, struct value **val,
     error (_("Error parsing tags argument. Tags should be 2 digits per byte."));
 
   tags = hex2bin (tags_string.c_str ());
-
-  /* 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));
 }
 
 /* Implement the "memory-tag set-allocation-tag" command.
@@ -3127,6 +3122,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 v4 3/8] 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>
Approved-By: Luis Machado <luis.machado@arm.com>
Tested-By: Luis Machado <luis.machado@arm.com>
---
 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 v4 4/8] 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>
Suggested-by: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
Approved-By: Luis Machado <luis.machado@arm.com>
Tested-By: Luis Machado <luis.machado@arm.com>
---
 gdb/printcmd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/printcmd.c b/gdb/printcmd.c
index 5635f605314..4edbd458e4d 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 v4 5/8] 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 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   | 15 +++++++++++++++
 gdb/aarch64-linux-tdep.c  | 10 +++-------
 gdb/arch-utils.c          |  2 +-
 gdb/arch-utils.h          |  2 +-
 gdb/corelow.c             | 10 ++++++++++
 gdb/gdbarch-gen.h         |  4 ++--
 gdb/gdbarch.c             |  2 +-
 gdb/gdbarch_components.py |  2 +-
 gdb/printcmd.c            | 26 ++++++++++++++------------
 gdb/remote.c              | 10 ++++++++++
 gdb/target-delegates.c    | 30 ++++++++++++++++++++++++++++++
 gdb/target.c              |  6 ++++++
 gdb/target.h              |  6 ++++++
 13 files changed, 100 insertions(+), 25 deletions(-)

diff --git a/gdb/aarch64-linux-nat.c b/gdb/aarch64-linux-nat.c
index 3face34ce79..297f56cdbf1 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,19 @@ 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)
+{
+  /* Here we take a detour going to linux-tdep layer to read the smaps file,
+     because currently there isn't a better way to get that information to
+     check if a given address is tagged or not.
+
+     In the future, if this check is made, for instance, available via PTRACE,
+     it will be possible to drop the smaps path in favor of a PTRACE one for
+     this check.  */
+  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..35979875907 100644
--- a/gdb/aarch64-linux-tdep.c
+++ b/gdb/aarch64-linux-tdep.c
@@ -2451,17 +2451,13 @@ 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);
-
   /* 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..bdda742ef59 100644
--- a/gdb/corelow.c
+++ b/gdb/corelow.c
@@ -109,6 +109,10 @@ class core_target final : public process_stratum_target
   bool fetch_memtags (CORE_ADDR address, size_t len,
 		      gdb::byte_vector &tags, int type) override;
 
+  /* If the architecture supports it, check if ADDRESS is within a memory range
+     mapped with tags.  For example,  MTE tags for AArch64.  */
+  bool is_address_tagged (gdbarch *gdbarch, CORE_ADDR address) override;
+
   x86_xsave_layout fetch_x86_xsave_layout () override;
 
   /* A few helpers.  */
@@ -1410,6 +1414,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 4edbd458e4d..c9689a29f74 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);
@@ -3124,8 +3125,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))
@@ -3152,12 +3154,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 v4 6/8] gdb: Add qIsAddressTagged packet

[Gustavo Romero]

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

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 introduction of qIsAddressTagged packet allows checking
if an address is tagged in an agnostic way.

The is_address_tagged target hook in remote.c attempts to use the
qIsAddressTagged packet first for checking if an address is tagged and
if the stub does not support such a packet (reply is empty) it falls
back to using the current mechanism that reads the contents of
/proc/<PID>/smaps via vFile requests.

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

diff --git a/gdb/remote.c b/gdb/remote.c
index 9717db55e27..63799ac5e3f 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -15534,6 +15534,40 @@ create_store_memtags_request (gdb::char_vector &packet, CORE_ADDR address,
   strcpy (packet.data (), request.c_str ());
 }
 
+static void
+create_is_address_tagged_request (gdbarch *gdbarch, gdb::char_vector &packet,
+				  CORE_ADDR address)
+{
+  int addr_size;
+  std::string request;
+
+  addr_size = gdbarch_addr_bit (gdbarch) / 8;
+  request = string_printf ("qIsAddressTagged:%s", phex_nz (address, addr_size));
+
+  if (packet.size () < request.length () + 1)
+    error (_("Contents too big for packet qIsAddressTagged."));
+
+  strcpy (packet.data (), request.c_str ());
+}
+
+static bool
+check_is_address_tagged_reply (gdb::char_vector &packet, bool *tagged)
+{
+  if (packet_check_result (packet).status () != PACKET_OK)
+    return false;
+
+  gdb_byte reply;
+  /* Convert only 2 hex digits, i.e. 1 byte in hex format.  */
+  hex2bin (packet.data (), &reply, 1);
+
+  if (reply == 0x00 || reply == 0x01) {
+    *tagged = !!reply;
+    return true;
+  }
+
+  return false;
+}
+
 /* Implement the "fetch_memtags" target_ops method.  */
 
 bool
@@ -15580,6 +15614,21 @@ remote_target::store_memtags (CORE_ADDR address, size_t len,
 bool
 remote_target::is_address_tagged (gdbarch *gdbarch, CORE_ADDR address)
 {
+  struct remote_state *rs = get_remote_state ();
+  bool is_addr_tagged;
+
+  /* Firstly, attempt to check the address using the qIsAddressTagged
+     packet.  */
+  create_is_address_tagged_request (gdbarch, rs->buf, address);
+
+  putpkt (rs->buf);
+  getpkt (&rs->buf);
+
+  if (check_is_address_tagged_reply (rs->buf, &is_addr_tagged))
+    return is_addr_tagged;
+
+  /* Fallback to arch-specific method of checking whether an address is tagged
+     if qIsAddressTagged fails.  */
   return gdbarch_tagged_address_p (gdbarch, address);
 }
 
-- 
2.34.1

[PATCH v4 7/8] gdb/testsuite: Add unittest for qIsAddressTagged packet

[Gustavo Romero]

Add unittests for testing qIsAddressTagged packet request creation and
reply checks.

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

diff --git a/gdb/remote.c b/gdb/remote.c
index 63799ac5e3f..42f34a03c95 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -15658,6 +15658,8 @@ test_memory_tagging_functions ()
   scoped_restore restore_memtag_support_
     = make_scoped_restore (&config->support);
 
+  struct gdbarch *gdbarch = current_inferior ()->arch ();
+
   /* Test memory tagging packet support.  */
   config->support = PACKET_SUPPORT_UNKNOWN;
   SELF_CHECK (remote.supports_memory_tagging () == false);
@@ -15724,6 +15726,46 @@ test_memory_tagging_functions ()
   create_store_memtags_request (packet, 0xdeadbeef, 255, 1, tags);
   SELF_CHECK (memcmp (packet.data (), expected.c_str (),
 		      expected.length ()) == 0);
+
+  /* Test creating a qIsAddressTagged request.  */
+  expected = "qIsAddressTagged:deadbeef";
+  create_is_address_tagged_request (gdbarch, packet, 0xdeadbeef);
+  SELF_CHECK (strcmp (packet.data (), expected.c_str ()) == 0);
+
+  /* Test empty reply on qIsAddressTagged request.  */
+  reply = "E00";
+  /* is_tagged must not changed, hence it's tested too.  */
+  bool is_tagged = false;
+  strcpy (packet.data (), reply.c_str ());
+  SELF_CHECK (check_is_address_tagged_reply (packet, &is_tagged) == false);
+  SELF_CHECK (is_tagged == false);
+
+  /* Test if only the first byte (01) is correctly extracted from a long
+     numerical reply, with remaining garbage.  */
+  reply = "0104A590001234006mC0fe";
+  /* Because the first byte is 01, is_tagged should be set to true.  */
+  is_tagged = false;
+  strcpy (packet.data (), reply.c_str ());
+  SELF_CHECK (check_is_address_tagged_reply (packet, &is_tagged) == true);
+  SELF_CHECK (is_tagged == true);
+
+  /* Test if only the first byte (00) is correctly extracted from a long
+     numerical reply, with remaining garbage.  */
+  reply = "0004A590001234006mC0fe";
+  /* Because the first byte is 00, is_tagged should be set to false.  */
+  is_tagged = true;
+  strcpy (packet.data (), reply.c_str ());
+  SELF_CHECK (check_is_address_tagged_reply (packet, &is_tagged) == true);
+  SELF_CHECK (is_tagged == false);
+
+  /* Test if only the first byte, 04, is correctly extracted and recognized
+     as invalid (only 00 and 01 are valid replies).  */
+  reply = "0404A590001234006mC0fe";
+  /* Because the first byte is invalid is_tagged must not change.  */
+  is_tagged = false;
+  strcpy (packet.data (), reply.c_str ());
+  SELF_CHECK (check_is_address_tagged_reply (packet, &is_tagged) == false);
+  SELF_CHECK (is_tagged == false);
 }
 
 static void
-- 
2.34.1

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

[Gustavo Romero]

This commit documents the qIsAddressTagged packet.

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

diff --git a/gdb/NEWS b/gdb/NEWS
index feb3a37393a..1693a7a15f8 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -192,6 +192,16 @@ QThreadOptions in qSupported
   QThreadOptions packet, and the qSupported response can contain the
   set of thread options the remote stub supports.
 
+qIsAddressTagged
+  This new packet allows GDB to query the stub about a given address to check
+  if it is tagged or not.  Many memory tagging-related GDB commands need to
+  perform this check before they read/write the allocation tag related to an
+  address.  Currently, however, this is done through a 'vFile' request to read
+  the file /proc/<PID>/smaps and check if the address is in a region reported
+  as memory tagged.  Since not all targets have a notion of what the smaps
+  file is about, this new packet provides a more generic way to perform such
+  a check.
+
 *** Changes in GDB 14
 
 * GDB now supports the AArch64 Scalable Matrix Extension 2 (SME2), which
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 82a617e9ad3..cb6033bbbee 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -44093,6 +44093,35 @@ although this should not happen given @value{GDBN} will only send this packet
 if the stub has advertised support for memory tagging via @samp{qSupported}.
 @end table
 
+@item qIsAddressTagged:@var{address}
+@cindex check if a given address is in a memory tagged region.
+@cindex @samp{qIsAddressTagged} packet
+@anchor {qIsAddressTagged}
+Check if address @var{address} is in a memory tagged region; if it is, it's
+said to be tagged.  The target is responsible for checking it, as this is
+architecture-specific.
+
+@var{address} is the address to be checked.
+
+Reply:
+@table @samp
+Replies to this packet should all be in two hex digit format, as follows:
+
+@item @var{01}
+Address @var{address} is tagged.
+
+@item @var{00}
+Address @var{address} is not tagged.
+
+@item E @var{nn}
+An error occurred.  This means that address could not be checked for some
+reason.
+
+@item @w{}
+An empty reply indicates that @samp{qIsAddressTagged} is not supported by the
+stub.
+@end table
+
 @item QMemTags:@var{start address},@var{length}:@var{type}:@var{tag bytes}
 @anchor{QMemTags}
 @cindex store memory tags
@@ -45141,9 +45170,11 @@ The remote stub supports and implements the required memory tagging
 functionality and understands the @samp{qMemTags} (@pxref{qMemTags}) and
 @samp{QMemTags} (@pxref{QMemTags}) packets.
 
-For AArch64 GNU/Linux systems, this feature also requires access to the
-@file{/proc/@var{pid}/smaps} file so memory mapping page flags can be inspected.
-This is done via the @samp{vFile} requests.
+For AArch64 GNU/Linux systems, this feature can require access to the
+@file{/proc/@var{pid}/smaps} file so memory mapping page flags can be
+inspected, if @samp{qIsAddressTagged} (@pxref{qIsAddressTagged}) packet
+is not supported by the stub.  Access to the @file{/proc/@var{pid}/smaps}
+file is done via @samp{vFile} requests.
 
 @end table
 
-- 
2.34.1

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

[Eli Zaretskii]

> From: Gustavo Romero <gustavo.romero@linaro.org>
> Cc: luis.machado@arm.com,
> 	thiago.bauermann@linaro.org,
> 	eliz@gnu.org,
> 	tom@tromey.com,
> 	gustavo.romero@linaro.org
> Date: Tue, 16 Apr 2024 14:07:28 +0000
> 
> This commit documents the qIsAddressTagged packet.

Thanks.

> diff --git a/gdb/NEWS b/gdb/NEWS
> index feb3a37393a..1693a7a15f8 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -192,6 +192,16 @@ QThreadOptions in qSupported
>    QThreadOptions packet, and the qSupported response can contain the
>    set of thread options the remote stub supports.
>  
> +qIsAddressTagged
> +  This new packet allows GDB to query the stub about a given address to check
> +  if it is tagged or not.  Many memory tagging-related GDB commands need to
> +  perform this check before they read/write the allocation tag related to an
> +  address.  Currently, however, this is done through a 'vFile' request to read
> +  the file /proc/<PID>/smaps and check if the address is in a region reported
> +  as memory tagged.  Since not all targets have a notion of what the smaps
> +  file is about, this new packet provides a more generic way to perform such
> +  a check.
> +
>  *** Changes in GDB 14

This part is okay.

> +@item qIsAddressTagged:@var{address}
> +@cindex check if a given address is in a memory tagged region.
                                                                ^
Index entries should not end in a period.

> +Check if address @var{address} is in a memory tagged region; if it is, it's
> +said to be tagged.  The target is responsible for checking it, as this is

I suggest to use @dfn{tagged}, since this is new terminology.  It will
then look nicer in print (and will be quoted in Info).

> +@item @var{01}
> +Address @var{address} is tagged.
> +
> +@item @var{00}
> +Address @var{address} is not tagged.

I think 01 and 00 should not be in @var here, since they are literal
strings.  The @samp markup of the @table will do here.

> +@item E @var{nn}
> +An error occurred.  This means that address could not be checked for some
> +reason.

Here "nn" is the error value, right?  If so, I suggest to say

  @item E @var{nn}
  An error occurred whose code is @var{nn}.

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

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

[Luis Machado]

On 4/16/24 15:07, Gustavo Romero wrote:
> Remove check in parse_set_allocation_tag_input as it is redundant:
> currently the check happens at the end of parse_set_allocation_tag_input
> and also in set_memtag (called after parse_set_allocation_tag_input).
> 
> After it, 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
> on MTE address checks.
> 
> Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
> ---
>  gdb/aarch64-linux-tdep.c |  4 ----
>  gdb/printcmd.c           | 10 +++++-----
>  2 files changed, 5 insertions(+), 9 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..5635f605314 100644
> --- a/gdb/printcmd.c
> +++ b/gdb/printcmd.c
> @@ -3101,11 +3101,6 @@ parse_set_allocation_tag_input (const char *args, struct value **val,
>      error (_("Error parsing tags argument. Tags should be 2 digits per byte."));
>  
>    tags = hex2bin (tags_string.c_str ());
> -
> -  /* 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));
>  }
>  
>  /* Implement the "memory-tag set-allocation-tag" command.
> @@ -3127,6 +3122,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"));

Thanks.

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

Re: [PATCH v4 6/8] gdb: Add qIsAddressTagged packet

[Luis Machado]

Close, by I still have a few comments below.

On 4/16/24 15:07, Gustavo Romero wrote:
> This commit adds a new packet, qIsAddressTagged, allowing GDB remote
> targets to use it to query the stub if a given address is tagged.
> 
> 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 introduction of qIsAddressTagged packet allows checking
> if an address is tagged in an agnostic way.
> 
> The is_address_tagged target hook in remote.c attempts to use the
> qIsAddressTagged packet first for checking if an address is tagged and
> if the stub does not support such a packet (reply is empty) it falls
> back to using the current mechanism that reads the contents of
> /proc/<PID>/smaps via vFile requests.
> 
> Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
> ---
>  gdb/remote.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 49 insertions(+)
> 
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 9717db55e27..63799ac5e3f 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -15534,6 +15534,40 @@ create_store_memtags_request (gdb::char_vector &packet, CORE_ADDR address,
>    strcpy (packet.data (), request.c_str ());
>  }
>  
> +static void
> +create_is_address_tagged_request (gdbarch *gdbarch, gdb::char_vector &packet,
> +				  CORE_ADDR address)
> +{
> +  int addr_size;
> +  std::string request;
> +
> +  addr_size = gdbarch_addr_bit (gdbarch) / 8;
> +  request = string_printf ("qIsAddressTagged:%s", phex_nz (address, addr_size));
> +
> +  if (packet.size () < request.length () + 1)
> +    error (_("Contents too big for packet qIsAddressTagged."));
> +
> +  strcpy (packet.data (), request.c_str ());
> +}
> +
> +static bool
> +check_is_address_tagged_reply (gdb::char_vector &packet, bool *tagged)

Instead of passing TAGGED as pointer, make it a reference. It is safer.

> +{
> +  if (packet_check_result (packet).status () != PACKET_OK)

This function signature is incorrect and leads to a build error. This function has two
arguments.

Also, this check will return false if the packet yields an error and if the packet is not
supported. We need to be able to distinguish between unsupported and error here, right?

> +    return false;
> +
> +  gdb_byte reply;
> +  /* Convert only 2 hex digits, i.e. 1 byte in hex format.  */
> +  hex2bin (packet.data (), &reply, 1);
> +
> +  if (reply == 0x00 || reply == 0x01) {
> +    *tagged = !!reply;

Passing tagged as reference just use tagged instead of *tagged here.

> +    return true;
> +  }
> +
> +  return false;
> +}
> +
>  /* Implement the "fetch_memtags" target_ops method.  */
>  
>  bool
> @@ -15580,6 +15614,21 @@ remote_target::store_memtags (CORE_ADDR address, size_t len,
>  bool
>  remote_target::is_address_tagged (gdbarch *gdbarch, CORE_ADDR address)
>  {
> +  struct remote_state *rs = get_remote_state ();
> +  bool is_addr_tagged;
> +

Before sending the packet, we need to check if the packet is supported. Generally gdb
will send it the first time around, but if the packet isn't supported gdb shouldn't
keep sending these packets if the stub is gonna reply empty again.

See remote_target::remote_query_attached for an example of how we deal with this.

First you need to have a new enum PACKET_qIsAddressTagged, so we can register if
the packet is supported or not at runtime.

Then at the start of the function:

if (m_features.packet_support (PACKET_qIsAddressTagged) != PACKET_DISABLE)
  {
    /* Use the qIsTaggedAddress packet.  */
  }
else
  {
    /* Use the fallback smaps method.  */
  }

That way gdb only sends the qIsTaggedAddress packet once. If it works, then gdb
keeps using it. Otherwise it always uses the fallback.

> +  /* Firstly, attempt to check the address using the qIsAddressTagged
> +     packet.  */
> +  create_is_address_tagged_request (gdbarch, rs->buf, address);
> +
> +  putpkt (rs->buf);
> +  getpkt (&rs->buf);
> +
> +  if (check_is_address_tagged_reply (rs->buf, &is_addr_tagged))

We should pass is_addr_tagged by reference instead.

> +    return is_addr_tagged;> +
> +  /* Fallback to arch-specific method of checking whether an address is tagged
> +     if qIsAddressTagged fails.  */
>    return gdbarch_tagged_address_p (gdbarch, address);
>  }
> 

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

[Gustavo Romero]

Hi Eli,

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

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

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

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

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


Cheers,
Gustavo

Re: [PATCH v4 5/8] gdb: Introduce is_address_tagged target hook

[Luis Machado]

On 4/16/24 15:07, 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 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   | 15 +++++++++++++++
>  gdb/aarch64-linux-tdep.c  | 10 +++-------
>  gdb/arch-utils.c          |  2 +-
>  gdb/arch-utils.h          |  2 +-
>  gdb/corelow.c             | 10 ++++++++++
>  gdb/gdbarch-gen.h         |  4 ++--
>  gdb/gdbarch.c             |  2 +-
>  gdb/gdbarch_components.py |  2 +-
>  gdb/printcmd.c            | 26 ++++++++++++++------------
>  gdb/remote.c              | 10 ++++++++++
>  gdb/target-delegates.c    | 30 ++++++++++++++++++++++++++++++
>  gdb/target.c              |  6 ++++++
>  gdb/target.h              |  6 ++++++
>  13 files changed, 100 insertions(+), 25 deletions(-)
> 
> diff --git a/gdb/aarch64-linux-nat.c b/gdb/aarch64-linux-nat.c
> index 3face34ce79..297f56cdbf1 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,19 @@ 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)
> +{
> +  /* Here we take a detour going to linux-tdep layer to read the smaps file,
> +     because currently there isn't a better way to get that information to
> +     check if a given address is tagged or not.
> +
> +     In the future, if this check is made, for instance, available via PTRACE,
> +     it will be possible to drop the smaps path in favor of a PTRACE one for
> +     this check.  */
> +  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..35979875907 100644
> --- a/gdb/aarch64-linux-tdep.c
> +++ b/gdb/aarch64-linux-tdep.c
> @@ -2451,17 +2451,13 @@ 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);
> -
>    /* 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..bdda742ef59 100644
> --- a/gdb/corelow.c
> +++ b/gdb/corelow.c
> @@ -109,6 +109,10 @@ class core_target final : public process_stratum_target
>    bool fetch_memtags (CORE_ADDR address, size_t len,
>  		      gdb::byte_vector &tags, int type) override;
>  
> +  /* If the architecture supports it, check if ADDRESS is within a memory range
> +     mapped with tags.  For example,  MTE tags for AArch64.  */
> +  bool is_address_tagged (gdbarch *gdbarch, CORE_ADDR address) override;
> +
>    x86_xsave_layout fetch_x86_xsave_layout () override;
>  
>    /* A few helpers.  */
> @@ -1410,6 +1414,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 4edbd458e4d..c9689a29f74 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);
> @@ -3124,8 +3125,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))
> @@ -3152,12 +3154,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))


You can use arch instead of current_inferior ()->arch () since we already have
a local variable that saved this information.

> +    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.  */

Otherwise LGTM.

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

Re: [PATCH v4 7/8] gdb/testsuite: Add unittest for qIsAddressTagged packet

[Luis Machado]

Hi,

A few comments, mostly dependent on the previous patch's (06/08) behavior.

On 4/16/24 15:07, Gustavo Romero wrote:
> Add unittests for testing qIsAddressTagged packet request creation and
> reply checks.
> 
> Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
> ---
>  gdb/remote.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 42 insertions(+)
> 
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 63799ac5e3f..42f34a03c95 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -15658,6 +15658,8 @@ test_memory_tagging_functions ()
>    scoped_restore restore_memtag_support_
>      = make_scoped_restore (&config->support);
>  
> +  struct gdbarch *gdbarch = current_inferior ()->arch ();
> +
>    /* Test memory tagging packet support.  */
>    config->support = PACKET_SUPPORT_UNKNOWN;
>    SELF_CHECK (remote.supports_memory_tagging () == false);
> @@ -15724,6 +15726,46 @@ test_memory_tagging_functions ()
>    create_store_memtags_request (packet, 0xdeadbeef, 255, 1, tags);
>    SELF_CHECK (memcmp (packet.data (), expected.c_str (),
>  		      expected.length ()) == 0);
> +
> +  /* Test creating a qIsAddressTagged request.  */
> +  expected = "qIsAddressTagged:deadbeef";
> +  create_is_address_tagged_request (gdbarch, packet, 0xdeadbeef);
> +  SELF_CHECK (strcmp (packet.data (), expected.c_str ()) == 0);
> +
> +  /* Test empty reply on qIsAddressTagged request.  */
> +  reply = "E00";

The comment seems to be out of sync with what's being tested. Should we be
testing an empty reply instead of E00?

> +  /* is_tagged must not changed, hence it's tested too.  */

s/must not changed/must not change

> +  bool is_tagged = false;
> +  strcpy (packet.data (), reply.c_str ());
> +  SELF_CHECK (check_is_address_tagged_reply (packet, &is_tagged) == false);
> +  SELF_CHECK (is_tagged == false);
> +
> +  /* Test if only the first byte (01) is correctly extracted from a long
> +     numerical reply, with remaining garbage.  */
> +  reply = "0104A590001234006mC0fe";
> +  /* Because the first byte is 01, is_tagged should be set to true.  */
> +  is_tagged = false;
> +  strcpy (packet.data (), reply.c_str ());
> +  SELF_CHECK (check_is_address_tagged_reply (packet, &is_tagged) == true);

I think this particular reply (malformed) should make the packet check fail, as
we risk the remote returning garbage values and things working just because the
first byte was the expected number.

We should be more strict with the packet reply format and check its length.

> +  SELF_CHECK (is_tagged == true);
> +
> +  /* Test if only the first byte (00) is correctly extracted from a long
> +     numerical reply, with remaining garbage.  */
> +  reply = "0004A590001234006mC0fe";
> +  /* Because the first byte is 00, is_tagged should be set to false.  */
> +  is_tagged = true;
> +  strcpy (packet.data (), reply.c_str ());
> +  SELF_CHECK (check_is_address_tagged_reply (packet, &is_tagged) == true);
> +  SELF_CHECK (is_tagged == false);

Same case for the above test.

> +
> +  /* Test if only the first byte, 04, is correctly extracted and recognized
> +     as invalid (only 00 and 01 are valid replies).  */
> +  reply = "0404A590001234006mC0fe";
> +  /* Because the first byte is invalid is_tagged must not change.  */
> +  is_tagged = false;
> +  strcpy (packet.data (), reply.c_str ());
> +  SELF_CHECK (check_is_address_tagged_reply (packet, &is_tagged) == false);
> +  SELF_CHECK (is_tagged == false);
>  }
>  
>  static void

Also a similar situation.

We should exercise the following:

- Empty reply
- Not tagged - "00"
- Tagged - "01"
- Error - "E??"
- Malformed - packets of incorrect length and values.

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

[Eli Zaretskii]

> Cc: gdb-patches@sourceware.org, luis.machado@arm.com,
>  thiago.bauermann@linaro.org, tom@tromey.com
> From: Gustavo Romero <gustavo.romero@linaro.org>
> Date: Tue, 16 Apr 2024 20:10:43 -0300
> 
> Hi Eli,
> 
> Thanks a lot for the review. I just have one question.
> 
> >> +@item E @var{nn}
> >> +An error occurred.  This means that address could not be checked for some
> >> +reason.
> > 
> > Here "nn" is the error value, right?  If so, I suggest to say
> > 
> >    @item E @var{nn}
> >    An error occurred whose code is @var{nn}.
> 
> Do you mean remove the "This means that address could not be checked for some reason."
> text completely or just s/An error occurred/An error occurred whose code is @var{nn}/?

The latter.

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

[Gustavo Romero]

On 4/17/24 9:09 AM, Eli Zaretskii wrote:
>> Cc: gdb-patches@sourceware.org, luis.machado@arm.com,
>>   thiago.bauermann@linaro.org, tom@tromey.com
>> From: Gustavo Romero <gustavo.romero@linaro.org>
>> Date: Tue, 16 Apr 2024 20:10:43 -0300
>>
>> Hi Eli,
>>
>> Thanks a lot for the review. I just have one question.
>>
>>>> +@item E @var{nn}
>>>> +An error occurred.  This means that address could not be checked for some
>>>> +reason.
>>>
>>> Here "nn" is the error value, right?  If so, I suggest to say
>>>
>>>     @item E @var{nn}
>>>     An error occurred whose code is @var{nn}.
>>
>> Do you mean remove the "This means that address could not be checked for some reason."
>> text completely or just s/An error occurred/An error occurred whose code is @var{nn}/?
> 
> The latter.

Thanks for the clarification!


Cheers,
Gustavo
  

Re: [PATCH v4 7/8] gdb/testsuite: Add unittest for qIsAddressTagged packet

[Gustavo Romero]

Hi Luis,

On 4/17/24 6:38 AM, Luis Machado wrote:
> Hi,
> 
> A few comments, mostly dependent on the previous patch's (06/08) behavior.
> 
> On 4/16/24 15:07, Gustavo Romero wrote:
>> Add unittests for testing qIsAddressTagged packet request creation and
>> reply checks.
>>
>> Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
>> ---
>>   gdb/remote.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 42 insertions(+)
>>
>> diff --git a/gdb/remote.c b/gdb/remote.c
>> index 63799ac5e3f..42f34a03c95 100644
>> --- a/gdb/remote.c
>> +++ b/gdb/remote.c
>> @@ -15658,6 +15658,8 @@ test_memory_tagging_functions ()
>>     scoped_restore restore_memtag_support_
>>       = make_scoped_restore (&config->support);
>>   
>> +  struct gdbarch *gdbarch = current_inferior ()->arch ();
>> +
>>     /* Test memory tagging packet support.  */
>>     config->support = PACKET_SUPPORT_UNKNOWN;
>>     SELF_CHECK (remote.supports_memory_tagging () == false);
>> @@ -15724,6 +15726,46 @@ test_memory_tagging_functions ()
>>     create_store_memtags_request (packet, 0xdeadbeef, 255, 1, tags);
>>     SELF_CHECK (memcmp (packet.data (), expected.c_str (),
>>   		      expected.length ()) == 0);
>> +
>> +  /* Test creating a qIsAddressTagged request.  */
>> +  expected = "qIsAddressTagged:deadbeef";
>> +  create_is_address_tagged_request (gdbarch, packet, 0xdeadbeef);
>> +  SELF_CHECK (strcmp (packet.data (), expected.c_str ()) == 0);
>> +
>> +  /* Test empty reply on qIsAddressTagged request.  */
>> +  reply = "E00";
> 
> The comment seems to be out of sync with what's being tested. Should we be
> testing an empty reply instead of E00?

Right, just noticed that too. Fixed in v5, thanks.


>> +  /* is_tagged must not changed, hence it's tested too.  */
> 
> s/must not changed/must not change
> 
>> +  bool is_tagged = false;
>> +  strcpy (packet.data (), reply.c_str ());
>> +  SELF_CHECK (check_is_address_tagged_reply (packet, &is_tagged) == false);
>> +  SELF_CHECK (is_tagged == false);
>> +
>> +  /* Test if only the first byte (01) is correctly extracted from a long
>> +     numerical reply, with remaining garbage.  */
>> +  reply = "0104A590001234006mC0fe";
>> +  /* Because the first byte is 01, is_tagged should be set to true.  */
>> +  is_tagged = false;
>> +  strcpy (packet.data (), reply.c_str ());
>> +  SELF_CHECK (check_is_address_tagged_reply (packet, &is_tagged) == true);
> 
> I think this particular reply (malformed) should make the packet check fail, as
> we risk the remote returning garbage values and things working just because the
> first byte was the expected number.
> 
> We should be more strict with the packet reply format and check its length.
> 
>> +  SELF_CHECK (is_tagged == true);
>> +
>> +  /* Test if only the first byte (00) is correctly extracted from a long
>> +     numerical reply, with remaining garbage.  */
>> +  reply = "0004A590001234006mC0fe";
>> +  /* Because the first byte is 00, is_tagged should be set to false.  */
>> +  is_tagged = true;
>> +  strcpy (packet.data (), reply.c_str ());
>> +  SELF_CHECK (check_is_address_tagged_reply (packet, &is_tagged) == true);
>> +  SELF_CHECK (is_tagged == false);
> 
> Same case for the above test.
> 
>> +
>> +  /* Test if only the first byte, 04, is correctly extracted and recognized
>> +     as invalid (only 00 and 01 are valid replies).  */
>> +  reply = "0404A590001234006mC0fe";
>> +  /* Because the first byte is invalid is_tagged must not change.  */
>> +  is_tagged = false;
>> +  strcpy (packet.data (), reply.c_str ());
>> +  SELF_CHECK (check_is_address_tagged_reply (packet, &is_tagged) == false);
>> +  SELF_CHECK (is_tagged == false);
>>   }
>>   
>>   static void
> 
> Also a similar situation.
> 
> We should exercise the following:
> 
> - Empty reply
> - Not tagged - "00"
> - Tagged - "01"
> - Error - "E??"
> - Malformed - packets of incorrect length and values.

I agree, but regarding the malformed case due to an incorrect length,
in check_is_address_tagged_reply we explicitly request to convert
only one byte in hex format (2 hex digits):

   /* Convert only 2 hex digits, i.e. 1 byte in hex format.  */
   hex2bin (packet.data (), &reply, 1);

so the rest is truncated. I can add a test to verify if truncation is right,
for instance: "0104A590001234006mC0fe" should returned "Tagged". I don't
think it's worth checking for length in the code.

So, how about:

Empty reply
Not tagged - "00"
Tagged - "01"
Error - "E00"
Malformed, length truncation - "0104A590001234006"
Malformed, values - "0m" (not a hex value)


Cheers,
Gustavo

Re: [PATCH v4 7/8] gdb/testsuite: Add unittest for qIsAddressTagged packet

[Gustavo Romero]

On 4/17/24 4:03 PM, Gustavo Romero wrote:
> Hi Luis,
> 
> On 4/17/24 6:38 AM, Luis Machado wrote:
>> Hi,
>>
>> A few comments, mostly dependent on the previous patch's (06/08) behavior.
>>
>> On 4/16/24 15:07, Gustavo Romero wrote:
>>> Add unittests for testing qIsAddressTagged packet request creation and
>>> reply checks.
>>>
>>> Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
>>> ---
>>>   gdb/remote.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>>>   1 file changed, 42 insertions(+)
>>>
>>> diff --git a/gdb/remote.c b/gdb/remote.c
>>> index 63799ac5e3f..42f34a03c95 100644
>>> --- a/gdb/remote.c
>>> +++ b/gdb/remote.c
>>> @@ -15658,6 +15658,8 @@ test_memory_tagging_functions ()
>>>     scoped_restore restore_memtag_support_
>>>       = make_scoped_restore (&config->support);
>>> +  struct gdbarch *gdbarch = current_inferior ()->arch ();
>>> +
>>>     /* Test memory tagging packet support.  */
>>>     config->support = PACKET_SUPPORT_UNKNOWN;
>>>     SELF_CHECK (remote.supports_memory_tagging () == false);
>>> @@ -15724,6 +15726,46 @@ test_memory_tagging_functions ()
>>>     create_store_memtags_request (packet, 0xdeadbeef, 255, 1, tags);
>>>     SELF_CHECK (memcmp (packet.data (), expected.c_str (),
>>>                 expected.length ()) == 0);
>>> +
>>> +  /* Test creating a qIsAddressTagged request.  */
>>> +  expected = "qIsAddressTagged:deadbeef";
>>> +  create_is_address_tagged_request (gdbarch, packet, 0xdeadbeef);
>>> +  SELF_CHECK (strcmp (packet.data (), expected.c_str ()) == 0);
>>> +
>>> +  /* Test empty reply on qIsAddressTagged request.  */
>>> +  reply = "E00";
>>
>> The comment seems to be out of sync with what's being tested. Should we be
>> testing an empty reply instead of E00?
> 
> Right, just noticed that too. Fixed in v5, thanks.
> 
> 
>>> +  /* is_tagged must not changed, hence it's tested too.  */
>>
>> s/must not changed/must not change
>>
>>> +  bool is_tagged = false;
>>> +  strcpy (packet.data (), reply.c_str ());
>>> +  SELF_CHECK (check_is_address_tagged_reply (packet, &is_tagged) == false);
>>> +  SELF_CHECK (is_tagged == false);
>>> +
>>> +  /* Test if only the first byte (01) is correctly extracted from a long
>>> +     numerical reply, with remaining garbage.  */
>>> +  reply = "0104A590001234006mC0fe";
>>> +  /* Because the first byte is 01, is_tagged should be set to true.  */
>>> +  is_tagged = false;
>>> +  strcpy (packet.data (), reply.c_str ());
>>> +  SELF_CHECK (check_is_address_tagged_reply (packet, &is_tagged) == true);
>>
>> I think this particular reply (malformed) should make the packet check fail, as
>> we risk the remote returning garbage values and things working just because the
>> first byte was the expected number.
>>
>> We should be more strict with the packet reply format and check its length.
>>
>>> +  SELF_CHECK (is_tagged == true);
>>> +
>>> +  /* Test if only the first byte (00) is correctly extracted from a long
>>> +     numerical reply, with remaining garbage.  */
>>> +  reply = "0004A590001234006mC0fe";
>>> +  /* Because the first byte is 00, is_tagged should be set to false.  */
>>> +  is_tagged = true;
>>> +  strcpy (packet.data (), reply.c_str ());
>>> +  SELF_CHECK (check_is_address_tagged_reply (packet, &is_tagged) == true);
>>> +  SELF_CHECK (is_tagged == false);
>>
>> Same case for the above test.
>>
>>> +
>>> +  /* Test if only the first byte, 04, is correctly extracted and recognized
>>> +     as invalid (only 00 and 01 are valid replies).  */
>>> +  reply = "0404A590001234006mC0fe";
>>> +  /* Because the first byte is invalid is_tagged must not change.  */
>>> +  is_tagged = false;
>>> +  strcpy (packet.data (), reply.c_str ());
>>> +  SELF_CHECK (check_is_address_tagged_reply (packet, &is_tagged) == false);
>>> +  SELF_CHECK (is_tagged == false);
>>>   }
>>>   static void
>>
>> Also a similar situation.
>>
>> We should exercise the following:
>>
>> - Empty reply
>> - Not tagged - "00"
>> - Tagged - "01"
>> - Error - "E??"
>> - Malformed - packets of incorrect length and values.
> 
> I agree, but regarding the malformed case due to an incorrect length,
> in check_is_address_tagged_reply we explicitly request to convert
> only one byte in hex format (2 hex digits):
> 
>    /* Convert only 2 hex digits, i.e. 1 byte in hex format.  */
>    hex2bin (packet.data (), &reply, 1);
> 
> so the rest is truncated. I can add a test to verify if truncation is right,
> for instance: "0104A590001234006mC0fe" should returned "Tagged". I don't
> think it's worth checking for length in the code.
> 
> So, how about:
> 
> Empty reply
> Not tagged - "00"
> Tagged - "01"
> Error - "E00"
> Malformed, length truncation - "0104A590001234006"
> Malformed, values - "0m" (not a hex value)

ah, and an invalid reply as well, like 04 (which is neither 00 nor 01), so:

Empty reply - ""
Not tagged - "00"
Tagged - "01"
Invalid - "04"
Error - "E00"
Malformed, length truncation - "0104A590001234006"
Malformed, values - "0m" (not a hex value)


Cheers,
Gustavo

Re: [PATCH v4 6/8] gdb: Add qIsAddressTagged packet

[Gustavo Romero]

Hi Luis,

On 4/16/24 3:04 PM, Luis Machado wrote:
> Close, by I still have a few comments below.
> 
> On 4/16/24 15:07, Gustavo Romero wrote:
>> This commit adds a new packet, qIsAddressTagged, allowing GDB remote
>> targets to use it to query the stub if a given address is tagged.
>>
>> 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 introduction of qIsAddressTagged packet allows checking
>> if an address is tagged in an agnostic way.
>>
>> The is_address_tagged target hook in remote.c attempts to use the
>> qIsAddressTagged packet first for checking if an address is tagged and
>> if the stub does not support such a packet (reply is empty) it falls
>> back to using the current mechanism that reads the contents of
>> /proc/<PID>/smaps via vFile requests.
>>
>> Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
>> ---
>>   gdb/remote.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 49 insertions(+)
>>
>> diff --git a/gdb/remote.c b/gdb/remote.c
>> index 9717db55e27..63799ac5e3f 100644
>> --- a/gdb/remote.c
>> +++ b/gdb/remote.c
>> @@ -15534,6 +15534,40 @@ create_store_memtags_request (gdb::char_vector &packet, CORE_ADDR address,
>>     strcpy (packet.data (), request.c_str ());
>>   }
>>   
>> +static void
>> +create_is_address_tagged_request (gdbarch *gdbarch, gdb::char_vector &packet,
>> +				  CORE_ADDR address)
>> +{
>> +  int addr_size;
>> +  std::string request;
>> +
>> +  addr_size = gdbarch_addr_bit (gdbarch) / 8;
>> +  request = string_printf ("qIsAddressTagged:%s", phex_nz (address, addr_size));
>> +
>> +  if (packet.size () < request.length () + 1)
>> +    error (_("Contents too big for packet qIsAddressTagged."));
>> +
>> +  strcpy (packet.data (), request.c_str ());
>> +}
>> +
>> +static bool
>> +check_is_address_tagged_reply (gdb::char_vector &packet, bool *tagged)
> 
> Instead of passing TAGGED as pointer, make it a reference. It is safer.

Thanks, fixed in v5.


>> +{
>> +  if (packet_check_result (packet).status () != PACKET_OK)
> 
> This function signature is incorrect and leads to a build error. This function has two
> arguments.

argh, not sure how that happened, fixed in v5.


> Also, this check will return false if the packet yields an error and if the packet is not
> supported. We need to be able to distinguish between unsupported and error here, right?

Error replies (Exx) and empty replies are treat the same: they fail the
check. As a consequence, the fallback mechanism will be used.


>> +    return false;
>> +
>> +  gdb_byte reply;
>> +  /* Convert only 2 hex digits, i.e. 1 byte in hex format.  */
>> +  hex2bin (packet.data (), &reply, 1);
>> +
>> +  if (reply == 0x00 || reply == 0x01) {
>> +    *tagged = !!reply;
> 
> Passing tagged as reference just use tagged instead of *tagged here.
> 
>> +    return true;
>> +  }
>> +
>> +  return false;
>> +}
>> +
>>   /* Implement the "fetch_memtags" target_ops method.  */
>>   
>>   bool
>> @@ -15580,6 +15614,21 @@ remote_target::store_memtags (CORE_ADDR address, size_t len,
>>   bool
>>   remote_target::is_address_tagged (gdbarch *gdbarch, CORE_ADDR address)
>>   {
>> +  struct remote_state *rs = get_remote_state ();
>> +  bool is_addr_tagged;
>> +
> 
> Before sending the packet, we need to check if the packet is supported. Generally gdb
> will send it the first time around, but if the packet isn't supported gdb shouldn't
> keep sending these packets if the stub is gonna reply empty again.

hmm, I missed GDB's "auto" mechanism for this. Thanks, fixed in v5.


> See remote_target::remote_query_attached for an example of how we deal with this.
> 
> First you need to have a new enum PACKET_qIsAddressTagged, so we can register if
> the packet is supported or not at runtime.
> 
> Then at the start of the function:
> 
> if (m_features.packet_support (PACKET_qIsAddressTagged) != PACKET_DISABLE)
>    {
>      /* Use the qIsTaggedAddress packet.  */
>    }
> else
>    {
>      /* Use the fallback smaps method.  */
>    }
> 
> That way gdb only sends the qIsTaggedAddress packet once. If it works, then gdb
> keeps using it. Otherwise it always uses the fallback.

This if/else struct can't be used afaics. It is problematic on the first time
it is executed, because if the packet is not supported by the stub (the check
is performed inside the if statement, not before it) the fallback code must be
called, but since the fallback code is inside the else {} statement, and if {}
is already taken, it won't be executed. So, it must be instead:

if (m_features.packet_support (PACKET_qIsAddressTagged) != PACKET_DISABLE)
   {
     /* Use the qIsTaggedAddress packet.  */
   }

/* Use the fallback smaps method.  */

Please see v5.


Cheers,
Gustavo