This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [patch] Add support for single register window model on SPARC
- From: Mark Kettenis <mark dot kettenis at xs4all dot nl>
- To: ebotcazou at adacore dot com
- Cc: gdb-patches at sourceware dot org
- Date: Tue, 21 Jun 2011 14:12:05 +0200 (CEST)
- Subject: Re: [patch] Add support for single register window model on SPARC
- References: <201106161536.09111.ebotcazou@adacore.com>
> From: Eric Botcazou <ebotcazou@adacore.com>
> 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 <ebotcazou@adacore.com>
>
> * 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