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

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

[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 <[hidden email]>
+
+ * h8300-tdep.c (h8300_breakpoint_from_pc): Update to breakpoint
+ instruction
+
 2011-03-03  Tom Tromey  <[hidden email]>
 
  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
<[hidden email]>
Reply | Threaded
Open this post in threaded view
|

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

Mike Frysinger
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

signature.asc (853 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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
Reply | Threaded
Open this post in threaded view
|

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

Yoshinori Sato
In reply to this post by Mike Frysinger
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 <[hidden email]>
+
+ * h8300-tdep.c (h8300_breakpoint_from_pc): Update to breakpoint
+ instruction
+
 2011-03-10  Maxim Grigoriev  <[hidden email]>
 
  * 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
<[hidden email]>
Reply | Threaded
Open this post in threaded view
|

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

Joel Brobecker
> +2011-03-11  Yoshinori Sato <[hidden email]>
> +
> + * 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
Reply | Threaded
Open this post in threaded view
|

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 <[hidden email]>
> > +
> > + * 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 <[hidden email]>
+
+ * h8300-tdep.c (h8300_breakpoint_from_pc): Update to breakpoint
+ instruction
+
 2011-03-16  Phil Muldoon  <[hidden email]>
 
  * 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
<[hidden email]>

===File ~/h8300-tdep.diff===================================
============================================================
Reply | Threaded
Open this post in threaded view
|

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

Joel Brobecker
> +2011-03-16  Yoshinori Sato <[hidden email]>
> +
> + * h8300-tdep.c (h8300_breakpoint_from_pc): Update to breakpoint
> + instruction

This is OK.

Thanks,
--
Joel