Re: [PATCH V3] gdb/arm-tdep: fix to adapt insn "str rd,[sp, #-imm]!"

[Joel Brobecker <brobecker at adacore dot com>]

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