[RFA/RFC] fix problems with unwinder on mips-irix

[RFA/RFC] fix problems with unwinder on mips-irix

[Joel Brobecker]

[-- Attachment #1: Type: text/plain, Size: 3063 bytes --]

Hello,

Still working on this IRIX debugger on mips, the next big thing is that
GDB is having problems unwinding the stack. For instance, using
gdb.base/store:

        % gdb store
        (gdb) b wack_doublest
        (gdb) run
        (gdb) bt
        #0  wack_doublest (u=Unhandled dwarf expression opcode 0x93
        ) at ./gdb.base/store.c:125
        #1  0x100108d8 in ?? ()
        warning: GDB can't find the start of the function at 0x100108d8.
        
            GDB is unable to find the start of the function at 0x100108d8
        and thus can't determine the size of that function's stack frame.
        This means that GDB may be unable to access that stack frame, or
        the frames below it.
            This problem is most likely caused by an invalid program counter or
        stack pointer.
            However, if you think GDB should simply search farther back
        from 0x100108d8 for code which looks like the beginning of a
        function, you can increase the range of the search using the `set
        heuristic-fence-post' command.
        Previous frame inner to this frame (corrupt stack?)

What happens is that GDB calls heuristic_proc_desc() to "synthesize"
a procedure descriptor (I'll abbreviate: PDR). On IRIX, at least with
N32, we end up calling mips32_heuristic_proc_desc(). As expected, this
function scans the prologue, and computes the frame size, stores which
registers are saved, etc.

Unfortunately, it seems that we forgot to save one critical piece of
information: *where* the registers are saved in the stack. Looking at
mips_mdebug_frame_cache(), we see:

    CORE_ADDR reg_position = (cache->base + PROC_REG_OFFSET (proc_desc));
    int ireg;

    for (ireg = MIPS_NUMREGS - 1; gen_mask; --ireg, gen_mask <<= 1)
      if (gen_mask & 0x80000000)
        {
          cache->saved_regs[NUM_REGS + ireg].addr = reg_position;
          reg_position -= mips_abi_regsize (gdbarch);
        }

But the thing is, we forgot to set the PROC_REG_OFFSET. This value
is the distance between the frame base, and the start of the area
where registers are saved. It can be computed when we detect the
first register save instruction in the prologue by taking the
difference between the offset to SP/FP used in the isntruction,
and the frame size.

The attached patch implements this, for mips32_heuristic_proc_desc().
I am about to launch the testsuite with this patch.

I am guessing that mips16_heuristic_proc_desc() suffers from the same
problem and will require the same adjustments. I am happy to fix it
too, but it will be blind fixing as I won't be able to test it. Your
call.

2004-07-22  Joel Brobecker  <brobecker@gnat.com>

        * mips-tdep.c (mips_mdebug_frame_cache): Minor reformatting.
        (set_saved_reg_info): New function.
        (mips32_heuristic_proc_desc): Compute the procedure descriptor
        PROC_REG_OFFSET.

Tested on mips-irix, fixes tons of regressions. OK to commit?
If you would like me to fix mips16_heuristic_proc_desc as well,
I can submit a separate patch.

Thanks,
-- 
Joel

[-- Attachment #2: mips-unwind.diff --]
[-- Type: text/plain, Size: 2592 bytes --]

Index: mips-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/mips-tdep.c,v
retrieving revision 1.301
diff -u -p -r1.301 mips-tdep.c
--- mips-tdep.c	10 Jul 2004 01:17:52 -0000	1.301
+++ mips-tdep.c	22 Jul 2004 22:04:56 -0000
@@ -1587,9 +1587,9 @@ mips_mdebug_frame_cache (struct frame_in
   /* Fill in the offsets for the registers which gen_mask says were
      saved.  */
   {
-    CORE_ADDR reg_position = (cache->base
-			      + PROC_REG_OFFSET (proc_desc));
+    CORE_ADDR reg_position = (cache->base + PROC_REG_OFFSET (proc_desc));
     int ireg;
+
     for (ireg = MIPS_NUMREGS - 1; gen_mask; --ireg, gen_mask <<= 1)
       if (gen_mask & 0x80000000)
 	{
@@ -1851,6 +1851,23 @@ set_reg_offset (CORE_ADDR *saved_regs, i
     }
 }
 
+/* Record in the procedure descriptor INFO the fact that register REGNO
+   was saved at OFFSET bytes from the procedure descriptor frame register.
+   Also updates the offset between the frame base and the area in the stack
+   where the registers are stored.  */
+
+static void
+set_saved_reg_info (struct mips_extra_func_info *info,
+                    int regno, int offset)
+{
+  /* If this is the first register saved, then OFFSET points to
+     the begining of the stack where the registers are saved.
+     We can use this to compute the PROC_REG_OFFSET of this frame.  */
+  if (PROC_REG_MASK (info) == 0)
+    PROC_REG_OFFSET (info) = offset - PROC_FRAME_OFFSET (info);
+
+  PROC_REG_MASK (info) |= 1 << regno;
+}
 
 /* Test whether the PC points to the return instruction at the
    end of a function. */
@@ -2179,14 +2196,14 @@ restart:
 	}
       else if ((high_word & 0xFFE0) == 0xafa0)	/* sw reg,offset($sp) */
 	{
-	  PROC_REG_MASK (&temp_proc_desc) |= 1 << reg;
+         set_saved_reg_info (&temp_proc_desc, reg, low_word);
 	  set_reg_offset (temp_saved_regs, reg, sp + low_word);
 	}
       else if ((high_word & 0xFFE0) == 0xffa0)	/* sd reg,offset($sp) */
 	{
 	  /* Irix 6.2 N32 ABI uses sd instructions for saving $gp and
 	     $ra.  */
-	  PROC_REG_MASK (&temp_proc_desc) |= 1 << reg;
+         set_saved_reg_info (&temp_proc_desc, reg, low_word);
 	  set_reg_offset (temp_saved_regs, reg, sp + low_word);
 	}
       else if (high_word == 0x27be)	/* addiu $30,$sp,size */
@@ -2236,7 +2253,7 @@ restart:
 	}
       else if ((high_word & 0xFFE0) == 0xafc0)	/* sw reg,offset($30) */
 	{
-	  PROC_REG_MASK (&temp_proc_desc) |= 1 << reg;
+         set_saved_reg_info (&temp_proc_desc, reg, low_word);
 	  set_reg_offset (temp_saved_regs, reg, frame_addr + low_word);
 	}
     }

Re: [RFA/RFC] fix problems with unwinder on mips-irix

[Andrew Cagney]

Joel,

> 2004-07-22  Joel Brobecker  <brobecker@gnat.com>
> 
>         * mips-tdep.c (mips_mdebug_frame_cache): Minor reformatting.
>         (set_saved_reg_info): New function.
>         (mips32_heuristic_proc_desc): Compute the procedure descriptor
>         PROC_REG_OFFSET.
> 

It's time to cut our losses and split the mdebug and heuristic cases. 
Lets work through this change after I've done that.

In the mean time, can I encourage you to look at the s390 and it's 
stub-unwinder - the MIPS desperatly needs one of these (and it is 
something that can be wipped up in isolation).  To see where its needed 
do a STEPI into shlib trampoline code.

Andrew

Re: [RFA/RFC] fix problems with unwinder on mips-irix

[Joel Brobecker]

Andrew,

> >2004-07-22  Joel Brobecker  <brobecker@gnat.com>
> >
> >        * mips-tdep.c (mips_mdebug_frame_cache): Minor reformatting.
> >        (set_saved_reg_info): New function.
> >        (mips32_heuristic_proc_desc): Compute the procedure descriptor
> >        PROC_REG_OFFSET.
> >
> 
> It's time to cut our losses and split the mdebug and heuristic cases. 
> Lets work through this change after I've done that.

I am sorry. I didn't have time to follow very well your recent changes
to the mips unwinders. Is it time for me to come back to this issue,
or do you think I should wait some more?

> In the mean time, can I encourage you to look at the s390 and it's 
> stub-unwinder - the MIPS desperatly needs one of these (and it is 
> something that can be wipped up in isolation).  To see where its needed 
> do a STEPI into shlib trampoline code.

I would love to, IRIX looks like a fun platform to play with, but I am
a bit time-starved right now. Hopefully in september.

-- 
Joel

Re: [RFA/RFC] fix problems with unwinder on mips-irix

[Andrew Cagney]

> Andrew,
> 
> 
>>>> >2004-07-22  Joel Brobecker  <brobecker@gnat.com>
>>>> >
>>>> >        * mips-tdep.c (mips_mdebug_frame_cache): Minor reformatting.
>>>> >        (set_saved_reg_info): New function.
>>>> >        (mips32_heuristic_proc_desc): Compute the procedure descriptor
>>>> >        PROC_REG_OFFSET.
>>>> >
>>
>>> 
>>> It's time to cut our losses and split the mdebug and heuristic cases. 
>>> Lets work through this change after I've done that.
> 
> 
> I am sorry. I didn't have time to follow very well your recent changes
> to the mips unwinders. Is it time for me to come back to this issue,
> or do you think I should wait some more?

If you've the time, please do.

Two key things to know:

- with three unwinders handling three different cases previously handled 
by one, there's a lot of dead code paths.  For instance, 
mips32_heuristic_proc_desc is now only called by mips_insn32_frame_cache 
and hence can be inlined there, making it possible for your problem to 
be solved more locally.

- I'm interested in a brutal overhaul of an unwinder, not a small tweak :-)

>>> In the mean time, can I encourage you to look at the s390 and it's 
>>> stub-unwinder - the MIPS desperatly needs one of these (and it is 
>>> something that can be wipped up in isolation).  To see where its needed 
>>> do a STEPI into shlib trampoline code.
> 
> 
> I would love to, IRIX looks like a fun platform to play with, but I am
> a bit time-starved right now. Hopefully in september.

I've added a stub unwinder.

Andrew

Re: [RFA/RFC] fix problems with unwinder on mips-irix

[Joel Brobecker]

Andrew,

> If you've the time, please do.
> 
> Two key things to know:
> 
> - with three unwinders handling three different cases previously handled 
> by one, there's a lot of dead code paths.  For instance, 
> mips32_heuristic_proc_desc is now only called by mips_insn32_frame_cache 
> and hence can be inlined there, making it possible for your problem to 
> be solved more locally.
> 
> - I'm interested in a brutal overhaul of an unwinder, not a small tweak :-)

I think I can find the time for "a small tweak" within the next couple
of weeks, maybe this week-end. But if a "brutal overhaul" is required,
then the bar is too high for me. I just don't have the time.

If I may, I have the feeling sometimes that you expect the other
contributors to have the same level of involvment than you do. You
are extremely dedicated to the GDB project. That makes it very tough
at least for me to match even 10% of what you dedicate to GDB...

-- 
Joel

Re: [RFA/RFC] fix problems with unwinder on mips-irix

[Andrew Cagney]

> Andrew,
> 
> 
>>> If you've the time, please do.
>>> 
>>> Two key things to know:
>>> 
>>> - with three unwinders handling three different cases previously handled 
>>> by one, there's a lot of dead code paths.  For instance, 
>>> mips32_heuristic_proc_desc is now only called by mips_insn32_frame_cache 
>>> and hence can be inlined there, making it possible for your problem to 
>>> be solved more locally.
>>> 
>>> - I'm interested in a brutal overhaul of an unwinder, not a small tweak :-)
> 
> 
> I think I can find the time for "a small tweak" within the next couple
> of weeks, maybe this week-end. But if a "brutal overhaul" is required,
> then the bar is too high for me. I just don't have the time.

I think that you'll find that a brutal overhaul will in the end take 
less time than what, at first sight, appears to be just a single simple 
wafer thin tweak.

Here we've got what should be a single simple procedure 
(mips_insn32_frame_cache) with a very tight interface and an exaustive 
testsuite, but instead is anything but.

Can I suggest spending just an hour attacking it, and then posting the 
result (just as long as it compiles :-).

Andrew

Re: [RFA/RFC] fix problems with unwinder on mips-irix

[Joel Brobecker]

Andrew,

I would like to fix the following issue:

> >>>>>2004-07-22  Joel Brobecker  <brobecker@gnat.com>
> >>>>>
> >>>>>        * mips-tdep.c (mips_mdebug_frame_cache): Minor reformatting.
> >>>>>        (set_saved_reg_info): New function.
> >>>>>        (mips32_heuristic_proc_desc): Compute the procedure descriptor
> >>>>>        PROC_REG_OFFSET.
> >>>>>
> >>
> >>>
> >>>It's time to cut our losses and split the mdebug and heuristic cases. 
> >>>Lets work through this change after I've done that.

But you want a "major overhaul" of the mips unwinder as a precondition
to fixing the problem at hand. Could you explain a bit more what
overhaul you are interested in? I don't see what needs to be done.

In particular, you said:

> Two key things to know:
> 
> - with three unwinders handling three different cases previously handled 
> by one, there's a lot of dead code paths.  For instance, 
> mips32_heuristic_proc_desc is now only called by mips_insn32_frame_cache 
> and hence can be inlined there, making it possible for your problem to 
> be solved more locally.

mips32_heuristic_proc_desc is called by heuristic_proc_desc. (And
frankly I find inlining often counter productive, as we end up
with giant function just as we did with decode_line_1).

Thanks,
-- 
Joel

Re: [RFA/RFC] fix problems with unwinder on mips-irix

[Andrew Cagney]

> But you want a "major overhaul" of the mips unwinder as a precondition
> to fixing the problem at hand. Could you explain a bit more what
> overhaul you are interested in? I don't see what needs to be done.
> 
> In particular, you said:
> 
> 
>>> Two key things to know:
>>> 
>>> - with three unwinders handling three different cases previously handled 
>>> by one, there's a lot of dead code paths.  For instance, 
>>> mips32_heuristic_proc_desc is now only called by mips_insn32_frame_cache 
>>> and hence can be inlined there, making it possible for your problem to 
>>> be solved more locally.
> 
> 
> mips32_heuristic_proc_desc is called by heuristic_proc_desc. (And
> frankly I find inlining often counter productive, as we end up
> with giant function just as we did with decode_line_1).

Yes, sometimes inlineing doesn't help, here it does.  There's really no 
value in trying to preserve this code so be brutal.

If find_proc_desc is inlined:

> static mips_extra_func_info_t
> find_proc_desc (CORE_ADDR pc, struct frame_info *next_frame, int cur_frame)
> {
>   mips_extra_func_info_t proc_desc;
>   CORE_ADDR startaddr = 0;

then everything from here ....

>   proc_desc = non_heuristic_proc_desc (pc, &startaddr);
> 
>   if (proc_desc)
>     {
>       /* IF this is the topmost frame AND
>        * (this proc does not have debugging information OR
>        * the PC is in the procedure prologue)
>        * THEN create a "heuristic" proc_desc (by analyzing
>        * the actual code) to replace the "official" proc_desc.
>        */
>       if (next_frame == NULL)
> 	{
> 	  struct symtab_and_line val;
> 	  struct symbol *proc_symbol =
> 	    PROC_DESC_IS_DUMMY (proc_desc) ? 0 : PROC_SYMBOL (proc_desc);
> 
> 	  if (proc_symbol)
> 	    {
> 	      val = find_pc_line (BLOCK_START
> 				  (SYMBOL_BLOCK_VALUE (proc_symbol)), 0);
> 	      val.pc = val.end ? val.end : pc;
> 	    }
> 	  if (!proc_symbol || pc < val.pc)
> 	    {
> 	      mips_extra_func_info_t found_heuristic =
> 		heuristic_proc_desc (PROC_LOW_ADDR (proc_desc),
> 				     pc, next_frame, cur_frame);
> 	      if (found_heuristic)
> 		proc_desc = found_heuristic;
> 	    }
> 	}
>     }

>   else
>     {
>       /* Is linked_proc_desc_table really necessary?  It only seems to be used
>          by procedure call dummys.  However, the procedures being called ought
>          to have their own proc_descs, and even if they don't,
>          heuristic_proc_desc knows how to create them! */
> 
>       struct linked_proc_info *link;
> 
>       for (link = linked_proc_desc_table; link; link = link->next)
> 	if (PROC_LOW_ADDR (&link->info) <= pc
> 	    && PROC_HIGH_ADDR (&link->info) > pc)
> 	  return &link->info;

... to here is simply dead.  That is all for the non-heuristic case. 
That leaves:

>       if (startaddr == 0)
> 	startaddr = heuristic_proc_start (pc);
> 
>       proc_desc = heuristic_proc_desc (startaddr, pc, next_frame, cur_frame);
>     }
>   return proc_desc;
> }

If heuristic_proc_descr is then inlined we find:

> static mips_extra_func_info_t
> heuristic_proc_desc (CORE_ADDR start_pc, CORE_ADDR limit_pc,
> 		     struct frame_info *next_frame, int cur_frame)
> {
>   CORE_ADDR sp;
> 
>   if (cur_frame)
>     sp = read_next_frame_reg (next_frame, NUM_REGS + MIPS_SP_REGNUM);
>   else
>     sp = 0;
 >
>   if (start_pc == 0)
>     return NULL;
>   memset (&temp_proc_desc, '\0', sizeof (temp_proc_desc));
>   temp_saved_regs = xrealloc (temp_saved_regs, SIZEOF_FRAME_SAVED_REGS);
>   memset (temp_saved_regs, '\0', SIZEOF_FRAME_SAVED_REGS);
>   PROC_LOW_ADDR (&temp_proc_desc) = start_pc;
>   PROC_FRAME_REG (&temp_proc_desc) = MIPS_SP_REGNUM;
>   PROC_PC_REG (&temp_proc_desc) = RA_REGNUM;
>   if (start_pc + 200 < limit_pc)
>     limit_pc = start_pc + 200;
>   if (pc_is_mips16 (start_pc))

this mips16 call is dead.

>     mips16_heuristic_proc_desc (start_pc, limit_pc, next_frame, sp);
>   else
>     mips32_heuristic_proc_desc (start_pc, limit_pc, next_frame, sp);
>   return &temp_proc_desc;
> }

If we then inline the mips32_heuristic ... call into mips_insn32...:

> static void
> mips32_heuristic_proc_desc (CORE_ADDR start_pc, CORE_ADDR limit_pc,
> 			    struct frame_info *next_frame, CORE_ADDR sp)
> {
>   CORE_ADDR cur_pc;
>   CORE_ADDR frame_addr = 0;	/* Value of $r30. Used by gcc for frame-pointer */
> restart:
>   temp_saved_regs = xrealloc (temp_saved_regs, SIZEOF_FRAME_SAVED_REGS);
>   memset (temp_saved_regs, '\0', SIZEOF_FRAME_SAVED_REGS);
>   PROC_FRAME_OFFSET (&temp_proc_desc) = 0;
>   PROC_FRAME_ADJUST (&temp_proc_desc) = 0;	/* offset of FP from SP */
>   for (cur_pc = start_pc; cur_pc < limit_pc; cur_pc += MIPS_INSTLEN)
>     {
>       unsigned long inst, high_word, low_word;
>       int reg;
> 
>       /* Fetch the instruction.   */
>       inst = (unsigned long) mips_fetch_instruction (cur_pc);
> 
>       /* Save some code by pre-extracting some useful fields.  */
>       high_word = (inst >> 16) & 0xffff;
>       low_word = inst & 0xffff;
>       reg = high_word & 0x1f;
> 
>       if (high_word == 0x27bd	/* addiu $sp,$sp,-i */
> 	  || high_word == 0x23bd	/* addi $sp,$sp,-i */
> 	  || high_word == 0x67bd)	/* daddiu $sp,$sp,-i */
> 	{
> 	  if (low_word & 0x8000)	/* negative stack adjustment? */
> 	    PROC_FRAME_OFFSET (&temp_proc_desc) += 0x10000 - low_word;
> 	  else
> 	    /* Exit loop if a positive stack adjustment is found, which
> 	       usually means that the stack cleanup code in the function
> 	       epilogue is reached.  */
> 	    break;
> 	}
>       else if ((high_word & 0xFFE0) == 0xafa0)	/* sw reg,offset($sp) */
> 	{
> 	  PROC_REG_MASK (&temp_proc_desc) |= 1 << reg;
> 	  set_reg_offset (temp_saved_regs, reg, sp + low_word);
> 	}
>       else if ((high_word & 0xFFE0) == 0xffa0)	/* sd reg,offset($sp) */
> 	{
> 	  /* Irix 6.2 N32 ABI uses sd instructions for saving $gp and
> 	     $ra.  */
> 	  PROC_REG_MASK (&temp_proc_desc) |= 1 << reg;
> 	  set_reg_offset (temp_saved_regs, reg, sp + low_word);
> 	}
>       else if (high_word == 0x27be)	/* addiu $30,$sp,size */
> 	{
> 	  /* Old gcc frame, r30 is virtual frame pointer.  */
> 	  if ((long) low_word != PROC_FRAME_OFFSET (&temp_proc_desc))
> 	    frame_addr = sp + low_word;
> 	  else if (PROC_FRAME_REG (&temp_proc_desc) == MIPS_SP_REGNUM)
> 	    {
> 	      unsigned alloca_adjust;
> 	      PROC_FRAME_REG (&temp_proc_desc) = 30;
> 	      frame_addr = read_next_frame_reg (next_frame, NUM_REGS + 30);
> 	      alloca_adjust = (unsigned) (frame_addr - (sp + low_word));
> 	      if (alloca_adjust > 0)
> 		{
> 		  /* FP > SP + frame_size. This may be because
> 		   * of an alloca or somethings similar.
> 		   * Fix sp to "pre-alloca" value, and try again.
> 		   */
> 		  sp += alloca_adjust;
> 		  goto restart;
> 		}
> 	    }
> 	}
>       /* move $30,$sp.  With different versions of gas this will be either
>          `addu $30,$sp,$zero' or `or $30,$sp,$zero' or `daddu 30,sp,$0'.
>          Accept any one of these.  */
>       else if (inst == 0x03A0F021 || inst == 0x03a0f025 || inst == 0x03a0f02d)
> 	{
> 	  /* New gcc frame, virtual frame pointer is at r30 + frame_size.  */
> 	  if (PROC_FRAME_REG (&temp_proc_desc) == MIPS_SP_REGNUM)
> 	    {
> 	      unsigned alloca_adjust;
> 	      PROC_FRAME_REG (&temp_proc_desc) = 30;
> 	      frame_addr = read_next_frame_reg (next_frame, NUM_REGS + 30);
> 	      alloca_adjust = (unsigned) (frame_addr - sp);
> 	      if (alloca_adjust > 0)
> 		{
> 		  /* FP > SP + frame_size. This may be because
> 		   * of an alloca or somethings similar.
> 		   * Fix sp to "pre-alloca" value, and try again.
> 		   */
> 		  sp += alloca_adjust;
> 		  goto restart;
> 		}
> 	    }
> 	}
>       else if ((high_word & 0xFFE0) == 0xafc0)	/* sw reg,offset($30) */
> 	{
> 	  PROC_REG_MASK (&temp_proc_desc) |= 1 << reg;
> 	  set_reg_offset (temp_saved_regs, reg, frame_addr + low_word);
> 	}
>     }
> }
> 

and compare it against these parts of the body of mips_insn32's 
heuristic ..., we also find:

> static struct mips_frame_cache *
> mips_insn32_frame_cache (struct frame_info *next_frame, void **this_cache)
> {
>   mips_extra_func_info_t proc_desc;
>   struct mips_frame_cache *cache;
>   struct gdbarch *gdbarch = get_frame_arch (next_frame);
>   struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
>   /* r0 bit means kernel trap */
>   int kernel_trap;
>   /* What registers have been saved?  Bitmasks.  */
>   unsigned long gen_mask, float_mask;
> 
>   if ((*this_cache) != NULL)
>     return (*this_cache);
>   cache = FRAME_OBSTACK_ZALLOC (struct mips_frame_cache);
>   (*this_cache) = cache;
>   cache->saved_regs = trad_frame_alloc_saved_regs (next_frame);

this is has been inlined down to mips32_...

>   /* Get the mdebug proc descriptor.  */
>   proc_desc = find_proc_desc (frame_pc_unwind (next_frame), next_frame, 1);
>   if (proc_desc == NULL)
>     /* I'm not sure how/whether this can happen.  Normally when we
>        can't find a proc_desc, we "synthesize" one using
>        heuristic_proc_desc and set the saved_regs right away.  */
>     return cache;
> 
>   /* Extract the frame's base.  */
>   cache->base = (frame_unwind_register_signed (next_frame, NUM_REGS + PROC_FRAME_REG (proc_desc))
> 		 + PROC_FRAME_OFFSET (proc_desc) - PROC_FRAME_ADJUST (proc_desc));

This kernel_trap stuff isn't applicable here.

>   kernel_trap = PROC_REG_MASK (proc_desc) & 1;
>   gen_mask = kernel_trap ? 0xFFFFFFFF : PROC_REG_MASK (proc_desc);
>   float_mask = kernel_trap ? 0xFFFFFFFF : PROC_FREG_MASK (proc_desc);

THe following is just duplicating that inlined mips32... so the pair can 
be folded into one.

>   /* In any frame other than the innermost or a frame interrupted by a
>      signal, we assume that all registers have been saved.  This
>      assumes that all register saves in a function happen before the
>      first function call.  */
>   if (in_prologue (frame_pc_unwind (next_frame), PROC_LOW_ADDR (proc_desc))
>       /* Not sure exactly what kernel_trap means, but if it means the
> 	 kernel saves the registers without a prologue doing it, we
> 	 better not examine the prologue to see whether registers
> 	 have been saved yet.  */
>       && !kernel_trap)
>     {
>       /* We need to figure out whether the registers that the
>          proc_desc claims are saved have been saved yet.  */
> 
>       CORE_ADDR addr;
> 
>       /* Bitmasks; set if we have found a save for the register.  */
>       unsigned long gen_save_found = 0;
>       unsigned long float_save_found = 0;
>       int mips16;
> 
>       /* If the address is odd, assume this is MIPS16 code.  */
>       addr = PROC_LOW_ADDR (proc_desc);
>       mips16 = pc_is_mips16 (addr);
> 
>       /* Scan through this function's instructions preceding the
>          current PC, and look for those that save registers.  */
>       while (addr < frame_pc_unwind (next_frame))
> 	{
> 	  if (mips16)
> 	    {
> 	      mips16_decode_reg_save (mips16_fetch_instruction (addr),
> 				      &gen_save_found);
> 	      addr += MIPS16_INSTLEN;
> 	    }
> 	  else
> 	    {
> 	      mips32_decode_reg_save (mips32_fetch_instruction (addr),
> 				      &gen_save_found, &float_save_found);
> 	      addr += MIPS_INSTLEN;
> 	    }
> 	}
>       gen_mask = gen_save_found;
>       float_mask = float_save_found;
>     }
> 
>   /* Fill in the offsets for the registers which gen_mask says were
>      saved.  */
>   {
>     CORE_ADDR reg_position = (cache->base
> 			      + PROC_REG_OFFSET (proc_desc));
>     int ireg;
>     for (ireg = MIPS_NUMREGS - 1; gen_mask; --ireg, gen_mask <<= 1)
>       if (gen_mask & 0x80000000)
> 	{
> 	  cache->saved_regs[NUM_REGS + ireg].addr = reg_position;
> 	  reg_position -= mips_abi_regsize (gdbarch);
> 	}
>   }

this is dead ...

>   /* The MIPS16 entry instruction saves $s0 and $s1 in the reverse
>      order of that normally used by gcc.  Therefore, we have to fetch
>      the first instruction of the function, and if it's an entry
>      instruction that saves $s0 or $s1, correct their saved addresses.  */
>   if (pc_is_mips16 (PROC_LOW_ADDR (proc_desc)))
>     {
>       ULONGEST inst = mips16_fetch_instruction (PROC_LOW_ADDR (proc_desc));
>       if ((inst & 0xf81f) == 0xe809 && (inst & 0x700) != 0x700)
> 	/* entry */
> 	{
> 	  int reg;
> 	  int sreg_count = (inst >> 6) & 3;
> 
> 	  /* Check if the ra register was pushed on the stack.  */
> 	  CORE_ADDR reg_position = (cache->base
> 				    + PROC_REG_OFFSET (proc_desc));
> 	  if (inst & 0x20)
> 	    reg_position -= mips_abi_regsize (gdbarch);
> 
> 	  /* Check if the s0 and s1 registers were pushed on the
> 	     stack.  */
> 	  /* NOTE: cagney/2004-02-08: Huh?  This is doing no such
>              check.  */
> 	  for (reg = 16; reg < sreg_count + 16; reg++)
> 	    {
> 	      cache->saved_regs[NUM_REGS + reg].addr = reg_position;
> 	      reg_position -= mips_abi_regsize (gdbarch);
> 	    }
> 	}
>     }

this is redundant (We've got the same loop (or equivalent)) in the 
mips32_heuristic_proc_desc.

>   /* Fill in the offsets for the registers which float_mask says were
>      saved.  */
>   {
>     CORE_ADDR reg_position = (cache->base
> 			      + PROC_FREG_OFFSET (proc_desc));
>     int ireg;
>     /* Fill in the offsets for the float registers which float_mask
>        says were saved.  */
>     for (ireg = MIPS_NUMREGS - 1; float_mask; --ireg, float_mask <<= 1)
>       if (float_mask & 0x80000000)
> 	{
> 	  if (mips_abi_regsize (gdbarch) == 4
> 	      && TARGET_BYTE_ORDER == BFD_ENDIAN_BIG)
> 	    {
> 	      /* On a big endian 32 bit ABI, floating point registers
> 	         are paired to form doubles such that the most
> 	         significant part is in $f[N+1] and the least
> 	         significant in $f[N] vis: $f[N+1] ||| $f[N].  The
> 	         registers are also spilled as a pair and stored as a
> 	         double.
> 
> 	         When little-endian the least significant part is
> 	         stored first leading to the memory order $f[N] and
> 	         then $f[N+1].
> 
> 	         Unfortunately, when big-endian the most significant
> 	         part of the double is stored first, and the least
> 	         significant is stored second.  This leads to the
> 	         registers being ordered in memory as firt $f[N+1] and
> 	         then $f[N].
> 
> 	         For the big-endian case make certain that the
> 	         addresses point at the correct (swapped) locations
> 	         $f[N] and $f[N+1] pair (keep in mind that
> 	         reg_position is decremented each time through the
> 	         loop).  */
> 	      if ((ireg & 1))
> 		cache->saved_regs[NUM_REGS + mips_regnum (current_gdbarch)->fp0 + ireg]
> 		  .addr = reg_position - mips_abi_regsize (gdbarch);
> 	      else
> 		cache->saved_regs[NUM_REGS + mips_regnum (current_gdbarch)->fp0 + ireg]
> 		  .addr = reg_position + mips_abi_regsize (gdbarch);
> 	    }
> 	  else
> 	    cache->saved_regs[NUM_REGS + mips_regnum (current_gdbarch)->fp0 + ireg]
> 	      .addr = reg_position;
> 	  reg_position -= mips_abi_regsize (gdbarch);
> 	}
> 
>     cache->saved_regs[NUM_REGS + mips_regnum (current_gdbarch)->pc]
>       = cache->saved_regs[NUM_REGS + RA_REGNUM];
>   }
> 
>   /* SP_REGNUM, contains the value and not the address.  */
>   trad_frame_set_value (cache->saved_regs, NUM_REGS + MIPS_SP_REGNUM, cache->base);
> 
>   return (*this_cache);
> }

and so it goes

have fun!
Andrew

Re: [RFA/RFC] fix problems with unwinder on mips-irix

[Joel Brobecker]

Andrew,

> Yes, sometimes inlineing doesn't help, here it does.  There's really no 
> value in trying to preserve this code so be brutal.

Thanks a lot for the detailed message. Really appreciated. I'm trying to
find my way through all this. Let's first look at inlining find_proc_desc.
I'll work on the rest as soon as I have this one figured out.

Reading your last commit to this file, I discovered that you added frame
sniffers, so, if I understand correctly, we can now more or less predict
the circumstances under which find_proc_desc should be called (heuristic
vs non-heuristic). Is that right?

find_proc_desc is called by 4 routines:

  1. mips_mdebug_frame_cache

      /* Get the mdebug proc descriptor.  */
      proc_desc = find_proc_desc (frame_pc_unwind (next_frame), next_frame, 1);

     In that case, I think this call can be replaced by a call to
     non_heuristic_proc_desc? How about the handling this case:

      /* IF this is the topmost frame AND
       * (this proc does not have debugging information OR
       * the PC is in the procedure prologue)
       * THEN create a "heuristic" proc_desc (by analyzing
       * the actual code) to replace the "official" proc_desc.
       */
  
  2. mips_insn16_frame_cache
  3. mips_insn32_frame_cache

     In these two cases, the call to find_proc_desc can be reduced to
     the case where the heuristics have to be used. You said it can be
     inline using something like this:

      if (startaddr == 0)
        startaddr = heuristic_proc_start (pc);

      proc_desc = heuristic_proc_desc (startaddr, pc, next_frame, cur_frame);

     I see that linked_proc_desc_table is never used, which explains
     why we can get rid of:

      /* Is linked_proc_desc_table really necessary?  It only seems to be used
         by procedure call dummys.  However, the procedures being called ought
         to have their own proc_descs, and even if they don't,
         heuristic_proc_desc knows how to create them! */

      struct linked_proc_info *link;

      for (link = linked_proc_desc_table; link; link = link->next)
        if (PROC_LOW_ADDR (&link->info) <= pc
            && PROC_HIGH_ADDR (&link->info) > pc)
          return &link->info;
  
  4. after_prologue

     So far so good. But there there is the case of after_prologue:

        /* Pass cur_frame == 0 to find_proc_desc.  We should not attempt
           to read the stack pointer from the current machine state, because
           the current machine state has nothing to do with the information
           we need from the proc_desc; and the process may or may not exist
           right now.  */
        if (!proc_desc)
          proc_desc = find_proc_desc (pc, NULL, 0);

     The only place where this function is called is in mips_skip_prologue:

        CORE_ADDR post_prologue_pc = after_prologue (pc, NULL);


     So arguably we could remove this extra parameter from
     after_prologue. Should we do this?

     Back to find_proc_desc, I suppose the above code should be replaced
     by something like this:

        if (!proc_desc)
          proc_desc = non_heuristic_proc_desc (pc, &startaddr);

        if (!proc_desc)
          proc_desc = heuristic_proc_desc (pc);
          
     Would that be right? Same question as in point 1 above, actually.
     
Thanks,
-- 
Joel

Re: [RFA/RFC] fix problems with unwinder on mips-irix

[Andrew Cagney]

[something ate some e-mail during a local mailer upgrade, I'm replying 
using what I saw on the web]

> Reading your last commit to this file, I discovered that you added frame
> sniffers, so, if I understand correctly, we can now more or less predict
> the circumstances under which find_proc_desc should be called (heuristic
> vs non-heuristic). Is that right?

Right, but it is stronger than more-or-less :-)  mips_insn32_frame_cache 
need only worry about 32-bit sized instructions and a heuristic prologue 
analysis.

As for inlineing, remember the focus is solely on 
mips_insn32_frame_cache as that's the one with the immediate bug:

>   3. mips_insn32_frame_cache
> 
>      In these two cases, the call to find_proc_desc can be reduced to
>      the case where the heuristics have to be used. You said it can be
>      inline using something like this:
> 
>       if (startaddr == 0)
>         startaddr = heuristic_proc_start (pc);
> 
>       proc_desc = heuristic_proc_desc (startaddr, pc, next_frame, cur_frame);
> 
>      I see that linked_proc_desc_table is never used, which explains
>      why we can get rid of:
> 
>       /* Is linked_proc_desc_table really necessary?  It only seems to be used
>          by procedure call dummys.  However, the procedures being called ought
>          to have their own proc_descs, and even if they don't,
>          heuristic_proc_desc knows how to create them! */
> 
>       struct linked_proc_info *link;
> 
>       for (link = linked_proc_desc_table; link; link = link->next)
>         if (PROC_LOW_ADDR (&link->info) <= pc
>             && PROC_HIGH_ADDR (&link->info) > pc)
>           return &link->info;

as for the other potential inlines, I'd let them be.  The'll follow in 
their own good time.

Andrew

Re: [RFA/RFC] fix problems with unwinder on mips-irix

[Joel Brobecker]

Andrew,

FYI: This is something I have started investigating but can't finish
just yet because I have to move off to something else. You said:

> This kernel_trap stuff isn't applicable here.
> 
> >  kernel_trap = PROC_REG_MASK (proc_desc) & 1;
> >  gen_mask = kernel_trap ? 0xFFFFFFFF : PROC_REG_MASK (proc_desc);
> >  float_mask = kernel_trap ? 0xFFFFFFFF : PROC_FREG_MASK (proc_desc);

But in fact for a reason I don't completely understand just yet, we
do have kernel_trap set occasionally (I measured ~300+ times in our
testsuite). For instance, in all-bin.exp, debugging all-types:

        (gdb) break main
        (gdb) run 
        Starting program: /[...]/all-types 
        
        Breakpoint 1, main () at all-types.c:35
        35          dummy();
        (gdb) next
 !! ->  *** DEBUG: kernel_trap in mips_insn32_frame_cache ***
        36          return 0;

I don't have all the details just yet, but my semi-educated guess
is that the sequence of events that lead to this is the following:
   . User enters next command
   . GDB does a next, and waits for events
   . We land inside dummy (unverified)
   . Put a temporary breakpoint at return address, continue (unverified)
   . Land at return address
   . and somehow find ourselves unwinding past main inside __start.
     (and no, I don't think this qualifies as a kernel trap - but if
     I remove this code, I get lots of new failures)

I'll look at this and all your other suggestion a bit later, when
I have some time again.

-- 
Joel

Re: [RFA/RFC] fix problems with unwinder on mips-irix

[Andrew Cagney]

> Andrew,
> 
> FYI: This is something I have started investigating but can't finish
> just yet because I have to move off to something else. You said:
> 
> 
>>> This kernel_trap stuff isn't applicable here.
>>> 
>>
>>>> >  kernel_trap = PROC_REG_MASK (proc_desc) & 1;
>>>> >  gen_mask = kernel_trap ? 0xFFFFFFFF : PROC_REG_MASK (proc_desc);
>>>> >  float_mask = kernel_trap ? 0xFFFFFFFF : PROC_FREG_MASK (proc_desc);

Ok, first here's why this ``can't happen'':

If mips_insn32_frame_cache gets all the calls inlined we find the 
sequence (taking a birds eye view):

	-- from mips32_heuristic_proc_desc

	clear PROC_DESC
	for (pc = func_start; pc < magic; pc+=4)
	  fetch INSN at PC
	  set PROC_DESC* and TEMP_SAVED_REGS according to INSN

	-- from mips_insn32_frame_cache
	if (in prologue && !``kernel_trap'')
	  for (pc = func_start; pc < current_instruction; pc+=4)
	    fetch INSN at PC
	    set GEN_MASK according to INSN
	else
	  set GEN_MASK according to PROC_DESC

	-- from mips_insn32_frame_cache
	for (reg in gen_mask)
	  set saved regs according to GEN_MASK

what we notice (if bits are examined in detail):

- mips32_heuristic_proc_desc _never_ sets MIPS_REG_MASK&1 (i.e., 
kernel_flag) i.e., for kernel_flag to be set, a true proc_desc or

- TEMP_SAVED_REGS is never read!

- the code packs the saved register information into a scratch PROC_DESC 
only to immediatly turn around and unpack that same information (dumb!)

- when !``In any frame other than the innermost or a frame interrupted 
by a signal, we assume that all registers have been saved.  This assumes 
that all register saves in a function happen before the first function 
call.'' (i.e., when sitting in a prologue) the instructions are iterated 
over _twice_: first using the extreemly complex 
mips32_heuristic_proc_desc; and second by a remarkably simple algorithm.

- regardless of ``In any frame other than the innermost or a frame 
interrupted by a signal, we assume that all registers have been saved. 
This assumes that all register saves in a function happen before the 
first function call.'', the list of saved registers is determined by 
examining the prologue.

 From other architectures we know that there's no reason to 
differentiate between the in and out-of prologue cases and all the above 
can be reduced to:

	for (pc = func_start; pc < current-pc && pc in prologue; pc+=4)
	  fetch instruction at pc
	  according to INSN set saved-regs directly

where the saved reg table is updated directly and PROC_DESC / 
kernel_flag are both eliminated.

--

Now why the theory appears broken:

> But in fact for a reason I don't completely understand just yet, we
> do have kernel_trap set occasionally (I measured ~300+ times in our
> testsuite). For instance, in all-bin.exp, debugging all-types:
> 
>         (gdb) break main
>         (gdb) run 
>         Starting program: /[...]/all-types 
>         
>         Breakpoint 1, main () at all-types.c:35
>         35          dummy();
>         (gdb) next
>  !! ->  *** DEBUG: kernel_trap in mips_insn32_frame_cache ***
>         36          return 0;
> 
> I don't have all the details just yet, but my semi-educated guess
> is that the sequence of events that lead to this is the following:
>    . User enters next command
>    . GDB does a next, and waits for events
>    . We land inside dummy (unverified)

That would trigger a frame unwind (to get the caller PC so that the 
return-address breakpoint can be set) which most likely goes through 
this path:

> static const struct frame_unwind *
> mips_mdebug_frame_sniffer (struct frame_info *next_frame)
> ...
>   /* In any frame other than the innermost or a frame interrupted by a
>      signal, we assume that all registers have been saved.  This
>      assumes that all register saves in a function happen before the
>      first function call.  */
>   if (!in_prologue (pc, PROC_LOW_ADDR (proc_desc)))
>     return &mips_mdebug_frame_unwind;
> 
>   return NULL;

and lead to mips_insn32* being chosen.  mips_insn32 would then end up 
going through this path:
> 
> static mips_extra_func_info_t
> find_proc_desc (CORE_ADDR pc, struct frame_info *next_frame, int cur_frame)
> {
>   mips_extra_func_info_t proc_desc;
>   CORE_ADDR startaddr = 0;
> 
>   proc_desc = non_heuristic_proc_desc (pc, &startaddr);
> 
>   if (proc_desc)
>     {
>       /* IF this is the topmost frame AND
>        * (this proc does not have debugging information OR
>        * the PC is in the procedure prologue)
>        * THEN create a "heuristic" proc_desc (by analyzing
>        * the actual code) to replace the "official" proc_desc.
>        */
>       if (next_frame == NULL)
> 	{
> 	  struct symtab_and_line val;
> 	  struct symbol *proc_symbol =
> 	    PROC_DESC_IS_DUMMY (proc_desc) ? 0 : PROC_SYMBOL (proc_desc);
> 
> 	  if (proc_symbol)
> 	    {
> 	      val = find_pc_line (BLOCK_START
> 				  (SYMBOL_BLOCK_VALUE (proc_symbol)), 0);
> 	      val.pc = val.end ? val.end : pc;
> 	    }
> 	  if (!proc_symbol || pc < val.pc)
> 	    {
> 	      mips_extra_func_info_t found_heuristic =
> 		heuristic_proc_desc (PROC_LOW_ADDR (proc_desc),
> 				     pc, next_frame, cur_frame);
> 	      if (found_heuristic)
> 		proc_desc = found_heuristic;
> 	    }
> 	}
>     }
>   else

Remember: there is an mdebug PROC_DESC but it was being ignored; 
next_frame is never NULL.  That leads to the mdebug PROC_DESC being 
used.  Outch!

What I don't get is how come this bit of:

>   /* Not sure exactly what kernel_trap means, but if it means the
>      kernel saves the registers without a prologue doing it, we better
>      not examine the prologue to see whether registers have been saved
>      yet.  */
>   kernel_trap = PROC_REG_MASK (proc_desc) & 1;
>   if (kernel_trap)
>     return &mips_mdebug_frame_unwind;

didn't kick in forcing the use of the mdebug unwinder.

Anyway, eew!

This is where we need to be ruthless with the code - just inline the lot 
because (even when the testresults regress) we can see that it is more 
correct and more maintainable then what we're currently looking at.  We 
need to push the mips-tdep.c file out of the local minima that it has 
become trapped in.

Andrew