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

[Simon Marchi <simon.marchi@polymtl.ca>]

On 2021-05-26 10:08 a.m., Luis Machado via Gdb-patches wrote:
> +/* AArch64 Linux implementation of the decode_memtag_note gdbarch
> +   hook.  Decode a memory tag note and return the requested tags.
> +
> +   The note is guaranteed to cover the [ADDRESS, ADDRESS + length)
> +   range.  */
> +
> +static gdb::byte_vector
> +aarch64_linux_decode_memtag_note (struct gdbarch *gdbarch,
> +				  gdb::array_view <const gdb_byte> note,
> +				  CORE_ADDR address, size_t length)
> +{
> +  gdb::byte_vector tags;
> +
> +  /* Sanity check.  */
> +  if (note.size () < MEMTAG_NOTE_HEADER_SIZE)
> +    {
> +      warning (_("malformed core note - too short for MTE header"));

I would suggest indicating the expected minimum size and the actualy
size.  If that ever fails, this bit of information could help the user.

> +      return tags;
> +    }
> +
> +  /* The amount of memory tag granules we need to fetch.  */
> +  int granules
> +    = aarch64_mte_get_tag_granules (address, length, AARCH64_MTE_GRANULE_SIZE);
> +
> +  /* If there are no tag granules to decode, just return.  */
> +  if (granules == 0)
> +    return tags;
> +
> +  /* Read the generic header.  */
> +  enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
> +  struct tag_dump_header header;
> +  const gdb_byte *buf = note.data ();
> +
> +  header.format
> +    = extract_unsigned_integer (buf, sizeof (uint16_t), byte_order);
> +  buf += sizeof (uint16_t);
> +
> +  header.start_vma
> +    = extract_unsigned_integer (buf, sizeof (uint64_t), byte_order);
> +  buf += sizeof (uint64_t);
> +
> +  header.end_vma
> +    = extract_unsigned_integer (buf, sizeof (uint64_t), byte_order);
> +  buf += sizeof (uint64_t);
> +
> +  /* Sanity check.  */
> +  gdb_assert (address + length < header.end_vma);
> +
> +  /* Sanity check  */
> +  if (header.format != NT_MEMTAG_TYPE_AARCH_MTE)
> +    {
> +      warning (_("Unexpected memory tag note format (%x).\n"), header.format);

Here, it's ambiguous (from the point of view of the user) whether the
value you print is the expected value or actual value.  For clarity,
might was well print both.

> +      return tags;
> +    }
> +
> +  /* Calculate how many granules we need to skip to get to the granule of
> +     ADDRESS.  Align both the start address and the requested address
> +     so it is easier to get the number of granules to skip.  This way we
> +     don't need to consider cases where ADDRESS falls in the middle of a
> +     tag granule range.  */
> +  CORE_ADDR aligned_start_address
> +    = align_down (header.start_vma, AARCH64_MTE_GRANULE_SIZE);
> +  CORE_ADDR aligned_address = align_down (address, AARCH64_MTE_GRANULE_SIZE);
> +
> +  int skipped_granules
> +    = aarch64_mte_get_tag_granules (aligned_start_address,
> +				    aligned_address - aligned_start_address,
> +				    AARCH64_MTE_GRANULE_SIZE);

aarch64_mte_get_tag_granules returns size_t, so we might as well make
the local variable size_t...

> +
> +  /* Point to the block of data that contains the first granule we are
> +     interested in.  */
> +  const gdb_byte *tags_data
> +    = note.data () + MEMTAG_NOTE_HEADER_SIZE + skipped_granules;

To benefit from (potential, I've been wanting to add that for a while)
array_view bound_checking, I suggest making an array_view slice instead
of falling back on a plain pointer.  You should be able to call

  note.slice (MEMTAG_NOTE_HEADER_SIZE + skipped_granules)

which will get view another array view that starts where you need.

> +
> +  /* Read the tag granules.  */
> +  for (unsigned int i = 0; i < granules; i++)

... and the iteration variable size_t too.  This might apply to other
calls to aarch64_mte_get_tag_granules.

> +      tags.push_back (tags_data[i]);

I feel like we are missing some bound checking here.  Couldn't malformed
data send us past the array_view's window?

> +
> +  return tags;
> +}

In the computations above, the code seems to assume that each granule's
tag takes up one byte.  That is a safe assumption?

> @@ -1862,6 +2049,17 @@ aarch64_linux_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
>  
>        set_gdbarch_report_signal_info (gdbarch,
>  				      aarch64_linux_report_signal_info);
> +
> +      /* Core file helpers.  */
> +
> +      /* Core file helper to create memory tag notes for a particular range of
> +	 addresses.  */
> +      set_gdbarch_create_memtag_notes_from_range (gdbarch,
> +				  aarch64_linux_create_memtag_notes_from_range);

Indent.

> @@ -71,4 +72,18 @@ extern CORE_ADDR aarch64_mte_set_ltag (CORE_ADDR address, CORE_ADDR tag);
>     It is always possible to get the logical tag.  */
>  extern CORE_ADDR aarch64_mte_get_ltag (CORE_ADDR address);
>  
> +/* MTE-specific NT_MEMTAG header.  */
> +struct tag_dump_mte
> +{
> +  /* Size of the tag granule in bytes.  */
> +  uint16_t granule_byte_size;
> +  /* Size of the tag in bits.  */
> +  uint16_t tag_bit_size;
> +  /* Reserved field for the future.  */
> +  uint16_t __unused;
> +};

This struct is unused.

> +
> +/* Size of the MTE header for a NT_MEMTAG note.  */
> +#define MEMTAG_MTE_HEADER_SIZE (3 * sizeof (uint16_t))

NT_MEMTAG_MTE_HEADER_SIZE?  Otherwise there's nothing in the name that
refers to the fact that it's the header of the note.

> @@ -1115,6 +1122,107 @@ core_target::info_proc (const char *args, enum info_proc_what request)
>    return true;
>  }
>  
> +/* Implementation of the "supports_memory_tagging" target_ops method.  */
> +
> +bool
> +core_target::supports_memory_tagging ()
> +{
> +  /* Look for memory tag notes.  If they exist, that means this core file
> +     supports memory tagging.  */
> +  if (bfd_get_section_by_name (core_bfd, ".memtag") == nullptr)
> +    return false;
> +
> +  return true;

Use:

  bfd_get_section_by_name (...) != nullptr;

> +}
> +
> +/* Size of the generic header for the NT_MEMTAG note.  This is OS-independent
> +   and should be shared with OS' and arch-specific code.  */
> +#define NT_MEMTAG_HEADER_SIZE (sizeof (uint16_t) + 2 * sizeof (uint64_t))

It's a little strange, NT_MEMTAG_HEADER_SIZE is defined at two places,
linux-tdep.h and here.  If this is really OS-independent, there should
probably be just one definition, in an OS-independent header file.  Not
sure where though...

> +
> +/* Implementation of the "fetch_memtags" target_ops method.  */
> +
> +bool
> +core_target::fetch_memtags (CORE_ADDR address, size_t len,
> +			    gdb::byte_vector &tags, int type)
> +{
> +  struct gdbarch *gdbarch = target_gdbarch ();
> +
> +  /* Make sure we have a way to decode the memory tag notes.  */
> +  if (!gdbarch_decode_memtag_note_p (gdbarch))
> +    warning (_("gdbarch_decode_memtag_note not implemented for this "
> +	       "architecture."));

Just a warning?  You want to continue this function even if this
happens?

> +
> +  asection *section
> +    = bfd_get_section_by_name (core_bfd, ".memtag");

Fits on one line.

> @@ -1473,6 +1474,137 @@ linux_address_in_memtag_page (CORE_ADDR address)
>    return false;
>  }
>  
> +/* Helper that checks if an address is in a memory tag page for a core file
> +   process.  */
> +
> +static bool
> +linux_core_file_address_in_memtag_page (CORE_ADDR address)
> +{
> +  if (core_bfd == nullptr)
> +    return false;
> +
> +  asection *section
> +    = bfd_get_section_by_name (core_bfd, ".memtag");
> +
> +  /* Go through all the memtag sections and figure out if ADDRESS
> +     falls within one of the memory ranges that contain tags.  */
> +  while (section != nullptr)
> +    {
> +      size_t note_size = bfd_section_size (section);
> +
> +      /* If the note is smaller than the size of the header, this core note
> +	 is malformed.  */
> +      if (note_size < NT_MEMTAG_HEADER_SIZE)
> +	{
> +	  warning (_("malformed core note - too short for header"));
> +	  return false;
> +	}
> +
> +      gdb::byte_vector note (note_size);
> +
> +      /* Fetch the contents of this particular memtag note.  */
> +      if (!bfd_get_section_contents (core_bfd, section,
> +				     note.data (), 0, note_size))
> +	{
> +	  warning (_("could not get core note contents."));
> +	  return false;
> +	}
> +
> +      /* Read the generic header of the note.  Those contain the format,
> +	 start address and end address.  */
> +      uint64_t start_address
> +	= bfd_get_64 (core_bfd, note.data () + sizeof (uint16_t));
> +      uint64_t end_address
> +	= bfd_get_64 (core_bfd, note.data () + sizeof (uint16_t)
> +				+ sizeof (uint64_t));

The code of this function approximately up to here is really similar to
the code in corelow.c.  Is there a change there could be a single
implementation of it, that takes a callback?  Like

  for_each_matching_memtag_note_section_matching_address
    (CORE_ADDR address, gdb::function_view<...> callback)

except with a better name.

> +/* For each memory map entry that has memory tagging enabled, create a new
> +   core file note that contains all of its memory tags.  Save the data to
> +   NOTE_DATA and update NOTE_SIZE accordingly.  */
> +
> +static void
> +linux_make_memtag_corefile_notes (struct gdbarch *gdbarch, bfd *obfd,
> +				  gdb::unique_xmalloc_ptr<char> &note_data,
> +				  int *note_size)
> +{
> +  if (current_inferior ()->fake_pid_p)
> +    return;
> +
> +  /* If the architecture doesn't have a hook to return memory tag notes,
> +     there is nothing left to do.  */
> +  if (!gdbarch_create_memtag_notes_from_range_p (gdbarch))
> +    return;
> +
> +  pid_t pid = current_inferior ()->pid;
> +
> +  std::string smaps_file = string_printf ("/proc/%d/smaps", pid);
> +
> +  gdb::unique_xmalloc_ptr<char> data
> +    = target_fileio_read_stralloc (NULL, smaps_file.c_str ());
> +
> +  if (data == nullptr)
> +    return;
> +
> +  std::vector<struct smaps_data> smaps;
> +
> +  /* Parse the contents of smaps into a vector.  */
> +  smaps = parse_smaps_data (data.get (), smaps_file);

Declare when assigning:

  std::vector<smaps_data> smaps = parse_smaps_data (...);

> +
> +  for (const smaps_data &map : smaps)
> +    {
> +      /* Does this mapping have memory tagging enabled? If so, save the
> +	 memory tags to the core file note.  */
> +      if (map.vmflags.memory_tagging == 0)
> +	continue;
> +
> +      /* Ask the architecture to create (one or more) NT_MEMTAG notes for
> +	 this particular memory range, including the header.
> +
> +	 If the notes are too big, we may need to break up the transfer
> +	 into smaller chunks.
> +
> +	 If the architecture returns an empty vector, that means there are
> +	 no memory tag notes to write.  */
> +      std::vector<gdb::byte_vector> memory_tag_notes;
> +      memory_tag_notes
> +	= gdbarch_create_memtag_notes_from_range (gdbarch,
> +						  map.start_address,
> +						  map.end_address);

Here too.

> +      /* Write notes to the core file.  */
> +      for (gdb::byte_vector note : memory_tag_notes)

This copies each byte_vector in the iteration variable - iterate with
const reference:

  for (const gdb::byte_vector &note : memory_tag_notes)

> diff --git a/gdb/linux-tdep.h b/gdb/linux-tdep.h
> index 28b60e46579..1ff4d008df4 100644
> --- a/gdb/linux-tdep.h
> +++ b/gdb/linux-tdep.h
> @@ -26,6 +26,19 @@
>  struct inferior;
>  struct regcache;
>  
> +#define NT_MEMTAG_HEADER_SIZE (sizeof (uint16_t) + 2 * sizeof (uint64_t))
> +
> +/* Generic NT_MEMTAG header.  */
> +struct tag_dump_header
> +{
> +  /* Tag format.  */
> +  uint16_t format;
> +  /* Start address of the tagged range.  */
> +  uint64_t start_vma;
> +  /* End address of the tagged range.  */
> +  uint64_t end_vma;
> +};

This struct is used once, in aarch64_linux_decode_memtag_note, but where
it's used it's not very useful, it could very well be replaced with 3
local variables, and then it could be removed.  I'm guessing that an
earlier version of this patch used this struct to read the data contents
directly from the buffer, but that didn't work with different byte
orders?

> diff --git a/gdb/testsuite/gdb.arch/aarch64-mte-gcore.exp b/gdb/testsuite/gdb.arch/aarch64-mte-gcore.exp
> new file mode 100644
> index 00000000000..bb529a8b369
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/aarch64-mte-gcore.exp
> @@ -0,0 +1,115 @@
> +# Copyright (C) 2018-2021 Free Software Foundation, Inc.
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +# This file is part of the gdb testsuite.
> +
> +# Test generating and reading a core file with MTE memory tags.
> +
> +if {![is_aarch64_target]} {
> +    verbose "Skipping ${gdb_test_file_name}."
> +    return
> +}
> +
> +standard_testfile
> +if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile}] } {
> +    return -1
> +}
> +
> +if ![runto_main] {
> +    untested "could not run to main"
> +    return -1
> +}
> +
> +# Targets that don't support memory tagging should not execute the
> +# runtime memory tagging tests.
> +if {![supports_memtag]} {
> +    unsupported "memory tagging unsupported"
> +    return -1
> +}
> +
> +gdb_breakpoint "access_memory"
> +
> +if [gdb_continue "access_memory"] {
> +    return -1
> +}
> +
> +# Set each tag granule to a different tag value, from 0x0 to 0xf.
> +set atag_msg "Allocation tag\\(s\\) updated successfully\."
> +for {set i 15} {$i >= 0} {incr i -1} {
> +    set index [expr [expr 15 - $i] * 16]
> +    set tag [format "%02x" $i]
> +    gdb_test "memory-tag set-allocation-tag &tagged_ptr\[$index\] 1 $tag" \
> +	     $atag_msg \
> +	     "set memory tag of &tagged_ptr\[$index\] to $tag"
> +}
> +
> +# Run until a crash and confirm GDB displays memory tag violation
> +# information.
> +gdb_test "continue" \
> +    [multi_line \
> +	"Program received signal SIGSEGV, Segmentation fault" \
> +	"Memory tag violation while accessing address $hex" \
> +	"Allocation tag $hex" \
> +	"Logical tag $hex\." \
> +	"$hex in access_memory \\(.*\\) at .*" \
> +	".*tagged_ptr\\\[0\\\] = 'a';"] \
> +	 "display tag violation information for live process"
> +
> +# Generate the core file.
> +set core_filename [standard_output_file "$testfile.core"]
> +set core_generated [gdb_gcore_cmd "$core_filename" "generate core file"]
> +
> +if { !$core_generated } {
> +    return -1
> +}
> +
> +clean_restart
> +
> +# Load the program file.
> +set program_filename [standard_output_file $testfile]
> +set program_loaded [gdb_file_cmd $program_filename]

Can the two lines above just be replaced with passing $binfile to
clean_restart?

Simon