[PATCH V3] gdb/arm-tdep: fix to adapt insn "str rd,[sp, #-imm]!"
Joel Brobecker
brobecker@adacore.com
Sat Feb 1 13:10:00 GMT 2020
Re-sending a smaller version of the email, where I snipped all
of the ChangeLog diff, to keep it under the max email size
for this mailing list.
On Sat, Feb 01, 2020 at 05:02:56PM +0400, Joel Brobecker wrote:
> Hello,
>
> > Compiled the follow test case code with "-mtune=cortex-a15",
> > The function test1 dump core:
> > (gdb) disassemble test1
> > Dump of assembler code for function test1:
> > 0x00400648 <+0>: str r11, [sp, #-8]!
> > 0x0040064c <+4>: str lr, [sp, #4]
> > 0x00400650 <+8>: add r11, sp, #4
> > 0x00400654 <+12>: sub sp, sp, #32
> > => 0x00400658 <+16>: str r0, [r11, #-24] ; 0xffffffe8
> > 0x0040065c <+20>: str r1, [r11, #-28] ; 0xffffffe4
> > 0x00400660 <+24>: str r2, [r11, #-32] ; 0xffffffe0
> >
> > when make a breakpoint at test1 + <+16>, the backtrace with two same
> > of function show_info:
> > (gdb) bt
> > #0 0x00400658 in test1 ()
> > #1 0x00400758 in show_info ()
> > #2 0x00400758 in show_info ()
> > #3 0x0040079c in main ()
> >
> > For the arm_analyze_prologue can't deal with the insn
> > "str r11, [sp, #-8]!",
> > which should change the sp register.
> >
> > The test case code as follow:
> > void test1(int a, int b, int c, int d)
> > {
> > int e = a + b;
> > int f = c + d;
> > int g = e / f;
> > printf("a + b = %d\n", e);
> > printf("c + d = %d\n", f);
> > printf("e / f = %d\n", g);
> > }
> >
> > int show_info(int a, int b, int c)
> > {
> > int d;
> >
> > d = a + c;
> > printf("%d, %d %ld %c\n",a , d , c , b);
> >
> > test1(a, b, c, d);
> >
> > return d;
> > }
> >
> > int main(int argc, char *argv[])
> > {
> > return show_info(1, 2, 3);
> > }
>
> Thank you for the patch!
>
> I think we scan skip the sources from the revision log if
> we turn them into a testcase. Can you do that, and include it
> in this patch, please? In the description, you say to break
> at test1+16, but from what I can tell, if you break on
> the first line of source code of that function (e.g. the
> line that declares the local variable 'e'), the problem should
> show up, right? If confirmed, write your testcase so that
> it can be run on all platforms. There is one option question
> to which I don't have the answer: How do we decide when
> we should add -mtune=cortex-a15? Since I don't know,
> for now, I would write the testcase without. It's not going
> to be purely a reproducer for your specific issue, but it'll
> still be a testcase to have.
>
> I am not sufficiently familiar with the ARM architecture to really
> comment on the correctness of the patch, so I am going to have to
> trust you on it a bit. How did you test this patch. Did you run
> the testsuite? Or any testsuite?
>
> I'd like to suggest you reword the revision log to explain
> the problem with a different perspective. I think you are almost
> there already, but I think we can make it better.
>
> Here is how I usually approach revision logs:
> - First, start with a bit of context;
> - Then show the problematic behavior, followed by what you actually
> expected.
> - Then explain what the issue is;
> - Explain how you fixed it.
>
> Here is a suggestion:
>
> | fix backtrace on ARM due to unrecognized reg save insn in prologue
> |
> | When using the testcase provided in this commit, and compiling it
> | on ARM with "-mtune=cortex-a15", trying to get the backtrace from
> | location <xxx> yields:
> |
> | (gdb) bt
> | #0 0x00400658 in test1 ()
> | #1 0x00400758 in show_info ()
> | #2 0x00400758 in show_info ()
> | #3 0x0040079c in main ()
> |
> | The expected backtrace is:
> |
> | <*** expected backtrace here ***>
> |
> | The problem comes from the fact that GDB doesn't recognize
> | the following prologue instruction at the start of our function...
> |
> | 0x00400648 <+0>: str r11, [sp, #-8]!
> |
> | ... because it only handles -4 offsets. <*** double-check I understood ***>
> |
> | This patch fixes the issue by handling any offset, rather than
> | just an offset of -4.
> |
> | Tested on <xxx>.
> |
> | gdb/ChangeLog:
> |
> | <put gdb ChangeLog entry here>
> |
> | gdb/testsuite/ChangeLog:
> |
> | <put gdb/testsuite ChangeLog entry here>
>
> As you can see, I omitted the sources from the revision log,
> since they are part of the patch, and the sources are not super
> critical in explaining the problem.
>
> A few comments about your patch:
>
> (a) You need to redo the changes to gdb/ChangeLog.
> If you look at the diff below, I think you'll see why.
>
> (b) The arm-tdep.c looks fine, assuming that it has been properly
> tested.
>
> For the avoidance of doubt, this is _not_ an approval of the patch,
> yet. But I don't think you are very far. I'd just like to see
> the final patch, to make sure we don't forget anything.
>
> Thank you!
>
>
>
>
>
>
> > ---
> > gdb/ChangeLog | 16776 +-------------------------------------------------
> > gdb/ChangeLog-2019 | 16780 +++++++++++++++++++++++++++++++++++++++++++++++++++
> > gdb/arm-tdep.c | 7 +-
> > 3 files changed, 16789 insertions(+), 16774 deletions(-)
> > create mode 100644 gdb/ChangeLog-2019
> >
> > diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> > index 1a452be175..731449b6ef 100644
> > --- a/gdb/ChangeLog
> > +++ b/gdb/ChangeLog
[... snip ...]
> > diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
> > index 69c87c5a43..8a9f878b69 100644
> > --- a/gdb/arm-tdep.c
> > +++ b/gdb/arm-tdep.c
> > @@ -1539,12 +1539,13 @@ arm_analyze_prologue (struct gdbarch *gdbarch,
> > regs[rd] = pv_add_constant (regs[bits (insn, 16, 19)], -imm);
> > continue;
> > }
> > - else if ((insn & 0xffff0fff) == 0xe52d0004) /* str Rd,
> > - [sp, #-4]! */
> > + else if ((insn & 0xffff0000) == 0xe52d0000) /* str Rd,
> > + [sp, #-imm]! */
> > {
> > + unsigned imm = insn & 0xfff;
> > if (stack.store_would_trash (regs[ARM_SP_REGNUM]))
> > break;
> > - regs[ARM_SP_REGNUM] = pv_add_constant (regs[ARM_SP_REGNUM], -4);
> > + regs[ARM_SP_REGNUM] = pv_add_constant (regs[ARM_SP_REGNUM], -imm);
> > stack.store (regs[ARM_SP_REGNUM], 4,
> > regs[bits (insn, 12, 15)]);
> > continue;
> > --
> > 2.12.3
>
> --
> Joel
--
Joel
More information about the Gdb-patches
mailing list