Re: [sim] new port: Renesas RL78

[Mike Frysinger <vapier@gentoo.org>]

[-- Attachment #1: Type: Text/Plain, Size: 8736 bytes --]

On Wednesday 16 November 2011 01:06:30 DJ Delorie wrote:
> Note: This simulator reuses a lot of code from the RX simulator.
> Hopefully I removed all the RX-specific stuff ;-)

i know a lot of the comments below apply to the rx sim too ... oh well ;)

seems like there's a bunch of common/ code this port could utilize ... like 
all the tracing logic ...

> --- /dev/null	1 Jan 1970 00:00:00 -0000
> +++ sim/rl78/Makefile.in	16 Nov 2011 05:44:53 -0000
>
> +SIM_RUN_OBJS = \
> +	main.o \

can't you use nrun.o ?

> +	$(ENDLIST)

this $(ENDLIST) business looks like dead code ?

> +LIBS = $B/bfd/libbfd.a $B/libiberty/libiberty.a

common/Make-common.in already sets up $(BFD_LIB) and $(LIBIBERTY_LIB), and 
adds them to $(LIBDEPS) and $(EXTRA_LIBS), so you shouldn't have to either 
hardcode the reference yourself, or even refer to them at all ...

> +arch = rl78

looks like we should expand configure.tgt:SIM_ARCH to automatically export 
@SIM_ARCH@ to Makefiles, and have the default arch value be @SIM_ARCH@.

> --- /dev/null	1 Jan 1970 00:00:00 -0000
> +++ sim/rl78/configure.in	16 Nov 2011 05:44:54 -0000
>
> +AC_PREREQ(2.5)dnl
> +AC_INIT(Makefile.in)
> +AC_CONFIG_HEADER(config.h:config.in)
> +AC_CHECK_HEADERS(getopt.h)
> +
> +sinclude(../common/aclocal.m4)
> +
> +# Bugs in autoconf 2.59 break the call to SIM_AC_COMMON, hack around
> +# it by inlining the macro's contents.
> +sinclude(../common/common.m4)

this is out of date.  please look at latest rx/configure.ac to see what your 
code should now look like.

> --- /dev/null	1 Jan 1970 00:00:00 -0000
> +++ sim/rl78/cpu.c	16 Nov 2011 05:44:54 -0000
>
> +/* cpu.c --- CPU for RL78 simulator.
> +
> +Copyright (C) 2011
> +Free Software Foundation, Inc.
> +Contributed by Red Hat, Inc.
> +
> +This file is part of the GNU simulators.
> +
> +This program is free software; you can redistribute it and/or modify
> +it under the terms of the GNU General Public License as published by
> +the Free Software Foundation; either version 3 of the License, or
> +(at your option) any later version.
> +
> +This program is distributed in the hope that it will be useful,
> +but WITHOUT ANY WARRANTY; without even the implied warranty of
> +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +GNU General Public License for more details.
> +
> +You should have received a copy of the GNU General Public License
> +along with this program.  If not, see <http://www.gnu.org/licenses/>.  */

shouldn't these lines be indented ?  seems to apply to most of the files in 
this patchset, so i won't comment again ...

> +int verbose = 0;
> +int trace = 0;
> +int rl78_in_gdb = 1;
> +int timer_enabled = 2;

yikes, global variables make me cry

> +typedef struct {
> +  unsigned char x;
> +  unsigned char a;
> +  unsigned char c;
> +  unsigned char b;
> +  unsigned char e;
> +  unsigned char d;
> +  unsigned char l;
> +  unsigned char h;
> +} RegBank;

do we allow CamelCase in sim/ ?

> +static void trace_register_init ();

shouldn't that be "(void)" ?

> +char *
> +reg_names[] = {

const char * const reg_names[]

be nice if this weren't global too ...

> +static char *
> +psw_string (int psw)
> +{
> +  static char buf[30];
> +  char *comma = "";

const char *comma

> +  printf("%s", buf);

debug code ?

> +void
> +trace_register_changes ()

(void) ... seems to be a lot in this patchset like this, so i probably missed 
some ... might want to grep ...

> +      printf ("PSW: \033[31m");
> +      psw_string (old_psw);
> +      printf (" \033[32m");

yikes, unavoidable ascii escapes ?

> +static void
> +trace_register_init ()

(void)

> --- /dev/null	1 Jan 1970 00:00:00 -0000
> +++ sim/rl78/cpu.h	16 Nov 2011 05:44:54 -0000

should this header have ifdef protection against multiple inclusion ?

> +#include <setjmp.h>

doesn't seem to be used here

> +int  get_c ();

(void)

> --- /dev/null	1 Jan 1970 00:00:00 -0000
> +++ sim/rl78/load.c	16 Nov 2011 05:44:54 -0000
>
> +/* Helper function for invoking a GDB-specified printf.  */
> +static void
> +xprintf (host_callback *callback, const char *fmt, ...)
> +{
> +  va_list ap;
> +
> +  va_start (ap, fmt);
> +
> +  (*callback->vprintf_filtered) (callback, fmt, ap);
> +
> +  va_end (ap);
> +}

this looks like it could be generally useful.  wonder if we should move it to 
common/ and give it a better name like cb_printf().

> +void
> +rl78_load (bfd *prog, host_callback *callbacks)
> +{
> ...
> +  phdrs = malloc (sizeof_phdrs);
> +  if (phdrs == NULL)
> +    {
> +      fprintf (stderr, "Failed allocate memory to hold program
> headers\n"); +      return;
> +    }
> ...
> +      buf = malloc (size);
> +      if (buf == NULL)
> +	{
> +	  fprintf (stderr, "Failed to allocate buffer to hold program
> segment\n"); +	  continue;
> +	}

don't we have xmalloc() ?

> --- /dev/null	1 Jan 1970 00:00:00 -0000
> +++ sim/rl78/load.h	16 Nov 2011 05:44:54 -0000

ifdef's for multiple inclusion ?

> --- /dev/null	1 Jan 1970 00:00:00 -0000
> +++ sim/rl78/main.c	16 Nov 2011 05:44:54 -0000
>
> +#ifdef HAVE_STDLIB_H
> +#include <stdlib.h>
> +#endif

other places in this patchset you've included stdlib.h unconditionally.  
should probably be consistent ...

> +int
> +main (int argc, char **argv)
> +{
> ...
> +  setbuf(stdout, NULL);

doesn't this hurt performance ?  especially when tracing ?

also, need space before the "("

> +	  dump_counts_filename = strdup (optarg);

does it need to be strdup-ed ?  it's a pointer into argv, so it should be safe 
to use as is ... otherwise, this should be xstrdup().

could also constify dump_counts_filename

> +  prog = bfd_openr (argv[optind], 0);
> +  if (!prog)
> +    {
> +      fprintf (stderr, "Can't read %s\n", argv[optind]);
> +      exit (1);
> +    }
> +
> +  if (!bfd_check_format (prog, bfd_object))
> +    {
> +      fprintf (stderr, "%s not a rl78 program\n", argv[optind]);
> +      exit (1);
> +    }

most sim's output prefix argv[0] in the error message.  otherwise, it can be a 
little confusing where this error message is coming from when looking at 
testsuite logs like gcc.



> --- /dev/null	1 Jan 1970 00:00:00 -0000
> +++ sim/rl78/mem.c	16 Nov 2011 05:44:54 -0000

seems like much of the utility of this file is duplicating the core mappings 
logic in like common/sim-core.c :/

> +void
> +init_mem ()

(void)

> --- /dev/null	1 Jan 1970 00:00:00 -0000
> +++ sim/rl78/mem.h	16 Nov 2011 05:44:54 -0000

ifdef multiple inclusion protection ...

> +void init_mem ();

(void)

> --- /dev/null	1 Jan 1970 00:00:00 -0000
> +++ sim/rl78/rl78.c	16 Nov 2011 05:44:54 -0000
>
> +#define xSTACK_TTY "/dev/pts/1"
> +#ifdef STACK_TTY
> +static FILE *stack_tty = NULL;
> +static void
> +init_stack_tty ()

(void)

> +{
> +  stack_tty = fopen (STACK_TTY, "w");
> +  if (!stack_tty)
> +    {
> +      fprintf(stderr, "cannot open %s\n", STACK_TTY);
> +      exit(1);
> +    }
> +  setbuf(stack_tty, NULL);

need space before the "("

> +  fprintf (stack_tty, "\033[1;1H\033[J");
> +}
> +#endif

in general though, yikes ... looks like this would be better as a command line 
option specifying the output ...

> +#define WILD_JUMP_CHECK(new_pc) \
> +  if (new_pc == 0 || new_pc > 0xfffff) \
> +    { \
> +      pc = opcode_pc; \
> +      fprintf(stderr, "Wild jump to 0x%x from 0x%x!\n", new_pc, pc); \
> +      DO_RETURN (RL78_MAKE_HIT_BREAK ()); \
> +    }

put this into a do{...}while(0) block to avoid surprises wrt "else" ?

> +static int
> +get_carry ()

(void)

> +void
> +dump_counts_per_insn (char *filename)

const

> +  f = fopen(filename, "w");

needs space before the "("

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

needs spaces around those operators

> +int
> +decode_opcode ()

(void)

> +      tprintf("BRANCH_COND: ");
> +      CLOCKS(3);

space before the "(".  seems to apply to many tprintf's/CLOCK's in this file, 
so i've squashed other comments along these lines.

> +	time(&now);
> +	struct tm *tm = localtime(&now);
> +	fprintf(stack_tty, "%08x %02d:%02d:%02d\n",
> +		pc, tm->tm_hour, tm->tm_min, tm->tm_sec);

needs spaces before the "("

> +#if 0
> +      if (trace)
> +	{
> +	  int i;
> +	  skip_init ++;
> +	  for (i=0; i<8; i++)
> +	    printf(" %02x", mem_get_qi (0xf0000 | (a + i)) & 0xff);
> +	  skip_init --;
> +	}
> +#endif

just delete then ?

> --- /dev/null	1 Jan 1970 00:00:00 -0000
> +++ sim/rl78/trace.h	16 Nov 2011 05:44:54 -0000

ifdef multiple inclusion protection ...
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]