From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id jEhzHige61+lYQAAWB0awg (envelope-from ) for ; Tue, 29 Dec 2020 07:16:40 -0500 Received: by simark.ca (Postfix, from userid 112) id 6E2561F0AA; Tue, 29 Dec 2020 07:16:40 -0500 (EST) X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on simark.ca X-Spam-Level: X-Spam-Status: No, score=0.2 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,MAILING_LIST_MULTI,RDNS_NONE,URIBL_BLOCKED autolearn=no autolearn_force=no version=3.4.2 Received: from sourceware.org (unknown [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 E979A1E552 for ; Tue, 29 Dec 2020 07:16:39 -0500 (EST) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 593AA38618E2; Tue, 29 Dec 2020 12:16:39 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 593AA38618E2 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1609244199; bh=Qy/lTLyyqhCQ4ssSyUEe99O8IKFLmtTnJTylGwdYS/4=; h=Subject:To:References:Date:In-Reply-To:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=gl9B8F/P3v0knNy+L9RGnRngZBlG9HoFZq4bbeo+NdKKZacnsN97J3/pKl9bCcOwV tvBTKPcmHWXFiOj8Wy16qp18bbYA/WLplcZ4eBOqat4qqchM4SCdDAm8WeuU0VWHfF ANl0HAq70XWCnB+7HNgWC7yAT3cXm/bzwhdaaXfM= Received: from mail-qt1-x831.google.com (mail-qt1-x831.google.com [IPv6:2607:f8b0:4864:20::831]) by sourceware.org (Postfix) with ESMTPS id 82CE938618CC for ; Tue, 29 Dec 2020 12:16:36 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 82CE938618CC Received: by mail-qt1-x831.google.com with SMTP id 2so8726272qtt.10 for ; Tue, 29 Dec 2020 04:16:36 -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:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=Qy/lTLyyqhCQ4ssSyUEe99O8IKFLmtTnJTylGwdYS/4=; b=fU5vYALbqtwhcGRueDa4GSCQWQ9kwvYcsi5o90hcrY6tdeeRM7AomIxjMK+3AHhe3b oANmo3allUcys/FGeCdPKz14Y6myRyyeSvBRdJsL4ybS79wePwuiIQauZbewNFGwyVXN X8qCdTAOBDUso7HNLou11kXp2tE8rjSDpgi7xzWliAcnuMQaW7JYqY9ORKjo3ML7sk0Y 0Ww6MSlroJHpF1Y+jhqXMfA/ibDKTiD/PshBp6hphQvWaEjpFSFZ2gXY4ApchDT5VxCC oU7Z/KmnFoLlc/Wd+n3m1yVODUs/PIQC7puugbBJCx7AaeFonhetUJHUCUq2fK8NNEQu K5hw== X-Gm-Message-State: AOAM532f8A6mlCTM6NIxjKGVpjPfeq4ecllce/BcPF9IJnJ2sAmhy//k EY2h+/sGscd/i/8z8/l8EUl/sg== X-Google-Smtp-Source: ABdhPJxI/w+IudEjWGzBFwBYbjXxel9lapsgh815HTsgcam/hJW+C8dmI2ZeXpB6GQl5x//ZUanm5g== X-Received: by 2002:a05:622a:291:: with SMTP id z17mr47236768qtw.180.1609244195582; Tue, 29 Dec 2020 04:16:35 -0800 (PST) Received: from ?IPv6:2804:7f0:8284:370e:19e7:527f:e109:2734? ([2804:7f0:8284:370e:19e7:527f:e109:2734]) by smtp.gmail.com with ESMTPSA id d16sm25941099qkb.42.2020.12.29.04.16.34 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 29 Dec 2020 04:16:35 -0800 (PST) Subject: Re: [PATCH v3 14/24] AArch64: Implement the memory tagging gdbarch hooks To: Simon Marchi , gdb-patches@sourceware.org References: <20201109170435.15766-1-luis.machado@linaro.org> <20201109170435.15766-15-luis.machado@linaro.org> <7811415a-320c-5672-b4d8-a11425372c2f@polymtl.ca> Message-ID: <6809407d-14dd-945d-dad4-6530d9f6f97a@linaro.org> Date: Tue, 29 Dec 2020 09:16:32 -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: <7811415a-320c-5672-b4d8-a11425372c2f@polymtl.ca> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit 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: Luis Machado via Gdb-patches Reply-To: Luis Machado Cc: david.spickett@linaro.org Errors-To: gdb-patches-bounces@sourceware.org Sender: "Gdb-patches" 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 >> >> * 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, 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 >