[rfa] insert_hw_breakpoint

[rfa] insert_hw_breakpoint

[Grace Sainsbury]

[-- Attachment #1: Type: text/plain, Size: 257 bytes --]

I fixed a little bug in the insert/remove_hw_breakpoint functions.

ok to commit?

grace

2002-08-12  Grace Sainsbury  <graces@redhat.com>

	* remote.c (remote_insert_hw_breakpoint)
	(remote_remove_hw_breakpoint): Fix calculation of length field
	Z-packet.

[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 1190 bytes --]

Index: remote.c
===================================================================
RCS file: /cvs/src/src/gdb/remote.c,v
retrieving revision 1.91
diff -u -r1.91 remote.c
--- remote.c	9 Aug 2002 16:41:24 -0000	1.91
+++ remote.c	12 Aug 2002 20:00:30 -0000
@@ -4985,11 +4985,11 @@
   char *buf = alloca (rs->remote_packet_size);
   char *p = buf;
       
-  /* The length field should be set to soething so that the packet is
-     well formed.  */
+  /* The length field should be set to the size of a breakpoint
+     instruction.  */
 
-  len = strlen (shadow);
-  len = len ? len : 1;
+  BREAKPOINT_FROM_PC (&addr, &len);
+  
   if (remote_protocol_Z[Z_PACKET_HARDWARE_BP].support == PACKET_DISABLE)
     error ("Can't set hardware breakpoint without the '%s' (%s) packet\n",
 	   remote_protocol_Z[Z_PACKET_HARDWARE_BP].name,
@@ -5027,7 +5027,8 @@
   char *buf = alloca (rs->remote_packet_size);
   char *p = buf;
   
-  len = sizeof (shadow);
+  BREAKPOINT_FROM_PC (&addr, &len);
+ 
   if (remote_protocol_Z[Z_PACKET_HARDWARE_BP].support == PACKET_DISABLE)
     error ("Can't clear hardware breakpoint without the '%s' (%s) packet\n",
 	   remote_protocol_Z[Z_PACKET_HARDWARE_BP].name,

Re: [rfa] insert_hw_breakpoint

[Kevin Buettner]

On Aug 12,  4:04pm, Grace Sainsbury wrote:

> -  len = strlen (shadow);
> -  len = len ? len : 1;
> +  BREAKPOINT_FROM_PC (&addr, &len);
> +  
>    if (remote_protocol_Z[Z_PACKET_HARDWARE_BP].support == PACKET_DISABLE)
>      error ("Can't set hardware breakpoint without the '%s' (%s) packet\n",
>  	   remote_protocol_Z[Z_PACKET_HARDWARE_BP].name,
> @@ -5027,7 +5027,8 @@
>    char *buf = alloca (rs->remote_packet_size);
>    char *p = buf;
>    
> -  len = sizeof (shadow);
> +  BREAKPOINT_FROM_PC (&addr, &len);
> + 
>    if (remote_protocol_Z[Z_PACKET_HARDWARE_BP].support == PACKET_DISABLE)
>      error ("Can't clear hardware breakpoint without the '%s' (%s) packet\n",
>  	   remote_protocol_Z[Z_PACKET_HARDWARE_BP].name,

Do stubs actually care about this length field for the packets under
consideration?

I agree that the use of strlen() and sizeof() seem dubious if not
outright wrong, but, unfortunately, not all architectures can define
a sensible BREAKPOINT_FROM_PC. (*)  I would like to see gdb relying less
on BREAKPOINT_FROM_PC, not more.

(*) E.g, IA-64.  For IA-64, I did define a BREAKPOINT_FROM_PC, but I
would have preferred not to.  The values returned by
ia64_breakpoint_from_pc() are fairly arbitrary and are of absolutely
no use for saving/restoring actual breakpoints.

Kevin

Re: [rfa] insert_hw_breakpoint

[Andrew Cagney]

> On Aug 12,  4:04pm, Grace Sainsbury wrote:
> 
> 
>> -  len = strlen (shadow);
>> -  len = len ? len : 1;
>> +  BREAKPOINT_FROM_PC (&addr, &len);
>> +  
>>    if (remote_protocol_Z[Z_PACKET_HARDWARE_BP].support == PACKET_DISABLE)
>>      error ("Can't set hardware breakpoint without the '%s' (%s) packet\n",
>>  	   remote_protocol_Z[Z_PACKET_HARDWARE_BP].name,
>> @@ -5027,7 +5027,8 @@
>>    char *buf = alloca (rs->remote_packet_size);
>>    char *p = buf;
>>    
>> -  len = sizeof (shadow);
>> +  BREAKPOINT_FROM_PC (&addr, &len);
>> + 
>>    if (remote_protocol_Z[Z_PACKET_HARDWARE_BP].support == PACKET_DISABLE)
>>      error ("Can't clear hardware breakpoint without the '%s' (%s) packet\n",
>>  	   remote_protocol_Z[Z_PACKET_HARDWARE_BP].name,

Grace,

Assuming it still works with the (RedBoot?) stub you're using then yes 
ok.  (The stub must have been ignoring the length as that could be garbage).

> Do stubs actually care about this length field for the packets under
> consideration?

The length indicates the number of bytes to consider -- in theory this 
allows GDB to set h/w breakpoint across an address range.  I suspect, 
though, that all existing stubs ignore the length since GDB hasn't been 
doing anything sensible with it.

> I agree that the use of strlen() and sizeof() seem dubious if not
> outright wrong, but, unfortunately, not all architectures can define
> a sensible BREAKPOINT_FROM_PC. (*)  I would like to see gdb relying less
> on BREAKPOINT_FROM_PC, not more.

The address should definitly be run through BREAKPOINT_FROM_PC().  On 
Arm, MIPS16, sh5, ...it will strip out any least significant mode bit so 
that the stub is handed a proper address.

As for the length, the choices would be 1, instruction size or 
breakpoint size (returned by BREAKPOINT_FROM_PC).  Given breakpoint size 
is the size of the smallest instruction, it looks to be a good 
compromize. On the i386 it is one anyway, on Arm it would be 2/4 
depending on/indicating the mode of the cpu.

> (*) E.g, IA-64.  For IA-64, I did define a BREAKPOINT_FROM_PC, but I
> would have preferred not to.  The values returned by
> ia64_breakpoint_from_pc() are fairly arbitrary and are of absolutely
> no use for saving/restoring actual breakpoints.

I think that is a separate problem.

enjoy,
Andrew