From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qt1-x841.google.com (mail-qt1-x841.google.com [IPv6:2607:f8b0:4864:20::841]) by sourceware.org (Postfix) with ESMTPS id 8772C3857C7D for ; Mon, 10 Aug 2020 13:13:17 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 8772C3857C7D Received: by mail-qt1-x841.google.com with SMTP id d27so6630493qtg.4 for ; Mon, 10 Aug 2020 06:13:17 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=c780UjrEBnsA2QrmkXp9/qDE5rRuhcGq6h7gFeLwqsA=; b=AhzSLW3/y02tYhUJehVKCmEx0cNCVU/BbiDRuUDSFGKs8qjWzeAGf89f6Qw4zsaaym NdI0x2km3DsK3NSZfUaqmI5N2ArPfGA3bs9bGKqto+mENSnxdaEVrMnQyBmhcVbaJWoX f23wER0aQvI+5A/SM5Q7obhJay28UmrLSyZid4kl0scq6q7Dqd6nfjNZxB2TdTK8fvPT YZnZHkV8pwNbCJ2xbBFFc8ndf87FefSt9Elp/qa3v3EiEiQvqBILm2w2jgfySUc/gcTk 8DgYgz9R6qw5r0QJJDVLTmh6kxE/KeYeL3d+Ebtr0gRmdfZuuqSgOjTawfgfs3Kao6aH Tyuw== X-Gm-Message-State: AOAM530aJ6tta+YPQ3uuZ3sH1HNDT5wpSdzizOZHP38TBSFAw8VJFACA t2mwitt5R+T9juBIeClAeYfjQyDBirBWHA== X-Google-Smtp-Source: ABdhPJxHbVhYmtaIWZB/9AJIOaO+dWd+kQvwNGmmoVMbbwjxrj5iTCDDNpX23xLOTZqn3mtt0TI5Sw== X-Received: by 2002:ac8:4b69:: with SMTP id g9mr1219101qts.113.1597065196483; Mon, 10 Aug 2020 06:13:16 -0700 (PDT) Received: from ?IPv6:2804:7f0:8283:6f2c:b77d:47b2:730b:373? ([2804:7f0:8283:6f2c:b77d:47b2:730b:373]) by smtp.gmail.com with ESMTPSA id e21sm14993488qkl.88.2020.08.10.06.13.14 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 10 Aug 2020 06:13:15 -0700 (PDT) Subject: Re: [PATCH] [AArch64] Improve prologue handling (and fix PR26310) To: Alan Hayward Cc: gdb-patches , nd References: <20200806175920.8037-1-luis.machado@linaro.org> From: Luis Machado Message-ID: <834307f4-4a3d-ea05-2eaf-1aa6a669940d@linaro.org> Date: Mon, 10 Aug 2020 10:13:11 -0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-11.0 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 10 Aug 2020 13:13:19 -0000 On 8/10/20 5:49 AM, Alan Hayward wrote: > > >> On 6 Aug 2020, at 18:59, Luis Machado 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 >> 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 >> >> 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 >> >