Re: RFA - PATCH - Support H8/300H and H8S Normal Mode for GDB

[Corinna Vinschen <vinschen@redhat.com>]

On Mon, Sep 15, 2003 at 02:25:20PM +0530, Shrinivas Atre wrote:
> Hi,
> 
> The attached patch enables debugging and simulation of H8/300H and H8S 
> Normal Mode binaries in GDB.
> 
> Is that OK for mainline and 6.0 branch ?

Do you have an FSF copyright assignment?  AFAIK you need that.

> --- src/gdb/config/h8300/tm-h8300.h.orig	Mon Sep 15 10:46:23 2003
> +++ src/gdb/config/h8300/tm-h8300.h	Mon Sep 15 13:32:23 2003
> @@ -26,6 +26,7 @@
>     GDB_TARGET_IS_H8300 in remote-e7000.c */
>  extern int h8300hmode;
>  extern int h8300smode;
> +extern int NormalMode; /* 1 - Normal Mode , 0 - Advanced mode */

Please don't use UpperCase but only lower case and underscore for
variable names.  This variable should be named normal_mode.
In this very case, the variable is extern so the name should be much
less ambiguous.  Probably "h8300_normal_mode" would fit.

> -    case bfd_mach_h8300h:
>      case bfd_mach_h8300hn:
> +      NormalMode = 1;
> +      h8300sxmode = 0;
> +      h8300smode = 0;
> +      h8300hmode = 1;
> +      set_gdbarch_num_regs (gdbarch, 13);
> +      set_gdbarch_num_pseudo_regs (gdbarch, 1);
> +      set_gdbarch_ecoff_reg_to_regnum (gdbarch, h8300_dbg_reg_to_regnum);
> +      set_gdbarch_dwarf_reg_to_regnum (gdbarch, h8300_dbg_reg_to_regnum);
> +      set_gdbarch_dwarf2_reg_to_regnum (gdbarch, h8300_dbg_reg_to_regnum);
> +      set_gdbarch_stab_reg_to_regnum (gdbarch, h8300_dbg_reg_to_regnum);
> +      set_gdbarch_register_name (gdbarch, h8300_register_name);
> +      set_gdbarch_ptr_bit (gdbarch, 2 * TARGET_CHAR_BIT);
> +      set_gdbarch_addr_bit (gdbarch, 2 * TARGET_CHAR_BIT);
> +      set_gdbarch_extract_return_value (gdbarch, h8300h_extract_return_value);
> +      set_gdbarch_store_return_value (gdbarch, h8300h_store_return_value);
> +      set_gdbarch_print_insn (gdbarch, print_insn_h8300h);
> +      break;
> +    case bfd_mach_h8300h:
> +      NormalMode = 0;
>        h8300sxmode = 0;
>        h8300smode = 0;
>        h8300hmode = 1;

Is it necessary to duplicate all set_gdbarch_XXX calls?  Wouldn't
it be more clear to set "h8300_normal_mode" separately to show that
it's the same machine, just another mode?  E. g.

	case bfd_mach_h8300h:
	case bfd_mach_h8300hn:
	  if (info.bfd_arch_info->mach)
	    h8300_normal_mode = 1;

> @@ -510,6 +510,7 @@ enum { POLL_QUIT_INTERVAL = 0x80000 };
>  
>  int h8300hmode  = 0;
>  int h8300smode  = 0;
> +int NormalMode  = 0;

As above, "h8300_normal_mode" would be better.

Otherwise it looks ok but I guess we need a word from the h8300 sim
expert.  Michael?

Corinna

-- 
Corinna Vinschen
Cygwin Developer
Red Hat, Inc.