From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id fIdWGoY7DmYFrSIAWB0awg (envelope-from ) for ; Thu, 04 Apr 2024 01:32:54 -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=InRdukJn; dkim-atps=neutral Received: by simark.ca (Postfix, from userid 112) id 566EB1E0C0; Thu, 4 Apr 2024 01:32:54 -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 359441E08C for ; Thu, 4 Apr 2024 01:32:52 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id C75BC3858402 for ; Thu, 4 Apr 2024 05:32:51 +0000 (GMT) Received: from mail-oa1-x34.google.com (mail-oa1-x34.google.com [IPv6:2001:4860:4864:20::34]) by sourceware.org (Postfix) with ESMTPS id C5BDE3858D34 for ; Thu, 4 Apr 2024 05:32:28 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org C5BDE3858D34 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 C5BDE3858D34 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2001:4860:4864:20::34 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1712208752; cv=none; b=UHjyU8xKt8yLGvFIi51ZKURdgxGw+oPbk7CfT9tV0enbnSV3wSyw7ZM/KolDlgJ3jeY+Ah5K/hXUhqXh0Tyz+9VhNcJ+n/RShP3O+6gmg0zNCVmLqwXRN/EI9w4rENAn85qhljMixoeN/QVIFidHv8b7f2sUuNdwDY4MV2K3DTA= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1712208752; c=relaxed/simple; bh=wb8jDtgqadFkdQbCecRuPoDLb+nkILVPLxaoF7SKi3E=; h=DKIM-Signature:Subject:To:From:Message-ID:Date:MIME-Version; b=YlDdT+k7GhMsP0fAcWBKHB4jgFlKNmC9A9bEOsYieRvV7ew+8OHc3JCgNdsGAK0lbFq8dXTtBt94NgYon3uvxeMgocxuUItct0xIhpvMvDT3xZCoGBOD24Uz3Lm7sFOap5T0JHED4EETw2ASUNFsNiVtA6C+GfyV8FvSgneLMqI= ARC-Authentication-Results: i=1; server2.sourceware.org Received: by mail-oa1-x34.google.com with SMTP id 586e51a60fabf-2282597eff5so408432fac.3 for ; Wed, 03 Apr 2024 22:32:28 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1712208748; x=1712813548; darn=sourceware.org; h=content-transfer-encoding:content-language:in-reply-to:mime-version :user-agent:date:message-id:from:references:cc:to:subject:from:to:cc :subject:date:message-id:reply-to; bh=s500yWAkC0+WCcST1ZnqgONKwgLbH0xt3h0GUZzEMlc=; b=InRdukJnBdpYl8556feO+dMW5c1HCPQFNfQAPXNqub2bra/DX16tVxd8y4i60vkxII tkUqm0hzAtwi/4BQcCvUx4i9Xx65sFw/VPA2usgFLb7FJGDEz3D0mm4d8Ln6T/7ZTF3o RIcM0wXzFgJ2V5Rb9xiUV45iMlAjnevwufWnG+AyxdCBLWLricl5FBMpQjLdY9XoebNu +Tep9XFm4yTV34uOPJ1r7L1Df71lcDlTBYVtUzQVY7SzgH+SKLbWXlV/dDuiRWLNx1H/ yL1zdk0Ye/MQsNRvxcObZHmgx0eb4gnTfW3tZjpYTMnj8QCXgfZiUadjIlAPHERWKm04 iRGQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1712208748; x=1712813548; h=content-transfer-encoding:content-language:in-reply-to:mime-version :user-agent:date:message-id:from:references:cc:to:subject :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=s500yWAkC0+WCcST1ZnqgONKwgLbH0xt3h0GUZzEMlc=; b=XKYTzYe8PppeAHcYpakF73vDm3bwUJb/AkNUO6ViW8FaUvqm/9YkpIW7RoYt9Grhl5 1IyXTAW7cam4vqf2FR6kGxYdcAkNCLYsQYdcPb+m+Y4naPyqPkO9OApyvk8PnUuzZzeU PDeFL4uZodW4zyIB+U47d/Cq1hB+cG6Is5XR81doXpi+G4VofPaR+pONQ5QxAAslJrAu EGV4IJ94LLCVex1MBgN4FQNef6RRvjXqr5iuk/2otjD1zkSj6FlGQdoou6YPRYQt5SKW cr50KZTywbD9puOVkUaCk9TNIfkTRu6fvOBORYEm69I3h9aHjSO3Han7xzJxmLa0W+3T DwPQ== X-Gm-Message-State: AOJu0Yy3acMGaCB27meP5VGPVTH4PLHr28mjoChxknqWvwRpWedrpnez A1x6AeSAa0HAHGakk91U/E5VqVz/kVISi7OrNorUwrLsdkttAAH/m9KRam0iy6lyB6rNGrquzhF H X-Google-Smtp-Source: AGHT+IHhvBk+Pusda0BqfdZ0OIdCgwY4EG/FtDzACmgnkjfBsS70EG+bjt0lT7K5MsWuNnDyFyAM8Q== X-Received: by 2002:a05:6870:14cd:b0:22e:1514:8077 with SMTP id l13-20020a05687014cd00b0022e15148077mr1389020oab.43.1712208747951; Wed, 03 Apr 2024 22:32:27 -0700 (PDT) Received: from ?IPv6:2804:7f0:b402:d0dc:4f31:e30e:9a7a:2717? ([2804:7f0:b402:d0dc:4f31:e30e:9a7a:2717]) by smtp.gmail.com with ESMTPSA id g18-20020a62e312000000b006eadf4c2b67sm12442632pfh.92.2024.04.03.22.32.26 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 03 Apr 2024 22:32:27 -0700 (PDT) Subject: Re: [PATCH v2 4/4] gdb: Add new remote packet to check if address is tagged To: Thiago Jung Bauermann Cc: gdb-patches@sourceware.org, luis.machado@arm.com References: <20240328224850.2785280-1-gustavo.romero@linaro.org> <20240328224850.2785280-5-gustavo.romero@linaro.org> <878r20a9xe.fsf@linaro.org> From: Gustavo Romero Message-ID: <827e6376-5aa3-0436-a35d-ac55e1e342f6@linaro.org> Date: Thu, 4 Apr 2024 02:32:24 -0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.6.0 MIME-Version: 1.0 In-Reply-To: <878r20a9xe.fsf@linaro.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-13.3 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.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 Hi Thiago, Thanks a lot for the nice review. I think it helped a lot the series organization. On 3/29/24 8:35 PM, Thiago Jung Bauermann wrote: > > 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. Done in v3. > 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. Done in v3. >> 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. Done in v3 in commit "gdb: Use passed gdbarch instead of calling current_inferior". >> { >> /* 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. OK, done in v3. > >> } >> >> /* 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. Good catch, thanks. Fixed in v3. >> + 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". Done in v3. >> + 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. I removed rs->buf.empty() in v3. >> + 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". Done in v3. Cheers, Gustavo