From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id iIVtHltQB2bXhRsAWB0awg (envelope-from ) for ; Fri, 29 Mar 2024 19:35:55 -0400 Authentication-Results: simark.ca; dkim=pass (2048-bit key; unprotected) header.d=linaro.org header.i=@linaro.org header.a=rsa-sha256 header.s=google header.b=QhmdGtFu; dkim-atps=neutral Received: by simark.ca (Postfix, from userid 112) id 6BEDA1E0C0; Fri, 29 Mar 2024 19:35:55 -0400 (EDT) Received: from server2.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 ECDSA (prime256v1) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id 254101E08C for ; Fri, 29 Mar 2024 19:35:53 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id BC77F3858C32 for ; Fri, 29 Mar 2024 23:35:52 +0000 (GMT) Received: from mail-pf1-x433.google.com (mail-pf1-x433.google.com [IPv6:2607:f8b0:4864:20::433]) by sourceware.org (Postfix) with ESMTPS id 107B13858D33 for ; Fri, 29 Mar 2024 23:35:29 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 107B13858D33 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=linaro.org ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 107B13858D33 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2607:f8b0:4864:20::433 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1711755332; cv=none; b=QAzS77fNQtagq+AX9FLtbMNMhcAjEbQGO0HAq/BrVUPxHyZ19A2DUZjTe6b/pGjC/AqbrXmg6cdBPLHDTxiDPd4Ef5/Xgyofgk98rWojUnI61NuJnt+oMTersaPYw/X7b5xd8lHksPzedFd5gAzqCt++aKJX+jls/Zgfe9b+aaE= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1711755332; c=relaxed/simple; bh=bwIk8lp8hiSI/BEyudva8LrV5DZopvUSOEC84kvbUXQ=; h=DKIM-Signature:From:To:Subject:Date:Message-ID:MIME-Version; b=KUAXPG5cEeiA2wz2HJkwtQnL//MNNtWLxNFUFC4HM/3DBuWq3RAEfM10SnoCFu4z+X1h4iJAY9epqonC78k1HrHRBYO8BubDYMvGx56E3aHgReXUPzikv9ZYoTmDs4vVCJn+A/wpYBXTKuP1Z5pp/uqXkvQvUqMnqsFIc/5Tq+k= ARC-Authentication-Results: i=1; server2.sourceware.org Received: by mail-pf1-x433.google.com with SMTP id d2e1a72fcca58-6ea9a60f7f5so2041867b3a.3 for ; Fri, 29 Mar 2024 16:35:29 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1711755328; x=1712360128; darn=sourceware.org; h=mime-version:message-id:date:user-agent:references:in-reply-to :subject:cc:to:from:from:to:cc:subject:date:message-id:reply-to; bh=VVomMBQYoMraSotdNdvkopaBgKHEZAt8O/vUnn4O4So=; b=QhmdGtFu/eKcnTM+RXy6C3yRMN8LnIk3Q4SQ6ggvtceI/XLRBc84bthaWJdKTtdnyc BxCNImBcc7h6CpY9AGa61ZkzEMIV7AyoRYLZ1vbcivAAhdV2sgzf0VZbkKgP1YPQ0s10 WA1GvQBhepwk8/3JOQKOADA2zMWmg8Lim4Lxi8Fnz2pgRj7cFvXMLJ71S6vg0WNYNTrN 0VH7/Q2+wxwoLgXmrb9pJkEo2FezaRsF8b6UlKfvztiCwxAw0YLs7tRHzaPjibMNe7jp f2Yo4/IzyOALDYEW7rS3k+Xl4hg7mVPeRkurE4WhNrDWXqBtOr/xO11y6WBTpmEipPCn uGhA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1711755328; x=1712360128; h=mime-version:message-id:date:user-agent:references:in-reply-to :subject:cc:to:from:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=VVomMBQYoMraSotdNdvkopaBgKHEZAt8O/vUnn4O4So=; b=PARiCyKNSrAoXPRHYbnlegxA1mevNNGZdEcKH6EgoRC7M1btmWnGORUTMSzRw0qjjG V9o5x+c4G5RVgQOLbaIqLFBK+6SuEXBoIyMkSIS6Lu7GmCxCjOVU0WLEJm8B45Wgunf/ dSGdLgm+IGU0yxR711QVHfogzlkmuyW4/Iz2ISbJpBSYvc0CLzginWlY13bY/6/4GJCm R14WpSIVnAJU80zLsktPCjEbgC/NvsBUUU3YW75SZhajPzz3NJwmelQE1CwM19jCBU9R gwsWeFnPjUnYGirb8QcKrW3MfXLNw7ggVsx+z0VQuLztZPU0YzLYoUjHY7ruHvR7+9SD TnLw== X-Gm-Message-State: AOJu0YxBfcw0w54f9nWgVTNx5Z9nmECns5SniRm0nU1+HqIqx0l59cez UYUJVSHah75mY0fQ7F0qIedcxXj238+K+7fumi/m2HkpKZ6f/Cjs+Yck4JuTpZ0= X-Google-Smtp-Source: AGHT+IGXw9lJd1GBBBdMtWmQMn4eMmtzyVg70An7Emr80LyetylQ1EEO9HIB3eq0d/aXMLbSGC/V0A== X-Received: by 2002:a05:6a00:23c1:b0:6ea:8a2c:96fa with SMTP id g1-20020a056a0023c100b006ea8a2c96famr3840618pfc.23.1711755327913; Fri, 29 Mar 2024 16:35:27 -0700 (PDT) Received: from localhost ([2804:14d:7e39:8470:2981:86:c711:2bee]) by smtp.gmail.com with ESMTPSA id s14-20020a056a00178e00b006e6b9dd81bdsm3625317pfg.99.2024.03.29.16.35.27 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 29 Mar 2024 16:35:27 -0700 (PDT) From: Thiago Jung Bauermann To: Gustavo Romero Cc: gdb-patches@sourceware.org, luis.machado@arm.com Subject: Re: [PATCH v2 4/4] gdb: Add new remote packet to check if address is tagged In-Reply-To: <20240328224850.2785280-5-gustavo.romero@linaro.org> (Gustavo Romero's message of "Thu, 28 Mar 2024 22:48:50 +0000") References: <20240328224850.2785280-1-gustavo.romero@linaro.org> <20240328224850.2785280-5-gustavo.romero@linaro.org> User-Agent: mu4e 1.12.2; emacs 29.1 Date: Fri, 29 Mar 2024 20:35:25 -0300 Message-ID: <878r20a9xe.fsf@linaro.org> MIME-Version: 1.0 Content-Type: text/plain X-Spam-Status: No, score=-11.6 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.30 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: gdb-patches-bounces+public-inbox=simark.ca@sourceware.org Hello Gustavo, I started reviewing the patch series backwards... I'll provide comments on this patch first. Overall, it looks great. I just have some localised comments in a few places below. Also, for ease of review I suggest splitting this patch in two: - one that introduces the new check_memtag_addr target hook and converts the existing code to use it, - and another that adds a check_memtag_addr target hook implementation to the remote target. Gustavo Romero writes: > This commit adds a new packet qMemTagAddrCheck allowing GDB remote > targets to use it to query gdbservers if a given address is tagged. > > It also adds a new GDB remote feature, 'memory-tagging-check-add+', > which must be advertised by the GDB servers to inform GDB they can reply > to address checks via the new qMemTagAddrCheck remote packet. You will need to document the remote protocol changes in gdb.texinfo, in the "Remote Serial Protocol" appendix. > Currently, this address check is done via a read query, where the > contents of /proc//smaps is read and the flags in there are > inspected for MTE-related flags that indicate the address is in a tagged > memory region. > > This is not ideal, for example, on QEMU gdbstub and in other cases, > like in baremetal debugging, where there is no notion of any OS file > like smaps. Hence, qMemTagAddrCheck packet allows check addresses in > an OS-agnostic way. > > For supporting the new packet, a new target hook is introduced, > check_memtag_addr, which is used instead of the gdbarch_tagged_address_p > gdbarch hook in the upper layers (printcmd.c). > > The new target hook is then specialized per target, for remote.c, > aarch64-linux-nat.c, and corelow.c targets (the current targets that > are MTE-aware). > > The target hook in remote.c uses the qMemTagAddrCheck packet to check > an address if the server advertised the 'memory-tagging-check-add+' > feature, otherwise it falls back to using the current mechanism, i.e. it > reads the /proc//smaps contents. > > In the aarch64-linux-nat.c and corelow.c the target hook uses the > gdbarch_tagged_address_p gdbarch hook, so there is no change regarding > how an address is checked in these targets. Just the > gdbarch_tagged_address_p signature is changed for convenience, since > target_check_memtag_addr takes the address to be checked as a CORE_ADDR > type. I agree that a CORE_ADDR argument is more convenient. > @@ -1071,6 +1073,12 @@ aarch64_linux_nat_target::store_memtags (CORE_ADDR address, size_t len, > return false; > } > > +bool > +aarch64_linux_nat_target::check_memtag_addr (CORE_ADDR address) > +{ > + return gdbarch_tagged_address_p (current_inferior ()->arch (), address); I think it's better to pass the gdbarch as an argument to the check_memtag_addr hook rather than getting it from current_inferior here, even if in practice your patch is equivalent to the existing code. The reason is that we are trying to move calls to current_* functions (which is global state in disguise) up the stack, so that most of GDB needs to reference only local state. Then if callers have a gdbarch available in their context they can pass it to the hook, or else they can use current_inferior ()->arch (). > @@ -1410,6 +1412,12 @@ core_target::fetch_memtags (CORE_ADDR address, size_t len, > return false; > } > > +bool > +core_target::check_memtag_addr (CORE_ADDR address) > +{ > + return gdbarch_tagged_address_p (current_inferior ()->arch (), address); Same comment here, of course. > diff --git a/gdb/printcmd.c b/gdb/printcmd.c > index ae4d640ccf2..c81c75afc5d 100644 > --- a/gdb/printcmd.c > +++ b/gdb/printcmd.c > @@ -1132,7 +1132,7 @@ do_examine (struct format_data fmt, struct gdbarch *gdbarch, CORE_ADDR addr) > = value_from_ulongest (builtin_type (gdbarch)->builtin_data_ptr, > tag_laddr); > > - if (gdbarch_tagged_address_p (current_inferior ()->arch (), v_addr)) > + if (target_check_memtag_addr (value_as_address(v_addr))) Missing space between "value_as_address" and the opening parens. Also, not a problem introduced by you, but I don't understand why the code uses the gdbarch from current_inferior if it was passed a gdbarch in the arguments. So I'd add a separate patch before this one to fix this code to use the gdbarch that was passed as a parameter. Then this patch can also pass it as a parameter to target_check_memtag_addr as I mentioned in an earlier comment. > { > /* Fetch the allocation tag. */ > struct value *tag > @@ -1289,7 +1289,7 @@ should_validate_memtags (struct value *value) > return false; > > /* We do. Check whether it includes any tags. */ > - return gdbarch_tagged_address_p (current_inferior ()->arch (), value); > + return target_check_memtag_addr (value_as_address(value)); Here there's no gdbarch available in context, but since there's only one caller to this function, it's easy to add the gdbarch parameter to it and make the caller pass it down. The caller does get it from current_inferior, so perhaps I'm being too pedantic here but IMHO it's worth it. Also, a space is missing before the opening parens. > } > > /* Helper for parsing arguments for print_command_1. */ > @@ -2946,9 +2946,10 @@ memory_tag_print_tag_command (const char *args, enum memtag_type tag_type) > flag, it is no use trying to access/manipulate its allocation tag. > > It is OK to manipulate the logical tag though. */ > + CORE_ADDR addr = value_as_address(val); Missing space before the opening parens. > if (tag_type == memtag_type::allocation > - && !gdbarch_tagged_address_p (arch, val)) > - show_addr_not_tagged (value_as_address (val)); > + && !target_check_memtag_addr(addr)) Missing space before the opening parens. > + show_addr_not_tagged (addr); > > value *tag_value = gdbarch_get_memtag (arch, val, tag_type); > std::string tag = gdbarch_memtag_to_string (arch, tag_value); > @@ -3104,8 +3105,9 @@ parse_set_allocation_tag_input (const char *args, struct value **val, > > /* If the address is not in a region memory mapped with a memory tagging > flag, it is no use trying to access/manipulate its allocation tag. */ > - if (!gdbarch_tagged_address_p (current_inferior ()->arch (), *val)) > - show_addr_not_tagged (value_as_address (*val)); > + CORE_ADDR addr = value_as_address (*val); > + if (!target_check_memtag_addr (addr)) > + show_addr_not_tagged (addr); This is another instance where I'd suggest making the caller pass gdbarch as an argument so that it can be used here, since this function only has one caller. > } > > /* Implement the "memory-tag set-allocation-tag" command. > @@ -3129,8 +3131,9 @@ memory_tag_set_allocation_tag_command (const char *args, int from_tty) > > /* If the address is not in a region memory mapped with a memory tagging > flag, it is no use trying to manipulate its allocation tag. */ > - if (!gdbarch_tagged_address_p (current_inferior ()->arch (), val)) { > - show_addr_not_tagged (value_as_address(val)); > + CORE_ADDR addr = value_as_address (val); > + if (!target_check_memtag_addr (addr)) { > + show_addr_not_tagged (addr); > } This is a preexisting issue in the code, but since you're touching it: the GNU style is to not use curly braces when there's only one statement in the if block. > @@ -15532,6 +15547,19 @@ create_store_memtags_request (gdb::char_vector &packet, CORE_ADDR address, > strcpy (packet.data (), request.c_str ()); > } > > +static void > +create_check_memtag_addr_request (gdb::char_vector &packet, CORE_ADDR address) > +{ > + int addr_size = gdbarch_addr_bit (current_inferior ()->arch()) / 8; > + > + std::string request = string_printf ("qMemTagAddrCheck:%s", phex_nz (address, addr_size)); > + > + if (packet.size () < request.length ()) There's an off-by-one error here: packet is expected to be null-terminated, but request.length doesn't count a terminating null byte so a "+ 1" needs to be added here. > + error (_("Contents too big for packet qMemTagAddrCheck.")); > + > + strcpy (packet.data (), request.c_str ()); > +} > + > /* Implement the "fetch_memtags" target_ops method. */ > > bool > @@ -15573,6 +15601,36 @@ remote_target::store_memtags (CORE_ADDR address, size_t len, > return packet_check_result (rs->buf).status () == PACKET_OK; > } > > +bool > +remote_target::check_memtag_addr (CORE_ADDR address) > +{ > + struct remote_state *rs = get_remote_state (); > + > + if (!m_features.remote_memory_tagging_check_addr_p ()) > + /* Fallback to reading /proc//smaps for checking if an address is > + tagged or not. */ Currently this comment is accurate, but if some other architecture adds a gdbarch method that doesn't use /proc//smaps then it won't be anymore. I suggest saying something like "Fallback to arch-specific method of checking whether an address is tagged". > + return gdbarch_tagged_address_p (current_inferior ()->arch (), address); > + > + create_check_memtag_addr_request (rs->buf, address); > + > + putpkt (rs->buf); > + getpkt (&rs->buf); > + > + /* Check if reply is OK. */ > + if ((packet_check_result (rs->buf).status () != PACKET_OK) || rs->buf.empty()) Missing space between "empty" and opening parens. But I don't understand why check whether buf is empty. Looking at remote_target::getpkt, it doesn't look like buf is ever emptied. > + return false; > + > + gdb_byte tagged_addr; > + /* Convert only 2 hex digits, i.e. 1 byte in hex format. */ > + hex2bin(rs->buf.data(), &tagged_addr , 1); Missing space between "hex2bin", "data" and the opening parens. > + if (tagged_addr) > + /* 01 means address is tagged. */ > + return true; > + else > + /* 00 means address is not tagged. */ > + return false; The above can be simplified to "return tagged_addr != 0". -- Thiago