This is the mail archive of the gdb-patches@sources.redhat.com mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH RFA] arm-tdep.c: prologue scanner adjustments


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]