| Message ID | 20200113174239.7819-3-luis.machado@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,AArch64] Fix step-over-syscall.exp failure | expand |
This is ok too. Alan. > On 13 Jan 2020, at 17:42, Luis Machado <luis.machado@linaro.org> wrote: > > New in v2: > > - Reverted to using pc_adjust as bool/offset and added more comments to explain > how it is being used. > > -- > > 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 following change checks if we did not execute the instruction > (pc - to == 0), making proper adjustments for such case. > > Regression tested on aarch64-linux-gnu. > > gdb/ChangeLog: > > 2020-01-13 Luis Machado <luis.machado@linaro.org> > > * aarch64-tdep.c (struct aarch64_displaced_step_closure ) > <pc_adjust>: Adjust the documentation. > (aarch64_displaced_step_fixup): Check if PC really moved before > adjusting it. > --- > gdb/aarch64-tdep.c | 17 +++++++++++++---- > 1 file changed, 13 insertions(+), 4 deletions(-) > > diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c > index da41e22130..6a9d34dc67 100644 > --- a/gdb/aarch64-tdep.c > +++ b/gdb/aarch64-tdep.c > @@ -2737,7 +2737,8 @@ struct aarch64_displaced_step_closure : public displaced_step_closure > is being displaced stepping. */ > int cond = 0; > > - /* PC adjustment offset after displaced stepping. */ > + /* PC adjustment offset after displaced stepping. If 0, then we don't > + write the PC back, assuming the PC is already the right address. */ > int32_t pc_adjust = 0; > }; > > @@ -3032,11 +3033,12 @@ aarch64_displaced_step_fixup (struct gdbarch *gdbarch, > { > aarch64_displaced_step_closure *dsc = (aarch64_displaced_step_closure *) dsc_; > > + ULONGEST pc; > + > + 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. */ > @@ -3052,6 +3054,13 @@ aarch64_displaced_step_fixup (struct gdbarch *gdbarch, > > if (dsc->pc_adjust != 0) > { > + /* Make sure the previous instruction was executed (that is, the PC > + has changed). If the PC didn't change, then discard the adjustment > + offset. Otherwise we may skip an instruction before its execution > + took place. */ > + if ((pc - to) == 0) > + dsc->pc_adjust = 0; > + > if (debug_displaced) > { > debug_printf ("displaced: fixup: set PC to %s:%d\n", > -- > 2.17.1 >
Thanks. Pushed now. On 1/21/20 8:23 AM, Alan Hayward wrote: > This is ok too. > > Alan. > >> On 13 Jan 2020, at 17:42, Luis Machado <luis.machado@linaro.org> wrote: >> >> New in v2: >> >> - Reverted to using pc_adjust as bool/offset and added more comments to explain >> how it is being used. >> >> -- >> >> 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 following change checks if we did not execute the instruction >> (pc - to == 0), making proper adjustments for such case. >> >> Regression tested on aarch64-linux-gnu. >> >> gdb/ChangeLog: >> >> 2020-01-13 Luis Machado <luis.machado@linaro.org> >> >> * aarch64-tdep.c (struct aarch64_displaced_step_closure ) >> <pc_adjust>: Adjust the documentation. >> (aarch64_displaced_step_fixup): Check if PC really moved before >> adjusting it. >> --- >> gdb/aarch64-tdep.c | 17 +++++++++++++---- >> 1 file changed, 13 insertions(+), 4 deletions(-) >> >> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c >> index da41e22130..6a9d34dc67 100644 >> --- a/gdb/aarch64-tdep.c >> +++ b/gdb/aarch64-tdep.c >> @@ -2737,7 +2737,8 @@ struct aarch64_displaced_step_closure : public displaced_step_closure >> is being displaced stepping. */ >> int cond = 0; >> >> - /* PC adjustment offset after displaced stepping. */ >> + /* PC adjustment offset after displaced stepping. If 0, then we don't >> + write the PC back, assuming the PC is already the right address. */ >> int32_t pc_adjust = 0; >> }; >> >> @@ -3032,11 +3033,12 @@ aarch64_displaced_step_fixup (struct gdbarch *gdbarch, >> { >> aarch64_displaced_step_closure *dsc = (aarch64_displaced_step_closure *) dsc_; >> >> + ULONGEST pc; >> + >> + 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. */ >> @@ -3052,6 +3054,13 @@ aarch64_displaced_step_fixup (struct gdbarch *gdbarch, >> >> if (dsc->pc_adjust != 0) >> { >> + /* Make sure the previous instruction was executed (that is, the PC >> + has changed). If the PC didn't change, then discard the adjustment >> + offset. Otherwise we may skip an instruction before its execution >> + took place. */ >> + if ((pc - to) == 0) >> + dsc->pc_adjust = 0; >> + >> if (debug_displaced) >> { >> debug_printf ("displaced: fixup: set PC to %s:%d\n", >> -- >> 2.17.1 >> >
diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c index da41e22130..6a9d34dc67 100644 --- a/gdb/aarch64-tdep.c +++ b/gdb/aarch64-tdep.c @@ -2737,7 +2737,8 @@ struct aarch64_displaced_step_closure : public displaced_step_closure is being displaced stepping. */ int cond = 0; - /* PC adjustment offset after displaced stepping. */ + /* PC adjustment offset after displaced stepping. If 0, then we don't + write the PC back, assuming the PC is already the right address. */ int32_t pc_adjust = 0; }; @@ -3032,11 +3033,12 @@ aarch64_displaced_step_fixup (struct gdbarch *gdbarch, { aarch64_displaced_step_closure *dsc = (aarch64_displaced_step_closure *) dsc_; + ULONGEST pc; + + 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. */ @@ -3052,6 +3054,13 @@ aarch64_displaced_step_fixup (struct gdbarch *gdbarch, if (dsc->pc_adjust != 0) { + /* Make sure the previous instruction was executed (that is, the PC + has changed). If the PC didn't change, then discard the adjustment + offset. Otherwise we may skip an instruction before its execution + took place. */ + if ((pc - to) == 0) + dsc->pc_adjust = 0; + if (debug_displaced) { debug_printf ("displaced: fixup: set PC to %s:%d\n",