This is the mail archive of the gdb-patches@sources.redhat.com mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: Merging OC gdb with official gdb


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
This will need to be multi-arched (or deleted).

#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)
As part of the change:

2002-08-01 Grace Sainsbury <graces@redhat.com>

* target.h: Add to_insert_hw_breakpoint, to_remove_hw_breakpoint,
to_insert_watchpoint, to_remove_watchpoint,
to_stopped_by_watchpoint, to_stopped_data_address,
to_region_size_ok_for_hw_watchpoint, to_can_use_hw_breakpoint to
target vecctor. Define their corresponding macros so they call
them.

* target.c: Add default and debug versions of for
to_insert_hw_breakpoint, to_remove_hw_breakpoint,
to_insert_watchpoint, to_remove_watchpoint,
to_stopped_by_watchpoint, to_stopped_data_address,
to_region_size_ok_for_hw_watchpoint, to_can_use_hw_breakpoint.

Much of the above was moved into the target vector. The file remote-or1k.c would need to add entries for them in its target vector.

#define STEP_SKIPS_DELAY_P (1)
#define STEP_SKIPS_DELAY(pc) (or1k_step_skips_delay (pc))
Sane as for CANNOT_STEP_BREAKPOINT, this will need to be multi-arched.

The code that uses it mind:

      if (STEP_SKIPS_DELAY_P
          && breakpoint_here_p (read_pc () + 4)
          && STEP_SKIPS_DELAY (read_pc ()))
        oneproc = 1;

is pretty bogus -- what is ``4''?

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 ;)
The intent is to make it as easy as possible to debug for anyone that follows. You can't step into, breakpoint, or call a macro (and suprisingly it is to often the one line macro's that contain the bugs :-( )

> 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.
(A useful and generic feature would be a command to select the case of register names. Some people, apparently, like using the shift key)

> /* 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).
That's ok. A 1000 registers is nothing! I know of an arch with 4k (or was it 8k!).

The problem with the above is that it assumes that there will never be more than one outstanding register_name() call. That assumption is wrong.

register_name() should use (for want of a better word) permenant memory, instead of a scratch array, when returning a register's name.

Easiest might be to generate all the names once in _initialize_or1k_tdep(). Another would be to generate each on-demand.

>   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?
The more up-to-date multi-arch method print_registers_info() takes the architecture and frame as parameters.

I'll see about renaming DO_REGISTERS_INFO to DEPRECATED_DO_REGISTERS_INFO so that this is more obvious. The ARI picks it up but only after the event :-(

>     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?
These, from gdbtypes.h:

extern struct type *builtin_type_ieee_single_big;
extern struct type *builtin_type_ieee_single_little;
extern struct type *builtin_type_ieee_double_big;
extern struct type *builtin_type_ieee_double_little;
extern struct type *builtin_type_ieee_double_littlebyte_bigword;


Are you reffering to below printf_filtered?
No, I wasn't

>   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);
(Er, but thinking about it).

The ``-17.9g'' isn't going to be portable. DOUBLEST can either be ``double'' or ``long double''.

Does something like:
print_floating (raw_buffer, builtin_type_ieee_single_big, gdb_stdout)
or
print_floating (raw_buffer, REGISTER_VIRTUAL_TYPE(i), gdb_stdout)
do what you want?

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;) ?
A GDB may include support for more than one ISA. By including the <cpu> prefix any potential conflict between similar CPU commands is avoided (without introducing modal commands).

>   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.
If the above doesn't work, we get to fix it!

You might want to look at the head of cagney_sysregs-20020825-branch. It contains a new feature ``reggroups'' along with some other changes that are being developed to handle problems like this.

>   /* 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.
Can you summarize what the limitations were and post this, separatly, to gdb@. If there is some sort of limitation, people need to understand it and determine if fixing it, or living with it, is best.

>   /* 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.
Thanks! (Thinking about it, or1k-htrace.c might be better? It will otherwize be confused with tracepoints.)

>   /* 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.
Lets just get or1k-tdep.c in.  I'm currently ignoring the others.

How is your texinfo?

Andrew



Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]