[PATCH v2 0/4] Add another way to check for MTE-tagged addresses on remote targets

[PATCH v2 0/4] Add another way to check for MTE-tagged addresses on remote targets

[Gustavo Romero]

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

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

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

Since v1:
 - Fixed build error "no match for ‘operator!=’ (operand types are ‘packet_result’ and ‘packet_status’)"
   reported by Linaro CI bot, caused by a last-minute rebase;
 - Added instructions on how to test the series on a remote target using
   QEMU gdbstub (-g option) -- see below.
 
----

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) 


And, finally, testing it 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) 


Cheers,
Gustavo

Gustavo Romero (4):
  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: Add new remote packet to check if address is tagged

 gdb/aarch64-linux-nat.c   |  8 +++++
 gdb/aarch64-linux-tdep.c  | 22 +++-----------
 gdb/arch-utils.c          |  2 +-
 gdb/arch-utils.h          |  2 +-
 gdb/corelow.c             |  8 +++++
 gdb/gdbarch-gen.h         |  4 +--
 gdb/gdbarch.c             |  2 +-
 gdb/gdbarch_components.py |  2 +-
 gdb/printcmd.c            | 29 +++++++++++-------
 gdb/remote.c              | 62 +++++++++++++++++++++++++++++++++++++++
 gdb/target-delegates.c    | 28 ++++++++++++++++++
 gdb/target.c              |  6 ++++
 gdb/target.h              |  6 ++++
 13 files changed, 147 insertions(+), 34 deletions(-)

-- 
2.34.1

[PATCH v2 1/4] gdb: aarch64: Remove MTE address checking from get_memtag

[Gustavo Romero]

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

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

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

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

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

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

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

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

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

[PATCH v2 2/4] gdb: aarch64: Move MTE address check out of set_memtag

[Gustavo Romero]

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

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

diff --git a/gdb/aarch64-linux-tdep.c b/gdb/aarch64-linux-tdep.c
index 50055ac3f48..8e6e63d4dcb 100644
--- a/gdb/aarch64-linux-tdep.c
+++ b/gdb/aarch64-linux-tdep.c
@@ -2525,10 +2525,6 @@ aarch64_linux_set_memtags (struct gdbarch *gdbarch, struct value *address,
       /* Remove the top byte.  */
       addr = gdbarch_remove_non_address_bits (gdbarch, addr);
 
-      /* Make sure we are dealing with a tagged address to begin with.  */
-      if (!aarch64_linux_tagged_address_p (gdbarch, address))
-	return false;
-
       /* With G being the number of tag granules and N the number of tags
 	 passed in, we can have the following cases:
 
diff --git a/gdb/printcmd.c b/gdb/printcmd.c
index cb0d32aa4bc..ae4d640ccf2 100644
--- a/gdb/printcmd.c
+++ b/gdb/printcmd.c
@@ -3127,6 +3127,12 @@ 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 v2 3/4] gdb: aarch64: Remove MTE address checking from memtag_matches_p

[Gustavo Romero]

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

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

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

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

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

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

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

[PATCH v2 4/4] gdb: Add new remote packet to check if address is tagged

[Gustavo Romero]

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

It also adds a new GDB remote feature, 'memory-tagging-check-add+',
which must be advertised by the GDB servers to inform GDB they can reply
to address checks via the new qMemTagAddrCheck remote packet.

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

This is not ideal, for example, on QEMU gdbstub and in other cases,
like in baremetal debugging, where there is no notion of any OS file
like smaps. Hence, qMemTagAddrCheck packet allows check addresses in
an OS-agnostic way.

For supporting the new packet, a new target hook is introduced,
check_memtag_addr, which is used instead of the gdbarch_tagged_address_p
gdbarch hook in the upper layers (printcmd.c).

The new target hook is then specialized per target, for remote.c,
aarch64-linux-nat.c, and corelow.c targets (the current targets that
are MTE-aware).

The target hook in remote.c uses the qMemTagAddrCheck packet to check
an address if the server advertised the 'memory-tagging-check-add+'
feature, otherwise it falls back to using the current mechanism, i.e. it
reads the /proc/<PID>/smaps contents.

In the aarch64-linux-nat.c and corelow.c the target hook uses the
gdbarch_tagged_address_p gdbarch hook, so there is no change regarding
how an address is checked in these targets. Just the
gdbarch_tagged_address_p signature is changed for convenience, since
target_check_memtag_addr takes the address to be checked as a CORE_ADDR
type.

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

diff --git a/gdb/aarch64-linux-nat.c b/gdb/aarch64-linux-nat.c
index 3face34ce79..1c64df6af41 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 check_memtag_addr (CORE_ADDR address) override;
 };
 
 static aarch64_linux_nat_target the_aarch64_linux_nat_target;
@@ -1071,6 +1073,12 @@ aarch64_linux_nat_target::store_memtags (CORE_ADDR address, size_t len,
   return false;
 }
 
+bool
+aarch64_linux_nat_target::check_memtag_addr (CORE_ADDR address)
+{
+  return gdbarch_tagged_address_p (current_inferior ()->arch (), address);
+}
+
 void _initialize_aarch64_linux_nat ();
 void
 _initialize_aarch64_linux_nat ()
diff --git a/gdb/aarch64-linux-tdep.c b/gdb/aarch64-linux-tdep.c
index fc60e602748..2a47c3f0845 100644
--- a/gdb/aarch64-linux-tdep.c
+++ b/gdb/aarch64-linux-tdep.c
@@ -2451,17 +2451,15 @@ aarch64_mte_get_atag (CORE_ADDR address)
 /* Implement the tagged_address_p gdbarch method.  */
 
 static bool
-aarch64_linux_tagged_address_p (struct gdbarch *gdbarch, struct value *address)
+aarch64_linux_tagged_address_p (struct gdbarch *gdbarch, CORE_ADDR address)
 {
-  gdb_assert (address != nullptr);
-
-  CORE_ADDR addr = value_as_address (address);
+  gdb_assert (address);
 
   /* Remove the top byte for the memory range check.  */
-  addr = gdbarch_remove_non_address_bits (gdbarch, addr);
+  address = gdbarch_remove_non_address_bits (gdbarch, address);
 
   /* Check if the page that contains ADDRESS is mapped with PROT_MTE.  */
-  if (!linux_address_in_memtag_page (addr))
+  if (!linux_address_in_memtag_page (address))
     return false;
 
   /* We have a valid tag in the top byte of the 64-bit address.  */
diff --git a/gdb/arch-utils.c b/gdb/arch-utils.c
index 456bfe971ff..cb149c36bc9 100644
--- a/gdb/arch-utils.c
+++ b/gdb/arch-utils.c
@@ -102,7 +102,7 @@ default_memtag_to_string (struct gdbarch *gdbarch, struct value *tag)
 /* See arch-utils.h */
 
 bool
-default_tagged_address_p (struct gdbarch *gdbarch, struct value *address)
+default_tagged_address_p (struct gdbarch *gdbarch, CORE_ADDR address)
 {
   /* By default, assume the address is untagged.  */
   return false;
diff --git a/gdb/arch-utils.h b/gdb/arch-utils.h
index 2dcd8f6dc53..467be40c688 100644
--- a/gdb/arch-utils.h
+++ b/gdb/arch-utils.h
@@ -141,7 +141,7 @@ extern std::string default_memtag_to_string (struct gdbarch *gdbarch,
 					     struct value *tag);
 
 /* Default implementation of gdbarch_tagged_address_p.  */
-bool default_tagged_address_p (struct gdbarch *gdbarch, struct value *address);
+bool default_tagged_address_p (struct gdbarch *gdbarch, CORE_ADDR address);
 
 /* Default implementation of gdbarch_memtag_matches_p.  */
 extern bool default_memtag_matches_p (struct gdbarch *gdbarch,
diff --git a/gdb/corelow.c b/gdb/corelow.c
index f4e8273d962..676738825fb 100644
--- a/gdb/corelow.c
+++ b/gdb/corelow.c
@@ -109,6 +109,8 @@ class core_target final : public process_stratum_target
   bool fetch_memtags (CORE_ADDR address, size_t len,
 		      gdb::byte_vector &tags, int type) override;
 
+  bool check_memtag_addr (CORE_ADDR address) override;
+
   x86_xsave_layout fetch_x86_xsave_layout () override;
 
   /* A few helpers.  */
@@ -1410,6 +1412,12 @@ core_target::fetch_memtags (CORE_ADDR address, size_t len,
   return false;
 }
 
+bool
+core_target::check_memtag_addr (CORE_ADDR address)
+{
+  return gdbarch_tagged_address_p (current_inferior ()->arch (), 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 ae4d640ccf2..c81c75afc5d 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 (target_check_memtag_addr (value_as_address(v_addr)))
 	    {
 	      /* Fetch the allocation tag.  */
 	      struct value *tag
@@ -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_check_memtag_addr (value_as_address(value));
 }
 
 /* Helper for parsing arguments for print_command_1.  */
@@ -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_check_memtag_addr(addr))
+    show_addr_not_tagged (addr);
 
   value *tag_value = gdbarch_get_memtag (arch, val, tag_type);
   std::string tag = gdbarch_memtag_to_string (arch, tag_value);
@@ -3104,8 +3105,9 @@ parse_set_allocation_tag_input (const char *args, struct value **val,
 
   /* If the address is not in a region memory mapped with a memory tagging
      flag, it is no use trying to access/manipulate its allocation tag.  */
-  if (!gdbarch_tagged_address_p (current_inferior ()->arch (), *val))
-    show_addr_not_tagged (value_as_address (*val));
+  CORE_ADDR addr = value_as_address (*val);
+  if (!target_check_memtag_addr (addr))
+    show_addr_not_tagged (addr);
 }
 
 /* Implement the "memory-tag set-allocation-tag" command.
@@ -3129,8 +3131,9 @@ memory_tag_set_allocation_tag_command (const char *args, int from_tty)
 
   /* If the address is not in a region memory mapped with a memory tagging
      flag, it is no use trying to manipulate its allocation tag.  */
-  if (!gdbarch_tagged_address_p (current_inferior ()->arch (), val)) {
-    show_addr_not_tagged (value_as_address(val));
+  CORE_ADDR addr = value_as_address (val);
+  if (!target_check_memtag_addr (addr)) {
+    show_addr_not_tagged (addr);
   }
 
   if (!gdbarch_set_memtags (current_inferior ()->arch (), val, length, tags,
@@ -3158,12 +3161,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_check_memtag_addr (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..c5544d2e53c 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -337,6 +337,9 @@ enum {
      packets and the tag violation stop replies.  */
   PACKET_memory_tagging_feature,
 
+  /* Support checking if an address is tagged via qMemTagAddrCheck packet.  */
+  PACKET_memory_tagging_check_addr_feature,
+
   PACKET_MAX
 };
 
@@ -758,6 +761,10 @@ struct remote_features
   bool remote_memory_tagging_p () const
   { return packet_support (PACKET_memory_tagging_feature) == PACKET_ENABLE; }
 
+  bool remote_memory_tagging_check_addr_p () const
+  { return packet_support (PACKET_memory_tagging_check_addr_feature) ==
+			   PACKET_ENABLE; }
+
   /* Reset all packets back to "unknown support".  Called when opening a
      new connection to a remote target.  */
   void reset_all_packet_configs_support ();
@@ -1084,6 +1091,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 check_memtag_addr (CORE_ADDR address) override;
+
 public: /* Remote specific methods.  */
 
   void remote_download_command_source (int num, ULONGEST addr,
@@ -5762,6 +5771,8 @@ static const struct protocol_feature remote_protocol_features[] = {
   { "no-resumed", PACKET_DISABLE, remote_supported_packet, PACKET_no_resumed },
   { "memory-tagging", PACKET_DISABLE, remote_supported_packet,
     PACKET_memory_tagging_feature },
+  { "memory-tagging-check-addr", PACKET_DISABLE, remote_supported_packet,
+    PACKET_memory_tagging_check_addr_feature },
 };
 
 static char *remote_support_xml;
@@ -5873,6 +5884,10 @@ remote_target::remote_query_supported ()
 	  != AUTO_BOOLEAN_FALSE)
 	remote_query_supported_append (&q, "memory-tagging+");
 
+      if (m_features.packet_set_cmd_state (PACKET_memory_tagging_check_addr_feature)
+	  != AUTO_BOOLEAN_FALSE)
+	remote_query_supported_append (&q, "memory-tagging-check-addr+");
+
       /* Keep this one last to work around a gdbserver <= 7.10 bug in
 	 the qSupported:xmlRegisters=i386 handling.  */
       if (remote_support_xml != NULL
@@ -15532,6 +15547,19 @@ create_store_memtags_request (gdb::char_vector &packet, CORE_ADDR address,
   strcpy (packet.data (), request.c_str ());
 }
 
+static void
+create_check_memtag_addr_request (gdb::char_vector &packet, CORE_ADDR address)
+{
+  int addr_size = gdbarch_addr_bit (current_inferior ()->arch()) / 8;
+
+  std::string request = string_printf ("qMemTagAddrCheck:%s", phex_nz (address, addr_size));
+
+  if (packet.size () < request.length ())
+    error (_("Contents too big for packet qMemTagAddrCheck."));
+
+  strcpy (packet.data (), request.c_str ());
+}
+
 /* Implement the "fetch_memtags" target_ops method.  */
 
 bool
@@ -15573,6 +15601,36 @@ remote_target::store_memtags (CORE_ADDR address, size_t len,
   return packet_check_result (rs->buf).status () == PACKET_OK;
 }
 
+bool
+remote_target::check_memtag_addr (CORE_ADDR address)
+{
+  struct remote_state *rs = get_remote_state ();
+
+  if (!m_features.remote_memory_tagging_check_addr_p ())
+    /* Fallback to reading /proc/<PID>/smaps for checking if an address is
+       tagged or not.  */
+    return gdbarch_tagged_address_p (current_inferior ()->arch (), address);
+
+  create_check_memtag_addr_request (rs->buf, address);
+
+  putpkt (rs->buf);
+  getpkt (&rs->buf);
+
+  /* Check if reply is OK.  */
+  if ((packet_check_result (rs->buf).status () != PACKET_OK) || rs->buf.empty())
+    return false;
+
+  gdb_byte tagged_addr;
+  /* Convert only 2 hex digits, i.e. 1 byte in hex format.  */
+  hex2bin(rs->buf.data(), &tagged_addr , 1);
+  if (tagged_addr)
+    /* 01 means address is tagged.  */
+    return true;
+  else
+    /* 00 means address is not tagged.  */
+    return false;
+}
+
 /* Return true if remote target T is non-stop.  */
 
 bool
@@ -16056,6 +16114,10 @@ Show the maximum size of the address (in bits) in a memory packet."), NULL,
   add_packet_config_cmd (PACKET_memory_tagging_feature,
 			 "memory-tagging-feature", "memory-tagging-feature", 0);
 
+  add_packet_config_cmd (PACKET_memory_tagging_check_addr_feature,
+			 "memory-tagging-check-addr-feature",
+			 "memory-tagging-check-addr-feature", 0);
+
   /* Assert that we've registered "set remote foo-packet" commands
      for all packet configs.  */
   {
diff --git a/gdb/target-delegates.c b/gdb/target-delegates.c
index 59ea70458ad..fbd9e3f65b4 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 check_memtag_addr (CORE_ADDR arg0) 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 check_memtag_addr (CORE_ADDR arg0) override;
   x86_xsave_layout fetch_x86_xsave_layout () override;
 };
 
@@ -4562,6 +4564,32 @@ debug_target::store_memtags (CORE_ADDR arg0, size_t arg1, const gdb::byte_vector
   return result;
 }
 
+bool
+target_ops::check_memtag_addr (CORE_ADDR arg0)
+{
+  return this->beneath ()->check_memtag_addr (arg0);
+}
+
+bool
+dummy_target::check_memtag_addr (CORE_ADDR arg0)
+{
+  tcomplain ();
+}
+
+bool
+debug_target::check_memtag_addr (CORE_ADDR arg0)
+{
+  gdb_printf (gdb_stdlog, "-> %s->check_memtag_addr (...)\n", this->beneath ()->shortname ());
+  bool result
+    = this->beneath ()->check_memtag_addr (arg0);
+  gdb_printf (gdb_stdlog, "<- %s->check_memtag_addr (", this->beneath ()->shortname ());
+  target_debug_print_CORE_ADDR (arg0);
+  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..938a0f76595 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_check_memtag_addr (CORE_ADDR address)
+{
+  return current_inferior ()->top_target ()->check_memtag_addr (address);
+}
+
 x86_xsave_layout
 target_fetch_x86_xsave_layout ()
 {
diff --git a/gdb/target.h b/gdb/target.h
index c9eaff16346..bb64d32994e 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 check_memtag_addr (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_check_memtag_addr (CORE_ADDR address);
+
 extern x86_xsave_layout target_fetch_x86_xsave_layout ();
 
 /* Command logging facility.  */
-- 
2.34.1

Re: [PATCH v2 4/4] gdb: Add new remote packet to check if address is tagged

[Thiago Jung Bauermann]

Hello Gustavo,

I started reviewing the patch series backwards...
I'll provide comments on this patch first.

Overall, it looks great. I just have some localised comments in a few
places below.

Also, for ease of review I suggest splitting this patch in two:

- one that introduces the new check_memtag_addr target hook and converts
  the existing code to use it,
- and another that adds a check_memtag_addr target hook implementation
  to the remote target.

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

> This commit adds a new packet qMemTagAddrCheck allowing GDB remote
> targets to use it to query gdbservers if a given address is tagged.
>
> It also adds a new GDB remote feature, 'memory-tagging-check-add+',
> which must be advertised by the GDB servers to inform GDB they can reply
> to address checks via the new qMemTagAddrCheck remote packet.

You will need to document the remote protocol changes in gdb.texinfo, in
the "Remote Serial Protocol" appendix.

> Currently, this address check is done via a read query, where the
> contents of /proc/<PID>/smaps is read and the flags in there are
> inspected for MTE-related flags that indicate the address is in a tagged
> memory region.
>
> This is not ideal, for example, on QEMU gdbstub and in other cases,
> like in baremetal debugging, where there is no notion of any OS file
> like smaps. Hence, qMemTagAddrCheck packet allows check addresses in
> an OS-agnostic way.
>
> For supporting the new packet, a new target hook is introduced,
> check_memtag_addr, which is used instead of the gdbarch_tagged_address_p
> gdbarch hook in the upper layers (printcmd.c).
>
> The new target hook is then specialized per target, for remote.c,
> aarch64-linux-nat.c, and corelow.c targets (the current targets that
> are MTE-aware).
>
> The target hook in remote.c uses the qMemTagAddrCheck packet to check
> an address if the server advertised the 'memory-tagging-check-add+'
> feature, otherwise it falls back to using the current mechanism, i.e. it
> reads the /proc/<PID>/smaps contents.
>
> In the aarch64-linux-nat.c and corelow.c the target hook uses the
> gdbarch_tagged_address_p gdbarch hook, so there is no change regarding
> how an address is checked in these targets. Just the
> gdbarch_tagged_address_p signature is changed for convenience, since
> target_check_memtag_addr takes the address to be checked as a CORE_ADDR
> type.

I agree that a CORE_ADDR argument is more convenient.

> @@ -1071,6 +1073,12 @@ aarch64_linux_nat_target::store_memtags (CORE_ADDR address, size_t len,
>    return false;
>  }
>
> +bool
> +aarch64_linux_nat_target::check_memtag_addr (CORE_ADDR address)
> +{
> +  return gdbarch_tagged_address_p (current_inferior ()->arch (), address);

I think it's better to pass the gdbarch as an argument to the
check_memtag_addr hook rather than getting it from current_inferior
here, even if in practice your patch is equivalent to the existing
code.

The reason is that we are trying to move calls to current_* functions
(which is global state in disguise) up the stack, so that most of GDB
needs to reference only local state.

Then if callers have a gdbarch available in their context they can pass
it to the hook, or else they can use current_inferior ()->arch ().

> @@ -1410,6 +1412,12 @@ core_target::fetch_memtags (CORE_ADDR address, size_t len,
>    return false;
>  }
>
> +bool
> +core_target::check_memtag_addr (CORE_ADDR address)
> +{
> +  return gdbarch_tagged_address_p (current_inferior ()->arch (), address);

Same comment here, of course.

> diff --git a/gdb/printcmd.c b/gdb/printcmd.c
> index ae4d640ccf2..c81c75afc5d 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 (target_check_memtag_addr (value_as_address(v_addr)))

Missing space between "value_as_address" and the opening parens.

Also, not a problem introduced by you, but I don't understand why the code
uses the gdbarch from current_inferior if it was passed a gdbarch in the
arguments.

So I'd add a separate patch before this one to fix this code to use the
gdbarch that was passed as a parameter. Then this patch can also pass it
as a parameter to target_check_memtag_addr as I mentioned in an earlier
comment.

>  	    {
>  	      /* Fetch the allocation tag.  */
>  	      struct value *tag
> @@ -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_check_memtag_addr (value_as_address(value));

Here there's no gdbarch available in context, but since there's only one
caller to this function, it's easy to add the gdbarch parameter to it
and make the caller pass it down. The caller does get it from
current_inferior, so perhaps I'm being too pedantic here but IMHO it's
worth it.

Also, a space is missing before the opening parens.

>  }
>
>  /* Helper for parsing arguments for print_command_1.  */
> @@ -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);

Missing space before the opening parens.

>    if (tag_type == memtag_type::allocation
> -      && !gdbarch_tagged_address_p (arch, val))
> -    show_addr_not_tagged (value_as_address (val));
> +      && !target_check_memtag_addr(addr))

Missing space before the opening parens.

> +    show_addr_not_tagged (addr);
>
>    value *tag_value = gdbarch_get_memtag (arch, val, tag_type);
>    std::string tag = gdbarch_memtag_to_string (arch, tag_value);
> @@ -3104,8 +3105,9 @@ parse_set_allocation_tag_input (const char *args, struct value **val,
>
>    /* If the address is not in a region memory mapped with a memory tagging
>       flag, it is no use trying to access/manipulate its allocation tag.  */
> -  if (!gdbarch_tagged_address_p (current_inferior ()->arch (), *val))
> -    show_addr_not_tagged (value_as_address (*val));
> +  CORE_ADDR addr = value_as_address (*val);
> +  if (!target_check_memtag_addr (addr))
> +    show_addr_not_tagged (addr);

This is another instance where I'd suggest making the caller pass
gdbarch as an argument so that it can be used here, since this function
only has one caller.

>  }
>
>  /* Implement the "memory-tag set-allocation-tag" command.
> @@ -3129,8 +3131,9 @@ memory_tag_set_allocation_tag_command (const char *args, int from_tty)
>
>    /* If the address is not in a region memory mapped with a memory tagging
>       flag, it is no use trying to manipulate its allocation tag.  */
> -  if (!gdbarch_tagged_address_p (current_inferior ()->arch (), val)) {
> -    show_addr_not_tagged (value_as_address(val));
> +  CORE_ADDR addr = value_as_address (val);
> +  if (!target_check_memtag_addr (addr)) {
> +    show_addr_not_tagged (addr);
>    }

This is a preexisting issue in the code, but since you're touching it:
the GNU style is to not use curly braces when there's only one statement
in the if block.

> @@ -15532,6 +15547,19 @@ create_store_memtags_request (gdb::char_vector &packet, CORE_ADDR address,
>    strcpy (packet.data (), request.c_str ());
>  }
>
> +static void
> +create_check_memtag_addr_request (gdb::char_vector &packet, CORE_ADDR address)
> +{
> +  int addr_size = gdbarch_addr_bit (current_inferior ()->arch()) / 8;
> +
> +  std::string request = string_printf ("qMemTagAddrCheck:%s", phex_nz (address, addr_size));
> +
> +  if (packet.size () < request.length ())

There's an off-by-one error here: packet is expected to be
null-terminated, but request.length doesn't count a terminating null
byte so a "+ 1" needs to be added here.

> +    error (_("Contents too big for packet qMemTagAddrCheck."));
> +
> +  strcpy (packet.data (), request.c_str ());
> +}
> +
>  /* Implement the "fetch_memtags" target_ops method.  */
>
>  bool
> @@ -15573,6 +15601,36 @@ remote_target::store_memtags (CORE_ADDR address, size_t len,
>    return packet_check_result (rs->buf).status () == PACKET_OK;
>  }
>
> +bool
> +remote_target::check_memtag_addr (CORE_ADDR address)
> +{
> +  struct remote_state *rs = get_remote_state ();
> +
> +  if (!m_features.remote_memory_tagging_check_addr_p ())
> +    /* Fallback to reading /proc/<PID>/smaps for checking if an address is
> +       tagged or not.  */

Currently this comment is accurate, but if some other architecture adds
a gdbarch method that doesn't use /proc/<PID>/smaps then it won't be
anymore.

I suggest saying something like "Fallback to arch-specific method of
checking whether an address is tagged".

> +    return gdbarch_tagged_address_p (current_inferior ()->arch (), address);
> +
> +  create_check_memtag_addr_request (rs->buf, address);
> +
> +  putpkt (rs->buf);
> +  getpkt (&rs->buf);
> +
> +  /* Check if reply is OK.  */
> +  if ((packet_check_result (rs->buf).status () != PACKET_OK) || rs->buf.empty())

Missing space between "empty" and opening parens.

But I don't understand why check whether buf is empty. Looking at
remote_target::getpkt, it doesn't look like buf is ever emptied.

> +    return false;
> +
> +  gdb_byte tagged_addr;
> +  /* Convert only 2 hex digits, i.e. 1 byte in hex format.  */
> +  hex2bin(rs->buf.data(), &tagged_addr , 1);

Missing space between "hex2bin", "data" and the opening parens.

> +  if (tagged_addr)
> +    /* 01 means address is tagged.  */
> +    return true;
> +  else
> +    /* 00 means address is not tagged.  */
> +    return false;

The above can be simplified to "return tagged_addr != 0".

--
Thiago

Re: [PATCH v2 1/4] gdb: aarch64: Remove MTE address checking from get_memtag

[Thiago Jung Bauermann]

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

> 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.

In cases c) and d), gdbarch_get_memtag gets called even if
gdbarch_tagged_address_p returns false, so won't this commit change the
behaviour of those functions in that case? I don't know enough about MTE
to be sure.

--
Thiago

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

[Thiago Jung Bauermann]

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

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

gdbarch_set_memtags is also called from
memory_tag_with_logical_tag_command. Shouldn't the same check be added
there?

>
> Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
> ---
>  gdb/aarch64-linux-tdep.c | 4 ----
>  gdb/printcmd.c           | 6 ++++++
>  2 files changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/gdb/aarch64-linux-tdep.c b/gdb/aarch64-linux-tdep.c
> index 50055ac3f48..8e6e63d4dcb 100644
> --- a/gdb/aarch64-linux-tdep.c
> +++ b/gdb/aarch64-linux-tdep.c
> @@ -2525,10 +2525,6 @@ aarch64_linux_set_memtags (struct gdbarch *gdbarch, struct value *address,
>        /* Remove the top byte.  */
>        addr = gdbarch_remove_non_address_bits (gdbarch, addr);
>
> -      /* Make sure we are dealing with a tagged address to begin with.  */
> -      if (!aarch64_linux_tagged_address_p (gdbarch, address))
> -	return false;
> -
>        /* With G being the number of tag granules and N the number of tags
>  	 passed in, we can have the following cases:
>
> diff --git a/gdb/printcmd.c b/gdb/printcmd.c
> index cb0d32aa4bc..ae4d640ccf2 100644
> --- a/gdb/printcmd.c
> +++ b/gdb/printcmd.c
> @@ -3127,6 +3127,12 @@ 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));
> +  }

GNU style doesn't use curly braces in if blocks with only one statement.

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

--
Thiago

Re: [PATCH v2 1/4] gdb: aarch64: Remove MTE address checking from get_memtag

[Thiago Jung Bauermann]

Thiago Jung Bauermann <thiago.bauermann@linaro.org> writes:

> Gustavo Romero <gustavo.romero@linaro.org> writes:
>
>> 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.
>
> In cases c) and d), gdbarch_get_memtag gets called even if
> gdbarch_tagged_address_p returns false, so won't this commit change the
> behaviour of those functions in that case? I don't know enough about MTE
> to be sure.

I just realised that in cases c) and d) an error will be thrown by
show_addr_not_tagged if gdbarch_tagged_address_p returns false, so
gdbarch_get_memtag doesn't get called in that case and my concern isn't
valid. Sorry about that.

Reviewed-by: Thiago Jung Bauermann <thiago.bauermann@linaro.org>

--
Thiago

Re: [PATCH v2 3/4] gdb: aarch64: Remove MTE address checking from memtag_matches_p

[Thiago Jung Bauermann]

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

> This commit removes aarch64_linux_tagged_address_p from
> aarch64_linux_memtag_matches_p. aarch64_linux_tagged_address_p checks if
> an address is tagged (MTE) or not.
>
> The check is redundant because aarch64_linux_memtag_matches_p is always
> called from the upper layers (i.e. from printcmd.c via gdbarch hook
> gdbarch_memtag_matches_p) after either gdbarch_tagged_address_p (that
> already points to aarch64_linux_tagged_address_p) has been called or
> after should_validate_memtags (that calls gdbarch_tagged_address_p at
> the end) has been called, so the address is already checked. Hence:
>
> a) in print_command_1, gdbarch_memtag_matches_p is called only after
> should_validate_memtags is called, which checks the address at its end;
>
> b) in memory_tag_check_command, gdbarch_memtag_matches_p is called only
> after gdbarch_tagged_address_p is called directly.
>
> Also, because after this change the address checking only happens at the
> upper layer it now allows the address checking to be specialized easily
> per target, via a target hook.
>
> Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
> ---
>  gdb/aarch64-linux-tdep.c | 4 ----
>  1 file changed, 4 deletions(-)

Looks good to me. Thanks!

Reviewed-by: Thiago Jung Bauermann <thiago.bauermann@linaro.org>

--
Thiago

Re: [PATCH v2 4/4] gdb: Add new remote packet to check if address is tagged

[Thiago Jung Bauermann]

Just one more suggestion that occurred to me later:

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

> For supporting the new packet, a new target hook is introduced,
> check_memtag_addr, which is used instead of the gdbarch_tagged_address_p
> gdbarch hook in the upper layers (printcmd.c).

"check_memtag_addr" is a bit vague: what does it check? This confused me
a bit when I was reading the code with these patches
applied. Alternatives I can think of are "is_addr_tagged", or
"tagged_address_p".

I'm not too fond of the "_p" suffix, but it has the advantage of being
consistent with the existing gdbarch hook so it may be preferable in
this case.

Ah, one other thing that just occurred to me: I think this warrants an
entry in gdb/NEWS.

--
Thiago

Re: [PATCH v2 0/4] Add another way to check for MTE-tagged addresses on remote targets

[Luis Machado]

Hi,

On 3/28/24 22:48, Gustavo Romero wrote:
> This series introduces a new method to check for MTE-tagged addresses on
> remote targets.
> 
> A new remote packet, qMemTagAddrCheck, is introduced, along with a new
> remote feature associated with it, 'memory-tagging-check-add+'. Only
> when 'memory-tagging-check-add+' feature is advertised GDB will use the
> new packet to query if an address is tagged.
> 
> This new mechanism allows for checking MTE addresses in an OS-agnostic
> way, which is necessary when debugging targets that do not support
> '/proc/<PID>/smaps', as the current method of reading the smaps contents
> fails in such cases.
> 
> Since v1:
>  - Fixed build error "no match for ‘operator!=’ (operand types are ‘packet_result’ and ‘packet_status’)"
>    reported by Linaro CI bot, caused by a last-minute rebase;
>  - Added instructions on how to test the series on a remote target using
>    QEMU gdbstub (-g option) -- see below.
>  
> ----
> 
> 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) 
> 
> 
> And, finally, testing it 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) 

Out of curiosity, I see you exercised native, core and QEMU-based remote debugging. Did you give the gdbserver-based remote debugging a try?

I think that is an important check given a gdb + gdbserver debugging session will also use the remote target, but will instead rely on accessing the remote smaps file.

Re: [PATCH v2 4/4] gdb: Add new remote packet to check if address is tagged

[Luis Machado]

On 3/30/24 03:08, Thiago Jung Bauermann wrote:
> 
> Just one more suggestion that occurred to me later:
> 
> Gustavo Romero <gustavo.romero@linaro.org> writes:
> 
>> For supporting the new packet, a new target hook is introduced,
>> check_memtag_addr, which is used instead of the gdbarch_tagged_address_p
>> gdbarch hook in the upper layers (printcmd.c).
> 
> "check_memtag_addr" is a bit vague: what does it check? This confused me
> a bit when I was reading the code with these patches
> applied. Alternatives I can think of are "is_addr_tagged", or
> "tagged_address_p".

That last bit seems more in line with the gdb terminology. A target took "tagged_address_p" or "target_is_address_tagged" should be a bit more clear.

Plus we have precedent in the target hooks is_async_p, can_async_p, always_non_stop_p etc.

> 
> I'm not too fond of the "_p" suffix, but it has the advantage of being
> consistent with the existing gdbarch hook so it may be preferable in
> this case.
> 
> Ah, one other thing that just occurred to me: I think this warrants an
> entry in gdb/NEWS.
> 
> --
> Thiago

Re: [PATCH v2 0/4] Add another way to check for MTE-tagged addresses on remote targets

[Gustavo Romero]

Hi Luis,

On 4/3/24 8:51 AM, Luis Machado wrote:
> Hi,
> 
> On 3/28/24 22:48, Gustavo Romero wrote:
>> This series introduces a new method to check for MTE-tagged addresses on
>> remote targets.
>>
>> A new remote packet, qMemTagAddrCheck, is introduced, along with a new
>> remote feature associated with it, 'memory-tagging-check-add+'. Only
>> when 'memory-tagging-check-add+' feature is advertised GDB will use the
>> new packet to query if an address is tagged.
>>
>> This new mechanism allows for checking MTE addresses in an OS-agnostic
>> way, which is necessary when debugging targets that do not support
>> '/proc/<PID>/smaps', as the current method of reading the smaps contents
>> fails in such cases.
>>
>> Since v1:
>>   - Fixed build error "no match for ‘operator!=’ (operand types are ‘packet_result’ and ‘packet_status’)"
>>     reported by Linaro CI bot, caused by a last-minute rebase;
>>   - Added instructions on how to test the series on a remote target using
>>     QEMU gdbstub (-g option) -- see below.
>>   
>> ----
>>
>> 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)
>>
>>
>> And, finally, testing it 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)
> 
> Out of curiosity, I see you exercised native, core and QEMU-based remote debugging. Did you give the gdbserver-based remote debugging a try?
> 
> I think that is an important check given a gdb + gdbserver debugging session will also use the remote target, but will instead rely on accessing the remote smaps file.

Nope. I'll give it a try before sending v3 today.
  

Cheers,
Gustavo

Re: [PATCH v2 0/4] Add another way to check for MTE-tagged addresses on remote targets

[Luis Machado]

On 4/3/24 15:29, Gustavo Romero wrote:
> Hi Luis,
> 
> On 4/3/24 8:51 AM, Luis Machado wrote:
>> Hi,
>>
>> On 3/28/24 22:48, Gustavo Romero wrote:
>>> This series introduces a new method to check for MTE-tagged addresses on
>>> remote targets.
>>>
>>> A new remote packet, qMemTagAddrCheck, is introduced, along with a new
>>> remote feature associated with it, 'memory-tagging-check-add+'. Only
>>> when 'memory-tagging-check-add+' feature is advertised GDB will use the
>>> new packet to query if an address is tagged.
>>>
>>> This new mechanism allows for checking MTE addresses in an OS-agnostic
>>> way, which is necessary when debugging targets that do not support
>>> '/proc/<PID>/smaps', as the current method of reading the smaps contents
>>> fails in such cases.
>>>
>>> Since v1:
>>>   - Fixed build error "no match for ‘operator!=’ (operand types are ‘packet_result’ and ‘packet_status’)"
>>>     reported by Linaro CI bot, caused by a last-minute rebase;
>>>   - Added instructions on how to test the series on a remote target using
>>>     QEMU gdbstub (-g option) -- see below.
>>>   ----
>>>
>>> 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)
>>>
>>>
>>> And, finally, testing it 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)
>>
>> Out of curiosity, I see you exercised native, core and QEMU-based remote debugging. Did you give the gdbserver-based remote debugging a try?
>>
>> I think that is an important check given a gdb + gdbserver debugging session will also use the remote target, but will instead rely on accessing the remote smaps file.
> 
> Nope. I'll give it a try before sending v3 today.

Awesome. Thanks.

I'll do some checking locally with system emulation.

Re: [PATCH v2 4/4] gdb: Add new remote packet to check if address is tagged

[Gustavo Romero]

Hi Thiago, Luis

On 4/3/24 11:04 AM, Luis Machado wrote:
> On 3/30/24 03:08, Thiago Jung Bauermann wrote:
>>
>> Just one more suggestion that occurred to me later:
>>
>> Gustavo Romero <gustavo.romero@linaro.org> writes:
>>
>>> For supporting the new packet, a new target hook is introduced,
>>> check_memtag_addr, which is used instead of the gdbarch_tagged_address_p
>>> gdbarch hook in the upper layers (printcmd.c).
>>
>> "check_memtag_addr" is a bit vague: what does it check? This confused me
>> a bit when I was reading the code with these patches
>> applied. Alternatives I can think of are "is_addr_tagged", or
>> "tagged_address_p".
> 
> That last bit seems more in line with the gdb terminology. A target took "tagged_address_p" or "target_is_address_tagged" should be a bit more clear.
> 
> Plus we have precedent in the target hooks is_async_p, can_async_p, always_non_stop_p etc.
> 
>>
>> I'm not too fond of the "_p" suffix, but it has the advantage of being
>> consistent with the existing gdbarch hook so it may be preferable in
>> this case.

So, I really thought of something like the is_address_tagged at first,
without the _p suffix because I thought this suffix would be present
only in gdbarch hooks, not in the target hooks, but that seems not right,
which is still a bit confusing to me tbh, because I have this in mind:

   /* MTE-specific settings and hooks.  */
   if (tdep->has_mte ())
     {
       /* Register a hook for checking if an address is tagged or not.  */
       set_gdbarch_tagged_address_p (gdbarch, aarch64_linux_tagged_address_p);

which I understand that functions with _p would be the functions to be
set as the hooks for the gdbarch hooks only, using the set_gdbarch_*
"API" functions. That, afaics, doesn't happen for the target hooks, like
is_async_p, which is never explicitly set to any hook using a set_* function
(so I'm also not fond of the _p in the is_async_p case too).
  
Anyways, I'm going for "target_is_address_tagged", which I think is the
clearest option and we all seem to agree on it :) Thanks!


Cheers,
Gustavo

Re: [PATCH v2 1/4] gdb: aarch64: Remove MTE address checking from get_memtag

[Gustavo Romero]

Hi Thiago,

On 3/29/24 9:55 PM, Thiago Jung Bauermann wrote:
> Thiago Jung Bauermann <thiago.bauermann@linaro.org> writes:
> 
>> Gustavo Romero <gustavo.romero@linaro.org> writes:
>>
>>> 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.
>>
>> In cases c) and d), gdbarch_get_memtag gets called even if
>> gdbarch_tagged_address_p returns false, so won't this commit change the
>> behaviour of those functions in that case? I don't know enough about MTE
>> to be sure.
> 
> I just realised that in cases c) and d) an error will be thrown by
> show_addr_not_tagged if gdbarch_tagged_address_p returns false, so
> gdbarch_get_memtag doesn't get called in that case and my concern isn't
> valid. Sorry about that.

Yeah. No worries, I also missed that show_addr_not_tagged throws an error
the first time I read this part, it looked like just printing function :)


> Reviewed-by: Thiago Jung Bauermann <thiago.bauermann@linaro.org>

Thanks for the review.


Cheers,
Gustavo

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

[Gustavo Romero]

On 3/29/24 9:47 PM, Thiago Jung Bauermann wrote:
> Gustavo Romero <gustavo.romero@linaro.org> writes:
> 
>> Move MTE address check out of set_memtag and add this check to the
>> upper layer, before set_memtag is called. This is a preparation for
>> using a target hook instead of a gdbarch hook MTE address checks.
> 
> gdbarch_set_memtags is also called from
> memory_tag_with_logical_tag_command. Shouldn't the same check be added
> there?

In aarch64_linux_set_memtags, the memory check is inside the else {}, which
is only executed when tag_type == memtag_type::allocation, but not in the if {},
which is executed when memtag_type::logical. Because
memory_tag_with_logical_tag_command always calls set_memtags with the argument
for tag_type param == memtag_type::logical there is no need to check the address.

In other words, memory_tag_with_logical_tag_command is about logical tags only,
so it's a local operation that does not touch any memory in the target, it just
changes a local pointer, so it's ok to change the pointer, being it tagged or not,
hence not memory checks are needed.

These comments try to clarify a bit it:

In memory_tag_with_logical_tag_command:

   /* Setting the logical tag is just a local operation that does not touch
      any memory from the target.  Given an input value, we modify the value
      to include the appropriate tag.  <--


In memory_tag_print_tag_command:

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

      It is OK to manipulate the logical tag though.  */ <--


>>
>> Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
>> ---
>>   gdb/aarch64-linux-tdep.c | 4 ----
>>   gdb/printcmd.c           | 6 ++++++
>>   2 files changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/gdb/aarch64-linux-tdep.c b/gdb/aarch64-linux-tdep.c
>> index 50055ac3f48..8e6e63d4dcb 100644
>> --- a/gdb/aarch64-linux-tdep.c
>> +++ b/gdb/aarch64-linux-tdep.c
>> @@ -2525,10 +2525,6 @@ aarch64_linux_set_memtags (struct gdbarch *gdbarch, struct value *address,
>>         /* Remove the top byte.  */
>>         addr = gdbarch_remove_non_address_bits (gdbarch, addr);
>>
>> -      /* Make sure we are dealing with a tagged address to begin with.  */
>> -      if (!aarch64_linux_tagged_address_p (gdbarch, address))
>> -	return false;
>> -
>>         /* With G being the number of tag granules and N the number of tags
>>   	 passed in, we can have the following cases:
>>
>> diff --git a/gdb/printcmd.c b/gdb/printcmd.c
>> index cb0d32aa4bc..ae4d640ccf2 100644
>> --- a/gdb/printcmd.c
>> +++ b/gdb/printcmd.c
>> @@ -3127,6 +3127,12 @@ 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));
>> +  }
> 
> GNU style doesn't use curly braces in if blocks with only one statement.

Fixed in v3.


Cheers,
Gustavo

Re: [PATCH v2 4/4] gdb: Add new remote packet to check if address is tagged

[Gustavo Romero]

Hi Thiago,

Thanks a lot for the nice review. I think it helped a lot the
series organization.

On 3/29/24 8:35 PM, Thiago Jung Bauermann wrote:
> 
> Hello Gustavo,
> 
> I started reviewing the patch series backwards...
> I'll provide comments on this patch first.
> 
> Overall, it looks great. I just have some localised comments in a few
> places below.
> 
> Also, for ease of review I suggest splitting this patch in two:
> 
> - one that introduces the new check_memtag_addr target hook and converts
>    the existing code to use it,
> - and another that adds a check_memtag_addr target hook implementation
>    to the remote target.

Done in v3.

  
> Gustavo Romero <gustavo.romero@linaro.org> writes:
> 
>> This commit adds a new packet qMemTagAddrCheck allowing GDB remote
>> targets to use it to query gdbservers if a given address is tagged.
>>
>> It also adds a new GDB remote feature, 'memory-tagging-check-add+',
>> which must be advertised by the GDB servers to inform GDB they can reply
>> to address checks via the new qMemTagAddrCheck remote packet.
> 
> You will need to document the remote protocol changes in gdb.texinfo, in
> the "Remote Serial Protocol" appendix.

Done in v3.

  
>> Currently, this address check is done via a read query, where the
>> contents of /proc/<PID>/smaps is read and the flags in there are
>> inspected for MTE-related flags that indicate the address is in a tagged
>> memory region.
>>
>> This is not ideal, for example, on QEMU gdbstub and in other cases,
>> like in baremetal debugging, where there is no notion of any OS file
>> like smaps. Hence, qMemTagAddrCheck packet allows check addresses in
>> an OS-agnostic way.
>>
>> For supporting the new packet, a new target hook is introduced,
>> check_memtag_addr, which is used instead of the gdbarch_tagged_address_p
>> gdbarch hook in the upper layers (printcmd.c).
>>
>> The new target hook is then specialized per target, for remote.c,
>> aarch64-linux-nat.c, and corelow.c targets (the current targets that
>> are MTE-aware).
>>
>> The target hook in remote.c uses the qMemTagAddrCheck packet to check
>> an address if the server advertised the 'memory-tagging-check-add+'
>> feature, otherwise it falls back to using the current mechanism, i.e. it
>> reads the /proc/<PID>/smaps contents.
>>
>> In the aarch64-linux-nat.c and corelow.c the target hook uses the
>> gdbarch_tagged_address_p gdbarch hook, so there is no change regarding
>> how an address is checked in these targets. Just the
>> gdbarch_tagged_address_p signature is changed for convenience, since
>> target_check_memtag_addr takes the address to be checked as a CORE_ADDR
>> type.
> 
> I agree that a CORE_ADDR argument is more convenient.
> 
>> @@ -1071,6 +1073,12 @@ aarch64_linux_nat_target::store_memtags (CORE_ADDR address, size_t len,
>>     return false;
>>   }
>>
>> +bool
>> +aarch64_linux_nat_target::check_memtag_addr (CORE_ADDR address)
>> +{
>> +  return gdbarch_tagged_address_p (current_inferior ()->arch (), address);
> 
> I think it's better to pass the gdbarch as an argument to the
> check_memtag_addr hook rather than getting it from current_inferior
> here, even if in practice your patch is equivalent to the existing
> code.
> 
> The reason is that we are trying to move calls to current_* functions
> (which is global state in disguise) up the stack, so that most of GDB
> needs to reference only local state.
> 
> Then if callers have a gdbarch available in their context they can pass
> it to the hook, or else they can use current_inferior ()->arch ().
> 
>> @@ -1410,6 +1412,12 @@ core_target::fetch_memtags (CORE_ADDR address, size_t len,
>>     return false;
>>   }
>>
>> +bool
>> +core_target::check_memtag_addr (CORE_ADDR address)
>> +{
>> +  return gdbarch_tagged_address_p (current_inferior ()->arch (), address);
> 
> Same comment here, of course.
> 
>> diff --git a/gdb/printcmd.c b/gdb/printcmd.c
>> index ae4d640ccf2..c81c75afc5d 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 (target_check_memtag_addr (value_as_address(v_addr)))
> 
> Missing space between "value_as_address" and the opening parens.
> 
> Also, not a problem introduced by you, but I don't understand why the code
> uses the gdbarch from current_inferior if it was passed a gdbarch in the
> arguments.
> 
> So I'd add a separate patch before this one to fix this code to use the
> gdbarch that was passed as a parameter. Then this patch can also pass it
> as a parameter to target_check_memtag_addr as I mentioned in an earlier
> comment.

Done in v3 in commit "gdb: Use passed gdbarch instead of calling current_inferior".


>>   	    {
>>   	      /* Fetch the allocation tag.  */
>>   	      struct value *tag
>> @@ -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_check_memtag_addr (value_as_address(value));
> 
> Here there's no gdbarch available in context, but since there's only one
> caller to this function, it's easy to add the gdbarch parameter to it
> and make the caller pass it down. The caller does get it from
> current_inferior, so perhaps I'm being too pedantic here but IMHO it's
> worth it.
> 
> Also, a space is missing before the opening parens.
> 
>>   }
>>
>>   /* Helper for parsing arguments for print_command_1.  */
>> @@ -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);
> 
> Missing space before the opening parens.
> 
>>     if (tag_type == memtag_type::allocation
>> -      && !gdbarch_tagged_address_p (arch, val))
>> -    show_addr_not_tagged (value_as_address (val));
>> +      && !target_check_memtag_addr(addr))
> 
> Missing space before the opening parens.
> 
>> +    show_addr_not_tagged (addr);
>>
>>     value *tag_value = gdbarch_get_memtag (arch, val, tag_type);
>>     std::string tag = gdbarch_memtag_to_string (arch, tag_value);
>> @@ -3104,8 +3105,9 @@ parse_set_allocation_tag_input (const char *args, struct value **val,
>>
>>     /* If the address is not in a region memory mapped with a memory tagging
>>        flag, it is no use trying to access/manipulate its allocation tag.  */
>> -  if (!gdbarch_tagged_address_p (current_inferior ()->arch (), *val))
>> -    show_addr_not_tagged (value_as_address (*val));
>> +  CORE_ADDR addr = value_as_address (*val);
>> +  if (!target_check_memtag_addr (addr))
>> +    show_addr_not_tagged (addr);
> 
> This is another instance where I'd suggest making the caller pass
> gdbarch as an argument so that it can be used here, since this function
> only has one caller.

OK, done in v3.


> 
>>   }
>>
>>   /* Implement the "memory-tag set-allocation-tag" command.
>> @@ -3129,8 +3131,9 @@ memory_tag_set_allocation_tag_command (const char *args, int from_tty)
>>
>>     /* If the address is not in a region memory mapped with a memory tagging
>>        flag, it is no use trying to manipulate its allocation tag.  */
>> -  if (!gdbarch_tagged_address_p (current_inferior ()->arch (), val)) {
>> -    show_addr_not_tagged (value_as_address(val));
>> +  CORE_ADDR addr = value_as_address (val);
>> +  if (!target_check_memtag_addr (addr)) {
>> +    show_addr_not_tagged (addr);
>>     }
> 
> This is a preexisting issue in the code, but since you're touching it:
> the GNU style is to not use curly braces when there's only one statement
> in the if block.
> 
>> @@ -15532,6 +15547,19 @@ create_store_memtags_request (gdb::char_vector &packet, CORE_ADDR address,
>>     strcpy (packet.data (), request.c_str ());
>>   }
>>
>> +static void
>> +create_check_memtag_addr_request (gdb::char_vector &packet, CORE_ADDR address)
>> +{
>> +  int addr_size = gdbarch_addr_bit (current_inferior ()->arch()) / 8;
>> +
>> +  std::string request = string_printf ("qMemTagAddrCheck:%s", phex_nz (address, addr_size));
>> +
>> +  if (packet.size () < request.length ())
> 
> There's an off-by-one error here: packet is expected to be
> null-terminated, but request.length doesn't count a terminating null
> byte so a "+ 1" needs to be added here.

Good catch, thanks. Fixed in v3.

  
>> +    error (_("Contents too big for packet qMemTagAddrCheck."));
>> +
>> +  strcpy (packet.data (), request.c_str ());
>> +}
>> +
>>   /* Implement the "fetch_memtags" target_ops method.  */
>>
>>   bool
>> @@ -15573,6 +15601,36 @@ remote_target::store_memtags (CORE_ADDR address, size_t len,
>>     return packet_check_result (rs->buf).status () == PACKET_OK;
>>   }
>>
>> +bool
>> +remote_target::check_memtag_addr (CORE_ADDR address)
>> +{
>> +  struct remote_state *rs = get_remote_state ();
>> +
>> +  if (!m_features.remote_memory_tagging_check_addr_p ())
>> +    /* Fallback to reading /proc/<PID>/smaps for checking if an address is
>> +       tagged or not.  */
> 
> Currently this comment is accurate, but if some other architecture adds
> a gdbarch method that doesn't use /proc/<PID>/smaps then it won't be
> anymore.
> 
> I suggest saying something like "Fallback to arch-specific method of
> checking whether an address is tagged".

Done in v3.


>> +    return gdbarch_tagged_address_p (current_inferior ()->arch (), address);
>> +
>> +  create_check_memtag_addr_request (rs->buf, address);
>> +
>> +  putpkt (rs->buf);
>> +  getpkt (&rs->buf);
>> +
>> +  /* Check if reply is OK.  */
>> +  if ((packet_check_result (rs->buf).status () != PACKET_OK) || rs->buf.empty())
> 
> Missing space between "empty" and opening parens.
> 
> But I don't understand why check whether buf is empty. Looking at
> remote_target::getpkt, it doesn't look like buf is ever emptied.

I removed rs->buf.empty() in v3.


>> +    return false;
>> +
>> +  gdb_byte tagged_addr;
>> +  /* Convert only 2 hex digits, i.e. 1 byte in hex format.  */
>> +  hex2bin(rs->buf.data(), &tagged_addr , 1);
> 
> Missing space between "hex2bin", "data" and the opening parens.
> 
>> +  if (tagged_addr)
>> +    /* 01 means address is tagged.  */
>> +    return true;
>> +  else
>> +    /* 00 means address is not tagged.  */
>> +    return false;
> 
> The above can be simplified to "return tagged_addr != 0".

Done in v3.


Cheers,
Gustavo

Re: [PATCH v2 0/4] Add another way to check for MTE-tagged addresses on remote targets

[Gustavo Romero]

On 4/3/24 11:39 AM, Luis Machado wrote:
> On 4/3/24 15:29, Gustavo Romero wrote:
>> Hi Luis,
>>
>> On 4/3/24 8:51 AM, Luis Machado wrote:
>>> Hi,
>>>
>>> On 3/28/24 22:48, Gustavo Romero wrote:
>>>> This series introduces a new method to check for MTE-tagged addresses on
>>>> remote targets.
>>>>
>>>> A new remote packet, qMemTagAddrCheck, is introduced, along with a new
>>>> remote feature associated with it, 'memory-tagging-check-add+'. Only
>>>> when 'memory-tagging-check-add+' feature is advertised GDB will use the
>>>> new packet to query if an address is tagged.
>>>>
>>>> This new mechanism allows for checking MTE addresses in an OS-agnostic
>>>> way, which is necessary when debugging targets that do not support
>>>> '/proc/<PID>/smaps', as the current method of reading the smaps contents
>>>> fails in such cases.
>>>>
>>>> Since v1:
>>>>    - Fixed build error "no match for ‘operator!=’ (operand types are ‘packet_result’ and ‘packet_status’)"
>>>>      reported by Linaro CI bot, caused by a last-minute rebase;
>>>>    - Added instructions on how to test the series on a remote target using
>>>>      QEMU gdbstub (-g option) -- see below.
>>>>    ----
>>>>
>>>> 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)
>>>>
>>>>
>>>> And, finally, testing it 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)
>>>
>>> Out of curiosity, I see you exercised native, core and QEMU-based remote debugging. Did you give the gdbserver-based remote debugging a try?
>>>
>>> I think that is an important check given a gdb + gdbserver debugging session will also use the remote target, but will instead rely on accessing the remote smaps file.
>>
>> Nope. I'll give it a try before sending v3 today.
> 
> Awesome. Thanks.

The fallback mechanism works when tested against the gdbserver.

I'll paste the results into the cover letter for v3. That's a nice test, thanks!


Cheers,
Gustavo

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

[Thiago Jung Bauermann]

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

> On 3/29/24 9:47 PM, Thiago Jung Bauermann wrote:
>> Gustavo Romero <gustavo.romero@linaro.org> writes:
>>
>>> Move MTE address check out of set_memtag and add this check to the
>>> upper layer, before set_memtag is called. This is a preparation for
>>> using a target hook instead of a gdbarch hook MTE address checks.
>> gdbarch_set_memtags is also called from
>> memory_tag_with_logical_tag_command. Shouldn't the same check be added
>> there?
>
> In aarch64_linux_set_memtags, the memory check is inside the else {}, which
> is only executed when tag_type == memtag_type::allocation, but not in the if {},
> which is executed when memtag_type::logical. Because
> memory_tag_with_logical_tag_command always calls set_memtags with the argument
> for tag_type param == memtag_type::logical there is no need to check the address.
>
> In other words, memory_tag_with_logical_tag_command is about logical tags only,
> so it's a local operation that does not touch any memory in the target, it just
> changes a local pointer, so it's ok to change the pointer, being it tagged or not,
> hence not memory checks are needed.
>
> These comments try to clarify a bit it:
>
> In memory_tag_with_logical_tag_command:
>
>   /* Setting the logical tag is just a local operation that does not touch
>      any memory from the target.  Given an input value, we modify the value
>      to include the appropriate tag.  <--
>
>
> In memory_tag_print_tag_command:
>
>   /* 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.
>
>      It is OK to manipulate the logical tag though.  */ <--

Thanks for the explanation, makes sense!

--
Thiago