From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 27876 invoked by alias); 20 Sep 2002 00:33:46 -0000 Mailing-List: contact gdb-patches-help@sources.redhat.com; run by ezmlm Precedence: bulk List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sources.redhat.com Received: (qmail 27864 invoked from network); 20 Sep 2002 00:33:44 -0000 Received: from unknown (HELO localhost.redhat.com) (216.138.202.10) by sources.redhat.com with SMTP; 20 Sep 2002 00:33:44 -0000 Received: from ges.redhat.com (localhost [127.0.0.1]) by localhost.redhat.com (Postfix) with ESMTP id 3BCD03DBB; Thu, 19 Sep 2002 20:33:42 -0400 (EDT) Message-ID: <3D8A6CE5.5020304@ges.redhat.com> Date: Thu, 19 Sep 2002 17:33:00 -0000 From: Andrew Cagney User-Agent: Mozilla/5.0 (X11; U; NetBSD macppc; en-US; rv:1.0.0) Gecko/20020824 X-Accept-Language: en-us, en MIME-Version: 1.0 To: Marko Mlinar Cc: gdb-patches@sources.redhat.com Subject: Re: Merging OC gdb with official gdb References: <200209041256.47379.markom@opencores.org> <200209171444.53129.markom@opencores.org> <3D88E7C3.6050801@ges.redhat.com> <200209191100.49477.markom@opencores.org> Content-Type: text/plain; charset=us-ascii; format=flowed Content-Transfer-Encoding: 7bit X-SW-Source: 2002-09/txt/msg00506.txt.bz2 > 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 * 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= --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: ", REGISTER_NAME >> > (regnum)); else >> > printf_filtered (" %-5s flt:%-17.9g", REGISTER_NAME (regnum), flt); >> > printf_filtered (inv3 ? " dbl: " : >> > " 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 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 $ = >> >> 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 [ []]\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