[sim] new port: Renesas RL78
Mike Frysinger
vapier@gentoo.org
Wed Nov 16 19:09:00 GMT 2011
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://sourceware.org/pipermail/gdb-patches/attachments/20111116/cad56ed3/attachment.sig>
More information about the Gdb-patches
mailing list