From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id FDTtNvrEHGB4FAAAWB0awg (envelope-from ) for ; Thu, 04 Feb 2021 23:09:30 -0500 Received: by simark.ca (Postfix, from userid 112) id D24891EFCB; Thu, 4 Feb 2021 23:09:30 -0500 (EST) X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-1.1 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,MAILING_LIST_MULTI,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.2 Received: from sourceware.org (server2.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id 8A40F1E945 for ; Thu, 4 Feb 2021 23:09:29 -0500 (EST) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 1FB8B389365B; Fri, 5 Feb 2021 04:09:29 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 1FB8B389365B DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1612498169; bh=nxR3f+6yF46livRAm+7zJYXfbREz4ailmhVyd/o5RbQ=; h=Subject:To:References:Date:In-Reply-To:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To: From; b=LXCHShoTheaNNb7G1OCVarD8wxybmwWmHBSbvtXY0MwX0oXexlpw94TDMPDpCs0sU WmTvA1PYaF1/FVx1yWt639NG3lkPADKVAyxlWS7lF9WXIfKB6jBoJFV3m7aD5Ff8Se FK3BZf01cWB2ta+lLEiqvrgGIvS0oJ2e2pHyF4xY= Received: from smtp.polymtl.ca (smtp.polymtl.ca [132.207.4.11]) by sourceware.org (Postfix) with ESMTPS id 17A18385F029 for ; Fri, 5 Feb 2021 04:09:26 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 17A18385F029 Received: from simark.ca (simark.ca [158.69.221.121]) (authenticated bits=0) by smtp.polymtl.ca (8.14.7/8.14.7) with ESMTP id 11549JCV001080 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Thu, 4 Feb 2021 23:09:24 -0500 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp.polymtl.ca 11549JCV001080 Received: from [10.0.0.11] (192-222-157-6.qc.cable.ebox.net [192.222.157.6]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id DBE281E945; Thu, 4 Feb 2021 23:09:19 -0500 (EST) Subject: Re: [PATCH v5 15/25] AArch64: Implement the memory tagging gdbarch hooks To: Luis Machado , gdb-patches@sourceware.org References: <20210127202112.2485702-1-luis.machado@linaro.org> <20210127202112.2485702-16-luis.machado@linaro.org> Message-ID: <42f33abf-ec5f-e431-dce3-36ceab4a89a8@polymtl.ca> Date: Thu, 4 Feb 2021 23:09:19 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.6.1 MIME-Version: 1.0 In-Reply-To: <20210127202112.2485702-16-luis.machado@linaro.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Fri, 5 Feb 2021 04:09:20 +0000 X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Simon Marchi via Gdb-patches Reply-To: Simon Marchi Errors-To: gdb-patches-bounces@sourceware.org Sender: "Gdb-patches" On 2021-01-27 3:21 p.m., Luis Machado via Gdb-patches wrote: > Updates on v4: > > - Update function names to be more scoped. > - Use of gdb::optional values. > - Fixed up gdbarch hooks. > > 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 > > * aarch64-linux-tdep.c: Include target.h, arch-utils.h, value.h. > (aarch64_mte_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 (aarch64_mte_make_ltag_bits) > (aarch64_mte_make_ltag, aarch64_linux_set_ltag) > (aarch64_linux_get_ltag): New functions. > * arch/aarch64-mte-linux.h (AARCH64_MTE_LOGICAL_TAG_START_BIT) > (AARCH64_MTE_LOGICAL_MAX_VALUE): Define. > (aarch64_mte_make_ltag_bits, aarch64_mte_make_ltag) > (aarch64_mte_set_ltag, aarch64_mte_get_ltag): New prototypes. > --- > gdb/aarch64-linux-tdep.c | 210 +++++++++++++++++++++++++++++++++++ > gdb/arch/aarch64-mte-linux.c | 41 ++++++- > gdb/arch/aarch64-mte-linux.h | 19 ++++ > 3 files changed, 269 insertions(+), 1 deletion(-) > > diff --git a/gdb/aarch64-linux-tdep.c b/gdb/aarch64-linux-tdep.c > index f113d1ea30..2fe88cf016 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. > > +------------+ ^ > @@ -1513,6 +1517,187 @@ aarch64_linux_gcc_target_options (struct gdbarch *gdbarch) > return {}; > } > > +/* Helper to get the allocation tag from a 64-bit ADDRESS. > + > + Return the allocation tag if successful and nullopt otherwise. */ > + > +static gdb::optional > +aarch64_mte_get_atag (CORE_ADDR address) > +{ > + gdb::byte_vector tags; > + > + /* Attempt to fetch the allocation tag. */ > + if (!target_fetch_memtags (address, 0, tags, tag_allocation)) > + return {}; > + > + /* Only one tag should've been returned. Make sure we got exactly that. */ > + gdb_assert (tags.size () == 1); The tags are returned by the target, possibly the remote target. That means that you could make GDB assert by returning it a memtag packet with the wrong number of bytes. And we don't want it to be possible to make GDB assert when feeding it bad input. I'd probably use error() here just for this reason. > +/* Implement the set_memtags gdbarch method. */ > + > +static bool > +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 true; Can this can really happen? > + > + 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_mte_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 false; > + > + /* 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 = aarch64_mte_get_tag_granules (addr, length, > + AARCH64_MTE_GRANULE_SIZE); > + size_t n = tags.size (); > + > + if (g < n) > + warning (_("Got more tags than memory granules. Tags will be " > + "truncated.")); > + else if (g > n) > + warning (_("Using tag pattern to fill memory range.")); > + > + if (!target_store_memtags (addr, length, tags, tag_allocation)) > + return false; > + } > + return true; I just realized that the gdbarch methods are overloaded, as they are used to both set/get the logical tag of the pointer and set/get the allocation tags of the allocated memory. I think it would be easier to understand to have four methods, get/set the local tags and get/set the allocation tags. The documentation in gdbarch.sh for get_memtag also only talks about the logical tag (extracting the tag from the address). Since this is internal stuff, I suggest we leave this patch as-is to avoid some more churn, but please consider addressing it as a follow-up patch to the series, if you agree. > +} > + > +/* Implement the get_memtag gdbarch method. */ > + > +static struct value * > +aarch64_linux_get_memtag (struct gdbarch *gdbarch, struct value *address, > + enum memtag_type tag_type) > +{ > + gdb_assert (address != nullptr); > + > + CORE_ADDR addr = value_as_address (address); > + CORE_ADDR tag = 0; > + > + /* Get the logical tag or the allocation tag. */ > + if (tag_type == tag_logical) > + 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; > + > + gdb::optional atag = aarch64_mte_get_atag (addr); > + > + if (!atag.has_value ()) > + return nullptr; > + > + tag = *atag; > + } > + > + /* Convert the tag to a value. */ > + return value_from_ulongest (builtin_type (gdbarch)->builtin_unsigned_int, > + tag); > +} > + > +/* Implement the memtag_to_string gdbarch method. */ > + > +static std::string > +aarch64_linux_memtag_to_string (struct gdbarch *gdbarch, > + struct value *address, > + enum memtag_type tag_type) > +{ > + gdb_assert (address != nullptr); > + > + struct value *v_tag = aarch64_linux_get_memtag (gdbarch, address, tag_type); > + > + if (v_tag == nullptr && tag_allocation) > + error (_("Error getting tag from target")); > + > + CORE_ADDR tag = value_as_address (v_tag); > + > + return string_printf ("0x%s", phex_nz (tag, sizeof (tag))); > +} I'll see in the following patches how gdbarch_linux_memtag_to_string is used, but at first glance I think it would make sense for this method to have the prototype: std::string gdbarch_memtag_to_string (struct gdbarch *gdbarch, value *tag); Meaning that you would pass it a tag you already fetched (using gdbarch_get_memtag). This way if you already have a tag as a local variable and you want to format it as a string, it doesn't require going to the target. I don't think you would need the tag type parameter in this case, as we wouldn't care where the tag comes from, we'd just use the passed-in value. Also something that can be done as a follow-up patch. > + > static void > aarch64_linux_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch) > { > @@ -1570,6 +1755,31 @@ aarch64_linux_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch) > data associated with the address. */ > set_gdbarch_significant_addr_bit (gdbarch, 56); > > + /* 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); > + > + /* Register a hook for checking if there is a memory tag match. */ > + set_gdbarch_memtag_matches_p (gdbarch, > + aarch64_linux_memtag_matches_p); > + > + /* Register a hook for setting the logical/allocation tags for > + a range of addresses. */ > + set_gdbarch_set_memtags (gdbarch, aarch64_linux_set_memtags); > + > + /* Register a hook for extracting the logical/allocation tag from an > + address. */ > + set_gdbarch_get_memtag (gdbarch, aarch64_linux_get_memtag); > + > + /* Set the allocation tag granule size to 16 bytes. */ > + set_gdbarch_memtag_granule_size (gdbarch, AARCH64_MTE_GRANULE_SIZE); > + > + /* Register a hook for converting a memory tag to a string. */ > + set_gdbarch_memtag_to_string (gdbarch, aarch64_linux_memtag_to_string); > + } > + > /* Initialize the aarch64_linux_record_tdep. */ > /* These values are the size of the type that will be used in a system > call. They are obtained from Linux Kernel source. */ > diff --git a/gdb/arch/aarch64-mte-linux.c b/gdb/arch/aarch64-mte-linux.c > index ede5f5f2b9..4461864a32 100644 > --- a/gdb/arch/aarch64-mte-linux.c > +++ b/gdb/arch/aarch64-mte-linux.c > @@ -22,7 +22,8 @@ > /* See arch/aarch64-mte-linux.h */ > > size_t > -aarch64_mte_get_tag_granules (CORE_ADDR addr, size_t len, size_t granule_size) > +aarch64_mte_get_tag_granules (CORE_ADDR addr, size_t len, > + size_t granule_size) Unintended change. > { > /* Start address */ > CORE_ADDR s_addr = align_down (addr, granule_size); > @@ -32,3 +33,41 @@ aarch64_mte_get_tag_granules (CORE_ADDR addr, size_t len, size_t granule_size) > /* We always have at least 1 granule. */ > return 1 + (e_addr - s_addr) / granule_size; > } > + > +/* See arch/aarch64-mte-linux.h */ > + > +CORE_ADDR > +aarch64_mte_make_ltag_bits (CORE_ADDR value) > +{ > + return value & AARCH64_MTE_LOGICAL_MAX_VALUE; > +} > + > +/* See arch/aarch64-mte-linux.h */ > + > +CORE_ADDR > +aarch64_mte_make_ltag (CORE_ADDR value) > +{ > + return aarch64_mte_make_ltag_bits (value) > + << AARCH64_MTE_LOGICAL_TAG_START_BIT; We would usually use parenthesis when wrapping this: return (aarch64_mte_make_ltag_bits (value) << AARCH64_MTE_LOGICAL_TAG_START_BIT); The argument being that Emacs aligns it correctly if you do that (not that I care :)). The patch LGTM with the nits fixed. Simon