[RFA/mips] mips32 frame code simplification...

[RFA/mips] mips32 frame code simplification...

[Joel Brobecker]

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

Hello,

This implements an idea that Andrew suggested a few days ago.
Basically, we're trying to get rid of the faked proc_desc and
compute the frame cache directly. The goal is to simplify the
code, and get rid of a lot of code duplication.

2004-09-14  Joel Brobecker  <brobecker@gnat.com>

        * mips-tdep.c (mips_insn32_frame_cache): Pass frame cache in
        call to heuristic_proc_desc. Remove code that became redundant
        as a consequence.
        (read_next_frame_reg): Remove special case for SP_REGNUM.
        (set_reg_offset): Fix small typo.
        (reset_saved_regs): New procedure.
        (mips32_heuristic_proc_desc): No longer compute a fake
        procedure descriptor. Compute the full frame cache instead.
        Some minor comment reformatting.

Tested on mips-irix. I think it is a considerable improvement in
terms of testsuite results if you compare them to the result one
would obtain before this patch is applied. I didn't have the patience
to run it though, because I knew it would take forever to complete
and return poor results.

If I compare the new results to the ones I obtained with a debugger
containing the equivalent of the patch I submitted a few weeks back
(the one that cured a heuristic-proc-desc warning by fixing the faked
proc_desc), then the results are slightly worse. I think the area were
we regressed in mostly signal handlers, where I have noticed an infinite
loop while doing a backtrace inside one of the signal testcases. We can
fix that later. The good news is that the heuristic-proc-desc problem
I mentioned is now fixed as a side-effect, since we no longer rely on
the synthesized proc desc anymore.

I also have to say that I am saying that they are slight worse, but
that's only a guess. In reality, I had to deactivate the signal tests
because they were causing some "buffer full" error messages, and they
seem to screw dejaGNU big time. So much so that dejagnu is just useless
during the rest of the run. Also, I now get an premature abortion of
the testsuite run like this:

        Running ./gdb.threads/killed.exp ...
        ERROR: (DejaGnu) proc "(gdb) {$}" does not exist.
        The error code is NONE
        The info on the error is:
        close: spawn id exp8 not open
            while executing
        "close -i exp8"
            invoked from within
        "catch "close -i $spawn_id""

So the number if PASSes I am getting is not as high as if I had
been able to run the entire testsuite.

Frankly, I have reached a point with dejagnu where I just want to
throw it out the window, trample it, chop it, burn it, curse it,
bury it face down, etc. I think it's OK for now, but Michael might
want to crucify me for suggesting that it's OK.

I should also note that I am not sure of the effect of this change
to mips16, or the cases when a proc_desc *IS* available:

    @@ -2373,12 +2259,6 @@ read_next_frame_reg (struct frame_info *
           regcache_cooked_read_signed (current_regcache, regno, &val);
           return val;
         }
    -  else if ((regno % NUM_REGS) == MIPS_SP_REGNUM)
    -    /* MIPS_SP_REGNUM is special, its value is stored in saved_regs.
    -       In fact, it is so special that it can even only be fetched
    -       using a raw register number!  Once this code as been converted
    -       to frame-unwind the problem goes away.  */
    -    return frame_unwind_register_signed (fi, regno % NUM_REGS);
       else
         return frame_unwind_register_signed (fi, regno);
 
This chance is necessary once we stop relying on synthesized proc
descriptors for mips32. But I am guessing it breaks at least mips16.
We need to convert mips16 too, but I can't test it. Andrew?

And the last part I'd like to emphasize is the following new function:

    +/* Mark all the registers as unset in the saved_regs array
    +   of THIS_CACHE.  Do nothing if THIS_CACHE is null.  */
    +
    +void
    +reset_saved_regs (struct mips_frame_cache *this_cache)
    +{
    +  if (this_cache == NULL || this_cache->saved_regs == NULL)
    +    return;
    +
    +  {
    +    const int num_regs = NUM_REGS;
    +    int i;
    +
    +    for (i = 0; i < num_regs; i++)
    +      {
    +        this_cache->saved_regs[i].addr = -1;
    +      }
    +  }
    +}
    +

This function is called when we notice a frame using alloca.
The algorithm we use to scan the function prologue actually
re-computes a new frame address, and restarts the scan of
the prologue. If we don't reset the address of the saved_regs,
we hit a small gard inside set_reg_offset that prevents us from
storing the new register addresses when we hit instructions
saving registers on the stack.

My question is: Should we also reset the realreg?

All in all, I am pretty satisfied with this iteration. I think it's
a clear improvement, but that may be a controversial opinion.

Ahem. Ok to apply?

Next for me is to cleanup a bit the testsuite results, by fixing
the bugs that cause testsuite havoc. Once this is done, I can continue
the code cleanup more serenely.

-- 
Joel

[-- Attachment #2: mips-tdep.c.diff --]
[-- Type: text/plain, Size: 11864 bytes --]

Index: mips-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/mips-tdep.c,v
retrieving revision 1.322
diff -u -p -r1.322 mips-tdep.c
--- mips-tdep.c	5 Sep 2004 20:15:40 -0000	1.322
+++ mips-tdep.c	15 Sep 2004 05:59:26 -0000
@@ -2060,6 +2060,7 @@ mips_insn32_frame_cache (struct frame_in
 
   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);
@@ -2073,11 +2074,7 @@ mips_insn32_frame_cache (struct frame_in
     if (start_addr == 0)
       start_addr = heuristic_proc_start (pc);
 
-#ifdef NOT_YET
     proc_desc = heuristic_proc_desc (start_addr, pc, next_frame, *this_cache);
-#else
-    proc_desc = heuristic_proc_desc (start_addr, pc, next_frame, NULL);
-#endif
   }
   
   if (proc_desc == NULL)
@@ -2086,117 +2083,6 @@ mips_insn32_frame_cache (struct frame_in
        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));
-
-  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);
-  
-  /* 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;
-
-      addr = PROC_LOW_ADDR (proc_desc);
-
-      /* Scan through this function's instructions preceding the
-         current PC, and look for those that save registers.  */
-      while (addr < frame_pc_unwind (next_frame))
-	{
-          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);
-	}
-  }
-
-  /* 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)
-	{
-          const int regno =
-            NUM_REGS + mips_regnum (current_gdbarch)->fp0 + ireg;
-
-	  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[regno].addr =
-                  reg_position - mips_abi_regsize (gdbarch);
-	      else
-		cache->saved_regs[regno].addr =
-                 reg_position + mips_abi_regsize (gdbarch);
-	    }
-	  else
-	    cache->saved_regs[regno].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);
 
@@ -2373,12 +2259,6 @@ read_next_frame_reg (struct frame_info *
       regcache_cooked_read_signed (current_regcache, regno, &val);
       return val;
     }
-  else if ((regno % NUM_REGS) == MIPS_SP_REGNUM)
-    /* MIPS_SP_REGNUM is special, its value is stored in saved_regs.
-       In fact, it is so special that it can even only be fetched
-       using a raw register number!  Once this code as been converted
-       to frame-unwind the problem goes away.  */
-    return frame_unwind_register_signed (fi, regno % NUM_REGS);
   else
     return frame_unwind_register_signed (fi, regno);
 
@@ -2456,7 +2336,7 @@ set_reg_offset (struct mips_frame_cache 
 		CORE_ADDR offset)
 {
   if (this_cache != NULL
-      && this_cache->saved_regs[regnum].addr == 0)
+      && this_cache->saved_regs[regnum].addr == -1)
     {
       this_cache->saved_regs[regnum + 0 * NUM_REGS].addr = offset;
       this_cache->saved_regs[regnum + 1 * NUM_REGS].addr = offset;
@@ -2755,6 +2635,26 @@ mips16_heuristic_proc_desc (CORE_ADDR st
     }
 }
 
+/* Mark all the registers as unset in the saved_regs array
+   of THIS_CACHE.  Do nothing if THIS_CACHE is null.  */
+
+void
+reset_saved_regs (struct mips_frame_cache *this_cache)
+{
+  if (this_cache == NULL || this_cache->saved_regs == NULL)
+    return;
+
+  {
+    const int num_regs = NUM_REGS;
+    int i;
+
+    for (i = 0; i < num_regs; i++)
+      {
+        this_cache->saved_regs[i].addr = -1;
+      }
+  }
+}
+
 static void
 mips32_heuristic_proc_desc (CORE_ADDR start_pc, CORE_ADDR limit_pc,
 			    CORE_ADDR sp, struct frame_info *next_frame,
@@ -2762,9 +2662,12 @@ mips32_heuristic_proc_desc (CORE_ADDR st
 {
   CORE_ADDR cur_pc;
   CORE_ADDR frame_addr = 0;	/* Value of $r30. Used by gcc for frame-pointer */
+  long frame_offset;
+  int  frame_reg = MIPS_SP_REGNUM;
+
 restart:
-  PROC_FRAME_OFFSET (&temp_proc_desc) = 0;
-  PROC_FRAME_ADJUST (&temp_proc_desc) = 0;	/* offset of FP from SP */
+
+  frame_offset = 0;
   for (cur_pc = start_pc; cur_pc < limit_pc; cur_pc += MIPS_INSTLEN)
     {
       unsigned long inst, high_word, low_word;
@@ -2783,7 +2686,7 @@ restart:
 	  || high_word == 0x67bd)	/* daddiu $sp,$sp,-i */
 	{
 	  if (low_word & 0x8000)	/* negative stack adjustment? */
-	    PROC_FRAME_OFFSET (&temp_proc_desc) += 0x10000 - low_word;
+            frame_offset += 0x10000 - low_word;
 	  else
 	    /* Exit loop if a positive stack adjustment is found, which
 	       usually means that the stack cleanup code in the function
@@ -2792,34 +2695,37 @@ restart:
 	}
       else if ((high_word & 0xFFE0) == 0xafa0)	/* sw reg,offset($sp) */
 	{
-	  PROC_REG_MASK (&temp_proc_desc) |= 1 << reg;
 	  set_reg_offset (this_cache, 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 (this_cache, 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))
+	  if ((long) low_word != frame_offset)
 	    frame_addr = sp + low_word;
-	  else if (PROC_FRAME_REG (&temp_proc_desc) == MIPS_SP_REGNUM)
+	  else if (frame_reg == MIPS_SP_REGNUM)
 	    {
 	      unsigned alloca_adjust;
-	      PROC_FRAME_REG (&temp_proc_desc) = 30;
+
+	      frame_reg = 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.
-		   */
+                  /* 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;
+                  /* Need to reset the status of all registers.  Otherwise,
+                     we will hit a guard that prevents the new address
+                     for each register to be recomputed during the second
+                     pass.  */
+                  reset_saved_regs (this_cache);
 		  goto restart;
 		}
 	    }
@@ -2830,29 +2736,45 @@ restart:
       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)
+	  if (frame_reg == MIPS_SP_REGNUM)
 	    {
 	      unsigned alloca_adjust;
-	      PROC_FRAME_REG (&temp_proc_desc) = 30;
+
+	      frame_reg = 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;
-		}
+	        {
+                  /* FP > SP + frame_size. This may be because of
+                     an alloca or somethings similar.  Fix sp to
+                     "pre-alloca" value, and try again.  */
+	          sp = frame_addr;
+                  /* Need to reset the status of all registers.  Otherwise,
+                     we will hit a guard that prevents the new address
+                     for each register to be recomputed during the second
+                     pass.  */
+                  reset_saved_regs (this_cache);
+	          goto restart;
+	        }
 	    }
 	}
       else if ((high_word & 0xFFE0) == 0xafc0)	/* sw reg,offset($30) */
 	{
-	  PROC_REG_MASK (&temp_proc_desc) |= 1 << reg;
 	  set_reg_offset (this_cache, reg, frame_addr + low_word);
 	}
     }
+
+  if (this_cache != NULL)
+    {
+      this_cache->base = 
+        (frame_unwind_register_signed (next_frame, NUM_REGS + frame_reg)
+         + frame_offset);
+      /* FIXME: brobecker/2004-09-15: We should be able to get rid of
+         this assignment below, eventually.  But it's still needed
+         for now.  */
+      this_cache->saved_regs[NUM_REGS + mips_regnum (current_gdbarch)->pc]
+        = this_cache->saved_regs[NUM_REGS + RA_REGNUM];
+    }
 }
 
 static mips_extra_func_info_t

Re: [RFA/mips] mips32 frame code simplification...

[Michael Chastain]

Joel Brobecker <brobecker@gnat.com> wrote:
> Frankly, I have reached a point with dejagnu where I just want to
> throw it out the window, trample it, chop it, burn it, curse it,
> bury it face down, etc.

Yeah, welcome to the club.  Here's your membership card and baseball bat.

> I think it's OK for now, but Michael might
> want to crucify me for suggesting that it's OK.

Naw, we always have to start from wherever we are.

In the signals tests, are you getting another exp_continue loop?
I found that "exp_continue -continue_timer" worked well.
See sigstep.exp.

Re: [RFA/mips] mips32 frame code simplification...

[Joel Brobecker]

> > Frankly, I have reached a point with dejagnu where I just want to
> > throw it out the window, trample it, chop it, burn it, curse it,
> > bury it face down, etc.
> 
> Yeah, welcome to the club.  Here's your membership card and baseball bat.

Aaah, feel better now :-).

> > I think it's OK for now, but Michael might
> > want to crucify me for suggesting that it's OK.
> 
> Naw, we always have to start from wherever we are.

Great.

> In the signals tests, are you getting another exp_continue loop?
> I found that "exp_continue -continue_timer" worked well.
> See sigstep.exp.

I don't think that's the problem. I get an infinite loop inside GDB
itself (backtracing from a signal handler I think), and that causes
the expect/dejagnu buffer to get full. And apparently, this just
disables dejagnu for the rest of the run (can't spawn the compiler
nor the debugger anymore for instance). Maybe there is a way of
flushing that buffer when we do a gdb_exit?

-- 
Joel

Re: [RFA/mips] mips32 frame code simplification...

[Michael Chastain]

Joel Brobecker writes:
> I don't think that's the problem. I get an infinite loop inside GDB
> itself (backtracing from a signal handler I think), and that causes
> the expect/dejagnu buffer to get full.

How about:

  set backtrace limit 1000

That would look good in default_gdb_start and mi_gdb_start.

However a backtrace limit of 1000 would still overflow the
20000-character buffer.  Also I've heard that "set backtrace limit"
has a bug these days.

We could also change the individual "backtrace" and "bt" calls to
"backtrace 100".  That's simpler and more effective, but it fixes
just one nit at a time.

> And apparently, this just disables dejagnu for the rest of the run
> (can't spawn the compiler nor the debugger anymore for instance).
> Maybe there is a way of flushing that buffer when we do a gdb_exit?

I don't know enough about expect to get into that area.  :(

Re: [RFA/mips] mips32 frame code simplification...

[Andrew Cagney]

> Ahem. Ok to apply?

Definitly!  This code is almost at the point where I can understand it. 
  Signal handling on IRIX doesn't work anyway.

Andrew

Re: [RFA/mips] mips32 frame code simplification...

[Andrew Cagney]

> Joel Brobecker writes:
> 
>>> I don't think that's the problem. I get an infinite loop inside GDB
>>> itself (backtracing from a signal handler I think), and that causes
>>> the expect/dejagnu buffer to get full.
> 
> 
> How about:
> 
>   set backtrace limit 1000
> 
> That would look good in default_gdb_start and mi_gdb_start.

Someone somewhere pointed out (with a patch) that
	set backtrace limit 1000
didn't actually work!  I've lost the patch but am working on a tested fix.

Now if only I could remember who posted the original patch.

Andrew

> However a backtrace limit of 1000 would still overflow the
> 20000-character buffer.  Also I've heard that "set backtrace limit"
> has a bug these days.
> 
> We could also change the individual "backtrace" and "bt" calls to
> "backtrace 100".  That's simpler and more effective, but it fixes
> just one nit at a time.
> 
> 
>>> And apparently, this just disables dejagnu for the rest of the run
>>> (can't spawn the compiler nor the debugger anymore for instance).
>>> Maybe there is a way of flushing that buffer when we do a gdb_exit?
> 
> 
> I don't know enough about expect to get into that area.  :(
> 

Re: [RFA/mips] mips32 frame code simplification...

[Joel Brobecker]

> >Ahem. Ok to apply?
> 
> Definitly!  This code is almost at the point where I can understand it. 

Great! This one is in. Will you take care of mips16? I suppose I could
look at it, and try to clean it up the same way. But it would be blind
fixing, as I wouldn't be able to test these changes.

>  Signal handling on IRIX doesn't work anyway.

We'll keep them on the list of things to do. We'll revive that port.

-- 
Joel

Re: [RFA/mips] mips32 frame code simplification...

[Daniel Jacobowitz]

On Wed, Sep 15, 2004 at 05:37:33PM -0700, Joel Brobecker wrote:
> > >Ahem. Ok to apply?
> > 
> > Definitly!  This code is almost at the point where I can understand it. 
> 
> Great! This one is in. Will you take care of mips16? I suppose I could
> look at it, and try to clean it up the same way. But it would be blind
> fixing, as I wouldn't be able to test these changes.

FYI, with a little fiddling I believe you can build newlib for mips16
and test it on the simulator.

-- 
Daniel Jacobowitz

Re: [RFA/mips] mips32 frame code simplification...

[Andrew Cagney]

> FYI, with a little fiddling I believe you can build newlib for mips16
> and test it on the simulator.

That's an interesting definition of `a little fiddling' :-)  I'll just 
mimic the insn32 stuff.

Andrew

Re: [RFA/mips] mips32 frame code simplification...

[Daniel Jacobowitz]

On Thu, Sep 16, 2004 at 01:30:46PM -0400, Andrew Cagney wrote:
> >FYI, with a little fiddling I believe you can build newlib for mips16
> >and test it on the simulator.
> 
> That's an interesting definition of `a little fiddling' :-)  I'll just 
> mimic the insn32 stuff.

I thought you just had to turn on the relevant multilib.  I guess I'm
mistaken :-)

-- 
Daniel Jacobowitz