From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 17755 invoked by alias); 1 Mar 2012 19:11:02 -0000 Received: (qmail 17740 invoked by uid 22791); 1 Mar 2012 19:11:00 -0000 X-SWARE-Spam-Status: No, hits=-6.8 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_HI,SPF_HELO_PASS,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 01 Mar 2012 19:10:43 +0000 Received: from int-mx12.intmail.prod.int.phx2.redhat.com (int-mx12.intmail.prod.int.phx2.redhat.com [10.5.11.25]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q21JAU4l016797 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Thu, 1 Mar 2012 14:10:39 -0500 Received: from mesquite.lan (ovpn-113-100.phx2.redhat.com [10.3.113.100]) by int-mx12.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id q21IJRVL026774 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Thu, 1 Mar 2012 13:19:28 -0500 Date: Thu, 01 Mar 2012 19:11:00 -0000 From: Kevin Buettner To: gdb-patches@sourceware.org Cc: Thomas Schwinge Subject: Re: [RFC] sh-tdep.c: Don't fetch FPSCR register if it doesn't exist Message-ID: <20120301111926.14b1025b@mesquite.lan> In-Reply-To: <877gz44nre.fsf@schwinge.name> References: <20120229173408.78699013@mesquite.lan> <20120301013318.GJ3118@adacore.com> <20120229220932.17ad0d5f@mesquite.lan> <877gz44nre.fsf@schwinge.name> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-IsSubscribed: yes 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 X-SW-Source: 2012-03/txt/msg00034.txt.bz2 On Thu, 01 Mar 2012 11:30:13 +0100 Thomas Schwinge wrote: > On Wed, 29 Feb 2012 22:09:32 -0700, Kevin Buettner wrote: > > * sh-tdep.c (sh_frame_cache): Don't fetch the FPSCR register > > unless it exists for this architecture. > > Your reasoning and this patch look good to me. (But I didn't test it.) > Basically, you now relay the test to sh_register_reggroup_p, which > already contains the FPSCR existence test you first proposed. (The same > test exists in arch-utils.c:legacy_register_sim_regno, by the way.) Thanks for looking it over. Yes, that test in legacy_register_sim_regno bit me recently when doing the rl78 register rearrangement. That's an interesting case to consider, actually. For rl78 (and for mips too as I recall), there are a certain set of "raw" registers which are then mapped to "cooked" versions. The raw registers do not have names, but the cooked ones do. So, assuming the same implementation of `register_reggroup_p', asking about one of the raw registers will fail (return 0), but asking about a cooked register will succeed. The reason I'm pondering this is that I'm now wondering about the suitability of using `register_reggroup_p' for this purpose. I think it is still okay because presumably the code should be asking about the cooked version of the register instead of the uncooked version. In fact, for rl78, this is how the stack pointer is implemented; it's a cooked register formed from two raw registers. It is never correct for anything aside from the pseudo register read/write code to try to access the raw versions of the two stack pointer registers. > Still learning about GDB's code layout -- is there any benefit in > invoking gdbarch_register_reggroup_p (as you're doing) in contrast to > directly going for sh_register_reggroup_p? Well, the way I'm doing it is slower. But, if at some point someone were to add another `register_reggroup_p' method for some other sh variant (as is the case for `register_name'), the code that I wrote will still work. If I called sh_register_reggroup_p directly, then presumably the wrong function would be called for that new architecture variant. > Also, we assume that simply passing 0 for fpscr to sh_analyze_prologue > doesn't do any harm (which it doesn't). (Worth a comment, perhaps?) Yes, that's a good idea. Below is an updated patch. * sh-tdep.c (sh_frame_cache): Don't fetch the FPSCR register unless it exists for this architecture. Index: sh-tdep.c =================================================================== RCS file: /cvs/src/src/gdb/sh-tdep.c,v retrieving revision 1.239 diff -u -p -r1.239 sh-tdep.c --- sh-tdep.c 27 Feb 2012 16:40:48 -0000 1.239 +++ sh-tdep.c 1 Mar 2012 18:11:17 -0000 @@ -2553,7 +2553,16 @@ sh_frame_cache (struct frame_info *this_ if (cache->pc != 0) { ULONGEST fpscr; - fpscr = get_frame_register_unsigned (this_frame, FPSCR_REGNUM); + + /* Check for the existence of the FPSCR register. If it exists, + fetch its value for use in prologue analysis. Passing a zero + value is the best choice for architecture variants upon which + there's no FPSCR register. */ + if (gdbarch_register_reggroup_p (gdbarch, FPSCR_REGNUM, all_reggroup)) + fpscr = get_frame_register_unsigned (this_frame, FPSCR_REGNUM); + else + fpscr = 0; + sh_analyze_prologue (gdbarch, cache->pc, current_pc, cache, fpscr); }