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

[Luis Machado <luis.machado@linaro.org>]

On 6/24/21 11:00 AM, Alan Hayward wrote:
> 
> 
>> On 1 Jun 2021, at 18:45, Luis Machado <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>
>>
>> 	* 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>
>>
>> 	* 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>
>>
>> 	* 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>
>>
>> 	* 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.

>> +
>> +  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?