[PATCH] TARGET_ADJUST_BREAKPOINT_ADDRESS - patch 4 of 4

[PATCH] TARGET_ADJUST_BREAKPOINT_ADDRESS - patch 4 of 4

[Kevin Buettner]

Below is patch 4 of 4 of my current TARGET_ADJUST_BREAKPOINT_ADDRESS
patch set.  To better understand what this is all about, see patch 3
at:

    http://sources.redhat.com/ml/gdb-patches/2003-10/msg00073.html

I'll commit this patch sometime after patch 1 is committed.  (In order
to actually be used, it depends on patch 3, but no harm will occur if
it goes in before patch 3.)

	* frv-tdep.c (frv_target_adjust_breakpoint_address): New function.
	(frv_gdbarch_init): Initialize TARGET_ADJUST_BREAKPOINT_ADDRESS
	method.

Index: frv-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/frv-tdep.c,v
retrieving revision 1.53
diff -u -p -r1.53 frv-tdep.c
--- frv-tdep.c	19 Sep 2003 16:22:38 -0000	1.53
+++ frv-tdep.c	3 Oct 2003 23:38:25 -0000
@@ -37,6 +37,7 @@ static gdbarch_init_ftype frv_gdbarch_in
 
 static gdbarch_register_name_ftype frv_register_name;
 static gdbarch_breakpoint_from_pc_ftype frv_breakpoint_from_pc;
+static gdbarch_target_adjust_breakpoint_address_ftype frv_target_adjust_breakpoint_address;
 static gdbarch_skip_prologue_ftype frv_skip_prologue;
 static gdbarch_deprecated_extract_return_value_ftype frv_extract_return_value;
 static gdbarch_deprecated_extract_struct_value_address_ftype frv_extract_struct_value_address;
@@ -272,6 +273,42 @@ frv_breakpoint_from_pc (CORE_ADDR *pcptr
 }
 
 
+static CORE_ADDR
+frv_target_adjust_breakpoint_address (CORE_ADDR bpaddr)
+{
+  int count = 8;
+  CORE_ADDR addr = bpaddr - 4;
+  CORE_ADDR func_start = get_pc_function_start (bpaddr);
+
+  /* Find the end of the previous packing sequence.  This will be indicated
+     by either attempting to access some inaccessible memory or by finding
+     an instruction word whose packing bit is set to one. */
+  while (count-- > 0 && addr >= func_start)
+    {
+      int status;
+      char instr[4];
+
+      status = read_memory_nobpt (addr, instr, sizeof instr);
+
+      if (status != 0)
+	break;
+
+      /* This is a big endian architecture, so byte zero will have most
+         significant byte.  The most significant bit of this byte is the
+         packing bit.  */
+      if (instr[0] & 0x80)
+	break;
+
+      addr -= 4;
+    }
+
+  if (count > 0)
+    bpaddr = addr + 4;
+
+  return bpaddr;
+}
+
+
 /* Return true if REG is a caller-saves ("scratch") register,
    false otherwise.  */
 static int
@@ -1114,6 +1151,7 @@ frv_gdbarch_init (struct gdbarch_info in
 
   set_gdbarch_skip_prologue (gdbarch, frv_skip_prologue);
   set_gdbarch_breakpoint_from_pc (gdbarch, frv_breakpoint_from_pc);
+  set_gdbarch_target_adjust_breakpoint_address (gdbarch, frv_target_adjust_breakpoint_address);
 
   set_gdbarch_frame_args_skip (gdbarch, 0);
   set_gdbarch_frameless_function_invocation (gdbarch, frv_frameless_function_invocation);

Re: [PATCH] TARGET_ADJUST_BREAKPOINT_ADDRESS - patch 4 of 4

[Michael Snyder]

Kevin Buettner wrote:
> Below is patch 4 of 4 of my current TARGET_ADJUST_BREAKPOINT_ADDRESS
> patch set.  To better understand what this is all about, see patch 3
> at:
> 
>     http://sources.redhat.com/ml/gdb-patches/2003-10/msg00073.html
> 
> I'll commit this patch sometime after patch 1 is committed.  (In order
> to actually be used, it depends on patch 3, but no harm will occur if
> it goes in before patch 3.)
> 
> 	* frv-tdep.c (frv_target_adjust_breakpoint_address): New function.
> 	(frv_gdbarch_init): Initialize TARGET_ADJUST_BREAKPOINT_ADDRESS
> 	method.

Hi Kevin,

My first feedback is that "count" should be set to a const
rather than to 8 -- since we've seen it change from 4 to 8
just recently, and who knows when it will change again.
That'll also provide an opportunity to explain (via the
name of the const, if nothing else) what 'count' is.

Maybe "bpaddr - 4" could also be "bpaddr - sizeof (something)".

Michael

> 
> Index: frv-tdep.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/frv-tdep.c,v
> retrieving revision 1.53
> diff -u -p -r1.53 frv-tdep.c
> --- frv-tdep.c	19 Sep 2003 16:22:38 -0000	1.53
> +++ frv-tdep.c	3 Oct 2003 23:38:25 -0000
> @@ -37,6 +37,7 @@ static gdbarch_init_ftype frv_gdbarch_in
>  
>  static gdbarch_register_name_ftype frv_register_name;
>  static gdbarch_breakpoint_from_pc_ftype frv_breakpoint_from_pc;
> +static gdbarch_target_adjust_breakpoint_address_ftype frv_target_adjust_breakpoint_address;
>  static gdbarch_skip_prologue_ftype frv_skip_prologue;
>  static gdbarch_deprecated_extract_return_value_ftype frv_extract_return_value;
>  static gdbarch_deprecated_extract_struct_value_address_ftype frv_extract_struct_value_address;
> @@ -272,6 +273,42 @@ frv_breakpoint_from_pc (CORE_ADDR *pcptr
>  }
>  
>  
> +static CORE_ADDR
> +frv_target_adjust_breakpoint_address (CORE_ADDR bpaddr)
> +{
> +  int count = 8;
> +  CORE_ADDR addr = bpaddr - 4;
> +  CORE_ADDR func_start = get_pc_function_start (bpaddr);
> +
> +  /* Find the end of the previous packing sequence.  This will be indicated
> +     by either attempting to access some inaccessible memory or by finding
> +     an instruction word whose packing bit is set to one. */
> +  while (count-- > 0 && addr >= func_start)
> +    {
> +      int status;
> +      char instr[4];
> +
> +      status = read_memory_nobpt (addr, instr, sizeof instr);
> +
> +      if (status != 0)
> +	break;
> +
> +      /* This is a big endian architecture, so byte zero will have most
> +         significant byte.  The most significant bit of this byte is the
> +         packing bit.  */
> +      if (instr[0] & 0x80)
> +	break;
> +
> +      addr -= 4;
> +    }
> +
> +  if (count > 0)
> +    bpaddr = addr + 4;
> +
> +  return bpaddr;
> +}
> +
> +
>  /* Return true if REG is a caller-saves ("scratch") register,
>     false otherwise.  */
>  static int
> @@ -1114,6 +1151,7 @@ frv_gdbarch_init (struct gdbarch_info in
>  
>    set_gdbarch_skip_prologue (gdbarch, frv_skip_prologue);
>    set_gdbarch_breakpoint_from_pc (gdbarch, frv_breakpoint_from_pc);
> +  set_gdbarch_target_adjust_breakpoint_address (gdbarch, frv_target_adjust_breakpoint_address);
>  
>    set_gdbarch_frame_args_skip (gdbarch, 0);
>    set_gdbarch_frameless_function_invocation (gdbarch, frv_frameless_function_invocation);
> 

Re: [PATCH] TARGET_ADJUST_BREAKPOINT_ADDRESS - patch 4 of 4

[Kevin Buettner]

On Oct 7,  4:00pm, Michael Snyder wrote:

> My first feedback is that "count" should be set to a const
> rather than to 8 -- since we've seen it change from 4 to 8
> just recently, and who knows when it will change again.
> That'll also provide an opportunity to explain (via the
> name of the const, if nothing else) what 'count' is.

Thanks for the feedback.

I've added ``max_instrs_per_bundle'' along with a comment about what
this is.

> Maybe "bpaddr - 4" could also be "bpaddr - sizeof (something)".

I hear alarm bells go off in my head whenever I see sizeof() used in
target specific code.  (Sometimes it's perfectly okay, but I hear them
just the same.)  So, what I've done instead is to define
``frv_instr_size'' and use that constant instead of 4 in
frv_gdbarch_adjust_breakpoint_address().  The same constant could also
be used in the prologue scanning code.  (I'll prepare a separate
patch...)

Here's what I've committed:

	* frv-tdep.c (max_instrs_per_bundle, frv_instr_size): New constants.
	(frv_gdbarch_adjust_breakpoint_address): New function.
	(frv_gdbarch_init): Initialize ``gdbarch_adjust_breakpoint_address''
	method.

Index: frv-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/frv-tdep.c,v
retrieving revision 1.53
diff -u -p -r1.53 frv-tdep.c
--- frv-tdep.c	19 Sep 2003 16:22:38 -0000	1.53
+++ frv-tdep.c	14 Oct 2003 00:26:39 -0000
@@ -37,6 +37,7 @@ static gdbarch_init_ftype frv_gdbarch_in
 
 static gdbarch_register_name_ftype frv_register_name;
 static gdbarch_breakpoint_from_pc_ftype frv_breakpoint_from_pc;
+static gdbarch_adjust_breakpoint_address_ftype frv_gdbarch_adjust_breakpoint_address;
 static gdbarch_skip_prologue_ftype frv_skip_prologue;
 static gdbarch_deprecated_extract_return_value_ftype frv_extract_return_value;
 static gdbarch_deprecated_extract_struct_value_address_ftype frv_extract_struct_value_address;
@@ -271,6 +272,51 @@ frv_breakpoint_from_pc (CORE_ADDR *pcptr
   return breakpoint;
 }
 
+/* Define the maximum number of instructions which may be packed into a
+   bundle (VLIW instruction).  */
+static const int max_instrs_per_bundle = 8;
+
+/* Define the size (in bytes) of an FR-V instruction.  */
+static const int frv_instr_size = 4;
+
+/* Adjust a breakpoint's address to account for the FR-V architecture's
+   constraint that a break instruction must not appear as any but the
+   first instruction in the bundle.  */
+static CORE_ADDR
+frv_gdbarch_adjust_breakpoint_address (struct gdbarch *gdbarch, CORE_ADDR bpaddr)
+{
+  int count = max_instrs_per_bundle;
+  CORE_ADDR addr = bpaddr - frv_instr_size;
+  CORE_ADDR func_start = get_pc_function_start (bpaddr);
+
+  /* Find the end of the previous packing sequence.  This will be indicated
+     by either attempting to access some inaccessible memory or by finding
+     an instruction word whose packing bit is set to one. */
+  while (count-- > 0 && addr >= func_start)
+    {
+      char instr[frv_instr_size];
+      int status;
+
+      status = read_memory_nobpt (addr, instr, sizeof instr);
+
+      if (status != 0)
+	break;
+
+      /* This is a big endian architecture, so byte zero will have most
+         significant byte.  The most significant bit of this byte is the
+         packing bit.  */
+      if (instr[0] & 0x80)
+	break;
+
+      addr -= frv_instr_size;
+    }
+
+  if (count > 0)
+    bpaddr = addr + frv_instr_size;
+
+  return bpaddr;
+}
+
 
 /* Return true if REG is a caller-saves ("scratch") register,
    false otherwise.  */
@@ -1114,6 +1160,7 @@ frv_gdbarch_init (struct gdbarch_info in
 
   set_gdbarch_skip_prologue (gdbarch, frv_skip_prologue);
   set_gdbarch_breakpoint_from_pc (gdbarch, frv_breakpoint_from_pc);
+  set_gdbarch_adjust_breakpoint_address (gdbarch, frv_gdbarch_adjust_breakpoint_address);
 
   set_gdbarch_frame_args_skip (gdbarch, 0);
   set_gdbarch_frameless_function_invocation (gdbarch, frv_frameless_function_invocation);

Re: [PATCH] TARGET_ADJUST_BREAKPOINT_ADDRESS - patch 4 of 4

[Michael Snyder]

Kevin Buettner wrote:
> On Oct 7,  4:00pm, Michael Snyder wrote:
> 
>>Maybe "bpaddr - 4" could also be "bpaddr - sizeof (something)".
> 
> 
> I hear alarm bells go off in my head whenever I see sizeof() used in
> target specific code.  (Sometimes it's perfectly okay, but I hear them
> just the same.)  So, what I've done instead is to define
> ``frv_instr_size'' and use that constant instead of 4 in
> frv_gdbarch_adjust_breakpoint_address().  

That's kinda what I meant.  "sizeof" was a concept, not an operator.  ;-)

Thanks for the tweaks.