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 09C113858D33 for ; Fri, 22 Dec 2023 04:54:49 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 09C113858D33 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 09C113858D33 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=1703220890; cv=none; b=bai2txh54oo6/4G68dC82z4AdLqQhWkrhnuGoDwB7IEcZ8kUBb3Pqe4cSkzDtHws9CKq6/XwqV4wZPs9Y48ztjRABSOgAvpZzJ1M/xeav2t1AOPnfsrgLq/LY5GyiikijXxcn9j6CJMe/oi3b59p0S0u+qhRVTUdutVSmq4jHjM= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1703220890; c=relaxed/simple; bh=p+8BFFy/0gk/vaDXm+oqXjos9BN7rteL1up1pLnvUF8=; h=DKIM-Signature:Message-ID:Date:MIME-Version:Subject:To:From; b=UJeukmxvTnFG4WxaLIY/W+EIzeGxsZCGj3rS4A/WS//7FRUNt5vylj8RFRWAY28Q8wFFFzehT+ff/0bhFmELg1TaUTJ3D6fGyIKWwDgWmOfqr5cKmCJjC9/UxTPdyl5IqSEyFaEfAgeP8VKFmIXdpyGshJaDgezGoOgOqy9FJ5c= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=simark.ca; s=mail; t=1703220888; bh=p+8BFFy/0gk/vaDXm+oqXjos9BN7rteL1up1pLnvUF8=; h=Date:Subject:To:References:From:In-Reply-To:From; b=sdAxYE5a6Af96GvgwqJvqCjFhKn1PjrMYq5CtixjOuDWt4pt7UoPT1zcxGmN24YTS NKRUME5GJyr30PoQA/+ReyzmL6D4Bt2CbK/OOiHgFHSp6cu5xOtQHsJ+5Vh7xMLaFt +nnA0T/LP+pw4/pgTAMi1znItpsK0Qj1lC5NFyCA= 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 58D671E091; Thu, 21 Dec 2023 23:54:48 -0500 (EST) Message-ID: <1906794c-8f34-4d6e-9b41-be14ce74e96e@simark.ca> Date: Thu, 21 Dec 2023 23:54:48 -0500 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH 25/26] gdbserver: refuse null argument in regcache::supply_regblock Content-Language: en-US To: Tankut Baris Aktemur , gdb-patches@sourceware.org References: <39a4774140afc2f7253756971398fc2cabceb289.1677582745.git.tankut.baris.aktemur@intel.com> From: Simon Marchi In-Reply-To: <39a4774140afc2f7253756971398fc2cabceb289.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: > The regcache::supply_regblock method implements two different > behaviors based on whether its 'buf' argument is null. The null > behavior is used only in tracepoint.cc. Refuse the null pointer > argument and use the 'discard' method to obtain that second behavior. > > Note that the original code resets register statuses to > REG_UNAVAILABLE, but 'discard' sets them to REG_UNKNOWN. For the > current purposes of resetting the regcache, the difference does not > seem to be important. Ah, glad to see this, it matches one of my comment on an earlier patch (and as you can see, I read patch series in a very linear way with no lookahead). In the spots inside the in-process agent, the regcache doesn't even track statuses, so it doesn't make a difference indeed. > diff --git a/gdbserver/tracepoint.cc b/gdbserver/tracepoint.cc > index 9833e7c3b0f..9622d53cd82 100644 > --- a/gdbserver/tracepoint.cc > +++ b/gdbserver/tracepoint.cc > @@ -4707,7 +4707,7 @@ get_context_regcache (struct tracepoint_hit_ctx *ctx) > { > fctx->regcache_initted = 1; > fctx->regcache.initialize (ipa_tdesc, fctx->regspace); > - fctx->regcache.supply_regblock (nullptr); > + fctx->regcache.discard (); > supply_fast_tracepoint_registers (&fctx->regcache, fctx->regs); I think the call to discard is pointless. The call to discard does (and should) pretty much bring the regcache back to its state just after initialization. In this case, it was initialized at the line just before. > } > regcache = &fctx->regcache; > @@ -4722,7 +4722,7 @@ get_context_regcache (struct tracepoint_hit_ctx *ctx) > { > sctx->regcache_initted = 1; > sctx->regcache.initialize (ipa_tdesc, sctx->regspace); > - sctx->regcache.supply_regblock (nullptr); > + sctx->regcache.discard (); Same here. > /* Pass down the tracepoint address, because REGS doesn't > include the PC, but we know what it must have been. */ > supply_static_tracepoint_registers (&sctx->regcache, > @@ -5179,8 +5179,8 @@ fetch_traceframe_registers (int tfnum, struct regcache *regcache, int regnum) > dataptr = traceframe_find_regblock (tframe, tfnum); > if (dataptr == NULL) > { > - /* Mark registers unavailable. */ > - regcache->supply_regblock (nullptr); > + /* Wipe out the cache. */ > + regcache->discard (); So, I think this is a spot where you would actually want to make the registers REG_UNAVAILABLE. We are filling a regcache meant to represent the registers that were collected when hitting a given tracepoint. If traceframe_find_regblock returns nullptr, I guess it means that we didn't collect the registers. So all registers are unavailable. In practice, it won't make a difference, because that regcache is then fed to registers_to_string, which will have the same behavior for REG_UNAVAILABLE and REG_UNKNOWN. But if you want to be accurate, I think that REG_UNAVAILABLE makes more sense here. Simon