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 D64ED3851894 for ; Mon, 28 Nov 2022 15:47:13 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org D64ED3851894 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 [10.0.0.11] (unknown [217.28.27.60]) (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 6C19B1E0CB; Mon, 28 Nov 2022 10:47:13 -0500 (EST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=simark.ca; s=mail; t=1669650433; bh=GTzh3N4Umv0yS9W7likfweGcDH+p5XMob0v4pltXh6o=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=cLGCF4bVyarGXsMBc3trD5SVtTovn8hyyG3zLTxwlsQ3ZiPxr0Jtooi1kc+HLyylk L7yhO2Gyp9AkLNLx3AlBLnOzF2dxfPLmxFAmhDMA0mqBRvtCw9IRJyRJ6IwOlZrKFp xmtzH2hv2FU4W+CTWn8MKdJS6AXogng3b94hrYTA= Message-ID: <3539acb1-462d-62e3-5a70-40786942c325@simark.ca> Date: Mon, 28 Nov 2022 10:47:12 -0500 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.4.2 Subject: Re: [PATCH v2 4/6] gdbserver/linux-aarch64: When thread stops, update its target description Content-Language: en-US To: Thiago Jung Bauermann , gdb-patches@sourceware.org Cc: Luis Machado References: <20221126020452.1686509-1-thiago.bauermann@linaro.org> <20221126020452.1686509-5-thiago.bauermann@linaro.org> From: Simon Marchi In-Reply-To: <20221126020452.1686509-5-thiago.bauermann@linaro.org> 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/gdbserver/linux-aarch64-low.cc b/gdbserver/linux-aarch64-low.cc > index cab4fc0a4674..786ce4071279 100644 > --- a/gdbserver/linux-aarch64-low.cc > +++ b/gdbserver/linux-aarch64-low.cc > @@ -99,6 +99,9 @@ protected: > > void low_arch_setup () override; > > + gdb::optional > + arch_update_tdesc (const thread_info *thread) override; I'm all for using optional to be clear and explicit in general, but unless an empty optional and an optional wrapping nullptr are both valid and have different meanings (which doesn't seem, to be the case here), I wouldn't recommend wrapping a pointer in an optional. Pointers have a dedicated value for "no value", that is well understood by everyone. And then, it does create the possibility of returning an optional wrapping nullptr, which typically won't be a legitimate value. So it's just one more thing to worry about. > @@ -840,8 +845,16 @@ aarch64_target::low_arch_setup () > { > struct aarch64_features features > = aarch64_get_arch_features (current_thread); > + const target_desc *tdesc = aarch64_linux_read_description (features); > > - current_process ()->tdesc = aarch64_linux_read_description (features); > + /* Only SVE-enabled inferiors need per-thread target descriptions. */ > + if (features.vq > 0) > + { > + current_thread->tdesc = tdesc; > + current_process ()->priv->arch_private->has_sve = true; > + } Is it not possible for a process to start with SVE disabled (vq == 0) and have some threads enable it later? > @@ -853,6 +866,28 @@ aarch64_target::low_arch_setup () > aarch64_linux_get_debug_reg_capacity (lwpid_of (current_thread)); > } > > +/* Implementation of linux target ops method "arch_update_tdesc". */ > + > +gdb::optional > +aarch64_target::arch_update_tdesc (const thread_info *thread) > +{ > + /* Only processes using SVE need to update the thread's target description. */ > + if (!get_thread_process (thread)->priv->arch_private->has_sve) > + return {}; > + > + const struct aarch64_features features = aarch64_get_arch_features (thread); > + const struct target_desc *tdesc = aarch64_linux_read_description (features); > + > + if (tdesc == thread->tdesc) > + return {}; > + > + /* Adjust the register sets we should use for this particular set of > + features. */ > + aarch64_adjust_register_sets(features); > + > + return tdesc; Naming nit: I don't think we need "update" in the method name, there's no "updating component" to it AFAICT. It's basically "get this thread's tdesc if for some reason it differents from the process-wide we have". So, I don't know, "get_thread_tdesc" or just "thread_tdesc". > @@ -2348,6 +2354,18 @@ linux_process_target::filter_event (int lwpid, int wstat) > return; > } > } > + else > + { > + /* Give the arch code an opportunity to update the thread's target > + description. */ > + gdb::optional new_tdesc > + = arch_update_tdesc (thread); > + if (new_tdesc.has_value ()) > + { > + regcache_release (); Hmm, regcache_release frees the regcache for all threads. Can we free the regcache only for this thread? Simon