From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qt1-x830.google.com (mail-qt1-x830.google.com [IPv6:2607:f8b0:4864:20::830]) by sourceware.org (Postfix) with ESMTPS id CFC623857C7E for ; Fri, 5 Feb 2021 14:05:42 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org CFC623857C7E Received: by mail-qt1-x830.google.com with SMTP id l23so4957957qtq.13 for ; Fri, 05 Feb 2021 06:05:42 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=VLd6kr5XsBoZgqffH3sPmWi+CUN7/cofdxnOtlGfXKM=; b=X2W70uZs6IqBS3lbaTFBE+G422Cd3YDwatv89HBHHFT+1nrZepLGhOu69CG9onnL/T qxLg+mYgjLmLnKq1ezEIGwRSO2zRe1IpUFRqIZZjLY1zyepKXet4yvoq/DUUr0DR6x0t dfGKyPoebBXTwVGdCnfpGvWgmOi4f83Kg7oRm3Q0BCXnzVlebNFhCt9HFclAOzEJLob+ cCO/M+mxn/zE6wJopYdJuASxogx3QnfRFlFyQhAec10CpcZzBnD+QUgCl4fTkN4YOJgl konec8c0QNvguACETrgPkQOrHat7tYOnzx+TMooPJ5wzBvDoinc1lTr6vd1B0Sw/bni2 8K7A== X-Gm-Message-State: AOAM531zFGQh0UyY/VvJYjQCiS5Lzeo2DRALeG82nwZNvwG0wFw0SUku th6b2Ue0FOEMe0wFc5NJLM024WSatMoPfQ== X-Google-Smtp-Source: ABdhPJzEh3JuspeiF5pJF7elUH+UFfWs46mkz3wJ7va+Qo2bufRB8adYcpFzqHQrKlYy7AMaOIDZdw== X-Received: by 2002:aed:2f86:: with SMTP id m6mr4217603qtd.253.1612533942110; Fri, 05 Feb 2021 06:05:42 -0800 (PST) Received: from ?IPv6:2804:7f0:8284:848d:5d42:306c:a572:c6bb? ([2804:7f0:8284:848d:5d42:306c:a572:c6bb]) by smtp.gmail.com with ESMTPSA id m21sm8263422qtq.52.2021.02.05.06.05.40 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 05 Feb 2021 06:05:41 -0800 (PST) Subject: Re: [PATCH v5 15/25] AArch64: Implement the memory tagging gdbarch hooks To: Simon Marchi , gdb-patches@sourceware.org References: <20210127202112.2485702-1-luis.machado@linaro.org> <20210127202112.2485702-16-luis.machado@linaro.org> <42f33abf-ec5f-e431-dce3-36ceab4a89a8@polymtl.ca> From: Luis Machado Message-ID: <098c47b4-dc83-2aab-b502-027ebde3d17f@linaro.org> Date: Fri, 5 Feb 2021 11:05:38 -0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0 MIME-Version: 1.0 In-Reply-To: <42f33abf-ec5f-e431-dce3-36ceab4a89a8@polymtl.ca> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-12.9 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org 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: , X-List-Received-Date: Fri, 05 Feb 2021 14:05:45 -0000 On 2/5/21 1:09 AM, Simon Marchi wrote: > > > 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. > Right. That makes sense. I turned this into... if (tags.size () != 1) error (_("Target returned an unexpected number of tags.")); >> +/* 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? > In theory it could, if someone called gdbarch_set_memtags with an empty vector of tag bytes. Current code won't run into this. So this is more of a sanity check. Might as well error out on this, instead of silently succeeding. >> + >> + 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. I can do a follow-up splitting the code into separate functions. As more tags are introduced (potentially), this will eventually be needed. > > The documentation in gdbarch.sh for get_memtag also only talks about the > logical tag (extracting the tag from the address). That is an oversight. Addressed now. > > 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. > I think it would work. The code has changed a bit since it was first put together. The intention was to have a gdbarch hook to return a string representation of a tag so generic GDB code wouldn't have to worry about touching/manipulating arch-specific data. Then again, value struct types are somewhat generic already. >> + >> 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. > Oops. Fixed. >> { >> /* 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 :)). Fair enough. Fixed now. > > The patch LGTM with the nits fixed Thanks!