RE: [committed] Fix MIPS frame prologue scan problem

["Pierre Muller" <pierre dot muller at ics-cnrs dot unistra dot fr>]

Thank you so much for fixing this issue!

I can confirm that after your patch, I can use 
unmodified GDB CVS tree to generate a GDB executable
that is able to correctly handle the prologue generated by the
free pascal compiler which uses

Dump of assembler code for function TOPTION__INTERPRET_OPTION:
   0x005ef624 <+0>:     addiu   sp,sp,-1928
   0x005ef628 <+4>:     sw      ra,212(sp)
   0x005ef62c <+8>:     sw      s8,208(sp)
   0x005ef630 <+12>:    addiu   s8,sp,1928

  The value of $fp is equal to $s8
after your patch, which makes it undistinguishable
from the patch I proposed...


I did not find any gdb_7_6-branch yet, which probably means
that this will be in 7.6 release, no?

  Let me end by thanking you again  


Pierre Muller
as Free Pascal core developer


> -----Message d'origine-----
> De?: gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] De la part de Maciej W. Rozycki
> Envoyé?: dimanche 24 février 2013 13:55
> À?: Pierre Muller
> Cc?: gdb-patches@sourceware.org
> Objet?: [committed] Fix MIPS frame prologue scan problem
> 
> On Mon, 26 Nov 2012, Maciej W. Rozycki wrote:
> 
> > > > But in mips32_scan_prologue,
> > > > the first
> > > >   ADDI $s8,$sp,LocalSize
> > > > instruction,
> > > >   interpreted it in mips32_scan_prologue function
> > > > but ended up with a wrong position of my
> > > > non-ABI standard frame pointer
> > > > because it changed frame pointer register from sp to s8 register,
> > > > but kept frame_offset value as set by the
> > > >   SUBI $sp, $sp, LocalSize
> > > > instruction
> > > >   analyzed before.
> > > >
> > > >   Thus GDB wrongly ends up with a
> > > > frame pointer located a
> > > >   value of $s8 register (as from ADDI instruction analysis)
> > > > + LocalSize (from SUBI instruction)
> > > >
> > > >   This means that of
> > > >   $sp is say at address addr
> > > >   $s8  is at addr +LocalSize
> > > > and the virtual frame pointer
> > > >   $fp at  $s8 + LocalSize = addr + 2 * LocalSize
> > > >
> > > >
> > > >   This means that it would be better to remove
> > > > analysis of the ADDI $s8, $sp, LocalSize
> > > > than to leave the current behavior.
> > > >
> > > >   I think that we should either use my proposed patch,
> > > > or completely remove the analysis of this ADDI $s8, $sp,
LocalSize...
> >
> >  Given the ambiguities noted above and following the principle of being
> > liberal as to what to accept I agree your proposal makes sense.  I am
> > fairly sure though that in the presence of a hard frame pointer ($fp) it
> > is that value we should rely on as it's there for a reason.
> 
>  Thanks for your patience.  I have now looked through it in depth and your
> observation is actually a regression introduced sometime between 4.16 and
> 4.17, probably when the function was rewritten when MIPS16 support was
> added sometime in 1997 and the original heuristic_proc_desc code forked
> into mips32_heuristic_proc_desc and mips16_heuristic_proc_desc, which is
> how the respective functions were called back then.
> 
>  Before that regression frame_offset was unconditionally forced to zero
> whenever $fp was used to hold the virtual frame pointer -- the only use
> case supported for $fp back then.  Original code read:
> 
>     if (has_frame_reg) {
> 	PROC_FRAME_REG(&temp_proc_desc) = 30;
> 	PROC_FRAME_OFFSET(&temp_proc_desc) = 0;
>     }
>     else {
> 	PROC_FRAME_REG(&temp_proc_desc) = SP_REGNUM;
> 	PROC_FRAME_OFFSET(&temp_proc_desc) = frame_size;
>     }
> 
> After the problematic change the PROC_FRAME_OFFSET(&temp_proc_desc) = 0
> assignment was lost.
> 
>  I have therefore applied the change below; no change is needed for MIPS16
> or microMIPS analysers as they use different code -- with frame_adjust
> holding the extra frame adjustment in the corresponding case (it may make
> sense to make all the three pieces more uniform, but that's another
> matter).
> 
>  This is not a code path that is ever used with modern GCC or it would
> have likely been spotted much earlier than 15 years after the regression
> and it is therefore not really covered by the test suite.  I have run it
> regardless, for the MIPS/Linux target and a couple multilibs, just for the
> peace of mind.
> 
>  Again, thanks for your patience and apologies to take so long.
> 
>   Maciej
> 
> 2013-02-24  Maciej W. Rozycki  <macro@codesourcery.com>
> 
> 	gdb/
> 	* mips-tdep.c (mips32_scan_prologue): Reset frame_offset to zero
> 	if $fp is used as the virtual frame pointer.
> 
> Index: gdb-fsf-trunk-quilt/gdb/mips-tdep.c
> ===================================================================
> --- gdb-fsf-trunk-quilt.orig/gdb/mips-tdep.c	2013-02-24
00:29:58.000000000
> +0000
> +++ gdb-fsf-trunk-quilt/gdb/mips-tdep.c	2013-02-24
01:34:10.834052078
> +0000
> @@ -3316,6 +3316,7 @@ mips32_scan_prologue (struct gdbarch *gd
>  	      frame_reg = 30;
>  	      frame_addr = get_frame_register_signed
>  		(this_frame, gdbarch_num_regs (gdbarch) + 30);
> +	      frame_offset = 0;
> 
>  	      alloca_adjust = (unsigned) (frame_addr - (sp + low_word));
>  	      if (alloca_adjust > 0)