Re: Merging OC gdb with official gdb

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

> 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