[PATCH 1/2] [AArch64] Fix step-over-syscall.exp failure
Alan Hayward
Alan.Hayward@arm.com
Thu Jan 9 17:18:00 GMT 2020
> On 9 Jan 2020, at 16:24, Luis Machado <luis.machado@linaro.org> wrote:
>
> On 1/9/20 1:02 PM, Alan Hayward wrote:
>>> On 30 Dec 2019, at 16:25, Luis Machado <luis.machado@linaro.org> wrote:
>>>
>>> In particular, this one:
>>>
>>> FAIL: gdb.base/step-over-syscall.exp: fork: displaced=on: check_pc_after_cross_syscall: single step over fork final pc
>>>
>>> When ptrace fork event reporting is enabled, GDB gets a PTRACE_EVENT_FORK
>>> event whenever the inferior executes the fork syscall.
>>>
>>> Then the logic is that GDB needs to step the inferior yet again in order to
>>> receive a predetermined SIGTRAP, but no execution takes place because the
>>> signal was already queued for delivery. That means the PC should stay the same.
>>>
>>> I noticed the aarch64 code is currently adjusting the PC in this situation,
>>> making the inferior skip an instruction without executing it.
>>>
>>> The existing code abuses the pc_adjust variable to contain both an offset and
>>> also a bool telling GDB when to adjust the PC (pc_adjust != 0).
>>>
>>> This patch fixes this case by adding a new bool that tells us when we're
>>> supposed to adjust the PC, and then proceeding to check if we did not execute
>>> the instruction (pc - to == 0), making proper adjustments for such case.
>>>
>>> Regression tested on aarch64-linux-gnu on the tryserver.
>>>
>>> gdb/ChangeLog:
>>>
>>> 2019-12-30 Luis Machado <luis.machado@linaro.org>
>>>
>>> * aarch64-tdep.c (struct aarch64_displaced_step_closure )
>>> <should_adjust_pc>: New member.
>>> <pc_adjust>: Adjust the documentation.
>>> (aarch64_displaced_step_b): Set should_adjust_pc.
>>> (aarch64_displaced_step_b_cond): Likewise.
>>> (aarch64_displaced_step_cb): Likewise.
>>> (aarch64_displaced_step_tb): Likewise.
>>> (aarch64_displaced_step_adr): Likewise.
>>> (aarch64_displaced_step_ldr_literal): Likewise.
>>> (aarch64_displaced_step_others): Likewise.
>>> (aarch64_displaced_step_fixup): Check if PC really moved before
>>> adjusting it.
>>>
>>> Change-Id: I828b7b7f2726f42ce107708f9692f07c63bf728c
>>> ---
>>> gdb/aarch64-tdep.c | 46 +++++++++++++++++++++++++++++++++++++++-------
>>> 1 file changed, 39 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
>>> index 1d5fb2001d..a639b753cd 100644
>>> --- a/gdb/aarch64-tdep.c
>>> +++ b/gdb/aarch64-tdep.c
>>> @@ -2737,7 +2737,12 @@ struct aarch64_displaced_step_closure : public displaced_step_closure
>>> is being displaced stepping. */
>>> int cond = 0;
>> Optional, but it’ll be nice if cond was a bool instead of an int.
>> (maybe a different patch)
>
> Yeah. I'll do a follow-up.
>
>>>
>>> - /* PC adjustment offset after displaced stepping. */
>>> + /* True if we should adjust the PC after displaced stepping, false
>>> + otherwise. */
>>> + bool should_adjust_pc = false;
>>> +
>>> + /* PC adjustment offset after displaced stepping, if should_adjust_pc
>>> + is true. */
>>> int32_t pc_adjust = 0;
>>> };
>>>
>>> @@ -2783,6 +2788,9 @@ aarch64_displaced_step_b (const int is_bl, const int32_t offset,
>>> emit_nop (dsd->insn_buf);
>>> dsd->insn_count++;
>>> dsd->dsc->pc_adjust = offset;
>>> +
>>> + if (offset != 0)
>>> + dsd->dsc->should_adjust_pc = true;
>> I don’t understand why pc_adjust is set here (and in the functions below). What is special
>> about offset? I suspect this just needs an explanation >
>
> Unfortunately the code is not well documented and doesn't go into detail on why pc_adjust is set to offset. But in order to split pc_adjust from should_adjust_pc, i preserved the old behavior.
>
> The offsets are likely there to handle cases of pc-relative addressing or instructions that change the PC in particular ways that are not just doing PC + 4.
>
> I can try to document this code in a follow-up if you'd like.
What I failed to spot, is that you are always setting should_adjust_pc when writing non zero to pc_adjust.
>
>>> }
>>>
>>> if (is_bl)
>>> @@ -2818,6 +2826,9 @@ aarch64_displaced_step_b_cond (const unsigned cond, const int32_t offset,
>>> dsd->dsc->cond = 1;
>>> dsd->dsc->pc_adjust = offset;
>>> dsd->insn_count = 1;
>>> +
>>> + if (offset != 0)
>>> + dsd->dsc->should_adjust_pc = true;
>>> }
>>>
>>> /* Dynamically allocate a new register. If we know the register
>>> @@ -2852,6 +2863,9 @@ aarch64_displaced_step_cb (const int32_t offset, const int is_cbnz,
>>> dsd->insn_count = 1;
>>> dsd->dsc->cond = 1;
>>> dsd->dsc->pc_adjust = offset;
>>> +
>>> + if (offset != 0)
>>> + dsd->dsc->should_adjust_pc = true;
>>> }
>>>
>>> /* Implementation of aarch64_insn_visitor method "tb". */
>>> @@ -2877,6 +2891,9 @@ aarch64_displaced_step_tb (const int32_t offset, int is_tbnz,
>>> dsd->insn_count = 1;
>>> dsd->dsc->cond = 1;
>>> dsd->dsc->pc_adjust = offset;
>>> +
>>> + if (offset != 0)
>>> + dsd->dsc->should_adjust_pc = true;
>>> }
>>>
>>> /* Implementation of aarch64_insn_visitor method "adr". */
>>> @@ -2902,6 +2919,7 @@ aarch64_displaced_step_adr (const int32_t offset, const unsigned rd,
>>> address);
>>>
>>> dsd->dsc->pc_adjust = 4;
>>> + dsd->dsc->should_adjust_pc = true;
>>> emit_nop (dsd->insn_buf);
>>> dsd->insn_count = 1;
>>> }
>>> @@ -2929,6 +2947,7 @@ aarch64_displaced_step_ldr_literal (const int32_t offset, const int is_sw,
>>> aarch64_register (rt, 1), zero);
>>>
>>> dsd->dsc->pc_adjust = 4;
>>> + dsd->dsc->should_adjust_pc = true;
>>> }
>>>
>>> /* Implementation of aarch64_insn_visitor method "others". */
>>> @@ -2946,10 +2965,12 @@ aarch64_displaced_step_others (const uint32_t insn,
>>> if ((insn & 0xfffffc1f) == 0xd65f0000)
>>> {
>>> /* RET */
>>> - dsd->dsc->pc_adjust = 0;
>>> }
>>> else
>>> - dsd->dsc->pc_adjust = 4;
>>> + {
>>> + dsd->dsc->pc_adjust = 4;
>>> + dsd->dsc->should_adjust_pc = true;
>>> + }
>>> }
>>>
>>> static const struct aarch64_insn_visitor visitor =
>>> @@ -3030,13 +3051,15 @@ aarch64_displaced_step_fixup (struct gdbarch *gdbarch,
>>> CORE_ADDR from, CORE_ADDR to,
>>> struct regcache *regs)
>>> {
>>> + ULONGEST pc;
>>> +
>>> aarch64_displaced_step_closure *dsc = (aarch64_displaced_step_closure *) dsc_;
>>>
>>> + /* Fetch the current PC, after the displaced execution took place. */
>>> + regcache_cooked_read_unsigned (regs, AARCH64_PC_REGNUM, &pc);
>>> +
>>> if (dsc->cond)
>>> {
>>> - ULONGEST pc;
>>> -
>>> - regcache_cooked_read_unsigned (regs, AARCH64_PC_REGNUM, &pc);
>>> if (pc - to == 8)
>>> {
>>> /* Condition is true. */
>>> @@ -3045,13 +3068,22 @@ aarch64_displaced_step_fixup (struct gdbarch *gdbarch,
>>> {
>>> /* Condition is false. */
>>> dsc->pc_adjust = 4;
>>> + dsc->should_adjust_pc = true;
>>> }
>>> else
>>> gdb_assert_not_reached ("Unexpected PC value after displaced stepping");
>>> }
>>>
>>> - if (dsc->pc_adjust != 0)
>>> + if (dsc->should_adjust_pc)
>>> {
>>> +
>>> + /* Sometimes we may get a SIGTRAP even before executing an instruction.
>>> + Such is the case when we are stepping over a fork/vfork/clone syscall
>>> + and the instruction after the syscall instruction. Make sure we don't
>>> + adjust the PC when we did not really move. */
>>> + if ((pc - to) == 0)
>>> + dsc->pc_adjust = 0;
>>> +
>> Instead of setting dsc->pc_adjust to 0, would it be better to do:
>> if (dsc->should_adjust_pc && (pc - to) != 0)
>> {
>> if (debug_displaced) debug_printf...
>> regcache_cooked_write_unsigned....
>> }
>> Or maybe you do need to pc_adjust to 0. If so, then disregard this.
>
> Right. The problematic case is exactly when we need to adjust the PC, but it hasn't changed after single-stepping. This means we executed a jump to self or didn't execute anything.
>
> Whatever the case, we need to write the PC back to the register.
Ok, so this patch contains two changes:
1) Fix the bug by checking (pc - to)
2) split pc_adjust into two variables - the adjust value and a bool.
Both are essentially separate changes, and don’t rely on each other.
I’m happy with what 1) is doing.
I’m a little confused in the need for 2). As far as I understand it,
when should_adjust_pc is true, pc_adjust is non zero, and when
should_adjust_pc is false, should_adjust is zero.
The only time that doesn’t hold is inside the block with the pc-to check,
but that can be coded by just using:
if (dsc->pc_adjust != 0)
{
if ((pc - to) == 0)
dsc->pc_adjust = 0;
if (debug_displaced)
{
debug_printf ("displaced: fixup: set PC to %s:%d\n",
paddress (gdbarch, from), dsc->pc_adjust);
}
regcache_cooked_write_unsigned (regs, AARCH64_PC_REGNUM,
from + dsc->pc_adjust);
}
Yes, it’s not the nicest, but it feels better than having lots additional
writes to a bool everywhere else. (It’s possible I’m still missing something).
More information about the Gdb-patches
mailing list