Re: [PATCH v3 14/24] AArch64: Implement the memory tagging gdbarch hooks

Luis Machado via Gdb-patches
2020-12-29T12:16:40Z

On 12/26/20 3:52 AM, Simon Marchi wrote:
>
>
> On 2020-11-09 12:04 p.m., Luis Machado via Gdb-patches wrote:
>> Updates on v2:
>>
>> - Update target methods to contain a tag type parameter.
>>
>> --
>>
>> This patch implements the memory tagging gdbarch hooks for AArch64, for
>> the MTE feature.
>>
>> gdb/ChangeLog:
>>
>> YYYY-MM-DD Luis Machado <luis.machado@linaro.org>
>>
>> * aarch64-linux-tdep.c: Include target.h, arch-utils.h, value.h.
>> (aarch64_linux_get_atag, aarch64_linux_tagged_address_p)
>> (aarch64_linux_memtag_mismatch_p, aarch64_linux_set_memtags)
>> (aarch64_linux_get_memtag, aarch64_linux_memtag_to_string): New
>> functions.
>> (aarch64_linux_init_abi): Initialize MTE-related gdbarch hooks.
>> * arch/aarch64-mte-linux.c (make_ltag_bits, make_ltag)
>> (aarch64_linux_set_ltag, aarch64_linux_get_ltag): New functions.
>> * arch/aarch64-mte-linux.h (MTE_LOGICAL_TAG_START_BIT)
>> (MTE_LOGICAL_MAX_VALUE): Define.
>> (make_ltag_bits, make_ltag, aarch64_linux_set_ltag)
>> (aarch64_linux_get_ltag): New prototype.
>> ---
>> gdb/aarch64-linux-tdep.c | 212 +++++++++++++++++++++++++++++++++++
>> gdb/arch/aarch64-mte-linux.c | 36 ++++++
>> gdb/arch/aarch64-mte-linux.h | 19 ++++
>> 3 files changed, 267 insertions(+)
>>
>> diff --git a/gdb/aarch64-linux-tdep.c b/gdb/aarch64-linux-tdep.c
>> index 5f6fb42792..00c4f45035 100644
>> --- a/gdb/aarch64-linux-tdep.c
>> +++ b/gdb/aarch64-linux-tdep.c
>> @@ -30,6 +30,7 @@
>> #include "symtab.h"
>> #include "tramp-frame.h"
>> #include "trad-frame.h"
>> +#include "target.h"
>> #include "target/target.h"
>>
>> #include "regcache.h"
>> @@ -46,6 +47,9 @@
>>
>> #include "arch/aarch64-mte-linux.h"
>>
>> +#include "arch-utils.h"
>> +#include "value.h"
>> +
>> /* Signal frame handling.
>>
>> +------------+ ^
>> @@ -1437,6 +1441,189 @@ aarch64_linux_gcc_target_options (struct gdbarch *gdbarch)
>> return {};
>> }
>>
>> +/* Helper to get the allocation tag from a 64-bit ADDRESS.
>> +
>> + Return 0 for success and non-zero otherwise. */
>> +
>> +static int
>> +aarch64_linux_get_atag (CORE_ADDR address, CORE_ADDR *tag)
>> +{
>
> I'd suggest making the return value a bool. Or, even better IMO
> would be to make the return value gdb::optional<CORE_ADDR>, an
> empty optional meaning failure.
>

Done now.

>> + gdb::byte_vector tags;
>> +
>> + /* Attempt to fetch the allocation tag. */
>> + if (target_fetch_memtags (address, 0, tags, tag_allocation) != 0)
>> + return 1;
>> +
>> + /* Only one tag should've been returned. Make sure we got exactly that. */
>> + gdb_assert (tags.size () == 1);
>> +
>> + /* Although our tags are 4 bits in size, they are stored in a
>> + byte. */
>> + *tag = tags[0];
>> +
>> + return 0;
>> +}
>> +
>> +/* Implement the tagged_address_p gdbarch method. */
>> +
>> +static bool
>> +aarch64_linux_tagged_address_p (struct gdbarch *gdbarch, struct value *address)
>> +{
>> + gdb_assert (address != nullptr);
>> +
>> + CORE_ADDR addr = value_as_address (address);
>> +
>> + /* Remove the top byte for the memory range check. */
>> + addr = address_significant (gdbarch, addr);
>> +
>> + /* Check if the page that contains ADDRESS is mapped with PROT_MTE. */
>> + if (!linux_address_in_memtag_page (addr))
>> + return false;
>> +
>> + /* We have a valid tag in the top byte of the 64-bit address. */
>> + return true;
>> +}
>> +
>> +/* Implement the memtag_mismatch_p gdbarch method. */
>> +
>> +static bool
>> +aarch64_linux_memtag_mismatch_p (struct gdbarch *gdbarch,
>> + struct value *address)
>> +{
>> + 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 false;
>> +
>> + CORE_ADDR addr = value_as_address (address);
>> +
>> + /* Fetch the allocation tag for ADDRESS. */
>> + CORE_ADDR atag = 0;
>> +
>> + if (aarch64_linux_get_atag (addr, &atag) != 0)
>> + return false;
>> +
>> + /* Fetch the logical tag for ADDRESS. */
>> + gdb_byte ltag = aarch64_linux_get_ltag (addr);
>> +
>> + /* Are the tags the same? */
>> + if (ltag == atag)
>> + return false;
>> +
>> + return true;
>
> That could very well be
>
> return ltag != atag;
>
> I'm wondering about the potential performance impacts of all this. When
> memory tagging is in use, how often are the memtag functions expected to be
> called? By the looks of it, it seems like checking for a mismatch does
> 2 open and parse of /proc/pid/smaps. Is that right, and is this function
> something that is going to be called often, say, when printing values?
>

If MTE is enabled, the checks will happen every time one prints a
pointer or dumps some tagged memory region with the "x" command.

I considered and discussed the impact of this mechanism during the
implementation of MTE support in GDB. Unfortunately the interface the
kernel uses to communicate the required data to GDB isn't great.

The maps file is small and quick to parse, but the smaps file is not
presented in a way that makes it easy for consumers to parse the data.
Even worse, we can't cache the data, as threads may potentially
map/unmap PROT_MTE pages during execution. It needs to be a read freshly.

To mitigate this, we could default to not check tags when printing,
while still handling tag faults.

>> +}
>> +
>> +/* Implement the set_memtags gdbarch method. */
>> +
>> +static int
>> +aarch64_linux_set_memtags (struct gdbarch *gdbarch, struct value *address,
>> + size_t length, const gdb::byte_vector &tags,
>> + enum memtag_type tag_type)
>> +{
>> + if (tags.empty ())
>> + return 0;
>> +
>> + gdb_assert (address != nullptr);
>> +
>> + CORE_ADDR addr = value_as_address (address);
>> +
>> + /* Set the logical tag or the allocation tag. */
>> + if (tag_type == tag_logical)
>> + {
>> + /* When setting logical tags, we don't care about the length, since
>> + we are only setting a single logical tag. */
>> + addr = aarch64_linux_set_ltag (addr, tags[0]);
>> +
>> + /* Update the value's content with the tag. */
>> + enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
>> + gdb_byte *srcbuf = value_contents_raw (address);
>> + store_unsigned_integer (srcbuf, sizeof (addr), byte_order, addr);
>> + }
>> + else
>> + {
>> + /* Make sure we are dealing with a tagged address to begin with. */
>> + if (!aarch64_linux_tagged_address_p (gdbarch, address))
>> + return 1;
>> +
>> + /* With G being the number of tag granules and N the number of tags
>> + passed in, we can have the following cases:
>> +
>> + 1 - G == N: Store all the N tags to memory.
>> +
>> + 2 - G < N : Warn about having more tags than granules, but write G
>> + tags.
>> +
>> + 3 - G > N : This is a "fill tags" operation. We should use the tags
>> + as a pattern to fill the granules repeatedly until we have
>> + written G tags to memory.
>> + */
>> +
>> + size_t g = get_tag_granules (addr, length, MTE_GRANULE_SIZE);
>> + size_t n = tags.size ();
>> +
>> + if (g < n)
>> + {
>> + warning (_("Got more tags than memory granules. Tags will be "
>> + "truncated."));
>> + }
>
> Remove curly braces.
>

Isn't the rule to have curly braces for one-statement conditionals if
such statement spans more than a single line? That has been my
understanding so far.

>> diff --git a/gdb/arch/aarch64-mte-linux.h b/gdb/arch/aarch64-mte-linux.h
>> index e555f0af19..5c5783f28b 100644
>> --- a/gdb/arch/aarch64-mte-linux.h
>> +++ b/gdb/arch/aarch64-mte-linux.h
>> @@ -32,10 +32,29 @@
>>
>> /* We have one tag per 16 bytes of memory. */
>> #define MTE_GRANULE_SIZE 16
>> +#define MTE_LOGICAL_TAG_START_BIT 56
>> +#define MTE_LOGICAL_MAX_VALUE 0xf
>>
>> /* Return the number of tag granules in the memory range
>> [ADDR, ADDR + LEN) given GRANULE_SIZE. */
>> extern size_t get_tag_granules (CORE_ADDR addr, size_t len,
>> size_t granule_size);
>>
>> +/* Return the 4-bit tag made from VALUE. */
>> +extern CORE_ADDR make_ltag_bits (CORE_ADDR value);
>
> As mentioned previously, I'd suggest giving a name that's better scoped,
> since these symbols are in a header file / exported. Or if they are just
> used in aarch64-mte-linux.c, move them there (the defines as well, if
> possible).

I'm adjusting the MTE-related functions to have more scoped names.

>
> Simon
>