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: [PATCH 4/5] Simulator for nds32


On Monday 08 July 2013 05:29:50 Wei-cheng Wang wrote:

sorry for the delay.  i was on vacation for a month, and then this is a big 
patchset :).

> --- a/sim/configure.tgt
> +++ b/sim/configure.tgt
>
> +   nds32*)
> +       SIM_ARCH(nds32)
> +       sim_testsuite=yes

not a big deal, but setting yes here probably makes more sense in the follow 
up patch where you added the sim testsuite

> --- /dev/null
> +++ b/sim/nds32/Makefile.in
>
> +#    Makefile template for Configure for the AVR sim library.

not AVR anymore ...

> +#    Copyright (C) 2009, 2010, 2011 Free Software Foundation, Inc.

guess it should be 2009-2013 ?

> +SIM_EXTRA_CFLAGS = -DHAVE_ENVIRON

do you actually need this ?  i suspect you dont'.

> +SIM_EXTRA_LIBS = -lm

or this ?  probably just a copy & paste from AVR rather than you needing it 
...

> +SIM_OBJS = \
> +	$(SIM_NEW_COMMON_OBJS) \
> ...
> +	sim-load.o \

no need to list this -- it's already in $SIM_NEW_COMMON_OBJS

> +	sim-resume.o \
> +	sim-stop.o \
> +	sim-reason.o \
> +	sim-reg.o \

fix the sorting here

> +	$(SIM_EXTRA_OBJS)
> +## COMMON_POST_CONFIG_FRAG

stick a blank line above that ## line please

> +nds32-syscall.o: nds32-syscall.c targ-vals.h mingw32-hdep.h

do you need to manually list mingw32-hdep.h here ?  or any of the other 
entries for that matter ?  the latest build should generate dependency info 
automatically.

> --- /dev/null
> +++ b/sim/nds32/interp.c
>
> +#if defined (__linux__) || defined (__CYGWIN__)
> +/* FIXME */
> +#include <sys/types.h>

fix what ?  the configure script already checked for sys/types.h for you, so 
just use HAVE_SYS_TYPES_H instead of these OS checks.

this comes up a few times in this patch.  fix all of them please.

> +static void nds32_set_nia (sim_cpu *cpu, sim_cia nia);

should use "SIM_CPU *cpu" rather than "sim_cpu *cpu" in pretty much this 
entire patch

> +/* Recent $pc, for debugging.  */
> +#define RECENT_CIA_MASK	0xf
> +static sim_cia recent_cia[RECENT_CIA_MASK + 1];
> +static int recent_cia_idx = 0;

we don't really want global state.  this is what the sim_cpu state is for.

> +static ulongest_t
> +extract_unsigned_integer (unsigned char *addr, int len, int byte_order)

do you really want to be using ulongest_t here ?  you should pick a stable 
type that is the exact size you need.

add "const" to the addr.  might make more sense to have it be a void* so you 
don't have to cast it all the time.

i imagine the "len" should be "unsigned", and you should probably add an 
assert to make sure len does not exceed the size of the retval.

all of that said, there are a bunch of helper macros in sim-endian.h for doing 
byteswapping and stuff.  please review those and be sure that you actually need 
to implement this rather than using the already provided helpers.

> +{
> +  ulongest_t retval;
> +  const unsigned char *p;
> +  const unsigned char *startaddr = addr;
> +  const unsigned char *endaddr = startaddr + len;
> +
> +  retval = 0;
> +  if (byte_order == BIG_ENDIAN)
> +    {
> +      for (p = startaddr; p < endaddr; ++p)
> +	retval = (retval << 8) | *p;
> +    }
> +  else
> +    {
> +      for (p = endaddr - 1; p >= startaddr; --p)
> +	retval = (retval << 8) | *p;
> +    }
> +  return retval;

style says there should be a blank line above the return statement.  this 
comes up in a bunch of places, so please search + fix.

> +static void
> +store_unsigned_integer (unsigned char *addr, int len,
> +			int byte_order, ulongest_t val)

same comments here as the extract func

> +/* Find first zero byte or mis-match in sequential memory address.
> +   If no such byte is found, return 0.  */
> +
> +static uint32_t
> +find_null_mism (unsigned char *b1, unsigned char *b2)
> +{
> +  int i;

personally i'd use size_t here.

> +  for (i = 0; i < 4; i++)

the comment doesn't mention the 4 byte limit.  it probably should.

> +    {
> +      if ((b1[i] == '\0') || (b1[i] != b2[i]))
> +	return -4 + i;
> +    }

you don't really ned those braces if you wanted to drop them.

> +  return 0;
> +}

looks like you could code it a little more simply:
	size_t i;
	for (i = 0; i < 4; ++i)
		if ((b1[i] == '\0') || (b1[i] != b2[i]))
			break;
	return -4 + i;

in the case nothing is found, "i" will be "4" so it'll return 0 ...

> +/* Find first mis-match in sequential memory address.
> +   The 3rd argument inc: 1 means incremental memory address.
> +			-1 means decremental memory address.
> +   If no such byte is found, return 0.  */
> +
> +static uint32_t
> +find_mism (unsigned char *b1, unsigned char *b2, int inc)
> +{
> +  int i, end;
> +  i = (inc == 1) ? 0 : 3;
> +  end = (inc == 1) ? 3 : 0;
> +  while (1)
> +    {
> +      if ((b1[i] != b2[i]))
> +	return -4 + i;
> +      if (i == end)
> +	return 0;
> +      i += inc;
> +    }
> +}

there should be a newline after the decls.  this looks like a common style 
issue, so please globally search + fix.

i'd make "i", "end", and "inc" a ssize_t, and the while looks like it should 
be a for loop:

	ssize_t i = (inc == 1) ? 0 : 3;
	ssize_t end = (inc == 1) ? 3 : 0;

	for (; i != end; i += inc)
		if (b1[i] != b2[i])
			return -4 + i;

	return 0;

> +static void
> +nds32_dump_registers (SIM_DESC sd)
> +{
> +  static char *reg2names[] = {

mark it const

> +uint32_t
> +nds32_raise_exception (sim_cpu *cpu, enum nds32_exceptions e, int sig,
> +		       char *msg, ...)

make msg a const

> +{
> +  SIM_DESC sd = CPU_STATE (cpu);
> +  uint32_t cia = CCPU_USR[USR0_PC].u;
> +  int i;
> +
> +  /* TODO: Show message only if it is not handled by user.  */
> +  if (msg)
> +    {
> +      va_list va;
> +      va_start (va, msg);
> +      sim_io_evprintf (sd, msg, va);
> +      va_end (va);
> +    }
> +
> +  /* Dump registers before halt.  */
> +  if (STATE_OPEN_KIND (sd) != SIM_OPEN_DEBUG)
> +    {
> +      fprintf (stderr, "  ");
> +      print_insn_nds32 (cia, &dis_info);
> +      fprintf (stderr, "\n");
> +      nds32_dump_registers (sd);
> +    }

hmm, why doesn't the existing sim_io_xxx funcs work here ?  the 
nds32_dump_registers func is using them.  we don't want the sim writing 
directly to stdout/stderr in general.

> +void
> +nds32_bad_op (sim_cpu *cpu, uint32_t cia, uint32_t insn, char *tag)

mark tag as const

> +ulongest_t
> +__nds32_ld (sim_cpu *cpu, SIM_ADDR addr, int size, int aligned_p)

hmm, can you do a global search & fix to stop using ulongest_t ?

> +void
> +sim_size (int s)
> +{
> +}

unused -> delete

> +/* Set next-instructoin-address, so sim_engine_run () fetches `nia'
> +   instead of ($pc + 4) or ($pc + 2) for next instruction base on
> +   currenly instruction size. */

style says that there should be two spaces after the . and before the */.  
this comes up a bunch of times in this patch ... please to a global search & 
fix.

> +static void
> +nds32_decode32_lsmw (sim_cpu *cpu, const uint32_t insn, sim_cia cia)
> +{
> ...
> +  if (rb < GPR_FP && re < GPR_FP)
> +    {
> +      reg_cnt += (re - rb) + 1;
> +    }

pointless braces -> delete them

> +static void
> +nds32_decode32_jreg (sim_cpu *cpu, const uint32_t insn, sim_cia cia)
> +{
> ...
> +      return;

this func has a lot of empty return statements.  change the ones inside the 
switch to a break, and delete the useless return at the end of the func.

> +static void
> +nds32_decode32_br1 (sim_cpu *cpu, const uint32_t insn, sim_cia cia)

a lot of these internal helpers have no function level docs.  even a one line 
sentence explaining each one would be better than nothing.

> +static void
> +nds32_decode16 (sim_cpu *cpu, uint32_t insn, sim_cia cia)
> +{
> ...
> +      CCPU_GPR[GPR_TA].u = (CCPU_GPR[rt4].s < CCPU_GPR[ra5].s) ? 1 : 0;

you've got a bunch of statements like this.  i think the more common form is 
to use !! like:
	CCPU_GPR[GPR_TA].u = !!(CCPU_GPR[rt4].s < CCPU_GPR[ra5].s);

> +/* This function is mainly used for fetch general purpose registers.
> +   GDB remote-sim calls this too, so it will be used for fetch some
> +   USR (PC, D0, D1), FLOAT, SR (PSW).  */
> +
> +static int
> +nds32_fetch_register (sim_cpu *cpu, int rn, unsigned char *memory, int 
length)
> +{
> ...
> +  return 0;

in the invalid request case, shouldn't this return -1 ?

> +static int
> +nds32_store_register (sim_cpu *cpu, int rn, unsigned char *memory, int 
length)
> +{
> ...
> +  return 0;

same here ?

> +SIM_DESC
> +sim_open (SIM_OPEN_KIND kind, host_callback * callback,
> +	  struct bfd *abfd, char **argv)
> +{
> ...
> +#if 0
> +  /* COLE: Not sure whether this is necessary. */
> +
> +  /* Establish any remaining configuration options.  */
> +  if (sim_config (sd) != SIM_RC_OK)
> +    {
> +      nds32_free_state (sd);
> +      return 0;
> +    }
> +#endif

pretty sure it is.  so enable it ?

> +void
> +sim_close (SIM_DESC sd, int quitting)
> +{

shouldn't you also call sim_module_uninstall (sd); in here ?

> +void
> +sim_set_callbacks (host_callback * ptr)
> +{
> +  /* callback = ptr; */
> +}

dead code -> delete

> --- /dev/null
> +++ b/sim/nds32/mingw32-hdep.c
>
> +#define UNIMP(SYSCALL)	\
> +	void SYSCALL () { puts ("Unimplemented syscall " #SYSCALL); abort (); }

use sim_io_eprintf instead rather than just straight up aborting
	sim_io_eprintf (sd, "nds32-sim: "unimplemented syscall %s\n", #SYSCALL);

> --- /dev/null
> +++ b/sim/nds32/mingw32-hdep.h

i would really like to see a new syscall file added in the common/ tree for 
handling Linux syscalls.  if you look at bfin/interp.c, you'll find many funcs 
already implemented.

> --- /dev/null
> +++ b/sim/nds32/nds32-cop0.c
>
>  +/* Simulator for NDS32 processors.

be nice if these files had a better one line header

> +static int
> +nds32_decode32_fcmp (sim_fpu *sfa, sim_fpu *sfb)
> +{
> ...
> +  static int s2i[12] = {
> +  static char ctab[100] = {

const

> +sim_cia
> +nds32_decode32_cop (sim_cpu *cpu, const uint32_t insn, sim_cia cia)
> +{
> ...
> +      /* MAC instructions use value in fst.  */
> +      switch (__GF (insn, 6, 4))
> +	{
> +	case 4: case 5: case 8: case 9:

one case statement per line please

> +      int dp = (insn & 0x8) > 0;

better written:
	int dp = !!(insn & 0x8);

> --- /dev/null
> +++ b/sim/nds32/nds32-gmon.c

from a cursory search of the code base, it seems like your gmon logic is not 
tied to the target at all.  instead, you're letting the sim itself do the 
counting.

i'm pretty sure what you've open coded here is common/sim-profile.[ch].  if you 
implement the hooks it requires, then you'll get a lot more information for 
free.  check out examples here of it in action:
http://docs.blackfin.uclinux.org/doku.php?id=toolchain:sim#profiling

> --- /dev/null
> +++ b/sim/nds32/nds32-load.c
>
> +static void
> +nds32_alloc_memory (SIM_DESC sd, struct bfd *abfd)
> +{
> ...
> +  if (STATE_ENVIRONMENT (sd) != USER_ENVIRONMENT)
> +    {
> +      /* FIXME: We should only do this if user doesn't allocate one.
> +		But how can we know it? */
> +      sim_do_command (sd, "memory region 0,0x4000000"); /* 64 MB */
> +      return;
> +    }

see the comment below about mem_attached

> +  /* For emulating Linux VMA */
> +  sim_core_attach (sd, NULL, 0, access_read_write_exec, 0, 0x00004000,
> +		   0xFFFF8000, 0, &nds32_mm_devices, NULL);

i don't understand this at all.  what is all this Linux VMA business ?

> +static void
> +nds32_load_segments (SIM_DESC sd, bfd *abfd, uint32_t load_base)

looks like more stuff we should move to common/ and unify with existing 
targets.  there's nothing nds32-specific here that i can see.  you're just 
processing an ELF.

> +SIM_RC
> +sim_load (SIM_DESC sd, char *prog_name, struct bfd *prog_bfd, int from_tty)

it's extremely preferable you do not implement this.  instead, update your 
Makefile.in to add sim-hload.o to your SIM_OBJS.

> +void
> +nds32_init_libgloss (SIM_DESC sd, struct bfd *abfd, char **argv, char 
**env)
> +{
> +  int len, mlen, i;
> +
> +  STATE_CALLBACK (sd)->syscall_map = cb_nds32_libgloss_syscall_map;
> +
> +  /* Save argv for -mcrt-arg hacking.  */

what's going on here ?  the argc/argv are stored in "sd" and can be extracted 
by using STATE_PROG_ARGV().  at runtime, if the libgloss target wants access 
to argc/argv, it should use the CB_SYS_argc/etc... syscalls to get the 
information it needs.

i wrote up some documentation here:
https://sourceware.org/ml/gdb/2013-05/msg00150.html

> +void
> +nds32_init_linux (SIM_DESC sd, struct bfd *abfd, char **argv, char **env)
> +{

can we avoid yet another implementation here ?  bfin/interp.c:bfin_user_init() 
has a pretty complete argc/argv/env/auxval setup.  i can't see why we need to 
duplicate this in every target.  making a common/ implementation would be a 
lot better.

> --- /dev/null
> +++ b/sim/nds32/nds32-mm.c

it's more common for targets to implement the devices stuff in a file named 
devices.c.  please stick to that.

> +static struct nds32_vm_area *
> +nds32_alloc_vma ()

void funcs need to use "(void)", not "()".

> --- /dev/null
> +++ b/sim/nds32/nds32-mm.h
>
> +#define ALIGN(x, a)		((x) & ~(a-1))
> +#define ROUNDUP(x, a)		(ALIGN ((x) + ((a) - 1), a))

let's add these to common/sim-basics.h instead

> --- /dev/null
> +++ b/sim/nds32/nds32-pfm.c
>
> +/* I put performance monitor definitions here.  */

useless comment -> delete

> --- /dev/null
> +++ b/sim/nds32/nds32-sim.h
>
> +#define SRIDX(M,m,e)  ((M << 7) | (m << 3) | e)
> +#define UXIDX(g,u)    ((g << 5) | u)

need spaces after those commas, and the RHS should have paren around the args.  
i.e.:
	#define UXIDX(g, u)	(((g) << 5) | (u))

> +uint32_t nds32_raise_exception (sim_cpu *cpu, enum nds32_exceptions
> e, int sig, char *msg, ...);

i guess you've got line wrap turned on in your e-mail client.  you should fix 
that, and then fix this so the line doesn't exceed 80 cols.

you should also mark "msg" as const, and use __attribute__((format (printf, 5, 
6))).

> +#if 1
> +#define SIM_IO_DPRINTF(sd, fmt, args...)   sim_io_printf (sd, fmt, ## args)
> +#else
> +#define SIM_IO_DPRINTF(...)	do { } while (0)
> +#endif

unused code -> delete

> --- /dev/null
> +++ b/sim/nds32/rbtree.c
>
> --- /dev/null
> +++ b/sim/nds32/rbtree.h

these don't look like they're specific to NDS32 at all.  or even the simulators 
project.  could you look at adding these to the libiberty/ subdir instead 
(there's already a splay tree implementation in there).

> --- /dev/null
> +++ b/sim/nds32/sim-main.h
>
> +  /* If the same memory-region is already attached (registered) to sim-
core,
> +     the program just crashs. Unfortunately, we have no way to know whether
> +     the region is attached or not, so I use `mem_attached' to bookkeep it.
> +
> +     MEMOPT provides `memory-delete all' command to delete all the 
mappings,
> +     but if sim_core_attach is used in order to attach device_io, then
> +     there is no way to detach all. */
> +  int mem_attached;

there's no need to do this.  other targets simply probe during sim_open:
	if (sim_core_read_buffer (sd, NULL, read_map, &c, 4, 1) == 0)

if that fails, the memory hasn't been allocated, so do it then.
-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]