[PATCH] [AArch64] Convert an int flag variable to bool

[PATCH] [AArch64] Convert an int flag variable to bool

[Luis Machado]

As suggested, the cond variable is really supposed to be a bool. So,
make it so.

gdb/ChangeLog:

2020-01-13  Luis Machado  <luis.machado@linaro.org>

	* aarch64-tdep.c (struct aarch64_displaced_step_closure)
	<cond>: Change type to bool.
	(aarch64_displaced_step_b_cond): Update cond to use bool type.
	(aarch64_displaced_step_cb): Likewise.
	(aarch64_displaced_step_tb): Likewise.
---
 gdb/aarch64-tdep.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index 29b712c155..02634b9c5c 100644
--- a/gdb/aarch64-tdep.c
+++ b/gdb/aarch64-tdep.c
@@ -2735,7 +2735,7 @@ struct aarch64_displaced_step_closure : public displaced_step_closure
 {
   /* It is true when condition instruction, such as B.CON, TBZ, etc,
      is being displaced stepping.  */
-  int cond = 0;
+  bool cond = false;
 
   /* PC adjustment offset after displaced stepping.  If 0, then we don't
      write the PC back, assuming the PC is already the right address.  */
@@ -2816,7 +2816,7 @@ aarch64_displaced_step_b_cond (const unsigned cond, const int32_t offset,
   */
 
   emit_bcond (dsd->insn_buf, cond, 8);
-  dsd->dsc->cond = 1;
+  dsd->dsc->cond = true;
   dsd->dsc->pc_adjust = offset;
   dsd->insn_count = 1;
 }
@@ -2851,7 +2851,7 @@ aarch64_displaced_step_cb (const int32_t offset, const int is_cbnz,
   */
   emit_cb (dsd->insn_buf, is_cbnz, aarch64_register (rn, is64), 8);
   dsd->insn_count = 1;
-  dsd->dsc->cond = 1;
+  dsd->dsc->cond = true;
   dsd->dsc->pc_adjust = offset;
 }
 
@@ -2876,7 +2876,7 @@ aarch64_displaced_step_tb (const int32_t offset, int is_tbnz,
   */
   emit_tb (dsd->insn_buf, is_tbnz, bit, aarch64_register (rt, 1), 8);
   dsd->insn_count = 1;
-  dsd->dsc->cond = 1;
+  dsd->dsc->cond = true;
   dsd->dsc->pc_adjust = offset;
 }
 
-- 
2.17.1

[PATCH,v2][AArch64] Add more debugging output to aarch64_displaced_step_fixup

[Luis Machado]

New in v2:

- Fixed misc comments from review.

--

While debugging the step-over-syscall problem, i wanted to see a bit more
debugging output to try to determine the root cause.

This patch does this.

gdb/ChangeLog:

2020-01-13  Luis Machado  <luis.machado@linaro.org>

	* aarch64-tdep.c (aarch64_displaced_step_fixup): Add more debugging
	output.
---
 gdb/aarch64-tdep.c | 27 +++++++++++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)

diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index 6a9d34dc67..29b712c155 100644
--- a/gdb/aarch64-tdep.c
+++ b/gdb/aarch64-tdep.c
@@ -3037,8 +3037,16 @@ aarch64_displaced_step_fixup (struct gdbarch *gdbarch,
 
   regcache_cooked_read_unsigned (regs, AARCH64_PC_REGNUM, &pc);
 
+  if (debug_displaced)
+    debug_printf ("Displaced: PC after stepping: %s (was %s).\n",
+		  paddress (gdbarch, pc), paddress (gdbarch, to));
+
   if (dsc->cond)
     {
+      if (debug_displaced)
+	debug_printf ("Displaced: [Conditional] pc_adjust before: %d\n",
+		      dsc->pc_adjust);
+
       if (pc - to == 8)
 	{
 	  /* Condition is true.  */
@@ -3050,8 +3058,18 @@ aarch64_displaced_step_fixup (struct gdbarch *gdbarch,
 	}
       else
 	gdb_assert_not_reached ("Unexpected PC value after displaced stepping");
+
+      if (debug_displaced)
+	debug_printf ("Displaced: [Conditional] pc_adjust after: %d\n",
+		      dsc->pc_adjust);
     }
 
+  if (debug_displaced)
+    debug_printf ("Displaced: %s PC by %d\n",
+		  dsc->pc_adjust? "adjusting" : "not adjusting",
+		  dsc->pc_adjust);
+
+
   if (dsc->pc_adjust != 0)
     {
       /* Make sure the previous instruction was executed (that is, the PC
@@ -3059,11 +3077,16 @@ aarch64_displaced_step_fixup (struct gdbarch *gdbarch,
 	 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: PC did not move. Discarding PC "
+			  "adjustment.\n");
+	  dsc->pc_adjust = 0;
+	}
 
       if (debug_displaced)
 	{
-	  debug_printf ("displaced: fixup: set PC to %s:%d\n",
+	  debug_printf ("Displaced: fixup: set PC to %s:%d\n",
 			paddress (gdbarch, from), dsc->pc_adjust);
 	}
       regcache_cooked_write_unsigned (regs, AARCH64_PC_REGNUM,
-- 
2.17.1

[PATCH,v2][AArch64] Fix step-over-syscall.exp failure

[Luis Machado]

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

Re: [PATCH] [AArch64] Convert an int flag variable to bool

[Simon Marchi]

On 2020-01-13 12:42 p.m., Luis Machado wrote:
> As suggested, the cond variable is really supposed to be a bool. So,
> make it so.

That looks good to me (and could probably be considered obvious).

Thanks,

Simon

Re: [PATCH,v2][AArch64] Add more debugging output to aarch64_displaced_step_fixup

[Alan Hayward]

Ok to push.

Alan.

> On 13 Jan 2020, at 17:42, Luis Machado <luis.machado@linaro.org> wrote:
> 
> New in v2:
> 
> - Fixed misc comments from review.
> 
> --
> 
> While debugging the step-over-syscall problem, i wanted to see a bit more
> debugging output to try to determine the root cause.
> 
> This patch does this.
> 
> gdb/ChangeLog:
> 
> 2020-01-13  Luis Machado  <luis.machado@linaro.org>
> 
> 	* aarch64-tdep.c (aarch64_displaced_step_fixup): Add more debugging
> 	output.
> ---
> gdb/aarch64-tdep.c | 27 +++++++++++++++++++++++++--
> 1 file changed, 25 insertions(+), 2 deletions(-)
> 
> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
> index 6a9d34dc67..29b712c155 100644
> --- a/gdb/aarch64-tdep.c
> +++ b/gdb/aarch64-tdep.c
> @@ -3037,8 +3037,16 @@ aarch64_displaced_step_fixup (struct gdbarch *gdbarch,
> 
>   regcache_cooked_read_unsigned (regs, AARCH64_PC_REGNUM, &pc);
> 
> +  if (debug_displaced)
> +    debug_printf ("Displaced: PC after stepping: %s (was %s).\n",
> +		  paddress (gdbarch, pc), paddress (gdbarch, to));
> +
>   if (dsc->cond)
>     {
> +      if (debug_displaced)
> +	debug_printf ("Displaced: [Conditional] pc_adjust before: %d\n",
> +		      dsc->pc_adjust);
> +
>       if (pc - to == 8)
> 	{
> 	  /* Condition is true.  */
> @@ -3050,8 +3058,18 @@ aarch64_displaced_step_fixup (struct gdbarch *gdbarch,
> 	}
>       else
> 	gdb_assert_not_reached ("Unexpected PC value after displaced stepping");
> +
> +      if (debug_displaced)
> +	debug_printf ("Displaced: [Conditional] pc_adjust after: %d\n",
> +		      dsc->pc_adjust);
>     }
> 
> +  if (debug_displaced)
> +    debug_printf ("Displaced: %s PC by %d\n",
> +		  dsc->pc_adjust? "adjusting" : "not adjusting",
> +		  dsc->pc_adjust);
> +
> +
>   if (dsc->pc_adjust != 0)
>     {
>       /* Make sure the previous instruction was executed (that is, the PC
> @@ -3059,11 +3077,16 @@ aarch64_displaced_step_fixup (struct gdbarch *gdbarch,
> 	 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: PC did not move. Discarding PC "
> +			  "adjustment.\n");
> +	  dsc->pc_adjust = 0;
> +	}
> 
>       if (debug_displaced)
> 	{
> -	  debug_printf ("displaced: fixup: set PC to %s:%d\n",
> +	  debug_printf ("Displaced: fixup: set PC to %s:%d\n",
> 			paddress (gdbarch, from), dsc->pc_adjust);
> 	}
>       regcache_cooked_write_unsigned (regs, AARCH64_PC_REGNUM,
> -- 
> 2.17.1
> 

Re: [PATCH,v2][AArch64] Fix step-over-syscall.exp failure

[Alan Hayward]

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
> 

Re: [PATCH,v2][AArch64] Fix step-over-syscall.exp failure

[Luis Machado]

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