[PATCH] h8300 target breakpoint doesn't work on Simulator

[PATCH] h8300 target breakpoint doesn't work on Simulator

[Yoshinori Sato]

Hi

h8300-elf simulator handling O_BPT instruction of breakpoint.
But gdb write to O_SLEEP instruction of breakpoint.

So breakpoint command doesn't work on h8300 simulator.

I will fix this patch.
Thanks,

Index: ChangeLog
===================================================================
RCS file: /cvs/src/src/gdb/ChangeLog,v
retrieving revision 1.12730
diff -u -r1.12730 ChangeLog
--- ChangeLog	3 Mar 2011 18:35:31 -0000	1.12730
+++ ChangeLog	4 Mar 2011 16:31:41 -0000
@@ -1,3 +1,8 @@
+2011-03-04  Yoshinori Sato <ysato@users.sourceforge.jp>
+
+	* h8300-tdep.c (h8300_breakpoint_from_pc): Update to breakpoint
+	instruction
+
 2011-03-03  Tom Tromey  <tromey@redhat.com>
 
 	PR gdb/12538:
Index: h8300-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/h8300-tdep.c,v
retrieving revision 1.128
diff -u -r1.128 h8300-tdep.c
--- h8300-tdep.c	25 Jan 2011 12:13:20 -0000	1.128
+++ h8300-tdep.c	4 Mar 2011 16:31:41 -0000
@@ -1197,8 +1197,7 @@
 h8300_breakpoint_from_pc (struct gdbarch *gdbarch, CORE_ADDR *pcptr,
 			  int *lenptr)
 {
-  /*static unsigned char breakpoint[] = { 0x7A, 0xFF }; *//* ??? */
-  static unsigned char breakpoint[] = { 0x01, 0x80 };	/* Sleep */
+  static unsigned char breakpoint[] = { 0x7A, 0xFF };	/* bpt (only simulator) */
 
   *lenptr = sizeof (breakpoint);
   return breakpoint;
============================================================

-- 
Yoshinori Sato
<ysato@users.sourceforge.jp>

Re: [PATCH] h8300 target breakpoint doesn't work on Simulator

[Mike Frysinger]

[-- Attachment #1: Type: Text/Plain, Size: 984 bytes --]

On Friday, March 04, 2011 11:38:42 Yoshinori Sato wrote:
> h8300-elf simulator handling O_BPT instruction of breakpoint.
> But gdb write to O_SLEEP instruction of breakpoint.
> 
> So breakpoint command doesn't work on h8300 simulator.
> 
> --- h8300-tdep.c	25 Jan 2011 12:13:20 -0000	1.128
> +++ h8300-tdep.c	4 Mar 2011 16:31:41 -0000
> @@ -1197,8 +1197,7 @@
>  h8300_breakpoint_from_pc (struct gdbarch *gdbarch, CORE_ADDR *pcptr,
>  			  int *lenptr)
>  {
> -  /*static unsigned char breakpoint[] = { 0x7A, 0xFF }; *//* ??? */
> -  static unsigned char breakpoint[] = { 0x01, 0x80 };	/* Sleep */
> +  static unsigned char breakpoint[] = { 0x7A, 0xFF };	/* bpt (only
> simulator) */

this sounds like you're fixing one system (sim) at the cost of breaking others 
(everyone else).  this func is used by all h8300 targets ... not just sim.  so 
are you sure this is what you want ?

try looking at the Blackfin tdep to see how we handle sim-specific bp's.
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] h8300 target breakpoint doesn't work on Simulator

[Joel Brobecker]

> > -  /*static unsigned char breakpoint[] = { 0x7A, 0xFF }; *//* ??? */
> > -  static unsigned char breakpoint[] = { 0x01, 0x80 };	/* Sleep */
> > +  static unsigned char breakpoint[] = { 0x7A, 0xFF };	/* bpt (only
> > simulator) */
> 
> this sounds like you're fixing one system (sim) at the cost of
> breaking others (everyone else).  this func is used by all h8300
> targets ... not just sim.  so are you sure this is what you want ?
> 
> try looking at the Blackfin tdep to see how we handle sim-specific bp's.

I was going to say exactly the very same thing.

-- 
Joel

Re: [PATCH] h8300 target breakpoint doesn't work on Simulator

[Yoshinori Sato]

At Fri, 4 Mar 2011 16:10:11 -0500,
Mike Frysinger wrote:
> 
> On Friday, March 04, 2011 11:38:42 Yoshinori Sato wrote:
> > h8300-elf simulator handling O_BPT instruction of breakpoint.
> > But gdb write to O_SLEEP instruction of breakpoint.
> > 
> > So breakpoint command doesn't work on h8300 simulator.
> > 
> > --- h8300-tdep.c	25 Jan 2011 12:13:20 -0000	1.128
> > +++ h8300-tdep.c	4 Mar 2011 16:31:41 -0000
> > @@ -1197,8 +1197,7 @@
> >  h8300_breakpoint_from_pc (struct gdbarch *gdbarch, CORE_ADDR *pcptr,
> >  			  int *lenptr)
> >  {
> > -  /*static unsigned char breakpoint[] = { 0x7A, 0xFF }; *//* ??? */
> > -  static unsigned char breakpoint[] = { 0x01, 0x80 };	/* Sleep */
> > +  static unsigned char breakpoint[] = { 0x7A, 0xFF };	/* bpt (only
> > simulator) */
> 
> this sounds like you're fixing one system (sim) at the cost of breaking others 
> (everyone else).  this func is used by all h8300 targets ... not just sim.  so 
> are you sure this is what you want ?
> 
> try looking at the Blackfin tdep to see how we handle sim-specific bp's.
> -mike

Thanks comment.
I rewrite fix.

Index: ChangeLog
===================================================================
RCS file: /cvs/src/src/gdb/ChangeLog,v
retrieving revision 1.12792
diff -u -r1.12792 ChangeLog
--- ChangeLog	11 Mar 2011 02:33:27 -0000	1.12792
+++ ChangeLog	11 Mar 2011 04:19:13 -0000
@@ -1,3 +1,8 @@
+2011-03-11  Yoshinori Sato <ysato@users.sourceforge.jp>
+
+	* h8300-tdep.c (h8300_breakpoint_from_pc): Update to breakpoint
+	instruction
+
 2011-03-10  Maxim Grigoriev  <maxim2405@gmail.com>
 
 	* xtensa-tdep.c (windowing_enabled): Remove inline attribute.
Index: h8300-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/h8300-tdep.c,v
retrieving revision 1.128
diff -u -r1.128 h8300-tdep.c
--- h8300-tdep.c	25 Jan 2011 12:13:20 -0000	1.128
+++ h8300-tdep.c	11 Mar 2011 04:19:13 -0000
@@ -1197,11 +1197,16 @@
 h8300_breakpoint_from_pc (struct gdbarch *gdbarch, CORE_ADDR *pcptr,
 			  int *lenptr)
 {
-  /*static unsigned char breakpoint[] = { 0x7A, 0xFF }; *//* ??? */
-  static unsigned char breakpoint[] = { 0x01, 0x80 };	/* Sleep */
+  static const unsigned char sim_breakpoint[] = { 0x7A, 0xFF };	/* bpt */
+  static const unsigned char breakpoint[] = { 0x57, 0x30 };	/* trap #3 */
 
-  *lenptr = sizeof (breakpoint);
-  return breakpoint;
+  if (strcmp(target_shortname, "sim") == 0) {
+    *lenptr = sizeof (sim_breakpoint);
+    return sim_breakpoint;
+  } else {
+    *lenptr = sizeof (breakpoint);
+    return breakpoint;
+  }
 }
 
 static void

-- 
Yoshinori Sato
<ysato@users.sourceforge.jp>

Re: [PATCH] h8300 target breakpoint doesn't work on Simulator

[Joel Brobecker]

> +2011-03-11  Yoshinori Sato <ysato@users.sourceforge.jp>
> +
> +	* h8300-tdep.c (h8300_breakpoint_from_pc): Update to breakpoint
> +	instruction
> +

A few minor comments:

> -  /*static unsigned char breakpoint[] = { 0x7A, 0xFF }; *//* ??? */
> -  static unsigned char breakpoint[] = { 0x01, 0x80 };	/* Sleep */
> +  static const unsigned char sim_breakpoint[] = { 0x7A, 0xFF };	/* bpt */
> +  static const unsigned char breakpoint[] = { 0x57, 0x30 };	/* trap #3 */

Can you remove a few spaces before the "/* bpt */" comment? It seems
to me that the spaces are not really necessary, and removing them
would make it a little closer to our soft-limit (70 characters).

> -  *lenptr = sizeof (breakpoint);
> -  return breakpoint;
> +  if (strcmp(target_shortname, "sim") == 0) {
> +    *lenptr = sizeof (sim_breakpoint);
> +    return sim_breakpoint;
> +  } else {
> +    *lenptr = sizeof (breakpoint);
> +    return breakpoint;
> +  }
>  }

The proper style for braces in GDB is to put the curly brace on
the next line. Also, we require a space before opening parens
in function calls. Thus

  if (strcmp (target_shortname, "sim") == 0)
    {
      *lenptr = sizeof (sim_breakpoint);
      return sim_breakpoint;
    }
  else
    {
      *lenptr = sizeof (breakpoint);
      return breakpoint;
    }

I wonder if there isn't a better way to detect the sim, other
than checking the target name. I don't know of any, but perhaps
other maintainers might...

-- 
Joel

Re: [PATCH] h8300 target breakpoint doesn't work on Simulator

[Yoshinori Sato]

At Fri, 11 Mar 2011 10:39:04 +0400,
Joel Brobecker wrote:
> 
> > +2011-03-11  Yoshinori Sato <ysato@users.sourceforge.jp>
> > +
> > +	* h8300-tdep.c (h8300_breakpoint_from_pc): Update to breakpoint
> > +	instruction
> > +
> 
> A few minor comments:

Thanks.
 
> > -  /*static unsigned char breakpoint[] = { 0x7A, 0xFF }; *//* ??? */
> > -  static unsigned char breakpoint[] = { 0x01, 0x80 };	/* Sleep */
> > +  static const unsigned char sim_breakpoint[] = { 0x7A, 0xFF };	/* bpt */
> > +  static const unsigned char breakpoint[] = { 0x57, 0x30 };	/* trap #3 */
> 
> Can you remove a few spaces before the "/* bpt */" comment? It seems
> to me that the spaces are not really necessary, and removing them
> would make it a little closer to our soft-limit (70 characters).
> 
> > -  *lenptr = sizeof (breakpoint);
> > -  return breakpoint;
> > +  if (strcmp(target_shortname, "sim") == 0) {
> > +    *lenptr = sizeof (sim_breakpoint);
> > +    return sim_breakpoint;
> > +  } else {
> > +    *lenptr = sizeof (breakpoint);
> > +    return breakpoint;
> > +  }
> >  }
> 
> The proper style for braces in GDB is to put the curly brace on
> the next line. Also, we require a space before opening parens
> in function calls. Thus
> 
>   if (strcmp (target_shortname, "sim") == 0)
>     {
>       *lenptr = sizeof (sim_breakpoint);
>       return sim_breakpoint;
>     }
>   else
>     {
>       *lenptr = sizeof (breakpoint);
>       return breakpoint;
>     }

Sorry. I mistake editor setting.
Cleanup.

Index: ChangeLog
===================================================================
RCS file: /cvs/src/src/gdb/ChangeLog,v
retrieving revision 1.12820
diff -u -r1.12820 ChangeLog
--- ChangeLog	16 Mar 2011 09:49:41 -0000	1.12820
+++ ChangeLog	16 Mar 2011 13:47:25 -0000
@@ -1,3 +1,8 @@
+2011-03-16  Yoshinori Sato <ysato@users.sourceforge.jp>
+
+	* h8300-tdep.c (h8300_breakpoint_from_pc): Update to breakpoint
+	instruction
+
 2011-03-16  Phil Muldoon  <pmuldoon@redhat.com>
 
 	* NEWS: Add Parameter sub-classing description.
Index: h8300-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/h8300-tdep.c,v
retrieving revision 1.128
diff -u -r1.128 h8300-tdep.c
--- h8300-tdep.c	25 Jan 2011 12:13:20 -0000	1.128
+++ h8300-tdep.c	16 Mar 2011 13:47:25 -0000
@@ -1197,11 +1197,19 @@
 h8300_breakpoint_from_pc (struct gdbarch *gdbarch, CORE_ADDR *pcptr,
 			  int *lenptr)
 {
-  /*static unsigned char breakpoint[] = { 0x7A, 0xFF }; *//* ??? */
-  static unsigned char breakpoint[] = { 0x01, 0x80 };	/* Sleep */
+  static const unsigned char sim_breakpoint[] = { 0x7A, 0xFF }; /*bpt*/
+  static const unsigned char breakpoint[] = { 0x57, 0x30 }; /*trap #3*/
 
-  *lenptr = sizeof (breakpoint);
-  return breakpoint;
+  if (strcmp(target_shortname, "sim") == 0)
+    {
+      *lenptr = sizeof (sim_breakpoint);
+      return sim_breakpoint;
+    } 
+  else
+    {
+      *lenptr = sizeof (breakpoint);
+      return breakpoint;
+    }
 }
 
 static void

> I wonder if there isn't a better way to detect the sim, other
> than checking the target name. I don't know of any, but perhaps
> other maintainers might...

Hmm...
I think so.
However, I think it is rare case. I do not think that there is so an 
enhanced necessity.  

> -- 
> Joel

-- 
Yoshinori Sato
<ysato@users.sourceforge.jp>

===File ~/h8300-tdep.diff===================================
============================================================

Re: [PATCH] h8300 target breakpoint doesn't work on Simulator

[Joel Brobecker]

> +2011-03-16  Yoshinori Sato <ysato@users.sourceforge.jp>
> +
> +	* h8300-tdep.c (h8300_breakpoint_from_pc): Update to breakpoint
> +	instruction

This is OK.

Thanks,
-- 
Joel