[AArch64] Improve prologue handling (and fix PR26310)

Message ID 20200806175920.8037-1-luis.machado@linaro.org
State New
Headers show
Series
  • [AArch64] Improve prologue handling (and fix PR26310)
Related show

Commit Message

Kevin Buettner via Gdb-patches Aug. 6, 2020, 5:59 p.m.
I initially noticed the problem with the addition of
gdb.dwarf2/dw2-line-number-zero.exp.  The following failures showed up:

FAIL: gdb.dwarf2/dw2-line-number-zero.exp: continue to breakpoint: bar1
FAIL: gdb.dwarf2/dw2-line-number-zero.exp: bar1, 1st next
FAIL: gdb.dwarf2/dw2-line-number-zero.exp: bar1, 2nd next
FAIL: gdb.dwarf2/dw2-line-number-zero.exp: continue to breakpoint: bar2
FAIL: gdb.dwarf2/dw2-line-number-zero.exp: bar2, 1st next
FAIL: gdb.dwarf2/dw2-line-number-zero.exp: bar2, 2nd next

They happen because AArch64's prologue analyzer skips too many instructions
and ends up indicating a stopping point further into user code.

Dump of assembler code for function bar1:
   0x00000000000006f8 <+0>:	stp	x29, x30, [sp, #-16]!
   0x00000000000006fc <+4>:	mov	x29, sp
   0x0000000000000700 <+8>:	mov	w0, #0x1                   	// #1
   0x0000000000000704 <+12>:	bl	0x6e4 <foo>
   0x0000000000000708 <+16>:	mov	w0, #0x2                   	// #2

We should've stopped at 0x700, but the analyzer actually skips
that instruction and stops at 0x704.  Then GDB ends up adjusting
the address further, and pushes the stopping point to 0x708 based on the
SAL information.

I'm not sure if this adjustment to 0x708 is correct though, as it ends up
skipping past a branch. But I'm leaving that aside for now.

One other complicating factor is that GCC seems to be hoisting up instructions
from user code, mixing them up with prologue instructions.

The following patch adjusts the heuristics a little bit, and tracks when the
SP and FP get used.  If we notice an instruction that is not supposed to be
in the prologue, and this happens *after* SP/FP adjustments and saving of
registers, we stop the analysis.

This means, for PR26310, that we will now stop at 0x700.

I've also added a few more unit tests to make sure the updated behavior is
validated.

gdb/ChangeLog:

YYYY-MM-DD  Luis Machado  <luis.machado@linaro.org>

	PR gdb/26310

	* aarch64-tdep.c (aarch64_analyze_prologue): Track use of SP/FP and
	act accordingly.
	(aarch64_analyze_prologue_test): Add more unit tests to exercise
	movz/str/stur/stp skipping behavior.
---
 gdb/aarch64-tdep.c | 134 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 134 insertions(+)

-- 
2.17.1

Comments

Alan Hayward Aug. 10, 2020, 8:49 a.m. | #1
> On 6 Aug 2020, at 18:59, Luis Machado <luis.machado@linaro.org> wrote:

> 

> I initially noticed the problem with the addition of

> gdb.dwarf2/dw2-line-number-zero.exp.  The following failures showed up:

> 

> FAIL: gdb.dwarf2/dw2-line-number-zero.exp: continue to breakpoint: bar1

> FAIL: gdb.dwarf2/dw2-line-number-zero.exp: bar1, 1st next

> FAIL: gdb.dwarf2/dw2-line-number-zero.exp: bar1, 2nd next

> FAIL: gdb.dwarf2/dw2-line-number-zero.exp: continue to breakpoint: bar2

> FAIL: gdb.dwarf2/dw2-line-number-zero.exp: bar2, 1st next

> FAIL: gdb.dwarf2/dw2-line-number-zero.exp: bar2, 2nd next

> 

> They happen because AArch64's prologue analyzer skips too many instructions

> and ends up indicating a stopping point further into user code.

> 

> Dump of assembler code for function bar1:

>   0x00000000000006f8 <+0>:	stp	x29, x30, [sp, #-16]!

>   0x00000000000006fc <+4>:	mov	x29, sp

>   0x0000000000000700 <+8>:	mov	w0, #0x1                   	// #1

>   0x0000000000000704 <+12>:	bl	0x6e4 <foo>

>   0x0000000000000708 <+16>:	mov	w0, #0x2                   	// #2

> 

> We should've stopped at 0x700, but the analyzer actually skips

> that instruction and stops at 0x704.  Then GDB ends up adjusting

> the address further, and pushes the stopping point to 0x708 based on the

> SAL information.

> 

> I'm not sure if this adjustment to 0x708 is correct though, as it ends up

> skipping past a branch. But I'm leaving that aside for now.

> 

> One other complicating factor is that GCC seems to be hoisting up instructions

> from user code, mixing them up with prologue instructions.


Sadly I don’t think there is anything preventing GCC doing more of this in the
future. From what I remember of GCC, there’s no explicit rules in the code
stopping the prologue and the main body mixing. It just never really mixes
because almost all the time there’s no real benefit of hoisting code that high.

I suspect there will be more fixes to this area as GCC (and llvm!) get more
tricksy with their optimisations in the future. My concern is that we’ll get
to a point where it’ll be ambiguous on where GDB needs to stop. Ideally a bug
could be raised against GCC, but it’d require careful wording and I doubt it’d
ever get looked at.

> 

> The following patch adjusts the heuristics a little bit, and tracks when the

> SP and FP get used.  If we notice an instruction that is not supposed to be

> in the prologue, and this happens *after* SP/FP adjustments and saving of

> registers, we stop the analysis.

> 

> This means, for PR26310, that we will now stop at 0x700.


Ok, that seems reasonable.

My other concern is the prologue analyser is just going to grow in size/complexity,
but I can’t see a way around that.


> 

> I've also added a few more unit tests to make sure the updated behavior is

> validated.

> 

> gdb/ChangeLog:

> 

> YYYY-MM-DD  Luis Machado  <luis.machado@linaro.org>

> 

> 	PR gdb/26310

> 

> 	* aarch64-tdep.c (aarch64_analyze_prologue): Track use of SP/FP and

> 	act accordingly.

> 	(aarch64_analyze_prologue_test): Add more unit tests to exercise

> 	movz/str/stur/stp skipping behavior.

> ---

> gdb/aarch64-tdep.c | 134 +++++++++++++++++++++++++++++++++++++++++++++

> 1 file changed, 134 insertions(+)

> 

> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c

> index 5e7d0d0b86..45b2b427c2 100644

> --- a/gdb/aarch64-tdep.c

> +++ b/gdb/aarch64-tdep.c

> @@ -287,6 +287,12 @@ aarch64_analyze_prologue (struct gdbarch *gdbarch,

> {

>   enum bfd_endian byte_order_for_code = gdbarch_byte_order_for_code (gdbarch);

>   int i;

> +

> +  /* Whether the stack has been set.  This should be true when we notice a SP

> +     to FP move or if we are using the SP as the base register for storing

> +     data, in case the FP is ommitted.  */

> +  bool seen_stack_set = false;

> +

>   /* Track X registers and D registers in prologue.  */

>   pv_t regs[AARCH64_X_REGISTER_COUNT + AARCH64_D_REGISTER_COUNT];

> 

> @@ -326,6 +332,10 @@ aarch64_analyze_prologue (struct gdbarch *gdbarch,

> 	      regs[rd] = pv_add_constant (regs[rn],

> 					  -inst.operands[2].imm.value);

> 	    }

> +

> +	  /* Did we move SP to FP?  */

> +	  if (rn == AARCH64_SP_REGNUM && rd == AARCH64_FP_REGNUM)

> +	    seen_stack_set = true;

> 	}

>       else if (inst.opcode->iclass == pcreladdr

> 	       && inst.operands[1].type == AARCH64_OPND_ADDR_ADRP)

> @@ -358,6 +368,12 @@ aarch64_analyze_prologue (struct gdbarch *gdbarch,

>       else if (inst.opcode->op == OP_MOVZ)

> 	{

> 	  gdb_assert (inst.operands[0].type == AARCH64_OPND_Rd);

> +

> +	  /* If this shows up before we set the stack, keep going.  Otherwise

> +	     stop the analysis.  */

> +	  if (seen_stack_set)

> +	    break;

> +

> 	  regs[inst.operands[0].reg.regno] = pv_unknown ();

> 	}

>       else if (inst.opcode->iclass == log_shift

> @@ -399,6 +415,10 @@ aarch64_analyze_prologue (struct gdbarch *gdbarch,

> 	  stack.store

> 	    (pv_add_constant (regs[rn], inst.operands[1].addr.offset.imm),

> 	     size, regs[rt]);

> +

> +	  /* Are we storing with SP as a base?  */

> +	  if (rn == AARCH64_SP_REGNUM)

> +	    seen_stack_set = true;

> 	}

>       else if ((inst.opcode->iclass == ldstpair_off

> 		|| (inst.opcode->iclass == ldstpair_indexed

> @@ -442,6 +462,10 @@ aarch64_analyze_prologue (struct gdbarch *gdbarch,

> 	  if (inst.operands[2].addr.writeback)

> 	    regs[rn] = pv_add_constant (regs[rn], imm);

> 

> +	  /* Ignore the instruction that allocates stack space and sets

> +	     the SP.  */

> +	  if (rn == AARCH64_SP_REGNUM && !inst.operands[2].addr.writeback)

> +	    seen_stack_set = true;

> 	}

>       else if ((inst.opcode->iclass == ldst_imm9 /* Signed immediate.  */

> 		|| (inst.opcode->iclass == ldst_pos /* Unsigned immediate.  */

> @@ -464,6 +488,10 @@ aarch64_analyze_prologue (struct gdbarch *gdbarch,

> 	  stack.store (pv_add_constant (regs[rn], imm), size, regs[rt]);

> 	  if (inst.operands[1].addr.writeback)

> 	    regs[rn] = pv_add_constant (regs[rn], imm);

> +

> +	  /* Are we storing with SP as a base?  */

> +	  if (rn == AARCH64_SP_REGNUM)

> +	    seen_stack_set = true;

> 	}

>       else if (inst.opcode->iclass == testbranch)

> 	{

> @@ -690,6 +718,112 @@ aarch64_analyze_prologue_test (void)

>       }

>   }

> 

> +  /* Test handling of movz before setting the frame pointer.  */

> +  {

> +    static const uint32_t insns[] = {

> +      0xa9bf7bfd, /* stp     x29, x30, [sp, #-16]! */

> +      0x52800020, /* mov     w0, #0x1 */

> +      0x910003fd, /* mov     x29, sp */

> +      0x528000a2, /* mov     w2, #0x5 */


Can you add a comment either here ...

> +      0x97fffff8, /* bl      6e4 */

> +    };

> +

> +    instruction_reader_test reader (insns);

> +

> +    trad_frame_reset_saved_regs (gdbarch, cache.saved_regs);

> +    CORE_ADDR end = aarch64_analyze_prologue (gdbarch, 0, 128, &cache, reader);

> +

> +    SELF_CHECK (end == (4 - 1) * 4);


... or here to make it clear which instruction you expect end to be on.

Likewise for the others below.


Otherwise, everything else looks good.


> +    SELF_CHECK (cache.framereg == AARCH64_FP_REGNUM);

> +    SELF_CHECK (cache.framesize == 16);

> +  }

> +

> +  /* Test handling of movz/stp when using the stack pointer as frame

> +     pointer.  */

> +  {

> +    static const uint32_t insns[] = {

> +      0xa9bc7bfd, /* stp     x29, x30, [sp, #-64]! */

> +      0x52800020, /* mov     w0, #0x1 */

> +      0x290207e0, /* stp     w0, w1, [sp, #16] */

> +      0xa9018fe2, /* stp     x2, x3, [sp, #24] */

> +      0x528000a2, /* mov     w2, #0x5 */

> +      0x97fffff8, /* bl      6e4 */

> +    };

> +

> +    instruction_reader_test reader (insns);

> +

> +    trad_frame_reset_saved_regs (gdbarch, cache.saved_regs);

> +    CORE_ADDR end = aarch64_analyze_prologue (gdbarch, 0, 128, &cache, reader);

> +

> +    SELF_CHECK (end == (5 - 1) * 4);

> +    SELF_CHECK (cache.framereg == AARCH64_SP_REGNUM);

> +    SELF_CHECK (cache.framesize == 64);

> +  }

> +

> +  /* Test handling of movz/str when using the stack pointer as frame

> +     pointer  */

> +  {

> +    static const uint32_t insns[] = {

> +      0xa9bc7bfd, /* stp     x29, x30, [sp, #-64]! */

> +      0x52800020, /* mov     w0, #0x1 */

> +      0xb9002be4, /* str     w4, [sp, #40] */

> +      0xf9001be5, /* str     x5, [sp, #48] */

> +      0x528000a2, /* mov     w2, #0x5 */

> +      0x97fffff8, /* bl      6e4 */

> +    };

> +

> +    instruction_reader_test reader (insns);

> +

> +    trad_frame_reset_saved_regs (gdbarch, cache.saved_regs);

> +    CORE_ADDR end = aarch64_analyze_prologue (gdbarch, 0, 128, &cache, reader);

> +

> +    SELF_CHECK (end == (5 - 1) * 4);

> +    SELF_CHECK (cache.framereg == AARCH64_SP_REGNUM);

> +    SELF_CHECK (cache.framesize == 64);

> +  }

> +

> +  /* Test handling of movz/stur when using the stack pointer as frame

> +     pointer.  */

> +  {

> +    static const uint32_t insns[] = {

> +      0xa9bc7bfd, /* stp     x29, x30, [sp, #-64]! */

> +      0x52800020, /* mov     w0, #0x1 */

> +      0xb80343e6, /* stur    w6, [sp, #52] */

> +      0xf80383e7, /* stur    x7, [sp, #56] */

> +      0x528000a2, /* mov     w2, #0x5 */

> +      0x97fffff8, /* bl      6e4 */

> +    };

> +

> +    instruction_reader_test reader (insns);

> +

> +    trad_frame_reset_saved_regs (gdbarch, cache.saved_regs);

> +    CORE_ADDR end = aarch64_analyze_prologue (gdbarch, 0, 128, &cache, reader);

> +

> +    SELF_CHECK (end == (5 - 1) * 4);

> +    SELF_CHECK (cache.framereg == AARCH64_SP_REGNUM);

> +    SELF_CHECK (cache.framesize == 64);

> +  }

> +

> +  /* Test handling of movz when there is no frame pointer set or no stack

> +     pointer used.  */

> +  {

> +    static const uint32_t insns[] = {

> +      0xa9bf7bfd, /* stp     x29, x30, [sp, #-16]! */

> +      0x52800020, /* mov     w0, #0x1 */

> +      0x528000a2, /* mov     w2, #0x5 */

> +      0x97fffff8, /* bl      6e4 */

> +    };

> +

> +    instruction_reader_test reader (insns);

> +

> +    trad_frame_reset_saved_regs (gdbarch, cache.saved_regs);

> +    CORE_ADDR end = aarch64_analyze_prologue (gdbarch, 0, 128, &cache, reader);

> +

> +    SELF_CHECK (end == (4 - 1) * 4);

> +    SELF_CHECK (cache.framereg == AARCH64_SP_REGNUM);

> +    SELF_CHECK (cache.framesize == 16);

> +  }

> +

>   /* Test a prologue in which there is a return address signing instruction.  */

>   if (tdep->has_pauth ())

>     {

> -- 

> 2.17.1

>
Kevin Buettner via Gdb-patches Aug. 10, 2020, 1:13 p.m. | #2
On 8/10/20 5:49 AM, Alan Hayward wrote:
> 

> 

>> On 6 Aug 2020, at 18:59, Luis Machado <luis.machado@linaro.org> wrote:

>>

>> I initially noticed the problem with the addition of

>> gdb.dwarf2/dw2-line-number-zero.exp.  The following failures showed up:

>>

>> FAIL: gdb.dwarf2/dw2-line-number-zero.exp: continue to breakpoint: bar1

>> FAIL: gdb.dwarf2/dw2-line-number-zero.exp: bar1, 1st next

>> FAIL: gdb.dwarf2/dw2-line-number-zero.exp: bar1, 2nd next

>> FAIL: gdb.dwarf2/dw2-line-number-zero.exp: continue to breakpoint: bar2

>> FAIL: gdb.dwarf2/dw2-line-number-zero.exp: bar2, 1st next

>> FAIL: gdb.dwarf2/dw2-line-number-zero.exp: bar2, 2nd next

>>

>> They happen because AArch64's prologue analyzer skips too many instructions

>> and ends up indicating a stopping point further into user code.

>>

>> Dump of assembler code for function bar1:

>>    0x00000000000006f8 <+0>:	stp	x29, x30, [sp, #-16]!

>>    0x00000000000006fc <+4>:	mov	x29, sp

>>    0x0000000000000700 <+8>:	mov	w0, #0x1                   	// #1

>>    0x0000000000000704 <+12>:	bl	0x6e4 <foo>

>>    0x0000000000000708 <+16>:	mov	w0, #0x2                   	// #2

>>

>> We should've stopped at 0x700, but the analyzer actually skips

>> that instruction and stops at 0x704.  Then GDB ends up adjusting

>> the address further, and pushes the stopping point to 0x708 based on the

>> SAL information.

>>

>> I'm not sure if this adjustment to 0x708 is correct though, as it ends up

>> skipping past a branch. But I'm leaving that aside for now.

>>

>> One other complicating factor is that GCC seems to be hoisting up instructions

>> from user code, mixing them up with prologue instructions.

> 

> Sadly I don’t think there is anything preventing GCC doing more of this in the

> future. From what I remember of GCC, there’s no explicit rules in the code

> stopping the prologue and the main body mixing. It just never really mixes

> because almost all the time there’s no real benefit of hoisting code that high.

> 

> I suspect there will be more fixes to this area as GCC (and llvm!) get more

> tricksy with their optimisations in the future. My concern is that we’ll get

> to a point where it’ll be ambiguous on where GDB needs to stop. Ideally a bug

> could be raised against GCC, but it’d require careful wording and I doubt it’d

> ever get looked at.

> 


Right. For optimized binaries without debug info, all bets are off. GDB 
tends to do prologue analysis on a best effort way... But when proper 
debug info is available, GDB doesn't need to go through the trouble.

>>

>> The following patch adjusts the heuristics a little bit, and tracks when the

>> SP and FP get used.  If we notice an instruction that is not supposed to be

>> in the prologue, and this happens *after* SP/FP adjustments and saving of

>> registers, we stop the analysis.

>>

>> This means, for PR26310, that we will now stop at 0x700.

> 

> Ok, that seems reasonable.

> 

> My other concern is the prologue analyser is just going to grow in size/complexity,

> but I can’t see a way around that.


Indeed. It really depends on how well we want to handle things. Right 
now it seems to do a reasonable job.

Skipping movz instructions seems a bit odd to be honest, as it doesn't 
get used to move SP/FP. It may have been an oversight, or the compilers 
from the initial AArch64 port back then used to produce different 
prologues that used movz.

I checked prologues from glibc/gdb, and I only see movz's for non SP/FP 
registers.

> 

> 

>>

>> I've also added a few more unit tests to make sure the updated behavior is

>> validated.

>>

>> gdb/ChangeLog:

>>

>> YYYY-MM-DD  Luis Machado  <luis.machado@linaro.org>

>>

>> 	PR gdb/26310

>>

>> 	* aarch64-tdep.c (aarch64_analyze_prologue): Track use of SP/FP and

>> 	act accordingly.

>> 	(aarch64_analyze_prologue_test): Add more unit tests to exercise

>> 	movz/str/stur/stp skipping behavior.

>> ---

>> gdb/aarch64-tdep.c | 134 +++++++++++++++++++++++++++++++++++++++++++++

>> 1 file changed, 134 insertions(+)

>>

>> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c

>> index 5e7d0d0b86..45b2b427c2 100644

>> --- a/gdb/aarch64-tdep.c

>> +++ b/gdb/aarch64-tdep.c

>> @@ -287,6 +287,12 @@ aarch64_analyze_prologue (struct gdbarch *gdbarch,

>> {

>>    enum bfd_endian byte_order_for_code = gdbarch_byte_order_for_code (gdbarch);

>>    int i;

>> +

>> +  /* Whether the stack has been set.  This should be true when we notice a SP

>> +     to FP move or if we are using the SP as the base register for storing

>> +     data, in case the FP is ommitted.  */

>> +  bool seen_stack_set = false;

>> +

>>    /* Track X registers and D registers in prologue.  */

>>    pv_t regs[AARCH64_X_REGISTER_COUNT + AARCH64_D_REGISTER_COUNT];

>>

>> @@ -326,6 +332,10 @@ aarch64_analyze_prologue (struct gdbarch *gdbarch,

>> 	      regs[rd] = pv_add_constant (regs[rn],

>> 					  -inst.operands[2].imm.value);

>> 	    }

>> +

>> +	  /* Did we move SP to FP?  */

>> +	  if (rn == AARCH64_SP_REGNUM && rd == AARCH64_FP_REGNUM)

>> +	    seen_stack_set = true;

>> 	}

>>        else if (inst.opcode->iclass == pcreladdr

>> 	       && inst.operands[1].type == AARCH64_OPND_ADDR_ADRP)

>> @@ -358,6 +368,12 @@ aarch64_analyze_prologue (struct gdbarch *gdbarch,

>>        else if (inst.opcode->op == OP_MOVZ)

>> 	{

>> 	  gdb_assert (inst.operands[0].type == AARCH64_OPND_Rd);

>> +

>> +	  /* If this shows up before we set the stack, keep going.  Otherwise

>> +	     stop the analysis.  */

>> +	  if (seen_stack_set)

>> +	    break;

>> +

>> 	  regs[inst.operands[0].reg.regno] = pv_unknown ();

>> 	}

>>        else if (inst.opcode->iclass == log_shift

>> @@ -399,6 +415,10 @@ aarch64_analyze_prologue (struct gdbarch *gdbarch,

>> 	  stack.store

>> 	    (pv_add_constant (regs[rn], inst.operands[1].addr.offset.imm),

>> 	     size, regs[rt]);

>> +

>> +	  /* Are we storing with SP as a base?  */

>> +	  if (rn == AARCH64_SP_REGNUM)

>> +	    seen_stack_set = true;

>> 	}

>>        else if ((inst.opcode->iclass == ldstpair_off

>> 		|| (inst.opcode->iclass == ldstpair_indexed

>> @@ -442,6 +462,10 @@ aarch64_analyze_prologue (struct gdbarch *gdbarch,

>> 	  if (inst.operands[2].addr.writeback)

>> 	    regs[rn] = pv_add_constant (regs[rn], imm);

>>

>> +	  /* Ignore the instruction that allocates stack space and sets

>> +	     the SP.  */

>> +	  if (rn == AARCH64_SP_REGNUM && !inst.operands[2].addr.writeback)

>> +	    seen_stack_set = true;

>> 	}

>>        else if ((inst.opcode->iclass == ldst_imm9 /* Signed immediate.  */

>> 		|| (inst.opcode->iclass == ldst_pos /* Unsigned immediate.  */

>> @@ -464,6 +488,10 @@ aarch64_analyze_prologue (struct gdbarch *gdbarch,

>> 	  stack.store (pv_add_constant (regs[rn], imm), size, regs[rt]);

>> 	  if (inst.operands[1].addr.writeback)

>> 	    regs[rn] = pv_add_constant (regs[rn], imm);

>> +

>> +	  /* Are we storing with SP as a base?  */

>> +	  if (rn == AARCH64_SP_REGNUM)

>> +	    seen_stack_set = true;

>> 	}

>>        else if (inst.opcode->iclass == testbranch)

>> 	{

>> @@ -690,6 +718,112 @@ aarch64_analyze_prologue_test (void)

>>        }

>>    }

>>

>> +  /* Test handling of movz before setting the frame pointer.  */

>> +  {

>> +    static const uint32_t insns[] = {

>> +      0xa9bf7bfd, /* stp     x29, x30, [sp, #-16]! */

>> +      0x52800020, /* mov     w0, #0x1 */

>> +      0x910003fd, /* mov     x29, sp */

>> +      0x528000a2, /* mov     w2, #0x5 */

> 

> Can you add a comment either here ...

> 

>> +      0x97fffff8, /* bl      6e4 */

>> +    };

>> +

>> +    instruction_reader_test reader (insns);

>> +

>> +    trad_frame_reset_saved_regs (gdbarch, cache.saved_regs);

>> +    CORE_ADDR end = aarch64_analyze_prologue (gdbarch, 0, 128, &cache, reader);

>> +

>> +    SELF_CHECK (end == (4 - 1) * 4);

> 

> ... or here to make it clear which instruction you expect end to be on.

> 

> Likewise for the others below.

> 

> 

> Otherwise, everything else looks good.

> 

> 


I wrote it like that to make it a bit more obvious that it is, for 
example, the 4th instruction rather than the 3rd (0-indexed I guess).

I'll make it a bit more clear.

Thanks!

>> +    SELF_CHECK (cache.framereg == AARCH64_FP_REGNUM);

>> +    SELF_CHECK (cache.framesize == 16);

>> +  }

>> +

>> +  /* Test handling of movz/stp when using the stack pointer as frame

>> +     pointer.  */

>> +  {

>> +    static const uint32_t insns[] = {

>> +      0xa9bc7bfd, /* stp     x29, x30, [sp, #-64]! */

>> +      0x52800020, /* mov     w0, #0x1 */

>> +      0x290207e0, /* stp     w0, w1, [sp, #16] */

>> +      0xa9018fe2, /* stp     x2, x3, [sp, #24] */

>> +      0x528000a2, /* mov     w2, #0x5 */

>> +      0x97fffff8, /* bl      6e4 */

>> +    };

>> +

>> +    instruction_reader_test reader (insns);

>> +

>> +    trad_frame_reset_saved_regs (gdbarch, cache.saved_regs);

>> +    CORE_ADDR end = aarch64_analyze_prologue (gdbarch, 0, 128, &cache, reader);

>> +

>> +    SELF_CHECK (end == (5 - 1) * 4);

>> +    SELF_CHECK (cache.framereg == AARCH64_SP_REGNUM);

>> +    SELF_CHECK (cache.framesize == 64);

>> +  }

>> +

>> +  /* Test handling of movz/str when using the stack pointer as frame

>> +     pointer  */

>> +  {

>> +    static const uint32_t insns[] = {

>> +      0xa9bc7bfd, /* stp     x29, x30, [sp, #-64]! */

>> +      0x52800020, /* mov     w0, #0x1 */

>> +      0xb9002be4, /* str     w4, [sp, #40] */

>> +      0xf9001be5, /* str     x5, [sp, #48] */

>> +      0x528000a2, /* mov     w2, #0x5 */

>> +      0x97fffff8, /* bl      6e4 */

>> +    };

>> +

>> +    instruction_reader_test reader (insns);

>> +

>> +    trad_frame_reset_saved_regs (gdbarch, cache.saved_regs);

>> +    CORE_ADDR end = aarch64_analyze_prologue (gdbarch, 0, 128, &cache, reader);

>> +

>> +    SELF_CHECK (end == (5 - 1) * 4);

>> +    SELF_CHECK (cache.framereg == AARCH64_SP_REGNUM);

>> +    SELF_CHECK (cache.framesize == 64);

>> +  }

>> +

>> +  /* Test handling of movz/stur when using the stack pointer as frame

>> +     pointer.  */

>> +  {

>> +    static const uint32_t insns[] = {

>> +      0xa9bc7bfd, /* stp     x29, x30, [sp, #-64]! */

>> +      0x52800020, /* mov     w0, #0x1 */

>> +      0xb80343e6, /* stur    w6, [sp, #52] */

>> +      0xf80383e7, /* stur    x7, [sp, #56] */

>> +      0x528000a2, /* mov     w2, #0x5 */

>> +      0x97fffff8, /* bl      6e4 */

>> +    };

>> +

>> +    instruction_reader_test reader (insns);

>> +

>> +    trad_frame_reset_saved_regs (gdbarch, cache.saved_regs);

>> +    CORE_ADDR end = aarch64_analyze_prologue (gdbarch, 0, 128, &cache, reader);

>> +

>> +    SELF_CHECK (end == (5 - 1) * 4);

>> +    SELF_CHECK (cache.framereg == AARCH64_SP_REGNUM);

>> +    SELF_CHECK (cache.framesize == 64);

>> +  }

>> +

>> +  /* Test handling of movz when there is no frame pointer set or no stack

>> +     pointer used.  */

>> +  {

>> +    static const uint32_t insns[] = {

>> +      0xa9bf7bfd, /* stp     x29, x30, [sp, #-16]! */

>> +      0x52800020, /* mov     w0, #0x1 */

>> +      0x528000a2, /* mov     w2, #0x5 */

>> +      0x97fffff8, /* bl      6e4 */

>> +    };

>> +

>> +    instruction_reader_test reader (insns);

>> +

>> +    trad_frame_reset_saved_regs (gdbarch, cache.saved_regs);

>> +    CORE_ADDR end = aarch64_analyze_prologue (gdbarch, 0, 128, &cache, reader);

>> +

>> +    SELF_CHECK (end == (4 - 1) * 4);

>> +    SELF_CHECK (cache.framereg == AARCH64_SP_REGNUM);

>> +    SELF_CHECK (cache.framesize == 16);

>> +  }

>> +

>>    /* Test a prologue in which there is a return address signing instruction.  */

>>    if (tdep->has_pauth ())

>>      {

>> -- 

>> 2.17.1

>>

>

Patch

diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index 5e7d0d0b86..45b2b427c2 100644
--- a/gdb/aarch64-tdep.c
+++ b/gdb/aarch64-tdep.c
@@ -287,6 +287,12 @@  aarch64_analyze_prologue (struct gdbarch *gdbarch,
 {
   enum bfd_endian byte_order_for_code = gdbarch_byte_order_for_code (gdbarch);
   int i;
+
+  /* Whether the stack has been set.  This should be true when we notice a SP
+     to FP move or if we are using the SP as the base register for storing
+     data, in case the FP is ommitted.  */
+  bool seen_stack_set = false;
+
   /* Track X registers and D registers in prologue.  */
   pv_t regs[AARCH64_X_REGISTER_COUNT + AARCH64_D_REGISTER_COUNT];
 
@@ -326,6 +332,10 @@  aarch64_analyze_prologue (struct gdbarch *gdbarch,
 	      regs[rd] = pv_add_constant (regs[rn],
 					  -inst.operands[2].imm.value);
 	    }
+
+	  /* Did we move SP to FP?  */
+	  if (rn == AARCH64_SP_REGNUM && rd == AARCH64_FP_REGNUM)
+	    seen_stack_set = true;
 	}
       else if (inst.opcode->iclass == pcreladdr
 	       && inst.operands[1].type == AARCH64_OPND_ADDR_ADRP)
@@ -358,6 +368,12 @@  aarch64_analyze_prologue (struct gdbarch *gdbarch,
       else if (inst.opcode->op == OP_MOVZ)
 	{
 	  gdb_assert (inst.operands[0].type == AARCH64_OPND_Rd);
+
+	  /* If this shows up before we set the stack, keep going.  Otherwise
+	     stop the analysis.  */
+	  if (seen_stack_set)
+	    break;
+
 	  regs[inst.operands[0].reg.regno] = pv_unknown ();
 	}
       else if (inst.opcode->iclass == log_shift
@@ -399,6 +415,10 @@  aarch64_analyze_prologue (struct gdbarch *gdbarch,
 	  stack.store
 	    (pv_add_constant (regs[rn], inst.operands[1].addr.offset.imm),
 	     size, regs[rt]);
+
+	  /* Are we storing with SP as a base?  */
+	  if (rn == AARCH64_SP_REGNUM)
+	    seen_stack_set = true;
 	}
       else if ((inst.opcode->iclass == ldstpair_off
 		|| (inst.opcode->iclass == ldstpair_indexed
@@ -442,6 +462,10 @@  aarch64_analyze_prologue (struct gdbarch *gdbarch,
 	  if (inst.operands[2].addr.writeback)
 	    regs[rn] = pv_add_constant (regs[rn], imm);
 
+	  /* Ignore the instruction that allocates stack space and sets
+	     the SP.  */
+	  if (rn == AARCH64_SP_REGNUM && !inst.operands[2].addr.writeback)
+	    seen_stack_set = true;
 	}
       else if ((inst.opcode->iclass == ldst_imm9 /* Signed immediate.  */
 		|| (inst.opcode->iclass == ldst_pos /* Unsigned immediate.  */
@@ -464,6 +488,10 @@  aarch64_analyze_prologue (struct gdbarch *gdbarch,
 	  stack.store (pv_add_constant (regs[rn], imm), size, regs[rt]);
 	  if (inst.operands[1].addr.writeback)
 	    regs[rn] = pv_add_constant (regs[rn], imm);
+
+	  /* Are we storing with SP as a base?  */
+	  if (rn == AARCH64_SP_REGNUM)
+	    seen_stack_set = true;
 	}
       else if (inst.opcode->iclass == testbranch)
 	{
@@ -690,6 +718,112 @@  aarch64_analyze_prologue_test (void)
       }
   }
 
+  /* Test handling of movz before setting the frame pointer.  */
+  {
+    static const uint32_t insns[] = {
+      0xa9bf7bfd, /* stp     x29, x30, [sp, #-16]! */
+      0x52800020, /* mov     w0, #0x1 */
+      0x910003fd, /* mov     x29, sp */
+      0x528000a2, /* mov     w2, #0x5 */
+      0x97fffff8, /* bl      6e4 */
+    };
+
+    instruction_reader_test reader (insns);
+
+    trad_frame_reset_saved_regs (gdbarch, cache.saved_regs);
+    CORE_ADDR end = aarch64_analyze_prologue (gdbarch, 0, 128, &cache, reader);
+
+    SELF_CHECK (end == (4 - 1) * 4);
+    SELF_CHECK (cache.framereg == AARCH64_FP_REGNUM);
+    SELF_CHECK (cache.framesize == 16);
+  }
+
+  /* Test handling of movz/stp when using the stack pointer as frame
+     pointer.  */
+  {
+    static const uint32_t insns[] = {
+      0xa9bc7bfd, /* stp     x29, x30, [sp, #-64]! */
+      0x52800020, /* mov     w0, #0x1 */
+      0x290207e0, /* stp     w0, w1, [sp, #16] */
+      0xa9018fe2, /* stp     x2, x3, [sp, #24] */
+      0x528000a2, /* mov     w2, #0x5 */
+      0x97fffff8, /* bl      6e4 */
+    };
+
+    instruction_reader_test reader (insns);
+
+    trad_frame_reset_saved_regs (gdbarch, cache.saved_regs);
+    CORE_ADDR end = aarch64_analyze_prologue (gdbarch, 0, 128, &cache, reader);
+
+    SELF_CHECK (end == (5 - 1) * 4);
+    SELF_CHECK (cache.framereg == AARCH64_SP_REGNUM);
+    SELF_CHECK (cache.framesize == 64);
+  }
+
+  /* Test handling of movz/str when using the stack pointer as frame
+     pointer  */
+  {
+    static const uint32_t insns[] = {
+      0xa9bc7bfd, /* stp     x29, x30, [sp, #-64]! */
+      0x52800020, /* mov     w0, #0x1 */
+      0xb9002be4, /* str     w4, [sp, #40] */
+      0xf9001be5, /* str     x5, [sp, #48] */
+      0x528000a2, /* mov     w2, #0x5 */
+      0x97fffff8, /* bl      6e4 */
+    };
+
+    instruction_reader_test reader (insns);
+
+    trad_frame_reset_saved_regs (gdbarch, cache.saved_regs);
+    CORE_ADDR end = aarch64_analyze_prologue (gdbarch, 0, 128, &cache, reader);
+
+    SELF_CHECK (end == (5 - 1) * 4);
+    SELF_CHECK (cache.framereg == AARCH64_SP_REGNUM);
+    SELF_CHECK (cache.framesize == 64);
+  }
+
+  /* Test handling of movz/stur when using the stack pointer as frame
+     pointer.  */
+  {
+    static const uint32_t insns[] = {
+      0xa9bc7bfd, /* stp     x29, x30, [sp, #-64]! */
+      0x52800020, /* mov     w0, #0x1 */
+      0xb80343e6, /* stur    w6, [sp, #52] */
+      0xf80383e7, /* stur    x7, [sp, #56] */
+      0x528000a2, /* mov     w2, #0x5 */
+      0x97fffff8, /* bl      6e4 */
+    };
+
+    instruction_reader_test reader (insns);
+
+    trad_frame_reset_saved_regs (gdbarch, cache.saved_regs);
+    CORE_ADDR end = aarch64_analyze_prologue (gdbarch, 0, 128, &cache, reader);
+
+    SELF_CHECK (end == (5 - 1) * 4);
+    SELF_CHECK (cache.framereg == AARCH64_SP_REGNUM);
+    SELF_CHECK (cache.framesize == 64);
+  }
+
+  /* Test handling of movz when there is no frame pointer set or no stack
+     pointer used.  */
+  {
+    static const uint32_t insns[] = {
+      0xa9bf7bfd, /* stp     x29, x30, [sp, #-16]! */
+      0x52800020, /* mov     w0, #0x1 */
+      0x528000a2, /* mov     w2, #0x5 */
+      0x97fffff8, /* bl      6e4 */
+    };
+
+    instruction_reader_test reader (insns);
+
+    trad_frame_reset_saved_regs (gdbarch, cache.saved_regs);
+    CORE_ADDR end = aarch64_analyze_prologue (gdbarch, 0, 128, &cache, reader);
+
+    SELF_CHECK (end == (4 - 1) * 4);
+    SELF_CHECK (cache.framereg == AARCH64_SP_REGNUM);
+    SELF_CHECK (cache.framesize == 16);
+  }
+
   /* Test a prologue in which there is a return address signing instruction.  */
   if (tdep->has_pauth ())
     {