| Message ID | 20191230162535.21211-1-luis.machado@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2,AArch64] Fix step-over-syscall.exp failure | expand |
cc-ing Alan. On 12/30/19 1:25 PM, Luis Machado 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; > > - /* 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; > } > > 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; > + > if (debug_displaced) > { > debug_printf ("displaced: fixup: set PC to %s:%d\n", >
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; - /* 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; } 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; + if (debug_displaced) { debug_printf ("displaced: fixup: set PC to %s:%d\n",