Re: Merging OC gdb with official gdb

[Andrew Cagney <ac131313@ges.redhat.com>]

> Originaly it was written for gdb-5.0, but recently we wanted to join it with 
> official release, so I coded the macros in gdbarch.

Getting there.

> gdb/config/tm-or1k.h (or1k architecture macros)
> gdb/config/tm-or32.h (or32 implementation target macros)

Per, my other e-mail this is a new architecture that doesn't appear to 
require shared library support and hence shouldn't need the tm.h files. 
  If something needs to be shared between orxxx-tdep.c and other files 
then it should be declared in orxxx-tdep.h.

If you find that you do appear to need to define certain macro's then 
post a question to check what is going on.

Some other notes on or1k-tdep.c:

> #include "symcat.h"

GDB assumes that an ISO C compiler is being used so "symcat.h" shouldn't 
be needed.

> #define OR1K_IS_GPR(N) ((N) >= 0 && (N) < MAX_GPR_REGS)
> #define OR1K_IS_VF(N) ((N) >= MAX_GPR_REGS && (N) < MAX_GPR_REGS + MAX_VF_REGS)

Macro's like this can simply be written as functions.  GDB's convention 
turns out to be:

	static int
	or1k_gpr_p (int regnum)
	{
	   return (regnum >= 0 && regnum < MAX_GPR_REGS);
	}


> char *or1k_reg_names[] = {
> 
>   /* group 0 - general*/
>   "VR", "UPR", "CPUCFGR", "DMMUCFGR", "IMMUCFGR", "DCCFGR", "ICCFGR", "DCFGR",
>   "PCCFGR", "SPR0_9", "SPR0_10", "SPR0_11", "SPR0_12", "SPR0_13", "SPR0_14", "SPR0_15",
>   "NPC", "SR", "PPC", "SPR0_19", "SPR0_20", "SPR0_21", "SPR0_22", "SPR0_23",
>   "SPR0_24", "SPR0_25", "SPR0_26", "SPR0_27", "SPR0_28", "SPR0_29", "SPR0_30", "SPR0_31", 
>   "EPCR0", "EPCR1", "EPCR2", "EPCR3", "EPCR4", "EPCR5", "EPCR6", "EPCR7",
>   "EPCR8", "EPCR9", "EPCR10", "EPCR11", "EPCR12", "EPCR13", "EPCR14", "EPCR15",
>   "EEAR0","EEAR1", "EEAR2", "EEAR3", "EEAR4", "EEAR5", "EEAR6", "EEAR7",

Please change all the register names to lower case so that they are 
consistent with all other GDB targets.  Should the array be static.

> /* Builds and returns register name.  */
> 
> static char tmp_name[16];
> static char *
> or1k_spr_register_name (i)
>      int i;
> {

GDB assumes ISO C so all K&R functions should be converted to ISO C.

This particular function now returns ``const char *''.  Can you please 
check that GDB configured with:

	--target=<your-target> --enable-gdb-build-warnings=,-Werror

compiles (in particular your file).

>   int group = i >> SPR_GROUP_SIZE_BITS;
>   int index = i & (SPR_GROUP_SIZE - 1);
>   switch (group)
>     {
>       /* Names covered in or1k_reg_names.  */
>     case 0:
> 
>       /* Generate upper names.  */
>       if (index >= SPR_GPR_START)
> 	{
> 	  if (index < SPR_VFPR_START)
> 	    sprintf (tmp_name, "GPR%i", index - SPR_GPR_START);
> 	  else
> 	    sprintf (tmp_name, "VFR%i", index - SPR_VFPR_START);
> 	  return (char *)&tmp_name;

This code makes wrong assumptions about how the function will be used.

> static int
> do_vf_register (regnum)
>      int regnum;
> {
>   /* do values for FP (float) regs */
>   char *raw_buffer;
> 
>    /* doubles extracted from raw hex data */
>   double doub, flt;
>   int inv1, inv3, byte;
> 
>   raw_buffer = (char *) alloca (OR1K_VF_REGSIZE);
> 
>   /* Get the data in raw format.  */
> 
>   if (!frame_register_read (selected_frame, regnum, raw_buffer))

Yes! Many targets incorrectly display the hardware registers instead of 
the current frame's registers.

Suggest passing the relevant frame in as a parameter though. 
selected_frame will one day go away

>     error ("can't read register %d (%s)", regnum, REGISTER_NAME (regnum));
>   
>   flt = unpack_double (builtin_type_float, raw_buffer, &inv1);
>   doub = unpack_double (builtin_type_double, raw_buffer, &inv3);

Here use the ieee be/le size specific versions.  The code can't rely on 
float/double being something sensible.

>   if (inv1)
>     printf_filtered (" %-5s flt: <invalid float>", REGISTER_NAME (regnum));
>   else
>     printf_filtered (" %-5s flt:%-17.9g", REGISTER_NAME (regnum), flt);
>   printf_filtered (inv3 ? " dbl: <invalid double> " :
> 		   " dbl: %-24.17g ", doub);
> 


> void
> _initialize_or1k_tdep ()
> {
>   build_automata ();
>   register_gdbarch_init (bfd_arch_or32, or1k_gdbarch_init);
> 
>   if (TARGET_BYTE_ORDER == BIG_ENDIAN)
>     tm_print_insn = print_insn_big_or32;
>   else
>     tm_print_insn = print_insn_little_or32;
>     
>   /* Commands to show and set sprs.  */
>   add_info ("spr", info_spr_command, "Show information about the spr registers.");

This should be ``info or1k spr''.  See ppc for an example of how to do this.

>   add_com ("spr", class_support, spr_command, "Set specified SPR register.");

This command shouldn't be needed.

	set $<sprreg> = <VAL>

should work.

>   /* hwatch command.  */
>   add_com ("hwatch", class_breakpoint, hwatch_command, "Set hardware watch"
> 	   "point.\nExample: ($LEA == my_var)&&($LDATA < 50)||($SEA == my_"
> 	   "var)&&($SDATA >= 50).\nSee OR1k Architecture document for more"
> 	   " info.");

I don't think this command is needed.  GDB already has hardware 
watchpoint commands.

>   /* htrace commands.  */
>   add_prefix_cmd ("htrace", class_breakpoint, htrace_command,
> 		  "Group of commands for handling hardware assisted trace\n\n"
> 		  "See OR1k Architecture and gdb for or1k documents for more info.",
> 		  &htrace_cmdlist, "htrace ", 0, &cmdlist);
>   add_cmd ("info", class_breakpoint, htrace_info_command, "Display information about HW trace.",
> 	   &htrace_cmdlist);
>   add_alias_cmd ("i", "info", class_breakpoint, 1, &htrace_cmdlist);
>   add_cmd ("trigger", class_breakpoint, htrace_trigger_command, "Set starting criteria for trace.",
> 	   &htrace_cmdlist);
>   add_alias_cmd ("t", "trigger", class_breakpoint, 1, &htrace_cmdlist);
>   add_cmd ("qualifier", class_breakpoint, htrace_qualifier_command, "Set acquisition qualifier for HW trace.",
> 	   &htrace_cmdlist);
>   add_alias_cmd ("q", "qualifier", class_breakpoint, 1, &htrace_cmdlist);
>   add_cmd ("stop", class_breakpoint, htrace_stop_command, "Set HW trace stopping criteria.",
> 	   &htrace_cmdlist);
>   add_alias_cmd ("s", "stop", class_breakpoint, 1, &htrace_cmdlist);
>   add_cmd ("record", class_breakpoint, htrace_record_command, "Sets data to be recorded when expression occurs.",
> 	   &htrace_cmdlist);
>   add_alias_cmd ("r", "record", class_breakpoint, 1, &htrace_cmdlist);
>   add_cmd ("clear records", class_breakpoint, htrace_clear_records_command,
> 	   "Disposes all matchpoints used by records.", &htrace_cmdlist);
>   add_cmd ("enable", class_breakpoint, htrace_enable_command, "Enables the HW trace.", &htrace_cmdlist);
>   add_alias_cmd ("e", "enable", class_breakpoint, 1, &htrace_cmdlist);
>   add_cmd ("disable", class_breakpoint, htrace_disable_command, "Disables the HW trace.", &htrace_cmdlist);
>   add_alias_cmd ("d", "disable", class_breakpoint, 1, &htrace_cmdlist);
>   add_cmd ("rewind", class_breakpoint, htrace_rewind_command, "Clears currently recorded trace data.\n"
> 	   "If filename is specified, new trace file is made and any newly collected data\n"
> 	   "will be written there.", &htrace_cmdlist);
>   add_cmd ("print", class_breakpoint, htrace_print_command,
> 	   "Prints trace buffer, using current record configuration.\n"
> 	   "htrace print [<start> [<len>]]\n"
> 	   "htrace print"
> 	   , &htrace_cmdlist);
>   add_alias_cmd ("p", "print", class_breakpoint, 1, &htrace_cmdlist);
>   add_prefix_cmd ("mode", class_breakpoint, htrace_mode_command,
> 	   "Configures the HW trace.\n"
> 	   "htrace mode [continuous|suspend]"
> 	   , &htrace_mode_cmdlist, "htrace mode ", 0, &htrace_cmdlist);
>   add_alias_cmd ("m", "mode", class_breakpoint, 1, &htrace_cmdlist);
>   add_cmd ("continuous", class_breakpoint, htrace_mode_contin_command,
> 	   "Set continuous trace mode.\n", &htrace_mode_cmdlist);
>   add_cmd ("suspend", class_breakpoint, htrace_mode_suspend_command,
> 	   "Set suspend trace mode.\n", &htrace_mode_cmdlist);

Can I suggest, for the moment, moving this funcitonality out of 
or1k-tdep.c (to or1k-trace.c?).  This is a very significant chunk of 
work adding many new commands and hence is best separated and considered 
separatly.

>   /* Extra functions supported by simulator.  */
>   add_com ("sim", class_obscure, sim_command,
> 	   "Send a extended command to the simulator.");

There is already a sim command.  See remote-sim.c.

Andrew