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

[Yoshinori Sato <ysato@users.sourceforge.jp>]

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===================================
============================================================