Merging OC gdb with official gdb

Marko Mlinar markom@opencores.org
Thu Sep 19 02:00:00 GMT 2002


Andrew,

thanks for taking the time to look at ours code.

> 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.
I think some macros are (currently) needed (see below).
Should I split our header file in two?
where should I place these *.h files? Do I need to set something in 
Makefiles/or32.tm file?

> If you find that you do appear to need to define certain macro's then
> post a question to check what is going on.
Ok, how should we handle following macros:
(I looked at the current code, but they look like they are not yet in gdbarch)

#define GDB_MULTI_ARCH 1
#define CANNOT_STEP_BREAKPOINT
#define target_insert_hw_breakpoint(addr, cache) or1k_insert_breakpoint (addr, 
cache)
#define target_remove_hw_breakpoint(addr, cache) or1k_remove_breakpoint (addr, 
cache)
#define TARGET_HAS_HARDWARE_WATCHPOINTS
#define target_insert_watchpoint(addr, len, type) \
        or1k_insert_watchpoint (addr, len, type)
#define target_remove_watchpoint(addr, len, type) \
        or1k_remove_watchpoint (addr, len, type)
#define HAVE_NONSTEPPABLE_WATCHPOINT
#define STOPPED_BY_WATCHPOINT(w) or1k_stopped_by_watchpoint ()
#define TARGET_CAN_USE_HARDWARE_WATCHPOINT(bp_type, cnt, ot) \
  or1k_can_use_hardware_watchpoint(bp_type, cnt)
#define STEP_SKIPS_DELAY_P (1)
#define STEP_SKIPS_DELAY(pc) (or1k_step_skips_delay (pc))

> 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.
ok.

> > #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);
> 	}
Is this a must? They are just macros for internal use ;)

> > 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.
ok.

> > /* 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.
done (huh!).

> 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).
It compiles now.

> >   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.
or1k architecture has special address space of registers called Special 
Purpose Registers. These include cache, tick timer, supervision registers, 
debug registers, etc.
They have to be separatedfrom General Purpose Registers, also GPRs.
Due to large number of SPRs (several thousands), I did not include them into 
gdb register space (except program counter PC).

> >   if (!frame_register_read (selected_frame, regnum, raw_buffer))
>
> Yes! Many targets incorrectly display the hardware registers instead of
> the current frame's registers.
no wonder I did it incorrectly :)

> Suggest passing the relevant frame in as a parameter though.
> selected_frame will one day go away
How do I get relevant frame? Is there a target already doing what you are 
proposing?

> >     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.
What are be/le size specific versions?
Are you reffering to below printf_filtered?

> >   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);


> This should be ``info or1k spr''.  See ppc for an example of how to do
> this.
the command is already long, e.g.: info spr debug dmr1
since it is used a lot and it is registered only with or1k target, can we make 
an exception;) ?

> >   add_com ("spr", class_support, spr_command, "Set specified SPR
> > register.");
>
> This command shouldn't be needed.
> 	set $<sprreg> = <VAL>
>
> should work.
As I said above: the number of registers is too large and there is also some 
help involved with info spr/spr commands.

> >   /* 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.
We have proprietary hardware watches, which are resource limited and have some 
proprietary capabilities. Not all options can be set using gdb hardware 
watchpoints.

> >   /* 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.
ok. I will do this.

> >   /* 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.
ok, I've changed it to or1ksim.

I am reposting the files. Changes to config.gdb stays the same.

Marko
-------------- next part --------------
A non-text attachment was scrubbed...
Name: jtag.c
Type: text/x-csrc
Size: 28906 bytes
Desc: not available
URL: <http://sourceware.org/pipermail/gdb-patches/attachments/20020919/714f56fb/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: remote-or1k.c
Type: text/x-csrc
Size: 52469 bytes
Desc: not available
URL: <http://sourceware.org/pipermail/gdb-patches/attachments/20020919/714f56fb/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: or1k-tdep.c
Type: text/x-csrc
Size: 48418 bytes
Desc: not available
URL: <http://sourceware.org/pipermail/gdb-patches/attachments/20020919/714f56fb/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: or1k-trace.c
Type: text/x-csrc
Size: 30913 bytes
Desc: not available
URL: <http://sourceware.org/pipermail/gdb-patches/attachments/20020919/714f56fb/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: tm-or1k.h
Type: text/x-chdr
Size: 17820 bytes
Desc: not available
URL: <http://sourceware.org/pipermail/gdb-patches/attachments/20020919/714f56fb/attachment-0004.bin>


More information about the Gdb-patches mailing list