[1/2,AArch64] Fix step-over-syscall.exp failure

Message ID 20191230162535.21211-1-luis.machado@linaro.org
State New
Headers show
Series
  • [1/2,AArch64] Fix step-over-syscall.exp failure
Related show

Commit Message

Luis Machado Dec. 30, 2019, 4:25 p.m.
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(-)

-- 
2.17.1

Comments

Luis Machado Dec. 30, 2019, 4:50 p.m. | #1
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",

>

Patch

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