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 36A153858CDA for ; Fri, 22 Dec 2023 04:32:35 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 36A153858CDA 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 36A153858CDA 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=1703219556; cv=none; b=OWQib1qAbSrZF5XucwFzu8xYBCPdqD1CRtYHvUCqT2OaT9iYACkypADJHJ1NqYT9kSParJ2GEvuYWC8jF8Z5njhMQwt5OxKVaWtqGiUyyvh5dZrhVratMXs3utGkvrbaXy+5HomDEYXDxIgsRjbh4RMmJ9q7bq+WyHjiJR+Qn8M= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1703219556; c=relaxed/simple; bh=s5rkVq1+lHBKcHhbnOO3fNy5WNUoP3/0e1/ZJoEYN9s=; h=DKIM-Signature:Message-ID:Date:MIME-Version:Subject:To:From; b=Coy5hQ8gWWsYdGevYcaJTHeifqKITJr8ZO0drYwgr75HLi16hpWtsCp3N/rP8TxflnjpnIbssTlCV09fpuWEbzB02hvWgO56TwgHQzeYAySn2DI+Qv0Y6/SXKGeZE8auiZWfHCW6HSvUsp0kPL/S+gda/i5Ajr3a2pHFloPgB+g= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=simark.ca; s=mail; t=1703219554; bh=s5rkVq1+lHBKcHhbnOO3fNy5WNUoP3/0e1/ZJoEYN9s=; h=Date:Subject:To:References:From:In-Reply-To:From; b=tCDU1zda12LNd8FUe9qFYJKbDkFzjFhUOTCaKai7NL5IPhba/Npbog2/ugPE6/XDL EE5LNLeIScI+a+EM6Xdr1doI4wxlzpZHOP8/1wBSoPLYflTG42T02Ijd/DcXqt2Sxq CN90nzl8EUT7AbUa5ZYds2bVtXH2RzeoJPv2TPgU= Received: from [10.0.0.11] (modemcable238.237-201-24.mc.videotron.ca [24.201.237.238]) (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 CD1D71E091; Thu, 21 Dec 2023 23:32:34 -0500 (EST) Message-ID: <8aa2a7c6-4966-4278-a450-f0ede240012c@simark.ca> Date: Thu, 21 Dec 2023 23:32:34 -0500 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH 21/26] gdbserver: use REG_UNKNOWN for a regcache's register statuses Content-Language: en-US To: Tankut Baris Aktemur , gdb-patches@sourceware.org References: <1fcffbf8ffd62b07585baebff38b66f10ec0a112.1677582745.git.tankut.baris.aktemur@intel.com> From: Simon Marchi In-Reply-To: <1fcffbf8ffd62b07585baebff38b66f10ec0a112.1677582745.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 2023-02-28 06:28, Tankut Baris Aktemur via Gdb-patches wrote: > When a regcache is initialized, the values of registers are not > fetched yet. Thus, initialize the register statuses to REG_UNKNOWN > instead of REG_UNAVAILABLE, because the latter rather means "we > attempted to fetch but could not obtain the value". > > The definitions of the reg status enums (from > gdbsupport/common-regcache.h) as a reminder: > > /* The register value is not in the cache, and we don't know yet > whether it's available in the target (or traceframe). */ > REG_UNKNOWN = 0, > > /* The register value is valid and cached. */ > REG_VALID = 1, > > /* The register value is unavailable. E.g., we're inspecting a > traceframe, and this register wasn't collected. Note that this > "unavailable" is different from saying the register does not > exist in the target's architecture --- in that case, the target > should have given us a target description that does not include > the register in the first place. */ > REG_UNAVAILABLE = -1 > > Similarly, when the regcache is invalidated, change all the statuses > back to REG_UNKNOWN. That makes sense to me. > --- > gdbserver/regcache.cc | 16 +++++++++++++--- > 1 file changed, 13 insertions(+), 3 deletions(-) > > diff --git a/gdbserver/regcache.cc b/gdbserver/regcache.cc > index 09ea58bdbd6..2befb30e337 100644 > --- a/gdbserver/regcache.cc > +++ b/gdbserver/regcache.cc > @@ -64,9 +64,17 @@ regcache::fetch () > switch_to_thread (this->thread); > > /* Invalidate all registers, to prevent stale left-overs. */ > - memset (register_status, REG_UNAVAILABLE, tdesc->reg_defs.size ()); > + discard (); > fetch_inferior_registers (this, -1); > registers_fetched = true; > + > + /* Make sure that the registers that could not be fetched are > + now unavailable. */ > + for (int i = 0; i < tdesc->reg_defs.size (); ++i) > + { > + if (register_status[i] == REG_UNKNOWN) > + set_register_status (i, REG_UNAVAILABLE); > + } The braces are not needed. But is it really needed to do this? Unavailable is only meaningful for tracing, in a regcache that is the result of tracepoint collection. For a regcache created by reading registers from the target, I don't think that setting statuses to unavailable makes sense. Also, if asking the target to fetch all registers, why would some registers still be unknown? It's the target that provides the target description, it's supposed to only include registers that exist (like the comment on top of REG_UNAVAILABLE says). So it should be able to read them all. In other words, I think this loop should be asserting that all statuses are REG_VALID. But then for the tracing case, I wonder who sets the statuses to REG_UNAVAILABLE for those statuses that should really be REG_UNAVAILABLE. From what I can see, all regcaches used on the tracing code paths use the regcache constructor with 2 arguments, which means regcaches that don't track the status of registers (regcache::register_status is nullptr). So... now that you changed regcache to use REG_UNKNOWN by default, is REG_UNAVAILABLE really used in GDBserver? I noticed that do_action_at_tracepoint has a comment that says "Collect all registers for now.". I guess that if it didn't collect all registers, this would be a spot that would explicitly set some registers to REG_UNAVAILABLE. I see that for fast tracepoints, for instance supply_fast_tracepoint_registers in linux-amd64-ipa.cc, we supply a fixed set of registers. Clearly, these are not all the registers that an amd64 machine could have. But it just writes a register buffer that ends up in the trace, there doesn't seem to be a way to communicate which registers were collected and which weren't. When we create a new regcache to read these registers, when handling the 'g' packet in server.cc, there's no way to know which individual registers were collected and which weren't. Simon