Re: [PATCH,v5][AArch64] MTE corefile support

[Alan Hayward via Gdb-patches <gdb-patches@sourceware.org>]

On 24 Jun 2021, at 15:37, Luis Machado <luis.machado@linaro.org<mailto:luis.machado@linaro.org>> wrote:

On 6/24/21 11:00 AM, Alan Hayward wrote:
On 1 Jun 2021, at 18:45, Luis Machado <luis.machado@linaro.org<mailto:luis.machado@linaro.org>> wrote:

Updates on v5:

- Fixed format warning output.

Updates on v4:

- Calculate sizes based on individual struct field sizes.

Updates on v3:

- Addressed review comments.
- New files gdbsupport/memtag.h, gdb/memtag.h and gdb/memtag.c.
- Updated code documentation.
- Removed code duplication.

Updates on v2:

- Reworked core_target::fetch_memtags to handle cases where address + len runs
 over the NT_MEMTAG note.
- Turned a few magic numbers into constants. There is an unfortunate duplication
 of a constant (NT_MEMTAG_HEADER_SIZE). It is a generic constant, but there is
 no file generic enough that gets included by both corelow and linux-tdep.
- More sanity checks to make sure the note format is correct.
- Documented aarch64_linux_decode_memtag_note a little more.

---

Teach GDB how to dump memory tags when using the gcore command and how
to read them back from a core file generated via gcore or the kernel.

Each tagged memory range (listed in /proc/<pid>/smaps) gets dumped to its
own NT_MEMTAG note. A section named ".memtag" is created for each of those
when reading the core file back.

Dumping memory tags
-

When using the gcore command to dump a core file, GDB will go through the maps
in /proc/<pid>/smaps looking for tagged ranges. Each of those entries gets
passed to an arch-specific gdbarch hook that generates a vector of blobs of
memory tag data that are blindly put into a NT_MEMTAG note.

The vector is used because we may have, in the future,  multiple tag types for
a particular memory range.

Each of the NT_MEMTAG notes have a generic header and a arch-specific header,
like so:

struct tag_dump_header
{
 uint16_t format; // Only NT_MEMTAG_TYPE_AARCH_MTE at present
 uint64_t start_vma;
 uint64_t end_vma;
};

struct tag_dump_mte
{
 uint16_t granule_byte_size;
 uint16_t tag_bit_size;
 uint16_t __unused;
};

The only bits meant to be generic are the tag_dump_format, start_vma and
end_vma fields.

The format-specific data is supposed to be opaque and only useful for the
arch-specific code.

We can extend the format in the future to make room for other memory tag
layouts.

Reading memory tags
-

When reading a core file that contains NT_MEMTAG entries, GDB will use
a different approach to check for tagged memory range. Rather than looking
at /proc/<pid>/smaps, it will now look for ".memtag" sections with the right
memory range.

When reading tags, GDB will now use the core target's implementation of
fetch_memtags (store_memtags doesn't exist for core targets). Then the data
is fed into an arch-specific hook that will decode the memory tag format and
return a vector of tags.

I've added a test to exercise writing and reading of memory tags in core
files.

gdb/ChangeLog:

YYYY-MM-DD  Luis Machado  <luis.machado@linaro.org<mailto:luis.machado@linaro.org>>

* Makefile.in (COMMON_SFILES): Add memtag.c.
* NEWS: Mention core file support for memory tagging.
* aarch64-linux-tdep.c: Include elf/common.h.
Include gdbsupport/memtag.h.
(MAX_TAGS_TO_TRANSFER): New constant.
(aarch64_linux_create_memtag_notes_from_range): New function.
(aarch64_linux_decode_memtag_note): Likewise.
(aarch64_linux_init_abi): Register new core file hooks.
(NT_MEMTAG_TOTAL_HEADER_SIZE): New constant.
* arch/aarch64-mte-linux.h (tag_dump_mte): New struct.
(AARCH64_MTE_TAG_BIT_SIZE): New constant.
* corelow.c: Include gdbsupport/memtag.h and memtag.h.
(core_target) <supports_memory_tagging, fetch_memtags>: New
method overrides.
* gdbarch.c: Regenerate.
* gdbarch.h: Likewise.
* gdbarch.sh (create_memtag_notes_from_range): New hook.
(decode_memtag_note): Likewise.
* linux-tdep.c: Include gdbsupport/memtag.h and memtag.h.
(linux_address_in_memtag_page): Renamed to...
(linux_process_address_in_memtag_page): ... this.
(linux_core_file_address_in_memtag_page): New function.
(linux_address_in_memtag_page): Likewise.
(linux_make_memtag_corefile_notes): Likewise.
(linux_make_corefile_notes): Handle memory tag notes.
* memtag.c: New file.
* memtag.h: New file.

gdb/doc/ChangeLog:

YYYY-MM-DD  Luis Machado  <luis.machado@linaro.org<mailto:luis.machado@linaro.org>>

* gdb.texinfo (AArch64 Memory Tagging Extension): Mention support
for memory tagging in core files.

gdb/testsuite/ChangeLog:

YYYY-MM-DD  Luis Machado  <luis.machado@linaro.org<mailto:luis.machado@linaro.org>>

* gdb.arch/aarch64-mte-gcore.c: New file.
* gdb.arch/aarch64-mte-gcore.exp: New file.

gdbsupport/ChangeLog:

YYYY-MM-DD  Luis Machado  <luis.machado@linaro.org<mailto:luis.machado@linaro.org>>

* memtag.h: New file.
---
gdb/Makefile.in                              |   1 +
gdb/NEWS                                     |   4 +
gdb/aarch64-linux-tdep.c                     | 221 +++++++++++++++++++
gdb/arch/aarch64-mte-linux.h                 |  17 ++
gdb/corelow.c                                |  63 ++++++
gdb/doc/gdb.texinfo                          |   4 +
gdb/gdbarch.c                                |  64 ++++++
gdb/gdbarch.h                                |  16 ++
gdb/gdbarch.sh                               |   6 +
gdb/linux-tdep.c                             |  97 +++++++-
gdb/memtag.c                                 |  88 ++++++++
gdb/memtag.h                                 |  46 ++++
gdb/testsuite/gdb.arch/aarch64-mte-gcore.c   |  93 ++++++++
gdb/testsuite/gdb.arch/aarch64-mte-gcore.exp | 111 ++++++++++
gdbsupport/memtag.h                          |  39 ++++
15 files changed, 867 insertions(+), 3 deletions(-)
create mode 100644 gdb/memtag.c
create mode 100644 gdb/memtag.h
create mode 100644 gdb/testsuite/gdb.arch/aarch64-mte-gcore.c
create mode 100644 gdb/testsuite/gdb.arch/aarch64-mte-gcore.exp
create mode 100644 gdbsupport/memtag.h

Note there were a few minor merge conflicts, nothing to worry about though.

I'll check again and will refresh the patch.

diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index f664d964536..12fb3b390b1 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -1100,6 +1100,7 @@ COMMON_SFILES = \
memattr.c \
memory-map.c \
memrange.c \
+ memtag.c \
minidebug.c \
minsyms.c \
mipsread.c \
diff --git a/gdb/NEWS b/gdb/NEWS
index ab678acec8b..58b9f739d4f 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -3,6 +3,10 @@

*** Changes since GDB 10

+* GDB now supports dumping memory tag data for AArch64 MTE.  It also supports
+  reading memory tag data for AArch64 MTE from core files generated by
+  the gcore command or the Linux kernel.
+
* GDB now supports general memory tagging functionality if the underlying
  architecture supports the proper primitives and hooks.  Currently this is
  enabled only for AArch64 MTE.
diff --git a/gdb/aarch64-linux-tdep.c b/gdb/aarch64-linux-tdep.c
index e9761ed2189..04498f3b6c0 100644
--- a/gdb/aarch64-linux-tdep.c
+++ b/gdb/aarch64-linux-tdep.c
@@ -52,6 +52,9 @@
#include "value.h"

#include "gdbsupport/selftest.h"
+#include "gdbsupport/memtag.h"
+
+#include "elf/common.h"

/* Signal frame handling.

@@ -1779,6 +1782,213 @@ aarch64_linux_report_signal_info (struct gdbarch *gdbarch,
    }
}

+/* Memory tag note header size.  Includes both the generic and the
+   arch-specific parts.  */
+#define NT_MEMTAG_TOTAL_HEADER_SIZE (NT_MEMTAG_GENERIC_HEADER_SIZE \
+      + NT_MEMTAG_MTE_HEADER_SIZE)
+
+/* Maximum number of tags to request.  */
+#define MAX_TAGS_TO_TRANSFER 1024
+
+/* AArch64 Linux implementation of the aarch64_create_memtag_notes_from_range
+   gdbarch hook.  Create core file notes for memory tags.  */
+
+static std::vector<gdb::byte_vector>
+aarch64_linux_create_memtag_notes_from_range (struct gdbarch *gdbarch,
+       CORE_ADDR start_address,
+       CORE_ADDR end_address)
+{
+  /* We only handle MTE tags for now.  */
+
+  /* Figure out how many tags we need to store in this memory range.  */
+  size_t granules = aarch64_mte_get_tag_granules (start_address,
+   end_address - start_address,
+   AARCH64_MTE_GRANULE_SIZE);
+
+  /* Vector of memory tag notes. Add the MTE note (we only have MTE tags
+     at the moment).  */
+  std::vector<gdb::byte_vector> notes (1);
+
+  /* If there are no tag granules to fetch, just return.  */
+  if (granules == 0)
+    return notes;
+
+  /* Adjust the MTE note size to hold the header + tags.  */
+  notes[0].resize (NT_MEMTAG_TOTAL_HEADER_SIZE + granules);
Should this be NT_MEMTAG_TOTAL_HEADER_SIZE + (granules * AARCH64_MTE_GRANULE_SIZE)
Get_granules has already divided by the granule size.

We are storing 1 tag per byte. So the number of granules is the number of tags. If we multiply by AARCH64_MTE_GRANULE_SIZE, we will have 16 times more tags.

Ahh, right.
(Of course - this sort of buffer overrun is exactly what mte prevents :) )


+
+  CORE_ADDR address = start_address;
+  /* Vector of tags.  */
+  gdb::byte_vector tags;
+
+  while (granules > 0)
+    {
+      /* Transfer tags in chunks.  */
+      gdb::byte_vector tags_read;
+      size_t xfer_len
+ = (granules >= MAX_TAGS_TO_TRANSFER)?
+   MAX_TAGS_TO_TRANSFER * AARCH64_MTE_GRANULE_SIZE :
+   granules * AARCH64_MTE_GRANULE_SIZE;
+
+      if (!target_fetch_memtags (address, xfer_len, tags_read,
+  static_cast<int> (memtag_type::allocation)))
+ {
+   warning (_("Failed to read MTE tags from memory range [%s,%s]."),
+      phex_nz (start_address, sizeof (start_address)),
+      phex_nz (end_address, sizeof (end_address)));
+   notes.resize (0);
If you do the original resize after the while loop, then there would be no need to resize here.

I'm not sure I understand. Where do you suggest the resizing to be placed?

So something like:

std::vector<gdb::byte_vector> notes (1);

while (granules > 0)
{
...
  If (!fetch())
   return notes
...
}

notes[0].resize (NT_MEMTAG_TOTAL_HEADER_SIZE + all_granules);

Now I write it out, you’ll need to keep a copy of the original number of granules.
Given this is only going to happen on a rare failure, then maybe its not worth the change.


Ok, happy with all those then.
Rest of the patch looks good to me too.


Alan.