From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id 46DAA3948A71 for ; Mon, 5 Dec 2022 20:36:35 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 46DAA3948A71 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=simark.ca Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=simark.ca Received: from [172.16.0.64] (192-222-180-24.qc.cable.ebox.net [192.222.180.24]) (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 E7C561E112; Mon, 5 Dec 2022 15:36:34 -0500 (EST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=simark.ca; s=mail; t=1670272595; bh=t7RgQ8PG5MIvDYmF2Y0ejoOMYVCda/fahegoMQ5u7Gg=; h=Date:Subject:To:References:From:In-Reply-To:From; b=HpPq4dACQOmexMNvrBOqw1YU80op0vm/9e3JIK5Z6mvQztHBim4EOkvUc9IsTs8JM JZM0ZBEHMVWlg2AREOsvi/YXMxkDe4fNBAdaa9s8QbtsM6NzFjUzs/ljFgYHsfePuB K9y82MyFXzzYSXd6Sm7/VM1m1Re1/Dm31O8kjW+Q= Message-ID: <41c544e4-1832-d4c4-9ea4-b0d034ac67d2@simark.ca> Date: Mon, 5 Dec 2022 15:36:34 -0500 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.4.1 Subject: Re: [PATCH] [AArch64] Add TPIDR2 register support for Linux Content-Language: fr To: Luis Machado , gdb-patches@sourceware.org References: <20220923134648.114930-1-luis.machado@arm.com> From: Simon Marchi In-Reply-To: <20220923134648.114930-1-luis.machado@arm.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-11.2 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,NICE_REPLY_A,SPF_HELO_PASS,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 List-Id: > diff --git a/gdb/aarch64-linux-nat.c b/gdb/aarch64-linux-nat.c > index eda79ec6d35..03eec37a6c4 100644 > --- a/gdb/aarch64-linux-nat.c > +++ b/gdb/aarch64-linux-nat.c > @@ -442,19 +442,19 @@ fetch_tlsregs_from_thread (struct regcache *regcache) > = gdbarch_tdep (regcache->arch ()); > int regno = tdep->tls_regnum; > > - gdb_assert (regno != -1); > + gdb_assert (regno != -1 && tdep->tls_register_count > 0); Check just one thing per gdb_assert. > @@ -786,7 +790,16 @@ aarch64_linux_core_read_description (struct gdbarch *gdbarch, > features.vq = aarch64_linux_core_read_vq (gdbarch, abfd); > features.pauth = hwcap & AARCH64_HWCAP_PACA; > features.mte = hwcap2 & HWCAP2_MTE; > - features.tls = tls != nullptr; > + > + /* Handle the TLS section. */ > + asection *tls = bfd_get_section_by_name (abfd, ".reg-aarch-tls"); > + if (tls != nullptr) > + { > + size_t size = bfd_section_size (tls); > + /* Convert the size to the number of actual registers, by > + dividing by 8. */ > + features.tls = size >> 3; Why not write it as `size / 8`? Or, `size / AARCH64_TLS_REGISTER_SIZE`? > @@ -3544,13 +3555,26 @@ aarch64_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches) > /* Add the TLS register. */ > if (feature_tls != nullptr) > { > - tls_regnum = num_regs; > - /* Validate the descriptor provides the mandatory TLS register > - and allocate its number. */ > - valid_p = tdesc_numbered_register (feature_tls, tdesc_data.get (), > - tls_regnum, "tpidr"); > + const char *tls_register_names[2] = { "tpidr", "tpidr2" }; > + first_tls_regnum = num_regs; > + > + /* Look for the TLS registers. */ > + for (i = 0; i < ARRAY_SIZE (tls_register_names); i++) > + { > + valid_p > + = tdesc_numbered_register (feature_tls, tdesc_data.get (), > + first_tls_regnum + tls_register_count, > + tls_register_names[i]); > + if (valid_p) > + tls_register_count++; > + } > + valid_p = true; It would seem better to just use a different variable inside the loop, if you don't want it to affect valid_p. If valid_p is false before entering the loop, you still set it to true afterwards, is that what you want? It seems to me like you would want the lookup of tpidr to affect valid_p, as it would be invalid to have the tls feature without tpidr, but the lookup of tpidr2 to not affect valid_p. > diff --git a/gdb/aarch64-tdep.h b/gdb/aarch64-tdep.h > index d8513023c37..8166df0ada8 100644 > --- a/gdb/aarch64-tdep.h > +++ b/gdb/aarch64-tdep.h > @@ -111,8 +111,9 @@ struct aarch64_gdbarch_tdep : gdbarch_tdep_base > return mte_reg_base != -1; > } > > - /* TLS register. This is -1 if the TLS register is not available. */ > + /* TLS registers. This is -1 if the TLS registers are not available. */ > int tls_regnum = 0; > + int tls_register_count = 0; Perhaps rename tls_regnum to tls_regnum_base, since it not longer represents just one register. > diff --git a/gdb/nat/aarch64-linux.c b/gdb/nat/aarch64-linux.c > index 421d1ecb53c..017c30948a9 100644 > --- a/gdb/nat/aarch64-linux.c > +++ b/gdb/nat/aarch64-linux.c > @@ -250,3 +250,20 @@ aarch64_ps_get_thread_area (struct ps_prochandle *ph, > > return PS_OK; > } > + > +/* See nat/aarch64-linux.h */ Dot and double space. > + > +int > +aarch64_tls_register_count (int tid) > +{ > + uint64_t tls_regs[2]; > + struct iovec iovec; > + iovec.iov_base = tls_regs; > + iovec.iov_len = sizeof (tls_regs); > + > + if (ptrace (PTRACE_GETREGSET, tid, NT_ARM_TLS, &iovec) != 0) > + return 1; > + > + /* TPIDR2 is available. */ > + return 2; Can you explain what is happening here? How does checking for success reading NT_ARM_TLS telly ou that TPIDR2 exists? Simon