[RFA/mips] Remove mips16 code that seems redundant

[RFA/mips] Remove mips16 code that seems redundant

[Joel Brobecker]

Andrew,

The block quoted in this patch was moved from mips_insn16_frame_cache()
to there (mips16_heuristic_proc_desc), and I was contemplenting the idea
of merging it the block slightly above in the same function that handles
the case when the first instruction is an entry insn. But then, looking
a bit deeper, it really seems to me that everything done in that block
is already done above. No matter how hard I look, I can't see the catch.
Is there any?

If not, I suggest removing this code. Otherwise, I'll do the merge
I thought about.

2004-10-10  Joel Brobecker  <brobecker@gnat.com>

        * mips-tdep.c (mips16_heuristic_proc_desc): Remove redundant code.

What do you think?

Thanks,
-- 
Joel

Re: [RFA/mips] Remove mips16 code that seems redundant

[Andrew Cagney]

This?


   /* The entry instruction is typically the first instruction in a 
function,
      and it stores registers at offsets relative to the value of the old SP
      (before the prologue).  But the value of the sp parameter to this
      function is the new SP (after the prologue has been executed).  So we
      can't calculate those offsets until we've seen the entire prologue,
      and can calculate what the old SP must have been. */
   if (entry_inst != 0)
     {
       int areg_count = (entry_inst >> 8) & 7;
       int sreg_count = (entry_inst >> 6) & 3;

       /* The entry instruction always subtracts 32 from the SP.  */
       PROC_FRAME_OFFSET (&temp_proc_desc) += 32;

       /* Now we can calculate what the SP must have been at the
          start of the function prologue.  */
       sp += PROC_FRAME_OFFSET (&temp_proc_desc);

       /* Check if a0-a3 were saved in the caller's argument save area.  */
       for (reg = 4, offset = 0; reg < areg_count + 4; reg++)
         {
           PROC_REG_MASK (&temp_proc_desc) |= 1 << reg;
           set_reg_offset (this_cache, reg, sp + offset);
           offset += mips_abi_regsize (current_gdbarch);
         }

       /* Check if the ra register was pushed on the stack.  */
       offset = -4;
       if (entry_inst & 0x20)
         {
           PROC_REG_MASK (&temp_proc_desc) |= 1 << RA_REGNUM;
           set_reg_offset (this_cache, RA_REGNUM, sp + offset);
           offset -= mips_abi_regsize (current_gdbarch);
         }

       /* Check if the s0 and s1 registers were pushed on the stack.  */
       for (reg = 16; reg < sreg_count + 16; reg++)
         {
           PROC_REG_MASK (&temp_proc_desc) |= 1 << reg;
           set_reg_offset (this_cache, reg, sp + offset);
           offset -= mips_abi_regsize (current_gdbarch);
         }
     }

yes, the code should have only one loop.

Andrew

Re: [RFA/mips] Remove mips16 code that seems redundant

[Joel Brobecker]

> This?

Ah, bummer, keep forgetting the patch (that's because my mind is
already further ahead - sorry).

Here is the patch:

Index: mips-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/mips-tdep.c,v
retrieving revision 1.328
diff -u -p -r1.328 mips-tdep.c
--- mips-tdep.c	11 Oct 2004 01:00:57 -0000	1.328
+++ mips-tdep.c	11 Oct 2004 01:10:54 -0000
@@ -2409,36 +2409,6 @@ mips16_heuristic_proc_desc (CORE_ADDR st
       this_cache->saved_regs[NUM_REGS + mips_regnum (current_gdbarch)->pc]
         = this_cache->saved_regs[NUM_REGS + RA_REGNUM];
     }
-
-  /* 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.  */
-  /* FIXME: brobecker/2004-10-10: This code was moved here from
-     mips_insn16_frame_cache(), but can be merged with the block above
-     handling entry_inst.  Will be done in a separate pass, to make changes
-     more atomic.  Actually, this code seems completely redundant!  */
-    {
-      ULONGEST inst = mips16_fetch_instruction (start_pc);
-      if ((inst & 0xf81f) == 0xe809 && (inst & 0x700) != 0x700) /* entry */
-	{
-	  int reg;
-	  int sreg_count = (inst >> 6) & 3;
-	  CORE_ADDR reg_position = (this_cache->base);
-
-	  /* Check if the ra register was pushed on the stack.  */
-	  if (inst & 0x20)
-	    reg_position -= mips_abi_regsize (current_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++)
-	    {
-	      this_cache->saved_regs[NUM_REGS + reg].addr = reg_position;
-	      reg_position -= mips_abi_regsize (current_gdbarch);
-	    }
-	}
-    }
 }
 
 /* Mark all the registers as unset in the saved_regs array

>   /* The entry instruction is typically the first instruction in a 
> function,
>      and it stores registers at offsets relative to the value of the old SP
>      (before the prologue).  But the value of the sp parameter to this
>      function is the new SP (after the prologue has been executed).  So we
>      can't calculate those offsets until we've seen the entire prologue,
>      and can calculate what the old SP must have been. */
>   if (entry_inst != 0)
>     {
>       int areg_count = (entry_inst >> 8) & 7;
>       int sreg_count = (entry_inst >> 6) & 3;
> 
>       /* The entry instruction always subtracts 32 from the SP.  */
>       PROC_FRAME_OFFSET (&temp_proc_desc) += 32;
> 
>       /* Now we can calculate what the SP must have been at the
>          start of the function prologue.  */
>       sp += PROC_FRAME_OFFSET (&temp_proc_desc);
> 
>       /* Check if a0-a3 were saved in the caller's argument save area.  */
>       for (reg = 4, offset = 0; reg < areg_count + 4; reg++)
>         {
>           PROC_REG_MASK (&temp_proc_desc) |= 1 << reg;
>           set_reg_offset (this_cache, reg, sp + offset);
>           offset += mips_abi_regsize (current_gdbarch);
>         }
> 
>       /* Check if the ra register was pushed on the stack.  */
>       offset = -4;
>       if (entry_inst & 0x20)
>         {
>           PROC_REG_MASK (&temp_proc_desc) |= 1 << RA_REGNUM;
>           set_reg_offset (this_cache, RA_REGNUM, sp + offset);
>           offset -= mips_abi_regsize (current_gdbarch);
>         }
> 
>       /* Check if the s0 and s1 registers were pushed on the stack.  */
>       for (reg = 16; reg < sreg_count + 16; reg++)
>         {
>           PROC_REG_MASK (&temp_proc_desc) |= 1 << reg;
>           set_reg_offset (this_cache, reg, sp + offset);
>           offset -= mips_abi_regsize (current_gdbarch);
>         }
>     }
> 
> yes, the code should have only one loop.

Which loop where you refering to?

-- 
Joel

Re: [RFA/mips] Remove mips16 code that seems redundant

[Andrew Cagney]

>>This?
> 
> 
> Ah, bummer, keep forgetting the patch (that's because my mind is
> already further ahead - sorry).
> 
> Here is the patch:

Ah, that, I need to do an update.  Still ok.

Andrew
.

> Index: mips-tdep.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/mips-tdep.c,v
> retrieving revision 1.328
> diff -u -p -r1.328 mips-tdep.c
> --- mips-tdep.c	11 Oct 2004 01:00:57 -0000	1.328
> +++ mips-tdep.c	11 Oct 2004 01:10:54 -0000
> @@ -2409,36 +2409,6 @@ mips16_heuristic_proc_desc (CORE_ADDR st
>        this_cache->saved_regs[NUM_REGS + mips_regnum (current_gdbarch)->pc]
>          = this_cache->saved_regs[NUM_REGS + RA_REGNUM];
>      }
> -
> -  /* 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.  */
> -  /* FIXME: brobecker/2004-10-10: This code was moved here from
> -     mips_insn16_frame_cache(), but can be merged with the block above
> -     handling entry_inst.  Will be done in a separate pass, to make changes
> -     more atomic.  Actually, this code seems completely redundant!  */

Re: [RFA/mips] Remove mips16 code that seems redundant

[Joel Brobecker]

> Ah, that, I need to do an update.  Still ok.

Thanks! I have committed this change:

2004-10-10  Joel Brobecker  <brobecker@gnat.com>

        * mips-tdep.c (mips16_scan_prologue): Remove redundant code.

(just to note the slight change in the function name, since a rename
was made since then - patch is identical so not reproduced here).

-- 
Joel