From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 7443 invoked by alias); 21 Jun 2011 12:12:53 -0000 Received: (qmail 7424 invoked by uid 22791); 21 Jun 2011 12:12:47 -0000 X-SWARE-Spam-Status: No, hits=-1.9 required=5.0 tests=AWL,BAYES_00,TW_CN,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from sibelius.xs4all.nl (HELO glazunov.sibelius.xs4all.nl) (83.163.83.176) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 21 Jun 2011 12:12:32 +0000 Received: from glazunov.sibelius.xs4all.nl (kettenis@localhost [127.0.0.1]) by glazunov.sibelius.xs4all.nl (8.14.3/8.14.3) with ESMTP id p5LCC7Gu020100; Tue, 21 Jun 2011 14:12:07 +0200 (CEST) Received: (from kettenis@localhost) by glazunov.sibelius.xs4all.nl (8.14.3/8.14.3/Submit) id p5LCC5Go017378; Tue, 21 Jun 2011 14:12:05 +0200 (CEST) Date: Tue, 21 Jun 2011 12:12:00 -0000 Message-Id: <201106211212.p5LCC5Go017378@glazunov.sibelius.xs4all.nl> From: Mark Kettenis To: ebotcazou@adacore.com CC: gdb-patches@sourceware.org In-reply-to: <201106161536.09111.ebotcazou@adacore.com> (message from Eric Botcazou on Thu, 16 Jun 2011 15:36:08 +0200) Subject: Re: [patch] Add support for single register window model on SPARC References: <201106161536.09111.ebotcazou@adacore.com> 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: 2011-06/txt/msg00282.txt.bz2 > From: Eric Botcazou > Date: Thu, 16 Jun 2011 15:36:08 +0200 > > Hi, > > support for the single register window (aka flat) model on SPARC was just > re-introduced in GCC: http://gcc.gnu.org/ml/gcc-patches/2011-06/msg00820.html > The -mflat option had been present in the 3.x series of compilers but was > removed when the 4.x series debuted. Due to renewed interest, most notably > from the LEON folks, it will be supported again in future GCC releases. Ugh, that makes me feel dirty. But I guess it makes some sense to want this for a RT environment. > The implementation has been almost entirely overhauled and, in > particular, the weird register usage of the old flavor (e.g. %i7 as > frame pointer) has been dropped. Instead the new flavor preserves > the canonical register usage and frame layout; the only visible > change is that 'save' & 'restore' instructions are replaced with > their "manual" equivalents in the generated code. > > While CFIs are adjusted automatically by GCC, GDB has hardcoded assumptions > about how frames are established on SPARC, which makes it unable to unwind > the "flat" frames; in particular backtraces don't work. Hmm, if full CFI for the explicit register saving code is emitted, I'd expect backtraces to work. > The attached patch is aimed at fixing that. It extends the frame > sniffer to recognize the "flat" frames and decode the locations of > call-saved registers. Regtested (in normal mode) on SPARC/Solaris > and SPARC64/Solaris. It was also used to debug the new -mflat > implementation of GCC, both 32-bit and 64-bit, so it works > reasonably well in this mode. > > OK for the mainline? I'd like to get a chance to run a regression test on OpenBSD/sparc64. Can you give me a couple of days and perhaps ping me towards the end of this week to remind me if I haven't responded by then? The code itself makes sense to me, a few minor nits inline below. Cheers, Mark > 2011-06-16 Eric Botcazou > > * sparc-tdep.h (struct sparc_frame_cache): Add frame_offset, > saved_regs_mask and copied_regs_mask fields. > (sparc_record_save_insn): New prototype. > * sparc-tdep.c (sparc_alloc_frame_cache): Initialize the new fields. > (sparc_record_save_insn): New function. > (sparc_analyze_prologue): Add head comment. Recognize store insns > of call-saved registers. Use OFFSET consistently. Recognize flat > frames and cache their settings. > (sparc32_skip_prologue): Handle flat frames. > (sparc_frame_cache): Add frame_offset to the base address. > (sparc32_frame_cache): Adjust to new frame description. > (sparc32_frame_prev_register): Likewise. > * sparc64-tdep.c (sparc64_frame_prev_register): Likewise. > * sparc-sol2-tdep.c (sparc32_sol2_sigtramp_frame_cache): Likewise. > * sparc64-sol2-tdep.c (sparc64_sol2_sigtramp_frame_cache): Likewise. > * sparcnbsd-tdep.c (sparc32nbsd_sigcontext_frame_cache): Force the > frame by calling sparc_record_save_insn. > * sparc64nbsd-tdep.c (sparc64nbsd_sigcontext_frame_cache): Likewise. > * sparcobsd-tdep.c (sparc32obsd_sigtramp_frame_cache): Likewise. > * sparc64obsd-tdep.c (sparc64obsd_frame_cache): Likewise. > > Index: sparc-sol2-tdep.c > =================================================================== > RCS file: /cvs/src/src/gdb/sparc-sol2-tdep.c,v > retrieving revision 1.25 > diff -u -p -r1.25 sparc-sol2-tdep.c > --- sparc-sol2-tdep.c 18 Mar 2011 18:52:32 -0000 1.25 > +++ sparc-sol2-tdep.c 16 Jun 2011 13:13:24 -0000 > @@ -93,7 +93,7 @@ sparc32_sol2_sigtramp_frame_cache (struc > /* The third argument is a pointer to an instance of `ucontext_t', > which has a member `uc_mcontext' that contains the saved > registers. */ > - regnum = (cache->frameless_p ? SPARC_O2_REGNUM : SPARC_I2_REGNUM); > + regnum = (cache->copied_regs_mask & 4) ? SPARC_I2_REGNUM : SPARC_O2_REGNUM; For consistency that probably should be (cache->copied_regs_mask & 0x04). > Index: sparc-tdep.c > =================================================================== > RCS file: /cvs/src/src/gdb/sparc-tdep.c,v > retrieving revision 1.221 > diff -u -p -r1.221 sparc-tdep.c > --- sparc-tdep.c 23 May 2011 16:38:05 -0000 1.221 > +++ sparc-tdep.c 16 Jun 2011 13:13:24 -0000 > @@ -983,7 +1102,8 @@ sparc32_frame_cache (struct frame_info * > an "unimp" instruction. If it is, then it is a struct-return > function. */ > CORE_ADDR pc; > - int regnum = cache->frameless_p ? SPARC_O7_REGNUM : SPARC_I7_REGNUM; > + int regnum > + = (cache->copied_regs_mask & 0x80) ? SPARC_I7_REGNUM : SPARC_O7_REGNUM; Elsewhere in GDB, or at least in the code written by me, like most of the existing sparc/sparc64 code places the '=' sign on the line before. I can see how you can interpret the GNU coding style to say that you should put the '=' sign on the next line, but for now it'd be better to be consistent with the rest of the code. Can you change this? > pc = get_frame_register_unsigned (this_frame, regnum) + 8; > if (sparc_is_unimp_insn (pc)) > @@ -1025,7 +1145,8 @@ sparc32_frame_prev_register (struct fram > if (cache->struct_return_p) > pc += 4; > > - regnum = cache->frameless_p ? SPARC_O7_REGNUM : SPARC_I7_REGNUM; > + regnum > + = (cache->copied_regs_mask & 0x80)? SPARC_I7_REGNUM : SPARC_O7_REGNUM; Same here. > @@ -1034,7 +1155,9 @@ sparc32_frame_prev_register (struct fram > { > ULONGEST wcookie = sparc_fetch_wcookie (gdbarch); > > - if (wcookie != 0 && !cache->frameless_p && regnum == SPARC_I7_REGNUM) > + if (wcookie != 0 > + && (cache->copied_regs_mask & 0x80) > + && regnum == SPARC_I7_REGNUM) This change isn't quite right. The wcookie stuff is there to support StackGhost[1], which is unsupportable for the "flat" model. So what needs to be checked here is whether we executed a save instruction or not. As far as I know OpenBSD is the only OS that does StackGhost. On everything else wcookie will be 0. So you can probably leave this as is right now, since the existing frameless_p check is more correct than what you have in your diff. > Index: sparc64-tdep.c > =================================================================== > RCS file: /cvs/src/src/gdb/sparc64-tdep.c,v > retrieving revision 1.54 > diff -u -p -r1.54 sparc64-tdep.c > --- sparc64-tdep.c 21 May 2011 19:19:45 -0000 1.54 > +++ sparc64-tdep.c 16 Jun 2011 13:13:27 -0000 > @@ -520,7 +520,8 @@ sparc64_frame_prev_register (struct fram > { > CORE_ADDR pc = (regnum == SPARC64_NPC_REGNUM) ? 4 : 0; > > - regnum = cache->frameless_p ? SPARC_O7_REGNUM : SPARC_I7_REGNUM; > + regnum > + = (cache->copied_regs_mask) & 0x80? SPARC_I7_REGNUM : SPARC_O7_REGNUM; Missing space after between 0x80 and ?; same comment about '=' as above. > pc += get_frame_register_unsigned (this_frame, regnum) + 8; > return frame_unwind_got_constant (this_frame, regnum, pc); > } > @@ -529,7 +530,9 @@ sparc64_frame_prev_register (struct fram > { > ULONGEST wcookie = sparc_fetch_wcookie (gdbarch); > > - if (wcookie != 0 && !cache->frameless_p && regnum == SPARC_I7_REGNUM) > + if (wcookie != 0 > + && (cache->copied_regs_mask & 0x80) > + && regnum == SPARC_I7_REGNUM) StackGhost again. Better leave this alone as well. [1] http://projects.cerias.purdue.edu/stackghost/stackghost/index.html