This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [patch] Add support for single register window model on SPARC


> 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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]