From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 9773 invoked by alias); 19 Dec 2001 18:22:59 -0000 Mailing-List: contact gdb-patches-help@sources.redhat.com; run by ezmlm Precedence: bulk List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sources.redhat.com Received: (qmail 9749 invoked from network); 19 Dec 2001 18:22:54 -0000 Received: from unknown (HELO cygnus.com) (205.180.230.5) by sources.redhat.com with SMTP; 19 Dec 2001 18:22:54 -0000 Received: from redhat.com (totem.toronto.redhat.com [172.16.14.242]) by runyon.cygnus.com (8.8.7-cygnus/8.8.7) with ESMTP id KAA27698; Wed, 19 Dec 2001 10:21:29 -0800 (PST) Message-ID: <3C20DAA9.D8833BA0@redhat.com> Date: Wed, 19 Dec 2001 10:22:00 -0000 From: Fernando Nasser Organization: Red Hat , Inc. - Toronto X-Mailer: Mozilla 4.78 [en] (X11; U; Linux 2.4.7-10smp i686) X-Accept-Language: en MIME-Version: 1.0 To: Kevin Buettner CC: gdb-patches@sources.redhat.com Subject: Re: [PATCH RFA] arm-tdep.c: prologue scanner adjustments References: <1011127063801.ZM10619@ocotillo.lan> Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit X-SW-Source: 2001-12/txt/msg00471.txt.bz2 This patch actually requires http://sources.redhat.com/ml/gdb-patches/2001-12/msg00349.html After that one is applied it goes nice (otherwise it uncovers the problem fixed by the other patch in a nasty way that prevents the testsuite to run). It is approved to be applied after that one or as a combined patch (I tested Kevin's combined patch already). Thanks for the fixes Kevin. Once more. Fernando Kevin Buettner wrote: > > I've come across a case where the current ARM prologue scanner is > deficient. Here's the prologue in question... > > 0x400ef260 <__sigsuspend>: stmdb sp!, {r4, r5, r10, lr} > 0x400ef264 <__sigsuspend+4>: mov r4, r0 > 0x400ef268 <__sigsuspend+8>: > ldr r10, [pc, #144] ; 0x400ef300 <__sigsuspend+160> > 0x400ef26c <__sigsuspend+12>: > ldr r3, [pc, #144] ; 0x400ef304 <__sigsuspend+164> > 0x400ef270 <__sigsuspend+16>: add r10, pc, r10 > 0x400ef274 <__sigsuspend+20>: ldr r5, [r10, r3] > 0x400ef278 <__sigsuspend+24>: ldr r2, [r5] > 0x400ef27c <__sigsuspend+28>: cmp r2, #0 ; 0x0 > > It turns out that the ARM prologue scanner has two problems with > the above prologue. > > First, it does not start with "mov ip, sp". I've read the current > ABI documents and, while this may have been mandatory at one time, > I can't find any language which requires that this be the first > instruction in the prologue. I've revised the prologue scanner to > make it optional. > > Second, it appears to me that the "no frame pointer" case has never > been tested. The code which sets the frame offset will incorrectly > compute negative value which leads to some pretty weird behavior when > attempting to do a backtrace. > > I'm attaching two versions of the patch. The first will be of interest > to those of you examining this patch either for correctness or to just > see the salient changes. It uses the -w switch which causes the whitespace > caused by the (right) shifting of many lines to be ignored. The second > version of the patch should be used by anyone wanting to actually apply > the patch to the sources. > > In the testing I've done, this patch fixes a number of FAILs in > linux-dp.exp. I haven't seen any regressions. > > Okay to commit? > > * arm-tdep.c (arm_scan_prologue): Don't require "mov ip, sp" > to be the first instruction in the prologue. Also, revise > the way the frame offset is computed for frameless functions. > > Index: arm-tdep.c > =================================================================== > RCS file: /cvs/src/src/gdb/arm-tdep.c,v > retrieving revision 1.17 > diff -u -w -p -r1.17 arm-tdep.c > --- arm-tdep.c 2001/11/14 08:18:32 1.17 > +++ arm-tdep.c 2001/11/27 06:18:25 > @@ -797,15 +797,19 @@ arm_scan_prologue (struct frame_info *fi > traceback. > > In the APCS, the prologue should start with "mov ip, sp" so > - if we don't see this as the first insn, we will stop. */ > + if we don't see this as the first insn, we will stop. [Note: > + This doesn't seem to be true any longer, so it's now an optional > + part of the prologue. - Kevin Buettner, 2001-11-20] */ > > sp_offset = fp_offset = 0; > > if (read_memory_unsigned_integer (prologue_start, 4) > == 0xe1a0c00d) /* mov ip, sp */ > - { > - for (current_pc = prologue_start + 4; current_pc < prologue_end; > - current_pc += 4) > + current_pc = prologue_start + 4; > + else > + current_pc = prologue_start; > + > + for (; current_pc < prologue_end; current_pc += 4) > { > unsigned int insn = read_memory_unsigned_integer (current_pc, 4); > > @@ -882,13 +886,15 @@ arm_scan_prologue (struct frame_info *fi > so we just skip what we don't recognize. */ > continue; > } > - } > > /* The frame size is just the negative of the offset (from the original SP) > of the last thing thing we pushed on the stack. The frame offset is > [new FP] - [new SP]. */ > fi->framesize = -sp_offset; > + if (fi->framereg == FP_REGNUM) > fi->frameoffset = fp_offset - sp_offset; > + else > + fi->frameoffset = 0; > > save_prologue_cache (fi); > } > > ---- Second version which shows changes due to shifted lines... ---- > > Index: arm-tdep.c > =================================================================== > RCS file: /cvs/src/src/gdb/arm-tdep.c,v > retrieving revision 1.17 > diff -u -p -r1.17 arm-tdep.c > --- arm-tdep.c 2001/11/14 08:18:32 1.17 > +++ arm-tdep.c 2001/11/27 06:16:07 > @@ -797,98 +797,104 @@ arm_scan_prologue (struct frame_info *fi > traceback. > > In the APCS, the prologue should start with "mov ip, sp" so > - if we don't see this as the first insn, we will stop. */ > + if we don't see this as the first insn, we will stop. [Note: > + This doesn't seem to be true any longer, so it's now an optional > + part of the prologue. - Kevin Buettner, 2001-11-20] */ > > sp_offset = fp_offset = 0; > > if (read_memory_unsigned_integer (prologue_start, 4) > == 0xe1a0c00d) /* mov ip, sp */ > + current_pc = prologue_start + 4; > + else > + current_pc = prologue_start; > + > + for (; current_pc < prologue_end; current_pc += 4) > { > - for (current_pc = prologue_start + 4; current_pc < prologue_end; > - current_pc += 4) > + unsigned int insn = read_memory_unsigned_integer (current_pc, 4); > + > + if ((insn & 0xffff0000) == 0xe92d0000) > + /* stmfd sp!, {..., fp, ip, lr, pc} > + or > + stmfd sp!, {a1, a2, a3, a4} */ > { > - unsigned int insn = read_memory_unsigned_integer (current_pc, 4); > + int mask = insn & 0xffff; > > - if ((insn & 0xffff0000) == 0xe92d0000) > - /* stmfd sp!, {..., fp, ip, lr, pc} > - or > - stmfd sp!, {a1, a2, a3, a4} */ > - { > - int mask = insn & 0xffff; > + /* Calculate offsets of saved registers. */ > + for (regno = PC_REGNUM; regno >= 0; regno--) > + if (mask & (1 << regno)) > + { > + sp_offset -= 4; > + fi->fsr.regs[regno] = sp_offset; > + } > + } > + else if ((insn & 0xfffff000) == 0xe24cb000) /* sub fp, ip #n */ > + { > + unsigned imm = insn & 0xff; /* immediate value */ > + unsigned rot = (insn & 0xf00) >> 7; /* rotate amount */ > + imm = (imm >> rot) | (imm << (32 - rot)); > + fp_offset = -imm; > + fi->framereg = FP_REGNUM; > + } > + else if ((insn & 0xfffff000) == 0xe24dd000) /* sub sp, sp #n */ > + { > + unsigned imm = insn & 0xff; /* immediate value */ > + unsigned rot = (insn & 0xf00) >> 7; /* rotate amount */ > + imm = (imm >> rot) | (imm << (32 - rot)); > + sp_offset -= imm; > + } > + else if ((insn & 0xffff7fff) == 0xed6d0103) /* stfe f?, [sp, -#c]! */ > + { > + sp_offset -= 12; > + regno = F0_REGNUM + ((insn >> 12) & 0x07); > + fi->fsr.regs[regno] = sp_offset; > + } > + else if ((insn & 0xffbf0fff) == 0xec2d0200) /* sfmfd f0, 4, [sp!] */ > + { > + int n_saved_fp_regs; > + unsigned int fp_start_reg, fp_bound_reg; > > - /* Calculate offsets of saved registers. */ > - for (regno = PC_REGNUM; regno >= 0; regno--) > - if (mask & (1 << regno)) > - { > - sp_offset -= 4; > - fi->fsr.regs[regno] = sp_offset; > - } > - } > - else if ((insn & 0xfffff000) == 0xe24cb000) /* sub fp, ip #n */ > + if ((insn & 0x800) == 0x800) /* N0 is set */ > { > - unsigned imm = insn & 0xff; /* immediate value */ > - unsigned rot = (insn & 0xf00) >> 7; /* rotate amount */ > - imm = (imm >> rot) | (imm << (32 - rot)); > - fp_offset = -imm; > - fi->framereg = FP_REGNUM; > + if ((insn & 0x40000) == 0x40000) /* N1 is set */ > + n_saved_fp_regs = 3; > + else > + n_saved_fp_regs = 1; > } > - else if ((insn & 0xfffff000) == 0xe24dd000) /* sub sp, sp #n */ > + else > { > - unsigned imm = insn & 0xff; /* immediate value */ > - unsigned rot = (insn & 0xf00) >> 7; /* rotate amount */ > - imm = (imm >> rot) | (imm << (32 - rot)); > - sp_offset -= imm; > + if ((insn & 0x40000) == 0x40000) /* N1 is set */ > + n_saved_fp_regs = 2; > + else > + n_saved_fp_regs = 4; > } > - else if ((insn & 0xffff7fff) == 0xed6d0103) /* stfe f?, [sp, -#c]! */ > + > + fp_start_reg = F0_REGNUM + ((insn >> 12) & 0x7); > + fp_bound_reg = fp_start_reg + n_saved_fp_regs; > + for (; fp_start_reg < fp_bound_reg; fp_start_reg++) > { > sp_offset -= 12; > - regno = F0_REGNUM + ((insn >> 12) & 0x07); > - fi->fsr.regs[regno] = sp_offset; > + fi->fsr.regs[fp_start_reg++] = sp_offset; > } > - else if ((insn & 0xffbf0fff) == 0xec2d0200) /* sfmfd f0, 4, [sp!] */ > - { > - int n_saved_fp_regs; > - unsigned int fp_start_reg, fp_bound_reg; > - > - if ((insn & 0x800) == 0x800) /* N0 is set */ > - { > - if ((insn & 0x40000) == 0x40000) /* N1 is set */ > - n_saved_fp_regs = 3; > - else > - n_saved_fp_regs = 1; > - } > - else > - { > - if ((insn & 0x40000) == 0x40000) /* N1 is set */ > - n_saved_fp_regs = 2; > - else > - n_saved_fp_regs = 4; > - } > - > - fp_start_reg = F0_REGNUM + ((insn >> 12) & 0x7); > - fp_bound_reg = fp_start_reg + n_saved_fp_regs; > - for (; fp_start_reg < fp_bound_reg; fp_start_reg++) > - { > - sp_offset -= 12; > - fi->fsr.regs[fp_start_reg++] = sp_offset; > - } > - } > - else if ((insn & 0xf0000000) != 0xe0000000) > - break; /* Condition not true, exit early */ > - else if ((insn & 0xfe200000) == 0xe8200000) /* ldm? */ > - break; /* Don't scan past a block load */ > - else > - /* The optimizer might shove anything into the prologue, > - so we just skip what we don't recognize. */ > - continue; > } > + else if ((insn & 0xf0000000) != 0xe0000000) > + break; /* Condition not true, exit early */ > + else if ((insn & 0xfe200000) == 0xe8200000) /* ldm? */ > + break; /* Don't scan past a block load */ > + else > + /* The optimizer might shove anything into the prologue, > + so we just skip what we don't recognize. */ > + continue; > } > > /* The frame size is just the negative of the offset (from the original SP) > of the last thing thing we pushed on the stack. The frame offset is > [new FP] - [new SP]. */ > fi->framesize = -sp_offset; > - fi->frameoffset = fp_offset - sp_offset; > + if (fi->framereg == FP_REGNUM) > + fi->frameoffset = fp_offset - sp_offset; > + else > + fi->frameoffset = 0; > > save_prologue_cache (fi); > } -- Fernando Nasser Red Hat - Toronto E-Mail: fnasser@redhat.com 2323 Yonge Street, Suite #300 Toronto, Ontario M4P 2C9