This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
RE: [committed] Fix MIPS frame prologue scan problem
- From: "Pierre Muller" <pierre dot muller at ics-cnrs dot unistra dot fr>
- To: "'Maciej W. Rozycki'" <macro at codesourcery dot com>
- Cc: <gdb-patches at sourceware dot org>
- Date: Sun, 24 Feb 2013 22:51:55 +0100
- Subject: RE: [committed] Fix MIPS frame prologue scan problem
- References: <00a501cd495e$db6adea0$92409be0$@muller@ics-cnrs.unistra.fr> <alpine.DEB.1.10.1206212329420.23962@tp.orcam.me.uk> <002a01cd5043$a0e9f310$e2bdd930$@muller@ics-cnrs.unistra.fr> <005901cdcbef$c4a28560$4de79020$@muller@ics-cnrs.unistra.fr> <alpine.DEB.1.10.1211261641120.11298@tp.orcam.me.uk> <alpine.DEB.1.10.1302240235020.6762@tp.orcam.me.uk>
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)