From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id N9NEMaL3tGDmOwAAWB0awg (envelope-from ) for ; Mon, 31 May 2021 10:50:10 -0400 Received: by simark.ca (Postfix, from userid 112) id C23B51F163; Mon, 31 May 2021 10:50:10 -0400 (EDT) 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 D324A1E940 for ; Mon, 31 May 2021 10:50:09 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 431DD3938C05; Mon, 31 May 2021 14:50:09 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 431DD3938C05 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1622472609; bh=vTpHfkIHETPpetknGxtxlEd2cwAScFrp5Fh0BqJAcYM=; 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=A6v3ff1koDYq9+3BXbCTTkZ6T+wYforrmIvWLFsbYcVdNNj9O1naBw+/lH2QEdgKs tCnl6x7eYUgk7Kw+bt0pX72oEOjMV5lREzt37Cb0R5o1fOic1xYUVwaUmdRrpNC2oi UxuA85fkuY8Go669dJeOqTTrEboMgTuQhnMIqKWA= Received: from smtp.polymtl.ca (smtp.polymtl.ca [132.207.4.11]) by sourceware.org (Postfix) with ESMTPS id 6042C384BC21 for ; Mon, 31 May 2021 14:50:06 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 6042C384BC21 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 14VEns82013975 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Mon, 31 May 2021 10:49:59 -0400 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp.polymtl.ca 14VEns82013975 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)) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id E44511E940; Mon, 31 May 2021 10:49:53 -0400 (EDT) Subject: Re: [PATCH,v2] [AArch64] MTE corefile support To: Luis Machado , gdb-patches@sourceware.org References: <20210518202047.3492211-1-luis.machado@linaro.org> <20210526140838.3856474-1-luis.machado@linaro.org> <4ae84e2c-f0c4-bcac-1e5d-b9b34d8521db@linaro.org> Message-ID: <87af0c4e-7a26-ecde-dc4e-95f96ec2ccc8@polymtl.ca> Date: Mon, 31 May 2021 10:49:53 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.10.1 MIME-Version: 1.0 In-Reply-To: <4ae84e2c-f0c4-bcac-1e5d-b9b34d8521db@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 Mon, 31 May 2021 14:49:54 +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 Cc: david.spickett@linaro.org, catalin.marinas@arm.com Errors-To: gdb-patches-bounces@sourceware.org Sender: "Gdb-patches" On 2021-05-31 10:12 a.m., Luis Machado wrote: >>> @@ -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. >> > > I'm inclined to leave that as documentation instead, so it is clear what the header sizes are referring to. The thing is that somebody reading the code in the .c seeing the arbitrary sizeofs will note have a clue to go look at arch/aarch64-mte-linux.h for explanations. So another option would be to write it as a comment near the code. Something like: /* The header follows the following format: struct { /* 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; }; */ Another option would be to have the structure and refer to its fields in sizeof: store_unsigned_integer (buf, sizeof (tag_dump_mte::granule_byte_size), byte_order, AARCH64_MTE_GRANULE_SIZE); This has the advantage that the structure can still be present as documentation in a central place, the reader of that code will naturally be pointed to it if they need more information, and since the structure type is actually used, it's fine to keep it there. >> 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... >> > > Right. I ran into this as well. I thought about this a bit, and I guess the simplest way is to have a new memtag.h header file and define it there. > > It would be generic enough and other OS' would be able to include it as well. And it would work for Core files. Sounds good. >> 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. >> > > I'm aware of that. I was hoping to leave this cleanup for a later stage. > > A complicating factor here is that we need to traverse the NT_MEMTAG notes for two slightly different reasons. > > The first is to decide if an address (no length specified) falls within one of the NT_MEMTAG entries, in which case we just return true/false; > > The second is to actually fetch the tags the user has requested (via address + length). This may potentially span multiple NT_MEMTAG notes, so the callback may be invoked multiple times. > > It should be possible to refactor the code to invoke callbacks, but we would need to have two callbacks (one for corelow and another for linux-tdep). The one for linux-tdep just needs to check if a note exists for a particular range, so it would be an useless callback in my opinion. > > With that said, I reworked things and moved the common code to a new function. This new function goes through the notes and returns whenever we hit a note that contains the address we're looking for. Then we have a chance to do some processing, return or continue going through notes. > > I think this simplifies things. Ok, I'll see in the new version. >> 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? >> > > Like the MTE NT_MEMTAG header, this is mostly for documentation purposes. We don't have a lot of use for the header structs right now, but may have more uses for them in the future. We may want to pass the header around and access it in a simpler way than just using local variables and skipping individual sizes. > > This is because we're using a packed struct and we don't want to run into issues with alignments and padding inserted by the compilers, or having to handle endianness differences at the corelow.c level. Yep. > I'll do the same as I've done with the NT_MEMTAG_MTE_HEADER_SIZE case and will document this struct alongside the definition of NT_MEMTAG_HEADER_SIZE (in a new generic file, memtag.h). As I suggested above, I think having the structure and referring to its fields in sizeofs would be a nice solution. Simon