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

[Luis Machado <luis.machado@linaro.org>]

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