[PATCH] Fix displaced stepping watchpoint check order

[PATCH] Fix displaced stepping watchpoint check order

[Luis Machado]

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.

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);
+
   /* Restore memory of the buffer.  */
   write_memory_ptid (thread->ptid, buffer->addr,
 		     buffer->saved_copy.data (), len);
@@ -237,9 +242,6 @@ displaced_step_buffers::finish (gdbarch *arch, thread_info *thread,
 
   regcache *rc = get_thread_regcache (thread);
 
-  bool instruction_executed_successfully
-    = displaced_step_instruction_executed_successfully (arch, sig);
-
   if (instruction_executed_successfully)
     {
       gdbarch_displaced_step_fixup (arch, copy_insn_closure.get (),
-- 
2.25.1

[Ping][PATCH] Fix displaced stepping watchpoint check order

[Luis Machado]

On 6/8/21 12:42 PM, Luis Machado 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.
> 
> 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);
> +
>     /* Restore memory of the buffer.  */
>     write_memory_ptid (thread->ptid, buffer->addr,
>   		     buffer->saved_copy.data (), len);
> @@ -237,9 +242,6 @@ displaced_step_buffers::finish (gdbarch *arch, thread_info *thread,
>   
>     regcache *rc = get_thread_regcache (thread);
>   
> -  bool instruction_executed_successfully
> -    = displaced_step_instruction_executed_successfully (arch, sig);
> -
>     if (instruction_executed_successfully)
>       {
>         gdbarch_displaced_step_fixup (arch, copy_insn_closure.get (),
> 

[PING] [PATCH] Fix displaced stepping watchpoint check order

[Luis Machado]

On 6/8/21 12:42 PM, Luis Machado 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.
> 
> 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);
> +
>     /* Restore memory of the buffer.  */
>     write_memory_ptid (thread->ptid, buffer->addr,
>   		     buffer->saved_copy.data (), len);
> @@ -237,9 +242,6 @@ displaced_step_buffers::finish (gdbarch *arch, thread_info *thread,
>   
>     regcache *rc = get_thread_regcache (thread);
>   
> -  bool instruction_executed_successfully
> -    = displaced_step_instruction_executed_successfully (arch, sig);
> -
>     if (instruction_executed_successfully)
>       {
>         gdbarch_displaced_step_fixup (arch, copy_insn_closure.get (),
> 

[PING][PATCH] Fix displaced stepping watchpoint check order

[Luis Machado]

On 6/8/21 12:42 PM, Luis Machado 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.
> 
> 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);
> +
>     /* Restore memory of the buffer.  */
>     write_memory_ptid (thread->ptid, buffer->addr,
>   		     buffer->saved_copy.data (), len);
> @@ -237,9 +242,6 @@ displaced_step_buffers::finish (gdbarch *arch, thread_info *thread,
>   
>     regcache *rc = get_thread_regcache (thread);
>   
> -  bool instruction_executed_successfully
> -    = displaced_step_instruction_executed_successfully (arch, sig);
> -
>     if (instruction_executed_successfully)
>       {
>         gdbarch_displaced_step_fixup (arch, copy_insn_closure.get (),
> 

[PING][PATCH] Fix displaced stepping watchpoint check order

[Luis Machado]

On 6/8/21 12:42 PM, Luis Machado 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.
> 
> 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);
> +
>     /* Restore memory of the buffer.  */
>     write_memory_ptid (thread->ptid, buffer->addr,
>   		     buffer->saved_copy.data (), len);
> @@ -237,9 +242,6 @@ displaced_step_buffers::finish (gdbarch *arch, thread_info *thread,
>   
>     regcache *rc = get_thread_regcache (thread);
>   
> -  bool instruction_executed_successfully
> -    = displaced_step_instruction_executed_successfully (arch, sig);
> -
>     if (instruction_executed_successfully)
>       {
>         gdbarch_displaced_step_fixup (arch, copy_insn_closure.get (),
> 

Re: [PATCH] Fix displaced stepping watchpoint check order

[Simon Marchi]

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.

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?

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

Simon

Re: [PATCH] Fix displaced stepping watchpoint check order

[Luis Machado]

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
> 

Re: [PATCH] Fix displaced stepping watchpoint check order

[Simon Marchi]

On 2021-07-29 4:17 p.m., Luis Machado wrote:
> 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).

What does "need to step over that hardware watchpoint" means,
concretely?  After the watchpoint has triggered, are the side effects of
the instruction committed to memory and registers?  Or are we in a state
as if the instruction didn't complete?  How is that step over done?

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

Ok, but that patch you linked isn't merged yet?  So it sounds strange to
say "from now on", it sounds like there's a dependency between the two
patches.  Let's say the current patch is merged before the other one,
maybe it should say "but AArch64 will need to do it it an upcoming
patch", and then you can given the link.

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

Yes, this help.  Please feel free to include in the commit message any
additional detail that you gave here, since it might help somebody else
in the future.

The code change is fine, so once you send a revised version it should go
fairly quickly.  Sorry for the delay.

Thanks!

Simon

Re: [PATCH] Fix displaced stepping watchpoint check order

[Luis Machado]

On 7/29/21 9:59 PM, Simon Marchi wrote:
> 
> 
> On 2021-07-29 4:17 p.m., Luis Machado wrote:
>> 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).
> 
> What does "need to step over that hardware watchpoint" means,
> concretely?  After the watchpoint has triggered, are the side effects of
> the instruction committed to memory and registers?  Or are we in a state
> as if the instruction didn't complete?  How is that step over done?

No side effects of the instruction are committed in this case. Memory 
and registers will have their old values as if the instruction didn't 
execute.

 From reading the code, most architectures have non-steppable hardware 
watchpoints. GDB just disables the hardware watchpoints, single-steps 
past that instruction and then enables the hardware watchpoints again.

GDB just disables all hardware watchpoints for the sake of simplicity. 
You can see this logic in infrun.c:handle_signal_stop, around this comment:

/* If necessary, step over this watchpoint.  We'll be back to display
    it in a moment.  */
if (stopped_by_watchpoint
     && (target_have_steppable_watchpoint ()
         || gdbarch_have_nonsteppable_watchpoint (gdbarch)))

It is actually best to disable all hardware watchpoints. If we end up 
disabling just one hardware watchpoint, and then we happen to have 
another hardware watchpoint that is active and also getting triggered, 
we might be stuck in an endless loop as well.

> 
>>>
>>> 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.
> 
> Ok, but that patch you linked isn't merged yet?  So it sounds strange to
> say "from now on", it sounds like there's a dependency between the two

Absolutely. There is a dependency. My plan is to merge this fix first, 
and then merge the AArch64 hardware watchpoint detection fixes. I just 
didn't group those together, but that's the right order. I'll make sure 
to point out the dependency in the other patch.

> patches.  Let's say the current patch is merged before the other one,
> maybe it should say "but AArch64 will need to do it it an upcoming
> patch", and then you can given the link.
> 
>> 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?
> 
> Yes, this help.  Please feel free to include in the commit message any
> additional detail that you gave here, since it might help somebody else
> in the future.

I'll make the commit message more detailed.

> 
> The code change is fine, so once you send a revised version it should go
> fairly quickly.  Sorry for the delay.

No worries. Thanks for the feedback!

> 
> Thanks!
> 
> Simon
> 

Re: [PATCH] Fix displaced stepping watchpoint check order

[Simon Marchi]

> No side effects of the instruction are committed in this case. Memory and registers will have their old values as if the instruction didn't execute.
> 
> From reading the code, most architectures have non-steppable hardware watchpoints. GDB just disables the hardware watchpoints, single-steps past that instruction and then enables the hardware watchpoints again.
> 
> GDB just disables all hardware watchpoints for the sake of simplicity. You can see this logic in infrun.c:handle_signal_stop, around this comment:
> 
> /* If necessary, step over this watchpoint.  We'll be back to display
>    it in a moment.  */
> if (stopped_by_watchpoint
>     && (target_have_steppable_watchpoint ()
>         || gdbarch_have_nonsteppable_watchpoint (gdbarch)))
> 
> It is actually best to disable all hardware watchpoints. If we end up disabling just one hardware watchpoint, and then we happen to have another hardware watchpoint that is active and also getting triggered, we might be stuck in an endless loop as well.
> 

Ok, thanks for the explanation.  Watchpoints are usually (always?)
defined in some per-thread register, so unlike software breakpoints, I
suppose it's safe to remove the watchpoints just for the thread we step.
I suppose that is what we are doing?

>>>> 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.
>>
>> Ok, but that patch you linked isn't merged yet?  So it sounds strange to
>> say "from now on", it sounds like there's a dependency between the two
> 
> Absolutely. There is a dependency. My plan is to merge this fix first, and then merge the AArch64 hardware watchpoint detection fixes. I just didn't group those together, but that's the right order. I'll make sure to point out the dependency in the other patch.

Ok.

>> patches.  Let's say the current patch is merged before the other one,
>> maybe it should say "but AArch64 will need to do it it an upcoming
>> patch", and then you can given the link.
>>
>>> 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?
>>
>> Yes, this help.  Please feel free to include in the commit message any
>> additional detail that you gave here, since it might help somebody else
>> in the future.
> 
> I'll make the commit message more detailed.

Thanks,

Simon

Re: [PATCH] Fix displaced stepping watchpoint check order

[Luis Machado]

On 8/18/21 10:31 PM, Simon Marchi wrote:
>> No side effects of the instruction are committed in this case. Memory and registers will have their old values as if the instruction didn't execute.
>>
>>  From reading the code, most architectures have non-steppable hardware watchpoints. GDB just disables the hardware watchpoints, single-steps past that instruction and then enables the hardware watchpoints again.
>>
>> GDB just disables all hardware watchpoints for the sake of simplicity. You can see this logic in infrun.c:handle_signal_stop, around this comment:
>>
>> /* If necessary, step over this watchpoint.  We'll be back to display
>>     it in a moment.  */
>> if (stopped_by_watchpoint
>>      && (target_have_steppable_watchpoint ()
>>          || gdbarch_have_nonsteppable_watchpoint (gdbarch)))
>>
>> It is actually best to disable all hardware watchpoints. If we end up disabling just one hardware watchpoint, and then we happen to have another hardware watchpoint that is active and also getting triggered, we might be stuck in an endless loop as well.
>>
> 
> Ok, thanks for the explanation.  Watchpoints are usually (always?)
> defined in some per-thread register, so unlike software breakpoints, I
> suppose it's safe to remove the watchpoints just for the thread we step.
> I suppose that is what we are doing?

Although hardware watchpoints are usually per-thread, I don't think GDB 
handles it in that level of detail. It seems to mostly ignore insertion 
of hardware watchpoints if we're trying to step over a PC that caused a 
hardware watchpoint trigger.

See, for example, breakpoint.c:should_be_inserted

   /* Don't insert watchpoints if we're trying to step past the
      instruction that triggered one.  */
   if ((bl->loc_type == bp_loc_hardware_watchpoint)
       && stepping_past_nonsteppable_watchpoint ())
     {
       infrun_debug_printf ("stepping past non-steppable watchpoint. "
                            "skipping watchpoint at %s:%d\n",
                            paddress (bl->gdbarch, bl->address), 
bl->length);
       return 0;
     }

 From what I recall, GDB replicates the hardware watchpoints to all 
threads, so disabling all hardware watchpoints for all threads (actually 
not inserting them) makes sense. GDB doesn't try to add thread-specific 
hardware watchpoints at the moment.

Having per-thread hardware watchpoints might be desirable in some cases, 
but it may complicate the user interaction a bit.

I haven't investigated if GDB, during a step-over operation, keeps 
threads running with hardware watchpoints disabled. If it doesn't handle 
it correctly, we may miss hardware watchpoints hits, unless GDB 
serializes the step-overs. But I'm guessing there is some serialization, 
mostly due to this comment:

"Presently GDB can only step over one breakpoint at any given time."

> 
>>>>> 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.
>>>
>>> Ok, but that patch you linked isn't merged yet?  So it sounds strange to
>>> say "from now on", it sounds like there's a dependency between the two
>>
>> Absolutely. There is a dependency. My plan is to merge this fix first, and then merge the AArch64 hardware watchpoint detection fixes. I just didn't group those together, but that's the right order. I'll make sure to point out the dependency in the other patch.
> 
> Ok.
> 
>>> patches.  Let's say the current patch is merged before the other one,
>>> maybe it should say "but AArch64 will need to do it it an upcoming
>>> patch", and then you can given the link.
>>>
>>>> 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?
>>>
>>> Yes, this help.  Please feel free to include in the commit message any
>>> additional detail that you gave here, since it might help somebody else
>>> in the future.
>>
>> I'll make the commit message more detailed.
> 
> Thanks,
> 
> Simon
> 

Re: [PATCH] Fix displaced stepping watchpoint check order

[Simon Marchi]

> Although hardware watchpoints are usually per-thread, I don't think GDB handles it in that level of detail. It seems to mostly ignore insertion of hardware watchpoints if we're trying to step over a PC that caused a hardware watchpoint trigger.
> 
> See, for example, breakpoint.c:should_be_inserted
> 
>   /* Don't insert watchpoints if we're trying to step past the
>      instruction that triggered one.  */
>   if ((bl->loc_type == bp_loc_hardware_watchpoint)
>       && stepping_past_nonsteppable_watchpoint ())
>     {
>       infrun_debug_printf ("stepping past non-steppable watchpoint. "
>                            "skipping watchpoint at %s:%d\n",
>                            paddress (bl->gdbarch, bl->address), bl->length);
>       return 0;
>     }
> 
> From what I recall, GDB replicates the hardware watchpoints to all threads, so disabling all hardware watchpoints for all threads (actually not inserting them) makes sense. GDB doesn't try to add thread-specific hardware watchpoints at the moment.
> 
> Having per-thread hardware watchpoints might be desirable in some cases, but it may complicate the user interaction a bit.
> 
> I haven't investigated if GDB, during a step-over operation, keeps threads running with hardware watchpoints disabled. If it doesn't handle it correctly, we may miss hardware watchpoints hits, unless GDB serializes the step-overs.

That's where I was going too.  Just imagine in non-stop, you have many
threads running, one hits the watchpoint.  You then want to resume that
thread, that requires stepping over the watchpoint while other threads
are running.  If the watchpoint was really removed for all running
threads, a running thread could miss it.

But watchpoints aren't like memory breakpoints, they are usually set
using debug register, which are per-thread, so you can't really remove a
watchpoint from a running thread... that's why I was thinking that maybe
the watchpoint was only removed for the thread stepping over the
watchpoint.  But maybe not.

> "Presently GDB can only step over one breakpoint at any given time."

There can be only one in-line step at a time.  But there can be multiple
displaced steps at a time (since somewhat recently).  There can't be an
in-line step at the same time as some displaced steps.  While an in-line
step is in progress, it is guaranteed that there are no other running
threads.

Reading keep_going_pass_signal, I might understand that watchpoints are
always stepped using the in-line mechanism, while all other threads are
stopped.  I would have to try it.

Simon

Re: [PATCH] Fix displaced stepping watchpoint check order

[Luis Machado]

On 8/19/21 3:23 PM, Simon Marchi wrote:
>> Although hardware watchpoints are usually per-thread, I don't think GDB handles it in that level of detail. It seems to mostly ignore insertion of hardware watchpoints if we're trying to step over a PC that caused a hardware watchpoint trigger.
>>
>> See, for example, breakpoint.c:should_be_inserted
>>
>>    /* Don't insert watchpoints if we're trying to step past the
>>       instruction that triggered one.  */
>>    if ((bl->loc_type == bp_loc_hardware_watchpoint)
>>        && stepping_past_nonsteppable_watchpoint ())
>>      {
>>        infrun_debug_printf ("stepping past non-steppable watchpoint. "
>>                             "skipping watchpoint at %s:%d\n",
>>                             paddress (bl->gdbarch, bl->address), bl->length);
>>        return 0;
>>      }
>>
>>  From what I recall, GDB replicates the hardware watchpoints to all threads, so disabling all hardware watchpoints for all threads (actually not inserting them) makes sense. GDB doesn't try to add thread-specific hardware watchpoints at the moment.
>>
>> Having per-thread hardware watchpoints might be desirable in some cases, but it may complicate the user interaction a bit.
>>
>> I haven't investigated if GDB, during a step-over operation, keeps threads running with hardware watchpoints disabled. If it doesn't handle it correctly, we may miss hardware watchpoints hits, unless GDB serializes the step-overs.
> 
> That's where I was going too.  Just imagine in non-stop, you have many
> threads running, one hits the watchpoint.  You then want to resume that
> thread, that requires stepping over the watchpoint while other threads
> are running.  If the watchpoint was really removed for all running
> threads, a running thread could miss it.
> 
> But watchpoints aren't like memory breakpoints, they are usually set
> using debug register, which are per-thread, so you can't really remove a
> watchpoint from a running thread... that's why I was thinking that maybe
> the watchpoint was only removed for the thread stepping over the
> watchpoint.  But maybe not.

That's true. It might be the case that GDB is only removing the hardware 
watchpoint from the stopped thread. And it does so through context, by 
having selected a particular thread explicitly at some point. As opposed 
to commanding the inferior to remove hardware watchpoints from all threads.

I'm pretty sure we'd get a ptrace error otherwise.

> 
>> "Presently GDB can only step over one breakpoint at any given time."
> 
> There can be only one in-line step at a time.  But there can be multiple
> displaced steps at a time (since somewhat recently).  There can't be an
> in-line step at the same time as some displaced steps.  While an in-line
> step is in progress, it is guaranteed that there are no other running
> threads.
> 
> Reading keep_going_pass_signal, I might understand that watchpoints are
> always stepped using the in-line mechanism, while all other threads are
> stopped.  I would have to try it.

That makes sense.

> 
> Simon
>