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 549413858C56 for ; Thu, 21 Dec 2023 21:08:20 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 549413858C56 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=simark.ca Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=simark.ca ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 549413858C56 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=158.69.221.121 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1703192901; cv=none; b=wwEhkcbFdDrkS8l1WfCJ/J4OXEwPFP+IsvrzafN97CM99yNnSCo6TKEHR5tZ+txHscC1N8Vkbp7jSzhwNDkxp+HJXz4CkbaCrLUvUKzdqwPwKstYhpbf0f/ge+9YiJzqhLGPgEZtN1U23gLlijeCfFuACK8Ygz+t3Gjh8Jj8ffs= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1703192901; c=relaxed/simple; bh=Sf8wHxUJdO7iIb9uweOyD3dinE5bn6jn9QdpaKJ0uh8=; h=DKIM-Signature:Message-ID:Date:MIME-Version:Subject:To:From; b=We0ik68PPA4FC0DFjWyDb4B7aeVDqiaikyL518rqfguQ14O7FXfnaBc7cQUltJp8d/D/JtqF2JWnRCkaRwTUxpy7apMqVgsxZp9W9XEytgpxujrkgXHpXmaj/m38HmbEot8EoDFg/kIDQmyPx767TRloqSuu/7ji/KET99b2c9A= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=simark.ca; s=mail; t=1703192899; bh=Sf8wHxUJdO7iIb9uweOyD3dinE5bn6jn9QdpaKJ0uh8=; h=Date:Subject:To:References:From:In-Reply-To:From; b=p1dB//YE+FXAOZPVQRbVhInS7SsmRnSdkeEZeqhjw7+oq016Z+mAZuN/PzrGEjXHi GTZgYEPO9jjcmwWU/2nCHSnPJtgaQ5u6XFS88AmaII3Jgmqm9WTOoVHOwRTyqmfwh2 Von8N5fR0Nj8vftFBnyyUWOjjtCb0SCTQNEL4A4g= Received: from [172.16.0.192] (192-222-143-198.qc.cable.ebox.net [192.222.143.198]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature ECDSA (prime256v1) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id E21111E0AC; Thu, 21 Dec 2023 16:08:19 -0500 (EST) Message-ID: Date: Thu, 21 Dec 2023 16:08:19 -0500 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH 09/26] gdbserver: extract out regcache::invalidate and regcache::discard Content-Language: fr To: Tankut Baris Aktemur , gdb-patches@sourceware.org References: <80ed5ae6e1976610b2dfcf26ebe32f9af4aa9b33.1677582744.git.tankut.baris.aktemur@intel.com> From: Simon Marchi In-Reply-To: <80ed5ae6e1976610b2dfcf26ebe32f9af4aa9b33.1677582744.git.tankut.baris.aktemur@intel.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-10.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,SPF_HELO_PASS,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE 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: On 2/28/23 06:28, Tankut Baris Aktemur via Gdb-patches wrote: > Extract out a piece of code from the `regcache_invalidate_thread` > function and turn into a new method of regcache named 'invalidate'. For clarity, if you can think of a better name than invalidate, it would be nice. To me, invalidate sounds like we just mark the registers as stale (what you discard method does, basically). This method writes the registers back to the target and invalidates / discard. > We also introduce a small method named 'discard' to give the clients > an option to discard the cache without storing the contents. This > method is utilized in a downstream debugger. I would perhaps suggest keeping the discard method as a downstream modification, since it's trivial (and therefore doesn't need much maintenance effort to keep up to date). > --- > gdbserver/regcache.cc | 24 +++++++++++++++++------- > gdbserver/regcache.h | 6 ++++++ > 2 files changed, 23 insertions(+), 7 deletions(-) > > diff --git a/gdbserver/regcache.cc b/gdbserver/regcache.cc > index 7b6337166ad..1db78951cc2 100644 > --- a/gdbserver/regcache.cc > +++ b/gdbserver/regcache.cc > @@ -85,18 +85,22 @@ regcache_invalidate_thread (struct thread_info *thread) > > regcache = thread_regcache_data (thread); > > - if (regcache == NULL) > - return; > + if (regcache != nullptr) > + regcache->invalidate (); > +} > > - if (regcache->registers_valid) > +void > +regcache::invalidate () Add the typical: /* See regcache.h. */ > +{ > + if (registers_valid) > { > scoped_restore_current_thread restore_thread; > - > - switch_to_thread (thread); > - store_inferior_registers (regcache, -1); > + gdb_assert (this->thread != nullptr); > + switch_to_thread (this->thread); > + store_inferior_registers (this, -1); Ok, so I note that this is another place where you use regcache::thread (which makes sense, it's the mirror operation to fetch). I guess that the same idea could be use there, it could be either a method on thread_info, or event stay a free function. Similar comment as earlier, as a general improvement, it would be nice if we could get rid of this switch_to_thread. > } > > - regcache->registers_valid = 0; > + discard (); Why is it necessary to mark the register content invalid here? In theory, it should be valid as long as we don't resume the thread. It's already done like that, so it's fine to keep it as-is, I'm just wondering. Simon