Re: [PATCH 1/2] [AArch64] Fix step-over-syscall.exp failure

[Alan Hayward <Alan dot Hayward at arm dot com>]

> On 9 Jan 2020, at 18:26, Luis Machado <luis.machado@linaro.org> wrote:
> 
> 
> 
> On 1/9/20 2:18 PM, Alan Hayward wrote:
>>> On 9 Jan 2020, at 16:24, Luis Machado <luis.machado@linaro.org> wrote:
>>> 
>>> On 1/9/20 1:02 PM, Alan Hayward wrote:
>>>>> On 30 Dec 2019, at 16:25, Luis Machado <luis.machado@linaro.org> 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;
>>>> Optional, but it’ll be nice if cond was a bool instead of an int.
>>>> (maybe a different patch)
>>> 
>>> Yeah. I'll do a follow-up.
>>> 
>>>>> 
>>>>> -  /* 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;
>>>> I don’t understand why pc_adjust is set here (and in the functions below). What is special
>>>> about offset? I suspect this just needs an explanation >
>>> 
>>> Unfortunately the code is not well documented and doesn't go into detail on why pc_adjust is set to offset. But in order to split pc_adjust from should_adjust_pc, i preserved the old behavior.
>>> 
>>> The offsets are likely there to handle cases of pc-relative addressing or instructions that change the PC in particular ways that are not just doing PC + 4.
>>> 
>>> I can try to document this code in a follow-up if you'd like.
>> What I failed to spot, is that you are always setting should_adjust_pc when writing non zero to pc_adjust.
> 
> Got it.
> 
>>> 
>>>>>     }
>>>>> 
>>>>>   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;
>>>>> +
>>>> Instead of setting dsc->pc_adjust to 0, would it be better to do:
>>>> if (dsc->should_adjust_pc && (pc - to) != 0)
>>>> {
>>>>   if (debug_displaced) debug_printf...
>>>>   regcache_cooked_write_unsigned....
>>>> }
>>>> Or maybe you do need to pc_adjust to 0. If so, then disregard this.
>>> 
>>> Right. The problematic case is exactly when we need to adjust the PC, but it hasn't changed after single-stepping. This means we executed a jump to self or didn't execute anything.	
>>> 
>>> Whatever the case, we need to write the PC back to the register.
>> Ok, so this patch contains two changes:
>> 1) Fix the bug by checking (pc - to)
>> 2) split pc_adjust into two variables - the adjust value and a bool.
>> Both are essentially separate changes, and don’t rely on each other.
> 
> That's true. During debugging, i found it a bit confusing that pc_adjust serves double duty. And we also potentially adjust it for conditionals.
> 
>> I’m happy with what 1) is doing.
>> I’m a little confused in the need for 2). As far as I understand it,
>> when should_adjust_pc is true, pc_adjust is non zero, and when
>> should_adjust_pc is false, should_adjust is zero.
>> The only time that doesn’t hold is inside the block with the pc-to check,
>> but that can be coded by just using:
>>   if (dsc->pc_adjust != 0)
>>     {
>>       if ((pc - to) == 0)
>>         dsc->pc_adjust = 0;
>>       if (debug_displaced)
>> 	{
>> 	  debug_printf ("displaced: fixup: set PC to %s:%d\n",
>> 			paddress (gdbarch, from), dsc->pc_adjust);
>> 	}
>>       regcache_cooked_write_unsigned (regs, AARCH64_PC_REGNUM,
>> 				      from + dsc->pc_adjust);
>>     }
>> Yes, it’s not the nicest, but it feels better than having lots additional
>> writes to a bool everywhere else. (It’s possible I’m still missing something)
> 
> I see. I can keep pc_adjust and document this variable properly so it is clear what its purpose is. Does that sound better?


That sounds better to me (unless anyone else has an opinion here).


Alan.