Fix displaced stepping watchpoint check order

Message ID 20210608154230.354202-1-luis.machado@linaro.org
State New
Headers show
Series
  • Fix displaced stepping watchpoint check order
Related show

Commit Message

Lancelot SIX via Gdb-patches June 8, 2021, 3:42 p.m.
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(-)

-- 
2.25.1

Comments

Lancelot SIX via Gdb-patches June 15, 2021, 2:09 p.m. | #1
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 (),

>
Lancelot SIX via Gdb-patches June 22, 2021, 1:56 a.m. | #2
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 (),

>

Patch

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 (),