Re: [PATCH] Fix displaced stepping watchpoint check order

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

Hi,

On 7/29/21 4:36 PM, Simon Marchi wrote:
> I think this is ok, but in all honestly I don't completely understand
> how the interaction between watchpoints and displaced stepping is
> expected to work.

Let me try to clarify. When we attempt to execute an instruction in the 
scratch space (displaced stepping), we may potentially trigger a 
hardware watchpoint.

For AArch64, hardware watchpoints are non-steppable, so that means GDB 
will need to step over that hardware watchpoint so the instruction's 
execution completes (if there is no hardware watchpoint trigger, the 
instruction gets executed just fine).

Now, if the hardware watchpoint trigger did happen (and GDB detects that 
properly), then displaced_step_instruction_executed_successfully (...) 
will return false.

The above check happens after we restore the displaced stepping buffer 
contents. So the original instruction that caused the hardware 
watchpoint trigger is gone. That is fine if we don't have to look at the 
instruction being stepped-over.

> 
> Just some nits:
> 
> On 2021-06-08 11:42 a.m., Luis Machado via Gdb-patches wrote:
>> When checking the stopped data address, I noticed, under some circumstances,
>> that the instruction at PC wasn't the expected one. This happens because the
>> displaced stepping machinery restores the buffer before checking if the
>> instruction executed successfully, which in turn calls the watchpoint check.
>>
>> I guess this was never noticed because stopped data address checks usually
>> don't need to fetch the instruction at PC, but AArch64 needs to do it from
>> now on.
> 
> Can you clarify what you mean by "from now on"?  Can you indicate what
> change you are referring to?
> 

 From the following change 
(https://sourceware.org/pipermail/gdb-patches/2021-July/181095.html) 
onwards, we need to look at the load/store instruction to figure out the 
memory access size so we can reliably tell if a hardware watchpoint has 
triggered. This is due to how AArch64's spec defines how to provide a 
stopped data address, and the valid ranges.

With the old code, if we try to fetch the instruction at PC, we will get 
a bogus value that is not the real instruction that caused the hardware 
watchpoint trigger. Hence why the patch moves the call to 
displaced_step_instruction_executed_successfully (...) up and before we 
restore the displaced stepping buffer.

If a hardware watchpoint trigger takes place and GDB doesn't recognize 
it, then displaced_step_instruction_executed_successfully (...) will 
return true and GDB will move on and will attempt to execute the same 
instruction again, only to be halted due to the same hardware watchpoint 
trigger that it can't detect. So GDB gets into an infinite loop.

More generally, if we ever fail to acknowledge a hardware watchpoint 
trigger on an architecture with non-steppable watchpoints and displaced 
stepping support, we will run into this infinite loop (as far as I can 
tell).

Does that make sense?

>>
>> We should check if the instruction executed successfully before we restore the
>> scratchpad contents.
>>
>> Regression tested on aarch64-linux/Ubuntu 20.04.
>>
>> gdb/ChangeLog:
>>
>> YYYY-MM-DD  Luis Machado  <luis.machado@linaro.org>
>>
>> 	* displaced-stepping.c (displaced_step_buffers::finish): Move check
>> 	upwards.
>> ---
>>   gdb/displaced-stepping.c | 8 +++++---
>>   1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/gdb/displaced-stepping.c b/gdb/displaced-stepping.c
>> index 59b78c22f6a..06324d523d8 100644
>> --- a/gdb/displaced-stepping.c
>> +++ b/gdb/displaced-stepping.c
>> @@ -227,6 +227,11 @@ displaced_step_buffers::finish (gdbarch *arch, thread_info *thread,
>>   
>>     ULONGEST len = gdbarch_max_insn_length (arch);
>>   
>> +  /* Check if the execution was successful before restoring the buffer
>> +     contents.  */
>> +  bool instruction_executed_successfully
>> +    = displaced_step_instruction_executed_successfully (arch, sig);
> 
> Maybe extend the comment to say "why".  Right now I think it just states
> what is in plain sight when looking at the code, I think it would be
> more useful if it said why it's important to do that.

I can expand it to make it more clear.

> 
> Simon
>