From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 121033 invoked by alias); 21 Oct 2016 22:42:49 -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 121023 invoked by uid 89); 21 Oct 2016 22:42:48 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=3.6 required=5.0 tests=AWL,BAYES_00,GARBLED_SUBJECT,RCVD_IN_DNSWL_NONE,RP_MATCHES_RCVD,SPF_PASS autolearn=no version=3.3.2 spammy=scenarios, choices, bhushan, invalidated X-HELO: mailapp01.imgtec.com Received: from mailapp01.imgtec.com (HELO mailapp01.imgtec.com) (195.59.15.196) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 21 Oct 2016 22:42:47 +0000 Received: from HHMAIL01.hh.imgtec.org (unknown [10.100.10.19]) by Forcepoint Email with ESMTPS id 35B9CF39E46A2 for ; Fri, 21 Oct 2016 23:42:39 +0100 (IST) Received: from [10.20.78.168] (10.20.78.168) by HHMAIL01.hh.imgtec.org (10.100.10.21) with Microsoft SMTP Server id 14.3.294.0; Fri, 21 Oct 2016 23:42:43 +0100 Date: Fri, 21 Oct 2016 22:42:00 -0000 From: "Maciej W. Rozycki" To: Bhushan Attarde CC: , , , , Subject: Re: [PATCH 03/24] regcache: handle invalidated regcache In-Reply-To: <1467038991-6600-3-git-send-email-bhushan.attarde@imgtec.com> Message-ID: References: <1467038991-6600-1-git-send-email-bhushan.attarde@imgtec.com> <1467038991-6600-3-git-send-email-bhushan.attarde@imgtec.com> User-Agent: Alpine 2.00 (DEB 1167 2008-08-23) MIME-Version: 1.0 Content-Type: text/plain; charset="US-ASCII" X-SW-Source: 2016-10/txt/msg00649.txt.bz2 On Mon, 27 Jun 2016, Bhushan Attarde wrote: > When registers are marked as change, set a new regcache_invalidated > variable which is used by get_thread_regcache to decide whether to > recreate the gdbarch. Can you please be a bit more specific as to why this change is needed? > gdb/ChangeLog: > * regcache.c (regcache_invalidated): New variable. > (set_current_thread_ptid_arch): Use regcache_invalidated to > set registers_changed_p. > (get_thread_regcache): Reset regcache_invalidated to 0. > (registers_changed_ptid): Set regcache_invalidated to 1. Single-tab indentation please. But overall it looks like a bug fix to me, applying to 01/24 and addressing the assumption made there that there is only a single thread being debugged. Which may have been OK for the majority of bare-metal cases, but certainly not for OS debugging. So please fold this change into 01/24. But I have a concern about the implementation too. > diff --git a/gdb/regcache.c b/gdb/regcache.c > index c3405b6..3cbec6e 100644 > --- a/gdb/regcache.c > +++ b/gdb/regcache.c > @@ -489,6 +489,7 @@ struct regcache_list > }; > > static struct regcache_list *current_regcache; > +static int regcache_invalidated = 1; > > struct regcache * > get_thread_arch_aspace_regcache (ptid_t ptid, struct gdbarch *gdbarch, > @@ -547,7 +548,7 @@ set_current_thread_ptid_arch (ptid_t ptid) > struct regcache * > get_thread_regcache (ptid_t ptid) > { > - int registers_changed_p = current_regcache == NULL; > + int registers_changed_p = current_regcache == NULL || regcache_invalidated; > struct regcache *new_regcache; > > set_current_thread_ptid_arch (ptid); > @@ -560,6 +561,7 @@ get_thread_regcache (ptid_t ptid) > registers_changed (); > set_current_thread_ptid_arch (ptid); > new_regcache = get_thread_arch_regcache (ptid, current_thread_arch); > + regcache_invalidated = 0; > } > > return new_regcache; > @@ -646,6 +648,7 @@ registers_changed_ptid (ptid_t ptid) > forget about any frames we have cached, too. */ > reinit_frame_cache (); > } > + regcache_invalidated = 1; > } This uses a global `regcache_invalidated' flag for what looks to me like a thread-specific case, i.e. you really want to set `registers_changed_p' above only if `ptid' thread's registers have changed. Or in other words if the old regcache was gone and a new one has been allocated for `ptid' requested. Yes, with the MIPS FPU topology changes in particular this may not matter much, but let's get this piece right for the general case, like multiprocess scenarios perhaps. So it looks to me like a better approach will be to either scan the regcache list from `current_regcache' to see if there's a match for `ptid' or to make `get_thread_arch_regcache' return additional status of the scan it already does. I have no clear preference between these two choices, so please pick up whichever you like better or maybe a general maintainer can speak out. Maciej