[PATCH] [AArch64] Improve prologue handling (and fix PR26310)
Luis Machado
luis.machado@linaro.org
Mon Aug 10 13:13:11 GMT 2020
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
>>
>
More information about the Gdb-patches
mailing list