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]

Re: RFA: Add Epiphany simulator port


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]