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 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:
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] |