diff mbox

[v3,06/10] Replace breakpoint_reinsert_addr by get_next_pcs operation in GDBServer.

Message ID 1448287968-12907-7-git-send-email-antoine.tremblay@ericsson.com
State New, archived
Headers

Commit Message

Antoine Tremblay Nov. 23, 2015, 2:12 p.m. UTC 
  In this v3:

 * Changed get_next_pcs signature.
 * Remove common/get-next-pcs.h file.
 * CORE_ADDR is now in linux-low.h temporarely.

---

This patch in preparation for software single step support on ARM. It refactors
breakpoint_reinsert_addr into get_next_pcs so that multiple location can be
returned.

When software single stepping there can be multiple possible next addresses
because we're stepping over a conditional branch instruction for example.

The operation get_next_pcs handles that by returning a vector of all the
possible next addresses.

Software breakpoints are installed at each location returned.

No regressions, tested on ubuntu 14.04 ARMv7 and x86.
With gdbserver-{native,extended} / { -marm -mthumb }

gdb/gdbserver/ChangeLog:
	* linux-aarch64-low.c (struct linux_target_ops): Rename
	breakpoint_reinsert_addr to get_next_pcs.
	* linux-arm-low.c (struct linux_target_ops): Likewise.
	* linux-bfin-low.c (struct linux_target_ops): Likewise.
	* linux-cris-low.c (struct linux_target_ops): Likewise.
	* linux-crisv32-low.c (struct linux_target_ops): Likewise.
	* linux-low.c (can_software_single_step): Likewise.
	(install_software_single_step_breakpoints): New function.
	(start_step_over): Use install_software_single_step_breakpoints.
	* linux-low.h: New CORE_ADDR vector.
	(struct linux_target_ops) Rename breakpoint_reinsert_addr to
	get_next_pcs.
	* linux-mips-low.c (struct linux_target_ops): Likewise.
	* linux-nios2-low.c (struct linux_target_ops): Likewise.
	* linux-sparc-low.c (struct linux_target_ops): Likewise.
---
 gdb/gdbserver/linux-aarch64-low.c |  2 +-
 gdb/gdbserver/linux-arm-low.c     |  2 +-
 gdb/gdbserver/linux-bfin-low.c    |  2 +-
 gdb/gdbserver/linux-cris-low.c    |  2 +-
 gdb/gdbserver/linux-crisv32-low.c |  2 +-
 gdb/gdbserver/linux-low.c         | 28 ++++++++++++++++++++++++----
 gdb/gdbserver/linux-low.h         |  5 ++++-
 gdb/gdbserver/linux-mips-low.c    |  2 +-
 gdb/gdbserver/linux-nios2-low.c   |  2 +-
 gdb/gdbserver/linux-sparc-low.c   |  2 +-
 10 files changed, 36 insertions(+), 13 deletions(-)

Comments

Yao Qi Nov. 26, 2015, 10:29 a.m. UTC | #1 
Antoine Tremblay <antoine.tremblay@ericsson.com> writes:

> gdb/gdbserver/ChangeLog:
> 	* linux-aarch64-low.c (struct linux_target_ops): Rename
> 	breakpoint_reinsert_addr to get_next_pcs.

Nit: the change is to the_low_target, not "struct linux_target_ops", so
it should be

 	* linux-aarch64-low.c (the_low_target): ....

> 	* linux-arm-low.c (struct linux_target_ops): Likewise.
> 	* linux-bfin-low.c (struct linux_target_ops): Likewise.
> 	* linux-cris-low.c (struct linux_target_ops): Likewise.
> 	* linux-crisv32-low.c (struct linux_target_ops): Likewise.
> 	* linux-low.c (can_software_single_step): Likewise.
> 	(install_software_single_step_breakpoints): New function.
> 	(start_step_over): Use install_software_single_step_breakpoints.
> 	* linux-low.h: New CORE_ADDR vector.
> 	(struct linux_target_ops) Rename breakpoint_reinsert_addr to
> 	get_next_pcs.
> 	* linux-mips-low.c (struct linux_target_ops): Likewise.
> 	* linux-nios2-low.c (struct linux_target_ops): Likewise.
> 	* linux-sparc-low.c (struct linux_target_ops): Likewise.

Patch is good to me.
Pedro Alves Nov. 26, 2015, 10:50 a.m. UTC | #2 
On 11/23/2015 02:12 PM, Antoine Tremblay wrote:
> In this v3:
> 
>  * Changed get_next_pcs signature.
>  * Remove common/get-next-pcs.h file.
>  * CORE_ADDR is now in linux-low.h temporarely.
> 
> ---
> 
> This patch in preparation for software single step support on ARM. It refactors
> breakpoint_reinsert_addr into get_next_pcs so that multiple location can be
> returned.
> 
> When software single stepping there can be multiple possible next addresses
> because we're stepping over a conditional branch instruction for example.
> 
> The operation get_next_pcs handles that by returning a vector of all the
> possible next addresses.
> 
> Software breakpoints are installed at each location returned.
> 


Thanks for doing this.  This interface does look cleaner to me, and a
more direct mapping C++ (so easier to clean up in the future).

>  
> +/* Install breakpoints for software single stepping.  */
> +
> +static void
> +install_software_single_step_breakpoints (struct lwp_info *lwp)
> +{
> +  int i;
> +  CORE_ADDR pc;
> +  struct regcache *regcache = get_thread_regcache (current_thread, 1);
> +  VEC (CORE_ADDR) *next_pcs = NULL;

I think this should be freed with a cleanup.

> +
> +  pc = get_pc (lwp);
> +  next_pcs = (*the_low_target.get_next_pcs) (pc, regcache);
> +
> +  for (i = 0; VEC_iterate (CORE_ADDR, next_pcs, i, pc); ++i)
> +    {
> +      set_reinsert_breakpoint (pc);
> +    }

Single-line statements don't get braces.

Otherwise LGTM.

Thanks,
Pedro Alves
Antoine Tremblay Nov. 26, 2015, 1:47 p.m. UTC | #3 
On 11/26/2015 05:29 AM, Yao Qi wrote:
> Antoine Tremblay <antoine.tremblay@ericsson.com> writes:
>
>> gdb/gdbserver/ChangeLog:
>> 	* linux-aarch64-low.c (struct linux_target_ops): Rename
>> 	breakpoint_reinsert_addr to get_next_pcs.
>
> Nit: the change is to the_low_target, not "struct linux_target_ops", so
> it should be
>

Right indeed. fixed.
Antoine Tremblay Nov. 26, 2015, 1:50 p.m. UTC | #4 
On 11/26/2015 05:50 AM, Pedro Alves wrote:
> On 11/23/2015 02:12 PM, Antoine Tremblay wrote:
>> In this v3:
>>
>>   * Changed get_next_pcs signature.
>>   * Remove common/get-next-pcs.h file.
>>   * CORE_ADDR is now in linux-low.h temporarely.
>>
>> ---
>>
>> This patch in preparation for software single step support on ARM. It refactors
>> breakpoint_reinsert_addr into get_next_pcs so that multiple location can be
>> returned.
>>
>> When software single stepping there can be multiple possible next addresses
>> because we're stepping over a conditional branch instruction for example.
>>
>> The operation get_next_pcs handles that by returning a vector of all the
>> possible next addresses.
>>
>> Software breakpoints are installed at each location returned.
>>
>
>
> Thanks for doing this.  This interface does look cleaner to me, and a
> more direct mapping C++ (so easier to clean up in the future).
>

Indeed thanks for the idea!

>>
>> +/* Install breakpoints for software single stepping.  */
>> +
>> +static void
>> +install_software_single_step_breakpoints (struct lwp_info *lwp)
>> +{
>> +  int i;
>> +  CORE_ADDR pc;
>> +  struct regcache *regcache = get_thread_regcache (current_thread, 1);
>> +  VEC (CORE_ADDR) *next_pcs = NULL;
>
> I think this should be freed with a cleanup.
>
>> +
>> +  pc = get_pc (lwp);
>> +  next_pcs = (*the_low_target.get_next_pcs) (pc, regcache);
>> +
>> +  for (i = 0; VEC_iterate (CORE_ADDR, next_pcs, i, pc); ++i)
>> +    {
>> +      set_reinsert_breakpoint (pc);
>> +    }
>
> Single-line statements don't get braces.
>

Done, learning the style :)

Thank you,
Antoine
diff mbox

Patch

diff --git a/gdb/gdbserver/linux-aarch64-low.c b/gdb/gdbserver/linux-aarch64-low.c
index 17798ff..3b8a9ad 100644
--- a/gdb/gdbserver/linux-aarch64-low.c
+++ b/gdb/gdbserver/linux-aarch64-low.c
@@ -2963,7 +2963,7 @@  struct linux_target_ops the_low_target =
   aarch64_set_pc,
   NULL, /* breakpoint_kind_from_pc */
   aarch64_sw_breakpoint_from_kind,
-  NULL, /* breakpoint_reinsert_addr */
+  NULL, /* get_next_pcs */
   0,    /* decr_pc_after_break */
   aarch64_breakpoint_at,
   aarch64_supports_z_point_type,
diff --git a/gdb/gdbserver/linux-arm-low.c b/gdb/gdbserver/linux-arm-low.c
index 885aec9..78a4c8f 100644
--- a/gdb/gdbserver/linux-arm-low.c
+++ b/gdb/gdbserver/linux-arm-low.c
@@ -1032,7 +1032,7 @@  struct linux_target_ops the_low_target = {
   arm_set_pc,
   arm_breakpoint_kind_from_pc,
   arm_sw_breakpoint_from_kind,
-  NULL, /* breakpoint_reinsert_addr */
+  NULL, /* get_next_pcs */
   0,
   arm_breakpoint_at,
   arm_supports_z_point_type,
diff --git a/gdb/gdbserver/linux-bfin-low.c b/gdb/gdbserver/linux-bfin-low.c
index 912d253..1de2f78 100644
--- a/gdb/gdbserver/linux-bfin-low.c
+++ b/gdb/gdbserver/linux-bfin-low.c
@@ -141,7 +141,7 @@  struct linux_target_ops the_low_target = {
   bfin_set_pc,
   NULL, /* breakpoint_kind_from_pc */
   bfin_sw_breakpoint_from_kind,
-  NULL, /* breakpoint_reinsert_addr */
+  NULL, /* get_next_pcs */
   2,
   bfin_breakpoint_at,
   NULL, /* supports_z_point_type */
diff --git a/gdb/gdbserver/linux-cris-low.c b/gdb/gdbserver/linux-cris-low.c
index 9f4519c..6902a45 100644
--- a/gdb/gdbserver/linux-cris-low.c
+++ b/gdb/gdbserver/linux-cris-low.c
@@ -139,7 +139,7 @@  struct linux_target_ops the_low_target = {
   cris_set_pc,
   NULL, /* breakpoint_kind_from_pc */
   cris_sw_breakpoint_from_kind,
-  NULL, /* breakpoint_reinsert_addr */
+  NULL, /* get_next_pcs */
   0,
   cris_breakpoint_at,
 };
diff --git a/gdb/gdbserver/linux-crisv32-low.c b/gdb/gdbserver/linux-crisv32-low.c
index 2404d0e..28c1981 100644
--- a/gdb/gdbserver/linux-crisv32-low.c
+++ b/gdb/gdbserver/linux-crisv32-low.c
@@ -422,7 +422,7 @@  struct linux_target_ops the_low_target = {
   cris_set_pc,
   NULL, /* breakpoint_kind_from_pc */
   cris_sw_breakpoint_from_kind,
-  NULL, /* breakpoint_reinsert_addr */
+  NULL, /* get_next_pcs */
   0,
   cris_breakpoint_at,
   cris_supports_z_point_type,
diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index ce9cc2e..85a8e3f 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -281,12 +281,12 @@  can_hardware_single_step (void)
 }
 
 /* True if the low target can software single-step.  Such targets
-   implement the BREAKPOINT_REINSERT_ADDR callback.  */
+   implement the GET_NEXT_PCS callback.  */
 
 static int
 can_software_single_step (void)
 {
-  return (the_low_target.breakpoint_reinsert_addr != NULL);
+  return (the_low_target.get_next_pcs != NULL);
 }
 
 /* True if the low target supports memory breakpoints.  If so, we'll
@@ -3876,6 +3876,27 @@  enqueue_pending_signal (struct lwp_info *lwp, int signal, siginfo_t *info)
   lwp->pending_signals = p_sig;
 }
 
+/* Install breakpoints for software single stepping.  */
+
+static void
+install_software_single_step_breakpoints (struct lwp_info *lwp)
+{
+  int i;
+  CORE_ADDR pc;
+  struct regcache *regcache = get_thread_regcache (current_thread, 1);
+  VEC (CORE_ADDR) *next_pcs = NULL;
+
+  pc = get_pc (lwp);
+  next_pcs = (*the_low_target.get_next_pcs) (pc, regcache);
+
+  for (i = 0; VEC_iterate (CORE_ADDR, next_pcs, i, pc); ++i)
+    {
+      set_reinsert_breakpoint (pc);
+    }
+
+  VEC_free (CORE_ADDR, next_pcs);
+}
+
 /* Resume execution of LWP.  If STEP is nonzero, single-step it.  If
    SIGNAL is nonzero, give it that signal.  */
 
@@ -4439,8 +4460,7 @@  start_step_over (struct lwp_info *lwp)
     }
   else if (can_software_single_step ())
     {
-      CORE_ADDR raddr = (*the_low_target.breakpoint_reinsert_addr) ();
-      set_reinsert_breakpoint (raddr);
+      install_software_single_step_breakpoints (lwp);
       step = 0;
     }
   else
diff --git a/gdb/gdbserver/linux-low.h b/gdb/gdbserver/linux-low.h
index d60d97d..ee8d88f 100644
--- a/gdb/gdbserver/linux-low.h
+++ b/gdb/gdbserver/linux-low.h
@@ -124,6 +124,8 @@  struct process_info_private
 
 struct lwp_info;
 
+DEF_VEC_I (CORE_ADDR);
+
 struct linux_target_ops
 {
   /* Architecture-specific setup.  */
@@ -153,7 +155,8 @@  struct linux_target_ops
   /* See target.h for details.  */
   const gdb_byte *(*sw_breakpoint_from_kind) (int kind, int *size);
 
-  CORE_ADDR (*breakpoint_reinsert_addr) (void);
+  /* Find the next possible PCs after the current instruction executes.  */
+  VEC (CORE_ADDR) *(*get_next_pcs) (CORE_ADDR pc, struct regcache *regcache);
 
   int decr_pc_after_break;
   int (*breakpoint_at) (CORE_ADDR pc);
diff --git a/gdb/gdbserver/linux-mips-low.c b/gdb/gdbserver/linux-mips-low.c
index b8f8805..9da4610 100644
--- a/gdb/gdbserver/linux-mips-low.c
+++ b/gdb/gdbserver/linux-mips-low.c
@@ -880,7 +880,7 @@  struct linux_target_ops the_low_target = {
   mips_set_pc,
   NULL, /* breakpoint_kind_from_pc */
   mips_sw_breakpoint_from_kind,
-  NULL, /* breakpoint_reinsert_addr */
+  NULL, /* get_next_pcs */
   0,
   mips_breakpoint_at,
   mips_supports_z_point_type,
diff --git a/gdb/gdbserver/linux-nios2-low.c b/gdb/gdbserver/linux-nios2-low.c
index 9380c3b..1accc03 100644
--- a/gdb/gdbserver/linux-nios2-low.c
+++ b/gdb/gdbserver/linux-nios2-low.c
@@ -267,7 +267,7 @@  struct linux_target_ops the_low_target =
   nios2_set_pc,
   NULL, /* breakpoint_kind_from_pc */
   nios2_sw_breakpoint_from_kind,
-  NULL, /* breakpoint_reinsert_addr */
+  NULL, /* get_next_pcs */
   0,
   nios2_breakpoint_at,
 };
diff --git a/gdb/gdbserver/linux-sparc-low.c b/gdb/gdbserver/linux-sparc-low.c
index 54a849c..bfdd8c1 100644
--- a/gdb/gdbserver/linux-sparc-low.c
+++ b/gdb/gdbserver/linux-sparc-low.c
@@ -320,7 +320,7 @@  struct linux_target_ops the_low_target = {
   NULL,
   NULL, /* breakpoint_kind_from_pc */
   sparc_sw_breakpoint_from_kind,
-  NULL, /* breakpoint_reinsert_addr */
+  NULL, /* get_next_pcs */
   0,
   sparc_breakpoint_at,
   NULL,  /* supports_z_point_type */