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] |
Andrew, thanks for taking the time to look at ours code.I think some macros are (currently) needed (see below).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.
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)As part of the change:
#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))
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''?
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 :-( )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 ;)
(A useful and generic feature would be a command to select the case of register names. Some people, apparently, like using the shift key)> 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.
That's ok. A 1000 registers is nothing! I know of an arch with 4k (or was it 8k!).> /* 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=,-Werrorcompiles (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;or1k architecture has special address space of registers called Special Purpose Registers. These include cache, tick timer, supervision registers, debug registers, etc.This code makes wrong assumptions about how the function will be used.
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).
The more up-to-date multi-arch method print_registers_info() takes the architecture and frame as parameters.> 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 :)How do I get relevant frame? Is there a target already doing what you are proposing?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.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
(Er, but thinking about it).> 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);
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).the command is already long, e.g.: info spr debug dmr1This should be ``info or1k spr''. See ppc for an example of how to do this.
since it is used a lot and it is registered only with or1k target, can we make an exception;) ?
If the above doesn't work, we get to fix it!> add_com ("spr", class_support, spr_command, "Set specified SPR > register.");As I said above: the number of registers is too large and there is also some help involved with info spr/spr commands.This command shouldn't be needed. set $<sprreg> = <VAL> should work.
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.> /* 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.");We have proprietary hardware watches, which are resource limited and have some proprietary capabilities. Not all options can be set using gdb hardware watchpoints.I don't think this command is needed. GDB already has hardware watchpoint commands.
Thanks! (Thinking about it, or1k-htrace.c might be better? It will otherwize be confused with tracepoints.)> /* 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.
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] |