[prev in list] [next in list] [prev in thread] [next in thread] 

List:       gdb-patches
Subject:    [PATCH 1/2] [AArch64] Fix step-over-syscall.exp failure
From:       luis.machado () linaro ! org (Luis Machado)
Date:       2019-12-30 16:50:00
Message-ID: 8cfb1ce5-7046-48cb-24ee-0ecabaa46600 () linaro ! org
[Download RAW message or body]

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",
> 


[prev in list] [next in list] [prev in thread] [next in thread]