Fix prologue analysis for ldr.w and ldrd instruction

  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(-)
  

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.
  

> > 2014-05-06  Taimoor Mirza  <tmirza@codesourcery.com>
> >
> >         gdb/
> >         * arm-tdep.c (thumb_analyze_prologue): Fix offset calculation for
> >         ldr.w and ldrd instructions.
[...]
> This looks good to me, assuming the testsuite was run with no regressions.

Thanks, Will, for reviewing the patch. I concur on both counts:
patch is approved, but can you please also confirm how the patch
was tested?

Thank you,
  

On 05/07/2014 06:25 PM, Joel Brobecker wrote:
>>> 2014-05-06  Taimoor Mirza  <tmirza@codesourcery.com>
>>>
>>>          gdb/
>>>          * arm-tdep.c (thumb_analyze_prologue): Fix offset calculation for
>>>          ldr.w and ldrd instructions.
> [...]
>> This looks good to me, assuming the testsuite was run with no regressions.
Thanks for the review Will. Yes I ran testsuites.
> Thanks, Will, for reviewing the patch. I concur on both counts:
> patch is approved, but can you please also confirm how the patch
> was tested?
I ran testsuites before and after applying my patch and did not find any 
regression.
>
> Thank you,
  

Joel,

On 05/07/2014 12:33 PM, tmirza wrote:
> On 05/07/2014 06:25 PM, Joel Brobecker wrote:
>>>> 2014-05-06  Taimoor Mirza  <tmirza@codesourcery.com>
>>>>
>>>>          gdb/
>>>>          * arm-tdep.c (thumb_analyze_prologue): Fix offset
>>>> calculation for
>>>>          ldr.w and ldrd instructions.
>> [...]
>>> This looks good to me, assuming the testsuite was run with no
>>> regressions.
> Thanks for the review Will. Yes I ran testsuites.
>> Thanks, Will, for reviewing the patch. I concur on both counts:
>> patch is approved, but can you please also confirm how the patch
>> was tested?
> I ran testsuites before and after applying my patch and did not find any
> regression.
>>
>> Thank you,

Just for the sake of completeness, the patch was tested against real 
boards and a simulator.

Luis
  

> >>>2014-05-06  Taimoor Mirza  <tmirza@codesourcery.com>
> >>>
> >>>         gdb/
> >>>         * arm-tdep.c (thumb_analyze_prologue): Fix offset calculation for
> >>>         ldr.w and ldrd instructions.
> >[...]
> >>This looks good to me, assuming the testsuite was run with no regressions.
> Thanks for the review Will. Yes I ran testsuites.
> >Thanks, Will, for reviewing the patch. I concur on both counts:
> >patch is approved, but can you please also confirm how the patch
> >was tested?
> I ran testsuites before and after applying my patch and did not find
> any regression.

I noticed that there is a new branch on the binutils-gdb repository
called CB-2131, and that branch only contains your patch. Does this
ring a bell to you? Perhaps that's the name of the branch you used
on your end and you pushed it by accident?

Thanks,
  

On 05/19/2014 08:11 PM, Joel Brobecker wrote:
> I noticed that there is a new branch on the binutils-gdb repository
> called CB-2131, and that branch only contains your patch. Does this
> ring a bell to you? Perhaps that's the name of the branch you used
> on your end and you pushed it by accident?
yes, it was pushed accidentally. It was my local branch that I created 
to work on this issue and accidentally pushed it before realizing my 
mistake and pushing changes to origin.

I actually got some error message and I thought its not pushed. Anyways, 
I'll delete this CB-2131 remote branch. Is it Okay?

-Taimoor
  

> yes, it was pushed accidentally. It was my local branch that I
> created to work on this issue and accidentally pushed it before
> realizing my mistake and pushing changes to origin.

Thanks for confirming.

> I actually got some error message and I thought its not pushed.

I think the error message might actually been from the hook.
Something about 00000[...] not being a valid commit?

> Anyways, I'll delete this CB-2131 remote branch. Is it Okay?

Yes, please!

Thank you,
  

On 05/20/2014 05:09 PM, Joel Brobecker wrote:
>> yes, it was pushed accidentally. It was my local branch that I
>> created to work on this issue and accidentally pushed it before
>> realizing my mistake and pushing changes to origin.
>
> Thanks for confirming.
>
>> I actually got some error message and I thought its not pushed.
>
> I think the error message might actually been from the hook.
> Something about 00000[...] not being a valid commit?
yes, IIRC, it was something like this.
>
>> Anyways, I'll delete this CB-2131 remote branch. Is it Okay?
>
> Yes, please!
I have removed CB-2131 branch. kindly confirm its removal. I tried "git 
remote -ra | grep CB" and don't see it anymore.

Thanks,
Taimoor
  

> I have removed CB-2131 branch. kindly confirm its removal. I tried
> "git remote -ra | grep CB" and don't see it anymore.

Confirmed:

    % git fetch -p origin
    From ssh://sourceware.org/git/binutils-gdb
     x [deleted]         (none)     -> origin/CB-2131

Thank you!