Re: [PATCH] Fix prologue analysis for ldr.w and ldrd instruction

[Will Newton <will dot newton at linaro dot org>]

On 7 May 2014 08:41, Taimoor Mirza <tmirza@codesourcery.com> wrote:
> Hi Guys,
>
> We experienced a problem while debugging startup code on Cortex-M targets (LPC4350 and ATSAM3UEK). On stepping through the code, GDB was throwing "invalid memory access" error. We found out that problem
> is coming because of wrong offset calculation for ldr.w and ldrd instruction during prologue analysis in thumb-mode. Below is problematic code in *thumb_analyze_prologue* function:
>
> else if ((insn & 0xff7f) == 0xf85f)    /* ldr.w Rt,<label> */
>         {
>           /* Constant pool loads.  */
>           unsigned int constant;
>           CORE_ADDR loc;
>
>           offset = bits (insn, 0, 11);
>           if (insn & 0x0080)
>         loc = start + 4 + offset;
>           else
>         loc = start + 4 - offset;
>
>           constant = read_memory_unsigned_integer (loc, 4, byte_order);
>           regs[bits (inst2, 12, 15)] = pv_constant (constant);
>         }
>
> The problem is at line *offset = bits (insn, 0, 11);* where it is obtaining offset from first two bytes of instruction that contain opcode of ldr.w instruction. As per Cortex-M reference manual and BFD code, it should be:
>
> offset = bits (inst2, 0, 11);
>
> inst2 contains next two bytes of ldr.w instruction and it is correctly used to get register information. Similarly inst2 should be used to obtain offset. Similar problem exists in ldrd instruction's offset calculation. Below patch provides fix of this problem. Is it OK?
>
> Thanks,
> Taimoor
>
> 2014-05-06  Taimoor Mirza  <tmirza@codesourcery.com>
>
>         gdb/
>         * arm-tdep.c (thumb_analyze_prologue): Fix offset calculation for
>         ldr.w and ldrd instructions.
> ---
>  gdb/arm-tdep.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
> index e3b1c3d..7271777 100644
> --- a/gdb/arm-tdep.c
> +++ b/gdb/arm-tdep.c
> @@ -1071,7 +1071,7 @@ thumb_analyze_prologue (struct gdbarch *gdbarch,
>               unsigned int constant;
>               CORE_ADDR loc;
>
> -             offset = bits (insn, 0, 11);
> +             offset = bits (inst2, 0, 11);
>               if (insn & 0x0080)
>                 loc = start + 4 + offset;
>               else
> @@ -1087,7 +1087,7 @@ thumb_analyze_prologue (struct gdbarch *gdbarch,
>               unsigned int constant;
>               CORE_ADDR loc;
>
> -             offset = bits (insn, 0, 7) << 2;
> +             offset = bits (inst2, 0, 7) << 2;
>               if (insn & 0x0080)
>                 loc = start + 4 + offset;
>               else

This looks good to me, assuming the testsuite was run with no regressions.

-- 
Will Newton
Toolchain Working Group, Linaro