From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 27756 invoked by alias); 4 Dec 2014 13:33:02 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 27615 invoked by uid 89); 4 Dec 2014 13:33:01 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.0 required=5.0 tests=AWL,BAYES_00,SPF_HELO_PASS,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Thu, 04 Dec 2014 13:32:59 +0000 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id sB4DVrmO022148 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Thu, 4 Dec 2014 08:31:53 -0500 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id sB4DVpSO003132; Thu, 4 Dec 2014 08:31:52 -0500 Message-ID: <54806247.8050000@redhat.com> Date: Thu, 04 Dec 2014 13:33:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.2.0 MIME-Version: 1.0 To: Andreas Arnez CC: gdb-patches@sourceware.org, Ulrich Weigand Subject: Re: [PATCH 2/2] S390: Fix gdbserver support for TDB References: <1417002962-3424-1-git-send-email-arnez@linux.vnet.ibm.com> <1417002962-3424-3-git-send-email-arnez@linux.vnet.ibm.com> <87iohvp77u.fsf@br87z6lw.de.ibm.com> <547DD8C2.9000403@redhat.com> <8761dtq2rx.fsf@br87z6lw.de.ibm.com> <871togppgg.fsf@br87z6lw.de.ibm.com> In-Reply-To: <871togppgg.fsf@br87z6lw.de.ibm.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2014-12/txt/msg00089.txt.bz2 On 12/03/2014 06:18 PM, Andreas Arnez wrote: > On Tue, Dec 02 2014, Andreas Arnez wrote: >> On Tue, Dec 02 2014, Pedro Alves wrote: >>> It probably doesn't hurt to be explicit, but I should note that >>> registers' are unavailable by default on gdbserver, so a >>> 'if (buf == NULL) return;' probably would do as well: >>> >>> struct regcache * >>> init_register_cache (struct regcache *regcache, >>> const struct target_desc *tdesc, >>> unsigned char *regbuf) >>> ... >>> regcache->register_status = xcalloc (1, tdesc->num_registers); >>> gdb_assert (REG_UNAVAILABLE == 0); >> >> In general, if a prior call to fetch_inferior_registers has filled the >> regset already, I'd expect the store function to reset the registers to >> "unavailable" again. Otherwise we may have stale left-overs from >> before. > > Hm, I noticed that this probably deserves some more explanation. > > While it is true that the registers are marked unavailable when > initializing a new regcache, the regcache seems to survive without > another initialization between calls to fetch_inferior_registers. I've > verified this in my tests, and I've also not seen any code that would > perform such a re-initialization. Hmm, good find. > > I wonder why that is the case, and whether we would like to change that. "why" is just that nothing ever stumbled on this. The case that required teaching gdbserver about unavailable registers is tracepoint traceframes, in which case the regcache is always a new one: server.c: case 'g': require_running (own_buf); if (current_traceframe >= 0) { struct regcache *regcache = new_register_cache (current_target_desc ()); > If so, the patch could avoid touching ARM code, wouldn't need special > treatment of NULL in the TDB store function, and would treat ENODATA > like any other error from ptrace, except that the warning would be > suppressed. I think this would also improve the behavior of other > errors from ptrace, but maybe there's a good reason for falling back to > the "last known" register values in this case. Or maybe there's a > performance reason for avoiding the re-initialization? > > For illustration, why don't we do something like the (untested) patch > below? > > -- > diff --git a/gdb/gdbserver/regcache.c b/gdb/gdbserver/regcache.c > index 718ae8c..b0f6a22 100644 > --- a/gdb/gdbserver/regcache.c > +++ b/gdb/gdbserver/regcache.c > @@ -52,6 +52,8 @@ get_thread_regcache (struct thread_info *thread, int fetch) > struct thread_info *saved_thread = current_thread; > > current_thread = thread; > + memset (regcache->register_status, REG_UNAVAILABLE, > + regcache->tdesc->num_registers); This makes sense to me, it's similar to gdb's own handling. See gdb/regcache.c:regcache_raw_read (and regcache_invalidate). Can you check the patch on x86 too, please? You'll need the same #ifdef guard as init_register_cache uses; s390 doesn't build the IPA. > fetch_inferior_registers (regcache, -1); > current_thread = saved_thread; > regcache->registers_valid = 1; Thanks, Pedro Alves