From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id OfrkM/tXFGYZWygAWB0awg (envelope-from ) for ; Mon, 08 Apr 2024 16:47: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=STRQfrUN; dkim-atps=neutral Received: by simark.ca (Postfix, from userid 112) id B961A1E0C0; Mon, 8 Apr 2024 16:47: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 8EBCD1E030 for ; Mon, 8 Apr 2024 16:47:53 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 0086D385840B for ; Mon, 8 Apr 2024 20:47:53 +0000 (GMT) Received: from mail-pf1-x434.google.com (mail-pf1-x434.google.com [IPv6:2607:f8b0:4864:20::434]) by sourceware.org (Postfix) with ESMTPS id 22C7E3858D28 for ; Mon, 8 Apr 2024 20:47:30 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 22C7E3858D28 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 22C7E3858D28 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2607:f8b0:4864:20::434 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1712609253; cv=none; b=iPxazgPt+xhEbUByq2dxDk3Ee+2xFPGi07gWotOR9lJOtnTyeIV1yhffBEc+sSP1dftc4Y/w2ODWl3XQszqR+UFZDeHI5cEQdjloix8HIVzJdJVU09GGzesEaE/1JctdQFGTCWAzQE+V18LEcuqSBaScW2mI6oPdNW5VSM0hiDU= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1712609253; c=relaxed/simple; bh=r4FCRj1h7eomIWX21YffDzAI1UlFWcVMEbVxZtyDdIU=; h=DKIM-Signature:Subject:To:From:Message-ID:Date:MIME-Version; b=JBOh1O5GSAru6v7TYxdOLPChBZFhvXm/SjdDXvruYZQ+jpQE1+Fnakfb9ftP+bL5dRy7u/7l7nRMSiKsGctMBJiOmeVh+bV8H/x9kjQc1ihHt+7gxQ2fLPZ71qyvEKEM/O8utOL4kNnQtVog+ZxKiFQh5nbli4gJHK8A3MfrLLw= ARC-Authentication-Results: i=1; server2.sourceware.org Received: by mail-pf1-x434.google.com with SMTP id d2e1a72fcca58-6eaf9565e6bso3482449b3a.2 for ; Mon, 08 Apr 2024 13:47:30 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1712609249; x=1713214049; 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=ZhdIq+bRmIJnw25LT0JHjP08c4NCGhVuCV7Wu1Q/KSo=; b=STRQfrUNPxC3xEdrUTO93/2Rr8/LsAYv+VJeaUpCX7kjiJXSiZjVqQ2FH4YXPE0hDg hj4/7whYt9mzD6z7ruQvxJrxUBYFcS8p/kwRJj5YRdcae+29F1u+eY030yyV1wQiJ6iY Toa0ehfhBptR4FWj4CTUhyO3sBSLxVdRDj1XDSV8YSzuGth41MXAzVDRVX2MhOh/DP27 bS1WDr/EwOTh9yfOBC3g2jtgokYK13hQFxWd3m4lH9SdSfcwTTNX0AQKPgIPmvwadCG6 Al1jabq1uhNPZoa2kKCsuEGqWzPevpRONYwTMWzzcluc2RVrTUJGCDCbF70ESyTfLBRB zjBA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1712609249; x=1713214049; 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=ZhdIq+bRmIJnw25LT0JHjP08c4NCGhVuCV7Wu1Q/KSo=; b=pgk9zoCp0SUqbKFut2nXyrRxMC5+QQ/3xktPLCnvVRzo9jKzj8bCg96SghfRYUgaA3 xx5ntOgsFsenpudPwCbweriBvalocDaLBB6L9Hh29wYmXd5lSiDXPwY/Q5Pnq+gNne/u oOH7X3bRCRIT6geXcyscsx9uN+Z7+LS27BTE4DQ8e7SIQnDE3k89yc9kgIxal1IEVsG8 k4DIF2/00UGfWHRwOjrA2YKmuk2Y0KCxASLM0ADU0pFUoc/TDCwTypm4m8gw1IitFTXN GFqywdsMiMmCEc09AXHahBfpiHn1v1DveKUjuKtmM2MMZXIq0LN/7EUKBi9X6+ovC0J4 R6Ow== X-Forwarded-Encrypted: i=1; AJvYcCUirYVQGb5ZWdTfqEsiQAd5wHo/WGi81sBBViO56oxFK4fSmkwM4xnXqoQy3bB96aDayuSmCUQhCVk+vr7SBPw0VwnhYa77mMBaGg== X-Gm-Message-State: AOJu0Yz75H7slbdgxGj7cWQJvTrRwStloMUYyHq3R2zzMKTK6lpTyjkM cee5qQcSBW9PhUJjucCDLWRM36/I9rhvdC+68i/0nbalokgeBA6Ajykqe0zvaaE= X-Google-Smtp-Source: AGHT+IEI0js8D6DQ8OWdf5T6C+LJs7c+VwIWHDrBWZgixkBz8T0mvhM8kpCDbhmiEJb90XaK1kpboA== X-Received: by 2002:a05:6a00:3c8c:b0:6ea:c2ef:3b71 with SMTP id lm12-20020a056a003c8c00b006eac2ef3b71mr10854663pfb.20.1712609248986; Mon, 08 Apr 2024 13:47:28 -0700 (PDT) Received: from [192.168.0.4] (201-69-10-25.dial-up.telesp.net.br. [201.69.10.25]) by smtp.gmail.com with ESMTPSA id v16-20020aa799d0000000b006e567c81d14sm6964057pfi.43.2024.04.08.13.47.27 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 08 Apr 2024 13:47:28 -0700 (PDT) Subject: Re: [PATCH v3 5/7] gdb: Introduce is_address_tagged target hook To: Luis Machado , gdb-patches@sourceware.org Cc: thiago.bauermann@linaro.org References: <20240404064819.2848899-1-gustavo.romero@linaro.org> <20240404064819.2848899-6-gustavo.romero@linaro.org> From: Gustavo Romero Message-ID: Date: Mon, 8 Apr 2024 17:47:25 -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: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-11.4 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, NICE_REPLY_A, RCVD_IN_BARRACUDACENTRAL, 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 Luis, On 4/4/24 12:45 PM, Luis Machado wrote: > On 4/4/24 07:48, Gustavo Romero wrote: >> This commit introduces a new target hook, target_is_address_tagged, >> which is used instead of the gdbarch_tagged_address_p gdbarch hook in >> the upper layer (printcmd.c). >> >> This change allows the memory tagging address checking to be specialized >> easily per target in the future. Since target_is_address_tagged >> continues to use the gdbarch_tagged_address_p hook there is no change >> in behavior for the targets using the new target hook (the remote.c, >> aarch64-linux-nat.c, and corelow.c targets). > > The above block... > >> >> This change enables easy specialization of memory tagging address >> check per target in the future. As target_is_address_tagged continues >> to utilize the gdbarch_tagged_address_p hook, there is no change in >> behavior for all the targets that use the new target hook (i.e., the >> remote.c, aarch64-linux-nat.c, and corelow.c targets). > > ... seems to be somewhat duplicated above in the commit message. > > Also, as general rule, we usually make updates clear at the top of the > commit message. for instace: > > Updates in v3: > > - Something something. > - Fixed breakage. > > And then those update blocks don't get pushed when the series is > approved (sometimes some do push it). > >> >> Just the gdbarch_tagged_address_p signature is changed for convenience, >> since target_is_address_tagged takes the address to be checked as a >> CORE_ADDR type. >> >> Signed-off-by: Gustavo Romero >> --- >> gdb/aarch64-linux-nat.c | 8 ++++++++ >> gdb/aarch64-linux-tdep.c | 10 ++++------ >> gdb/arch-utils.c | 2 +- >> gdb/arch-utils.h | 2 +- >> gdb/corelow.c | 8 ++++++++ >> gdb/gdbarch-gen.h | 4 ++-- >> gdb/gdbarch.c | 2 +- >> gdb/gdbarch_components.py | 2 +- >> gdb/printcmd.c | 31 +++++++++++++++++-------------- >> gdb/remote.c | 10 ++++++++++ >> gdb/target-delegates.c | 30 ++++++++++++++++++++++++++++++ >> gdb/target.c | 6 ++++++ >> gdb/target.h | 6 ++++++ >> 13 files changed, 95 insertions(+), 26 deletions(-) >> >> diff --git a/gdb/aarch64-linux-nat.c b/gdb/aarch64-linux-nat.c >> index 3face34ce79..19c099832a5 100644 >> --- a/gdb/aarch64-linux-nat.c >> +++ b/gdb/aarch64-linux-nat.c >> @@ -110,6 +110,8 @@ class aarch64_linux_nat_target final >> /* Write allocation tags to memory via PTRACE. */ >> bool store_memtags (CORE_ADDR address, size_t len, >> const gdb::byte_vector &tags, int type) override; >> + /* Check if an address is tagged. */ >> + bool is_address_tagged (gdbarch *gdbarch, CORE_ADDR address) override; >> }; >> >> static aarch64_linux_nat_target the_aarch64_linux_nat_target; >> @@ -1071,6 +1073,12 @@ aarch64_linux_nat_target::store_memtags (CORE_ADDR address, size_t len, >> return false; >> } >> >> +bool >> +aarch64_linux_nat_target::is_address_tagged (gdbarch *gdbarch, CORE_ADDR address) >> +{ >> + return gdbarch_tagged_address_p (gdbarch, address); > > I'd add a comment here on why we take a detour going to the linux-tdep layer > to read the smaps file, because we don't have a better way to get that information. > > In the future, if this check is ever made available through PTRACE, one can come > here and drop the smaps path. > >> +} >> + >> void _initialize_aarch64_linux_nat (); >> void >> _initialize_aarch64_linux_nat () >> diff --git a/gdb/aarch64-linux-tdep.c b/gdb/aarch64-linux-tdep.c >> index fc60e602748..2a47c3f0845 100644 >> --- a/gdb/aarch64-linux-tdep.c >> +++ b/gdb/aarch64-linux-tdep.c >> @@ -2451,17 +2451,15 @@ aarch64_mte_get_atag (CORE_ADDR address) >> /* Implement the tagged_address_p gdbarch method. */ >> >> static bool >> -aarch64_linux_tagged_address_p (struct gdbarch *gdbarch, struct value *address) >> +aarch64_linux_tagged_address_p (struct gdbarch *gdbarch, CORE_ADDR address) >> { >> - gdb_assert (address != nullptr); >> - >> - CORE_ADDR addr = value_as_address (address); >> + gdb_assert (address); > > No need to assert for a non-zero address. We used to assert that we had a valid pointer, > but since we're no longer dealing with a pointer, we don't have to worry about it. > > Plus, 0x0 is a valid address (at least for bare-metal. We could run into an assertion if > the user explicitly tries to check for tags in a 0x0 address. See below: > > (gdb) memory-tag check 0x0 > ../../../repos/binutils-gdb/gdb/aarch64-linux-tdep.c:2456: internal-error: aarch64_linux_tagged_address_p: Assertion `address' failed. > A problem internal to GDB has been detected, > further debugging may prove unreliable. > ----- Backtrace ----- > 0xaaaac69ccf97 _Z22gdb_internal_backtracev > 0xaaaac6e443e7 _ZL17internal_vproblemP16internal_problemPKciS2_St9__va_list > 0xaaaac6e4464f _Z15internal_verrorPKciS0_St9__va_list > 0xaaaac734a8b3 _Z18internal_error_locPKciS0_z > 0xaaaac68dc407 _ZL30aarch64_linux_tagged_address_pP7gdbarchm > 0xaaaac6c849ab _ZL24memory_tag_check_commandPKci > 0xaaaac69fb8ab _Z8cmd_funcP16cmd_list_elementPKci > 0xaaaac6de5ed3 _Z15execute_commandPKci > 0xaaaac6adc203 _Z15command_handlerPKc > 0xaaaac6add45f _Z20command_line_handlerOSt10unique_ptrIcN3gdb13xfree_deleterIcEEE > 0xaaaac6adccd7 _ZL23gdb_rl_callback_handlerPc > 0xaaaac6ea7eeb rl_callback_read_char > 0xaaaac6adbc7b _ZL42gdb_rl_callback_read_char_wrapper_noexceptv > 0xaaaac6adcb3b _ZL33gdb_rl_callback_read_char_wrapperPv > 0xaaaac6e1ef5f _ZL19stdin_event_handleriPv > 0xaaaac734b29f _ZL18gdb_wait_for_eventi.part.0 > 0xaaaac734bc7f _Z16gdb_do_one_eventi > 0xaaaac6bdd977 _ZL21captured_command_loopv > 0xaaaac6bdf6bb _Z8gdb_mainP18captured_main_args > 0xaaaac68cee47 main > >> >> /* Remove the top byte for the memory range check. */ >> - addr = gdbarch_remove_non_address_bits (gdbarch, addr); >> + address = gdbarch_remove_non_address_bits (gdbarch, address); >> >> /* Check if the page that contains ADDRESS is mapped with PROT_MTE. */ >> - if (!linux_address_in_memtag_page (addr)) >> + if (!linux_address_in_memtag_page (address)) >> return false; >> >> /* We have a valid tag in the top byte of the 64-bit address. */ >> diff --git a/gdb/arch-utils.c b/gdb/arch-utils.c >> index 456bfe971ff..cb149c36bc9 100644 >> --- a/gdb/arch-utils.c >> +++ b/gdb/arch-utils.c >> @@ -102,7 +102,7 @@ default_memtag_to_string (struct gdbarch *gdbarch, struct value *tag) >> /* See arch-utils.h */ >> >> bool >> -default_tagged_address_p (struct gdbarch *gdbarch, struct value *address) >> +default_tagged_address_p (struct gdbarch *gdbarch, CORE_ADDR address) >> { >> /* By default, assume the address is untagged. */ >> return false; >> diff --git a/gdb/arch-utils.h b/gdb/arch-utils.h >> index 2dcd8f6dc53..467be40c688 100644 >> --- a/gdb/arch-utils.h >> +++ b/gdb/arch-utils.h >> @@ -141,7 +141,7 @@ extern std::string default_memtag_to_string (struct gdbarch *gdbarch, >> struct value *tag); >> >> /* Default implementation of gdbarch_tagged_address_p. */ >> -bool default_tagged_address_p (struct gdbarch *gdbarch, struct value *address); >> +bool default_tagged_address_p (struct gdbarch *gdbarch, CORE_ADDR address); >> >> /* Default implementation of gdbarch_memtag_matches_p. */ >> extern bool default_memtag_matches_p (struct gdbarch *gdbarch, >> diff --git a/gdb/corelow.c b/gdb/corelow.c >> index f4e8273d962..b13d0124471 100644 >> --- a/gdb/corelow.c >> +++ b/gdb/corelow.c >> @@ -109,6 +109,8 @@ class core_target final : public process_stratum_target >> bool fetch_memtags (CORE_ADDR address, size_t len, >> gdb::byte_vector &tags, int type) override; >> >> + bool is_address_tagged (gdbarch *gdbarch, CORE_ADDR address) override; >> + > > Maybe add a comment to this function stating what we're looking for: > > /* If the architecture supports it, check if ADDRESS is within a memory range > mapped with tags. For example, MTE tags for AArch64. */ > > Or some other variation of the above. > > The reason being that the corelow target is a bit special because it is generic, hence > why we see an override for a x86 target hook in there as well. > >> x86_xsave_layout fetch_x86_xsave_layout () override; >> >> /* A few helpers. */ >> @@ -1410,6 +1412,12 @@ core_target::fetch_memtags (CORE_ADDR address, size_t len, >> return false; >> } >> >> +bool >> +core_target::is_address_tagged (gdbarch *gdbarch, CORE_ADDR address) >> +{ >> + return gdbarch_tagged_address_p (gdbarch, address); >> +} >> + >> /* Implementation of the "fetch_x86_xsave_layout" target_ops method. */ >> >> x86_xsave_layout >> diff --git a/gdb/gdbarch-gen.h b/gdb/gdbarch-gen.h >> index ebcff80bb9e..63fab26987f 100644 >> --- a/gdb/gdbarch-gen.h >> +++ b/gdb/gdbarch-gen.h >> @@ -707,8 +707,8 @@ extern void set_gdbarch_memtag_to_string (struct gdbarch *gdbarch, gdbarch_memta >> /* Return true if ADDRESS contains a tag and false otherwise. ADDRESS >> must be either a pointer or a reference type. */ >> >> -typedef bool (gdbarch_tagged_address_p_ftype) (struct gdbarch *gdbarch, struct value *address); >> -extern bool gdbarch_tagged_address_p (struct gdbarch *gdbarch, struct value *address); >> +typedef bool (gdbarch_tagged_address_p_ftype) (struct gdbarch *gdbarch, CORE_ADDR address); >> +extern bool gdbarch_tagged_address_p (struct gdbarch *gdbarch, CORE_ADDR address); >> extern void set_gdbarch_tagged_address_p (struct gdbarch *gdbarch, gdbarch_tagged_address_p_ftype *tagged_address_p); >> >> /* Return true if the tag from ADDRESS matches the memory tag for that >> diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c >> index 9319571deba..2d92f604c49 100644 >> --- a/gdb/gdbarch.c >> +++ b/gdb/gdbarch.c >> @@ -3232,7 +3232,7 @@ set_gdbarch_memtag_to_string (struct gdbarch *gdbarch, >> } >> >> bool >> -gdbarch_tagged_address_p (struct gdbarch *gdbarch, struct value *address) >> +gdbarch_tagged_address_p (struct gdbarch *gdbarch, CORE_ADDR address) >> { >> gdb_assert (gdbarch != NULL); >> gdb_assert (gdbarch->tagged_address_p != NULL); >> diff --git a/gdb/gdbarch_components.py b/gdb/gdbarch_components.py >> index 7d913ade621..24e979431b6 100644 >> --- a/gdb/gdbarch_components.py >> +++ b/gdb/gdbarch_components.py >> @@ -1267,7 +1267,7 @@ must be either a pointer or a reference type. >> """, >> type="bool", >> name="tagged_address_p", >> - params=[("struct value *", "address")], >> + params=[("CORE_ADDR", "address")], >> predefault="default_tagged_address_p", >> invalid=False, >> ) >> diff --git a/gdb/printcmd.c b/gdb/printcmd.c >> index 62fbcb98cfb..24eac7c8421 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 (gdbarch, v_addr)) >> + if (target_is_address_tagged (gdbarch, value_as_address (v_addr))) >> { >> /* Fetch the allocation tag. */ >> struct value *tag >> @@ -1268,7 +1268,7 @@ print_value (value *val, const value_print_options &opts) >> /* Returns true if memory tags should be validated. False otherwise. */ >> >> static bool >> -should_validate_memtags (struct value *value) >> +should_validate_memtags (gdbarch *gdbarch, struct value *value) >> { >> gdb_assert (value != nullptr && value->type () != nullptr); >> >> @@ -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_is_address_tagged (gdbarch, value_as_address (value)); >> } >> >> /* Helper for parsing arguments for print_command_1. */ >> @@ -1346,7 +1346,7 @@ print_command_1 (const char *args, int voidprint) >> { >> gdbarch *arch = current_inferior ()->arch (); >> >> - if (should_validate_memtags (val) >> + if (should_validate_memtags (arch, val) >> && !gdbarch_memtag_matches_p (arch, val)) >> { >> /* Fetch the logical tag. */ >> @@ -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); >> if (tag_type == memtag_type::allocation >> - && !gdbarch_tagged_address_p (arch, val)) >> - show_addr_not_tagged (value_as_address (val)); >> + && !target_is_address_tagged (arch, addr)) >> + 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_is_address_tagged (current_inferior ()->arch (), addr)) >> + show_addr_not_tagged (addr); >> } >> >> /* 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_is_address_tagged (current_inferior ()-> arch(), addr)) >> + show_addr_not_tagged (addr); >> >> if (!gdbarch_set_memtags (current_inferior ()->arch (), val, length, tags, >> memtag_type::allocation)) >> @@ -3157,12 +3160,12 @@ memory_tag_check_command (const char *args, int from_tty) >> struct value *val = process_print_command_args (args, &print_opts, true); >> gdbarch *arch = current_inferior ()->arch (); >> >> + CORE_ADDR addr = value_as_address (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 (arch, val)) >> - show_addr_not_tagged (value_as_address (val)); >> - >> - CORE_ADDR addr = value_as_address (val); >> + if (!target_is_address_tagged (current_inferior ()->arch (), addr)) >> + show_addr_not_tagged (addr); > > I noticed the above checks happen at least 3 times in the code. Maybe a future > cleanup possibility to split the check into a separate function. > >> >> /* Check if the tag is valid. */ >> if (!gdbarch_memtag_matches_p (arch, val)) >> diff --git a/gdb/remote.c b/gdb/remote.c >> index e278711df7b..9717db55e27 100644 >> --- a/gdb/remote.c >> +++ b/gdb/remote.c >> @@ -1084,6 +1084,8 @@ class remote_target : public process_stratum_target >> bool store_memtags (CORE_ADDR address, size_t len, >> const gdb::byte_vector &tags, int type) override; >> >> + bool is_address_tagged (gdbarch *gdbarch, CORE_ADDR address) override; >> + >> public: /* Remote specific methods. */ >> >> void remote_download_command_source (int num, ULONGEST addr, >> @@ -15573,6 +15575,14 @@ remote_target::store_memtags (CORE_ADDR address, size_t len, >> return packet_check_result (rs->buf).status () == PACKET_OK; >> } >> >> +/* Implement the "is_address_tagged" target_ops method. */ >> + >> +bool >> +remote_target::is_address_tagged (gdbarch *gdbarch, CORE_ADDR address) >> +{ >> + return gdbarch_tagged_address_p (gdbarch, address); >> +} >> + >> /* Return true if remote target T is non-stop. */ >> >> bool >> diff --git a/gdb/target-delegates.c b/gdb/target-delegates.c >> index 59ea70458ad..e322bbbe481 100644 >> --- a/gdb/target-delegates.c >> +++ b/gdb/target-delegates.c >> @@ -197,6 +197,7 @@ struct dummy_target : public target_ops >> bool supports_memory_tagging () override; >> bool fetch_memtags (CORE_ADDR arg0, size_t arg1, gdb::byte_vector &arg2, int arg3) override; >> bool store_memtags (CORE_ADDR arg0, size_t arg1, const gdb::byte_vector &arg2, int arg3) override; >> + bool is_address_tagged (gdbarch *arg0, CORE_ADDR arg1) override; >> x86_xsave_layout fetch_x86_xsave_layout () override; >> }; >> >> @@ -373,6 +374,7 @@ struct debug_target : public target_ops >> bool supports_memory_tagging () override; >> bool fetch_memtags (CORE_ADDR arg0, size_t arg1, gdb::byte_vector &arg2, int arg3) override; >> bool store_memtags (CORE_ADDR arg0, size_t arg1, const gdb::byte_vector &arg2, int arg3) override; >> + bool is_address_tagged (gdbarch *arg0, CORE_ADDR arg1) override; >> x86_xsave_layout fetch_x86_xsave_layout () override; >> }; >> >> @@ -4562,6 +4564,34 @@ debug_target::store_memtags (CORE_ADDR arg0, size_t arg1, const gdb::byte_vector >> return result; >> } >> >> +bool >> +target_ops::is_address_tagged (gdbarch *arg0, CORE_ADDR arg1) >> +{ >> + return this->beneath ()->is_address_tagged (arg0, arg1); >> +} >> + >> +bool >> +dummy_target::is_address_tagged (gdbarch *arg0, CORE_ADDR arg1) >> +{ >> + tcomplain (); >> +} >> + >> +bool >> +debug_target::is_address_tagged (gdbarch *arg0, CORE_ADDR arg1) >> +{ >> + gdb_printf (gdb_stdlog, "-> %s->is_address_tagged (...)\n", this->beneath ()->shortname ()); >> + bool result >> + = this->beneath ()->is_address_tagged (arg0, arg1); >> + gdb_printf (gdb_stdlog, "<- %s->is_address_tagged (", this->beneath ()->shortname ()); >> + target_debug_print_gdbarch_p (arg0); >> + gdb_puts (", ", gdb_stdlog); >> + target_debug_print_CORE_ADDR (arg1); >> + gdb_puts (") = ", gdb_stdlog); >> + target_debug_print_bool (result); >> + gdb_puts ("\n", gdb_stdlog); >> + return result; >> +} >> + >> x86_xsave_layout >> target_ops::fetch_x86_xsave_layout () >> { >> diff --git a/gdb/target.c b/gdb/target.c >> index 107a84b3ca1..5c3c1a57dbd 100644 >> --- a/gdb/target.c >> +++ b/gdb/target.c >> @@ -796,6 +796,12 @@ target_store_memtags (CORE_ADDR address, size_t len, >> return current_inferior ()->top_target ()->store_memtags (address, len, tags, type); >> } >> >> +bool >> +target_is_address_tagged (gdbarch *gdbarch, CORE_ADDR address) >> +{ >> + return current_inferior ()->top_target ()->is_address_tagged (gdbarch, address); >> +} >> + >> x86_xsave_layout >> target_fetch_x86_xsave_layout () >> { >> diff --git a/gdb/target.h b/gdb/target.h >> index c9eaff16346..486a0a687b0 100644 >> --- a/gdb/target.h >> +++ b/gdb/target.h >> @@ -1334,6 +1334,10 @@ struct target_ops >> const gdb::byte_vector &tags, int type) >> TARGET_DEFAULT_NORETURN (tcomplain ()); >> >> + /* Returns true if ADDRESS is tagged, otherwise returns false. */ >> + virtual bool is_address_tagged (gdbarch *gdbarch, CORE_ADDR address) >> + TARGET_DEFAULT_NORETURN (tcomplain ()); >> + > > Should the default for is_address_tagged by to return false, meaning the address is never > tagged? No, I think tcomplain() is correct. This is the virtual (default) declaration, so it will be called if the target layer does not implement/override this method in the base class, and tcomplain() will correctly complain: "You can't do that when your target is `%s`" But this should be a rare condition, when GDB understands that the target supports memory tagging and so will call target_is_address_tagged at some point but the target layer does not implement the target hook. Cheers, Gustavo