[v2,7/7] Support software breakpoints for ARM linux in GDBServer.

Message ID 1444063455-31558-8-git-send-email-antoine.tremblay@ericsson.com
State New, archived
Headers

Commit Message

Antoine Tremblay Oct. 5, 2015, 4:44 p.m. UTC
  This patch implements the breakpoint_from_kind operation introduced
in a previous patch.

The proper breakpoint can then be returned to be inserted in memory.

It enables software breakpoints via GDB's Z0 packets on ARM.

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

gdb/ChangeLog:
	* NEWS: Add news for software breakpoints.

gdb/gdbserver/ChangeLog:
	* linux-arm-low.c (arm_breakpoint_from_kind): New function.
	(arm_supports_z_point_type): Add software breakpoint support.
	(struct linux_target_ops) <breakpoint_from_kind>: Initialize field.
---
 gdb/NEWS                      |  2 ++
 gdb/gdbserver/linux-arm-low.c | 31 +++++++++++++++++++++++++++++++
 2 files changed, 33 insertions(+)
  

Comments

Eli Zaretskii Oct. 5, 2015, 5 p.m. UTC | #1
> From: Antoine Tremblay <antoine.tremblay@ericsson.com>
> CC: Antoine Tremblay <antoine.tremblay@ericsson.com>
> Date: Mon, 5 Oct 2015 12:44:15 -0400
> 
> This patch implements the breakpoint_from_kind operation introduced
> in a previous patch.
> 
> The proper breakpoint can then be returned to be inserted in memory.
> 
> It enables software breakpoints via GDB's Z0 packets on ARM.
> 
> No regressions, tested on ubuntu 14.04 ARMv7 and x86.
> With gdbserver-{native,extended} / { -marm -mthumb }
> 
> gdb/ChangeLog:
> 	* NEWS: Add news for software breakpoints.
> 
> gdb/gdbserver/ChangeLog:
> 	* linux-arm-low.c (arm_breakpoint_from_kind): New function.
> 	(arm_supports_z_point_type): Add software breakpoint support.
> 	(struct linux_target_ops) <breakpoint_from_kind>: Initialize field.

OK for the NEWS part.
  
Pedro Alves Oct. 15, 2015, 4:07 p.m. UTC | #2
On 10/05/2015 05:44 PM, Antoine Tremblay wrote:

> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -3,6 +3,8 @@
>  
>  *** Changes since GDB 7.10
>  
> +* Support for software breakpoints on ARM linux was added in GDBServer.

Putting a user hat on, what does this mean?  Why is it news worthy?

> +
>  * Record btrace now supports non-stop mode.
>  
>  * Support for tracepoints on aarch64-linux was added in GDBserver.
> diff --git a/gdb/gdbserver/linux-arm-low.c b/gdb/gdbserver/linux-arm-low.c
> index d16ea60..bd499f8 100644
> --- a/gdb/gdbserver/linux-arm-low.c
> +++ b/gdb/gdbserver/linux-arm-low.c
> @@ -336,6 +336,28 @@ arm_breakpoint_from_pc (CORE_ADDR *pcptr, int *lenptr)
>      }
>  }
>  
> +/* Get the breakpoint from the remote kind
> +   2 is thumb-16
> +   3 is thumb2-32
> +   4 is arm
> +*/
> +static const unsigned char *
> +arm_breakpoint_from_kind (int *kind)
> +{
> +  switch (*kind) {
> +  case 2:

Formatting, break line before {, indent case.

> +    return (unsigned char *) &thumb_breakpoint;
> +  case 3:
> +    *kind = 4;
> +    return (unsigned char *) &thumb2_breakpoint;
> +  case 4:
> +    return (unsigned char *) &arm_breakpoint;
> +  default:
> +    return NULL;
> +  }
> +  return NULL;
> +}
> +

Thanks,
Pedro Alves
  
Antoine Tremblay Oct. 15, 2015, 6:23 p.m. UTC | #3
On 10/15/2015 12:07 PM, Pedro Alves wrote:
> On 10/05/2015 05:44 PM, Antoine Tremblay wrote:
>
>> --- a/gdb/NEWS
>> +++ b/gdb/NEWS
>> @@ -3,6 +3,8 @@
>>
>>   *** Changes since GDB 7.10
>>
>> +* Support for software breakpoints on ARM linux was added in GDBServer.
>
> Putting a user hat on, what does this mean?  Why is it news worthy?
>

Humm I had not thought about it this way would saying something like :

* Support for software breakpoints on ARM linux was added in GDBServer. 
Users can now call break to set a breakpoint with GDBServer on ARM linux.

Be better ?

>> +
>>   * Record btrace now supports non-stop mode.
>>
>>   * Support for tracepoints on aarch64-linux was added in GDBserver.
>> diff --git a/gdb/gdbserver/linux-arm-low.c b/gdb/gdbserver/linux-arm-low.c
>> index d16ea60..bd499f8 100644
>> --- a/gdb/gdbserver/linux-arm-low.c
>> +++ b/gdb/gdbserver/linux-arm-low.c
>> @@ -336,6 +336,28 @@ arm_breakpoint_from_pc (CORE_ADDR *pcptr, int *lenptr)
>>       }
>>   }
>>
>> +/* Get the breakpoint from the remote kind
>> +   2 is thumb-16
>> +   3 is thumb2-32
>> +   4 is arm
>> +*/
>> +static const unsigned char *
>> +arm_breakpoint_from_kind (int *kind)
>> +{
>> +  switch (*kind) {
>> +  case 2:
>
> Formatting, break line before {, indent case.
>
Indeed seems like the switch case indent was not in the Emacs GNU 
default config

Done.
  
Pedro Alves Oct. 15, 2015, 6:33 p.m. UTC | #4
On 10/15/2015 07:23 PM, Antoine Tremblay wrote:
> 
> 
> On 10/15/2015 12:07 PM, Pedro Alves wrote:
>> On 10/05/2015 05:44 PM, Antoine Tremblay wrote:
>>
>>> --- a/gdb/NEWS
>>> +++ b/gdb/NEWS
>>> @@ -3,6 +3,8 @@
>>>
>>>   *** Changes since GDB 7.10
>>>
>>> +* Support for software breakpoints on ARM linux was added in GDBServer.
>>
>> Putting a user hat on, what does this mean?  Why is it news worthy?
>>
> 
> Humm I had not thought about it this way would saying something like :
> 
> * Support for software breakpoints on ARM linux was added in GDBServer. 
> Users can now call break to set a breakpoint with GDBServer on ARM linux.

When I read this I'm left confused and wonder what's new, because
it suggests users couldn't already call break and set breakpoints before,
but, they obviously could.

Thanks,
Pedro Alves
  
Antoine Tremblay Oct. 15, 2015, 6:59 p.m. UTC | #5
On 10/15/2015 02:33 PM, Pedro Alves wrote:
> On 10/15/2015 07:23 PM, Antoine Tremblay wrote:
>>
>>
>> On 10/15/2015 12:07 PM, Pedro Alves wrote:
>>> On 10/05/2015 05:44 PM, Antoine Tremblay wrote:
>>>
>>>> --- a/gdb/NEWS
>>>> +++ b/gdb/NEWS
>>>> @@ -3,6 +3,8 @@
>>>>
>>>>    *** Changes since GDB 7.10
>>>>
>>>> +* Support for software breakpoints on ARM linux was added in GDBServer.
>>>
>>> Putting a user hat on, what does this mean?  Why is it news worthy?
>>>
>>
>> Humm I had not thought about it this way would saying something like :
>>
>> * Support for software breakpoints on ARM linux was added in GDBServer.
>> Users can now call break to set a breakpoint with GDBServer on ARM linux.
>
> When I read this I'm left confused and wonder what's new, because
> it suggests users couldn't already call break and set breakpoints before,
> but, they obviously could.
>

Right indeed, I can't explain what would this add from a user point of 
view so I will remove the NEWS entry.
  
Yao Qi Oct. 16, 2015, 9:30 a.m. UTC | #6
On 15/10/15 17:07, Pedro Alves wrote:
>> +* Support for software breakpoints on ARM linux was added in GDBServer.
> Putting a user hat on, what does this mean?  Why is it news worthy?
>

Supporting software breakpoint on ARM Linux GDBserver isn't user
visible, but isn't it a user-visible change that ARM Linux GDBserver 
supports Z0 packet?
  
Yao Qi Oct. 16, 2015, 11:52 a.m. UTC | #7
Antoine Tremblay <antoine.tremblay@ericsson.com> writes:

> +/* Get the breakpoint from the remote kind
> +   2 is thumb-16
> +   3 is thumb2-32
> +   4 is arm
> +*/

Need comment /* Implementation of .... */

> +static const unsigned char *
> +arm_breakpoint_from_kind (int *kind)
> +{
> +  switch (*kind) {

Wrong format.  "{" should be put in the next line.

> +  case 2:
> +    return (unsigned char *) &thumb_breakpoint;
> +  case 3:
> +    *kind = 4;
> +    return (unsigned char *) &thumb2_breakpoint;
> +  case 4:
> +    return (unsigned char *) &arm_breakpoint;
> +  default:
> +    return NULL;
> +  }
> +  return NULL;
> +}

Otherwise, patch is good to me.
  
Yao Qi Oct. 16, 2015, 11:53 a.m. UTC | #8
On 16/10/15 12:52, Yao Qi wrote:
> Otherwise, patch is good to me.

FAOD´╝î I meant non-NEWS part of patch is good to me.
  
Pedro Alves Oct. 16, 2015, 12:11 p.m. UTC | #9
On 10/16/2015 10:30 AM, Yao Qi wrote:
> On 15/10/15 17:07, Pedro Alves wrote:
>>> +* Support for software breakpoints on ARM linux was added in GDBServer.
>> Putting a user hat on, what does this mean?  Why is it news worthy?
>>
> 
> Supporting software breakpoint on ARM Linux GDBserver isn't user
> visible, but isn't it a user-visible change that ARM Linux GDBserver 
> supports Z0 packet?

How can the user tell?  Is there any user-visible functionality or feature
that this enables?  Something the user couldn't do before but now can?
It just looks like an implementation detail to me.

If we're going to mention it, then I think it should be described
in terms of the packet, otherwise we're back to "but I could always
set software breakpoints before, what's new?".

BTW, I think we should move all the new gdbserver features
to a new section, like in previous releases.  Then the end result
would be something like:

* New features in the GDB remote stub, GDBserver

   ** Support for software breakpoint packets (Z0) on ARM Linux.

   ** Support for tracepoints on Aarch64 Linux.

   ** Support for fast tracepoints on Aarch64 Linux, including JIT compiling
     fast tracepoint's conditional expression bytecode into native code.

Thanks,
Pedro Alves
  

Patch

diff --git a/gdb/NEWS b/gdb/NEWS
index 2e38d9a..17f1c05 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -3,6 +3,8 @@ 
 
 *** Changes since GDB 7.10
 
+* Support for software breakpoints on ARM linux was added in GDBServer.
+
 * Record btrace now supports non-stop mode.
 
 * Support for tracepoints on aarch64-linux was added in GDBserver.
diff --git a/gdb/gdbserver/linux-arm-low.c b/gdb/gdbserver/linux-arm-low.c
index d16ea60..bd499f8 100644
--- a/gdb/gdbserver/linux-arm-low.c
+++ b/gdb/gdbserver/linux-arm-low.c
@@ -336,6 +336,28 @@  arm_breakpoint_from_pc (CORE_ADDR *pcptr, int *lenptr)
     }
 }
 
+/* Get the breakpoint from the remote kind
+   2 is thumb-16
+   3 is thumb2-32
+   4 is arm
+*/
+static const unsigned char *
+arm_breakpoint_from_kind (int *kind)
+{
+  switch (*kind) {
+  case 2:
+    return (unsigned char *) &thumb_breakpoint;
+  case 3:
+    *kind = 4;
+    return (unsigned char *) &thumb2_breakpoint;
+  case 4:
+    return (unsigned char *) &arm_breakpoint;
+  default:
+    return NULL;
+  }
+  return NULL;
+}
+
 /* We only place breakpoints in empty marker functions, and thread locking
    is outside of the function.  So rather than importing software single-step,
    we can just run until exit.  */
@@ -577,6 +599,7 @@  arm_supports_z_point_type (char z_type)
 {
   switch (z_type)
     {
+    case Z_PACKET_SW_BP:
     case Z_PACKET_HW_BP:
     case Z_PACKET_WRITE_WP:
     case Z_PACKET_READ_WP:
@@ -988,6 +1011,14 @@  struct linux_target_ops the_low_target = {
   arm_new_thread,
   arm_new_fork,
   arm_prepare_to_resume,
+  NULL, /* process_qsupported */
+  NULL, /* supports_tracepoints */
+  NULL, /* get_thread_area */
+  NULL, /* install_fast_tracepoint_jump_pad */
+  NULL, /* emit_ops */
+  NULL, /* get_min_fast_tracepoint_insn_len */
+  NULL, /* supports_range_stepping */
+  arm_breakpoint_from_kind,
 };
 
 void