This is the mail archive of the gdb-patches@sourceware.org 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] |
On Friday 27 January 2012 20:13:19 Joern Rennecke wrote: > The full gdb port might take a while longer to be ready for submission, > so I would like to submit the simulator ahead, so that the toolchain can > be properly tested. the gdb/ changes would get merged with the gdb/ port i don't see the epiphany code merged into libgloss either yet. since the sim port relies on those headers existing (in order to regen header files), i think we need to wait for that to be merged before we can take the sim port. wrt copyright, do you have assignment papers in place ? i see the files all say "2011", so that'll prob need updating to include 2012. > The attached patches contain the missing pieces of the simulator, i.e. > a bit of configury in gdb/ and sim/, generated files and assorted glue > logic in sim/ , and the testsuite. the diff's look fine looking at the testsuite dir, the style could use a little tweaking -- you mix tabs/spaces in a bunch of places. i know the testsuite dir is a lot of hand coded asm, so i don't push people over it. testsuite/sim/epiphany/sim-build.sh looks weird. what's the point of basically inlining asm files ? i'd say punt that file. generally the ChangeLog tracks the start of the merge into the public tree, but keeping the previous history is OK i think. i can't comment too much about cgen as i've never used it. but let's see what else i can review ... i'm guessing i can ignore all the files that say "THIS FILE IS MACHINE GENERATED WITH CGEN". > --- sim/epiphany/Makefile.in > +++ sim/epiphany/Makefile.in > > CONFIG_DEVICES = dv-sockser.o > CONFIG_DEVICES = looks like dead code ? please delete. > SIM_EXTRA_DEPS = \ > $(CGEN_INCLUDE_DEPS) \ > arch.h cpuall.h epiphany-sim.h $(srcdir)/../../opcodes/epiphany-desc.h > > epiphany.o: epiphany.c $(EPIPHANYBF_INCLUDE_DEPS) > epiphany-fp.o : epiphany-fp.c $(EPIPHANYBF_INCLUDE_DEPS) > > arch.o: arch.c $(SIM_MAIN_DEPS) $(EPIPHANYBF_INCLUDE_DEPS) > > devices.o: devices.c $(SIM_MAIN_DEPS) > traps.o: traps.c $(SIM_MAIN_DEPS) $(EPIPHANYBF_INCLUDE_DEPS) > > cpu.o: cpu.c $(EPIPHANYBF_INCLUDE_DEPS) > decode.o: decode.c $(EPIPHANYBF_INCLUDE_DEPS) > sem.o: sem.c $(EPIPHANYBF_INCLUDE_DEPS) > model.o: model.c $(EPIPHANYBF_INCLUDE_DEPS) do you need all of these ? the latest cvs tree should auto-generate dependencies for you now ... > # FIXME: Use of `mono' is wip. > mloop.c eng.h: stamp-mloop > stamp-mloop: $(srcdir)/../common/genmloop.sh mloop.in Makefile > $(SHELL) $(srccom)/genmloop.sh -shell $(SHELL) \ > -mono -fast -pbb -switch sem-switch.c \ > -cpu epiphanybf -infile $(srcdir)/mloop.in > $(SHELL) $(srcroot)/move-if-change eng.hin eng.h > $(SHELL) $(srcroot)/move-if-change mloop.cin mloop.c > touch stamp-mloop there is no such mloop code in this archive that i can see, and i'm not sure we'd want something that'd require mono. best to just delete this for now. > --- a/sim/epiphany/sim-main.h > +++ b/sim/epiphany/sim-main.h > > /* Main header for the epiphany. */ needs copyright/license header > #define USING_SIM_BASE_H /* FIXME: quick hack */ dead code -> delete > struct _sim_cpu; /* FIXME: should be in sim-basics.h */ comment style ends with a period and two spaces after that > /*#include "cpu.h"*/ please delete dead code > epiphany_core_signal ((SD), (CPU), (CIA), (MAP), (NR_BYTES), (ADDR), \ > (TRANSFER), (ERROR)) indentation seems to be slightly off ;) > --- a/sim/epiphany/epiphany-fp.h > +++ b/sim/epiphany/epiphany-fp.h > > extern SI epiphany_iadd(SIM_CPU *current_cpu, SI , SI , SI ); > extern SI epiphany_imul(SIM_CPU *current_cpu, SI , SI , SI ); > extern SI epiphany_isub(SIM_CPU *current_cpu, SI , SI , SI ); > extern SI epiphany_imadd(SIM_CPU *current_cpu, SI , SI , SI ); > extern SI epiphany_imsub(SIM_CPU *current_cpu, SI , SI , SI ); style is: extern int foo (int arg, char *ptr); all the lines in this file seem to be incorrect, but please check all files > --- a/sim/epiphany/epiphany-sim.h > +++ b/sim/epiphany/epiphany-sim.h > > /* GDB register numbers. */ > /* TBS */ i don't know what "TBS" is ... best to just delete i guess > #ifndef GET_H_CR what's the point of these define checks ? the header has global protection against multiple inclusion, so these don't do anything useful ... > typedef struct please double check trailing spaces/tabs in these files. quick fix: sed -i 's:[ \t]*$::' *.[ch] > { > /* nop insn slot filler count. */ > unsigned int fillnop_count; > /* Number of parallel insns. */ > unsigned int parallel_count; > > /* FIXME: generalize this to handle all insn lengths, move to common. */ > /* Number of short insns, not including parallel ones. */ > unsigned int short_count; > /* Number of long insns. */ > unsigned int long_count; > > /* Working area for computing cycle counts. */ > unsigned long insn_cycles; /* FIXME: delete */ > unsigned long cti_stall; > unsigned long load_stall; > unsigned long biggest_cycles; > > /* Bitmask of registers loaded by previous insn. */ > unsigned int load_regs; > /* Bitmask of registers loaded by current insn. */ > unsigned int load_regs_pending; > } EPIPHANY_MISC_PROFILE; there is common/sim-profile.[ch]. can't you use that instead of open coding this yourself ? the Blackfin code uses this, so that might be a useful starting point. this would get you all the --profile-xxx options for free rather than having to add your own hook with print_epiphany_misc_cpu. > /* This is invoked by the execute section of mloop{,x}.in. */ > > /* This is invoked by the execute section of mloop{,x}.in. */ looks like you should delete the first comment here > > /* Additional execution support. */ > > punt the empty section ? > /* sim_core_attach device argument. */ > extern device epiphany_devices; you attach "struct hw", not "struct device" ... > /* FIXME: Temporary, until device support ready. */ > struct _device { int foo; }; you shouldn't need this at all > --- a/sim/epiphany/epiphany.c > +++ b/sim/epiphany/epiphany.c > > int > epiphany_decode_gdb_ctrl_regnum (int gdb_regnum) > { > switch (gdb_regnum) > { > default: > break; > } > abort (); > } err, if you don't define this, you can't actually read registers via gdb. you really should implement this and add an include/gdb/sim-epiphany.h in the process. > int > epiphanybf_fetch_register (SIM_CPU * current_cpu, int rn, unsigned char no space after that "*" > #ifdef DEBUG > fprintf(stderr, "epiphanybf_fetch_register %d \n" , rn); > #endif space before the "(", and none before the "," or "\n", and only one space before "rn" also, if these debug statements are generally useful, you might want to consider using the tracing framework instead. since i integrated the Blackfin port with the common sim tracing framework, i now get the --trace-xxx stuff for free. an example of running u-boot: $ bfin-elf-run --trace-{insn,decode,extract,memory,branch} -t \ --env operating /tftpboot/u-boot ... extract: 0x3f00010 -_interp_insn_bfin: iw0:0x6190 extract: 0x3f00010 - decode_COMPI2opD_0: op:0 src:50 dst:0 decode: 0x3f00010 - decode_COMPI2opD_0: imm7:0x32 insn: 0x3f00010 -R0 = 0x32 (X); ... insn: 0x3f020c4 #210 board_init_f -SP += -0x3c; extract: 0x3f020c6 #210 board_init_f -_interp_insn_bfin: iw0:0x6fe6 extract: 0x3f020c6 #210 board_init_f - decode_COMPI2opP_0: op:1 src:124 dst:6 decode: 0x3f020c6 #210 board_init_f - decode_COMPI2opP_0: imm:0xfffffffc insn: 0x3f020c6 #210 board_init_f -SP += -0x4; extract: 0x3f020c8 #216 board_init_f -_interp_insn_bfin: iw0:0xe301 iw1:0x45ae insn_len:4 extract: 0x3f020c8 #216 board_init_f -decode_CALLa_0: S:1 msw:0x1 lsw:0x45ae decode: 0x3f020c8 #216 board_init_f -decode_CALLa_0: pcrel24:0x28b5c insn: 0x3f020c8 #216 board_init_f -CALL 0x28b5c; branch: 0x3f020c8 #216 board_init_f -CALL to 0x3f2ac24 ... > fprintf (stderr, "epiphanybf_fetch_register REG VALHEX %x \n" , no spaces before "\n" > case TARGET_SYS_fstat: > cb->stat_map = stat_map; /* Fall through. */ you've got two extra spaces of indentation here ... > void > epiphanybf_model_insn_before (SIM_CPU * cpu ATTRIBUTE_UNUSED, > int first_p ATTRIBUTE_UNUSED) > { > > #ifdef DEBUG delete that empty newline after the { > USI > epiphany_post_isn_callback (SIM_CPU * current_cpu, USI pc) > { > > #ifdef DEBUG here too > pc = pc + 2; > > } > return pc + 2; should be a blank newline before the return, but not after that "pc = ..." > --- a/sim/epiphany/epiphany-fp.c > +++ b/sim/epiphany/epiphany-fp.c > > inline USI > extract_mant (USI x) > { > return (x & 0x7fffff); > } all of these little guys should probably be marked "static" > #define FADD_FP_OP 0 > #define FMUL_FP_OP 1 > #define FSUB_FP_OP 2 > #define FMADD_FP_OP 3 > #define FMSUB_FP_OP 4 > #define FIX_FP_OP 5 i think you want these to be an enum ? if not, then there should be just one space after the #define and not 4 > typedef long double float_calc_type; > typedef float float32_type; > > USI > float_as_int (float f) > { > union { float f; USI i; } u; > > u.f = f; > return u.i; > } > > float > int_as_float (USI i) > { > union { float f; USI i; } u; > > u.i = i; > return u.f; > } i'm not sure these are correct. how your host system (where the sim executes) could easily be using a different encoding format than your target cpu. you probably should be defining target types rather than using "double" or "float" and then manually doing the conversion yourself. if speed ends up being an issue, you could add a configure test that allows this cheating behavior when you detect that it will actually work out. > if (fRoundingMode != FE_TOWARDZERO && fRoundingMode != FE_TONEAREST) > { > a lot of these files have a bunch of arbitrary blank lines. please tighten it up globally. > fprintf (stderr, "Internal error: unknown RoundingMode\n"); > exit (19); that's bad behavior. use sim_engine_halt() to get a graceful exit. seems there are a bunch of places here that you'll need to update. > fprintf (stderr, "Internal error: unknown operation\n"); > exit (19); > }; no semi-colon after that brace > --- a/sim/epiphany/sim-if.c > +++ b/sim/epiphany/sim-if.c > > /* Records simulator descriptor so utilities like epiphany_dump_regs can be > called from gdb. */ > SIM_DESC current_state=0; > int is_sim_opened=0; mmm i don't see anyone reading these variables. just delete them ? > static SIM_RC > epiphany_extenal_memory_option_handler (SIM_DESC sd, sim_cpu *cpu, int opt, char *arg, int is_command) { > > if(strcmp(arg,"off") == 0 ) { > epiphany_add_ext_mem = 0; > } > if(strcmp(arg,"on") == 0 ) { > epiphany_add_ext_mem = 1; > } > return SIM_RC_OK; > } style in this func is completely off > #if 0 /* FIXME: pc is in mach-specific struct */ > /* FIXME: watchpoints code shouldn't need this */ > { > SIM_CPU *current_cpu = STATE_CPU (sd, 0); > STATE_WATCHPOINTS (sd)->pc = &(PC); > STATE_WATCHPOINTS (sd)->sizeof_pc = sizeof (PC); > } > #endif you should implement this ;) > #ifdef HAVE_DV_SOCKSER /* FIXME: was done differently before */ > if (dv_sockser_install (sd) != SIM_RC_OK) > { > free_state (sd); > return 0; > } > #endif this is handled via tconfig.in rather than in sim_open > #if 0 /* FIXME: 'twould be nice if we could do this */ > /* These options override any module options. > Obviously ambiguity should be avoided, however the caller may wish to > augment the meaning of an option. */ > if (extra_options != NULL) > sim_add_option_table (sd, extra_options); > #endif not sure what this is, so just delete it > #if 0 > /* Allocate a handler for the control registers and other devices > if no memory for that range has been allocated by the user. > All are allocated in one chunk to keep things from being > unnecessarily complicated. */ you actually want to do this on a per-device basis to keep things simpler. that way each device only registers the small range that it cares about and the sim core automatically takes care of calling the right device for you. > is_sim_opened = 1; /* To distinguish between HW and simulator target. */ > > SIM_CPU *current_cpu = STATE_CPU (sd, 0); > cgen_init_accurate_fpu (current_cpu, CGEN_CPU_FPU (current_cpu), > epiphany_fpu_error); variable decls are not allowed inline. they must appear at the beginning of scope blocks (i.e. start of funcs). > SIM_RC > sim_create_inferior (sd, abfd, argv, envp) > SIM_DESC sd; > struct bfd *abfd; > char **argv; > char **envp; > { > ... > #if 0 > STATE_ARGV (sd) = sim_copy_argv (argv); > STATE_ENVP (sd) = sim_copy_argv (envp); > #endif why's this disabled ? -mike
Attachment:
signature.asc
Description: This is a digitally signed message part.
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |