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

[Joel Brobecker <brobecker at adacore dot com>]

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