From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 6561 invoked by alias); 16 Nov 2011 19:09:36 -0000 Received: (qmail 6550 invoked by uid 22791); 16 Nov 2011 19:09:34 -0000 X-SWARE-Spam-Status: No, hits=-2.6 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from smtp.gentoo.org (HELO smtp.gentoo.org) (140.211.166.183) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 16 Nov 2011 19:09:19 +0000 Received: from vapier.localnet (localhost [127.0.0.1]) by smtp.gentoo.org (Postfix) with ESMTP id 9C8D31B401D; Wed, 16 Nov 2011 19:09:18 +0000 (UTC) From: Mike Frysinger To: gdb-patches@sourceware.org Subject: Re: [sim] new port: Renesas RL78 Date: Wed, 16 Nov 2011 19:09:00 -0000 User-Agent: KMail/1.13.7 (Linux/3.1.0; KDE/4.6.5; x86_64; ; ) Cc: DJ Delorie References: <201111160606.pAG66U4p017015@greed.delorie.com> In-Reply-To: <201111160606.pAG66U4p017015@greed.delorie.com> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="nextPart2581149.KxL5sWQNxG"; protocol="application/pgp-signature"; micalg=pgp-sha1 Content-Transfer-Encoding: 7bit Message-Id: <201111161409.19823.vapier@gentoo.org> X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2011-11/txt/msg00446.txt.bz2 --nextPart2581149.KxL5sWQNxG Content-Type: Text/Plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Content-length: 8496 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= =20 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 =3D \ > + main.o \ can't you use nrun.o ? > + $(ENDLIST) this $(ENDLIST) business looks like dead code ? > +LIBS =3D $B/bfd/libbfd.a $B/libiberty/libiberty.a common/Make-common.in already sets up $(BFD_LIB) and $(LIBIBERTY_LIB), and= =20 adds them to $(LIBDEPS) and $(EXTRA_LIBS), so you shouldn't have to either= =20 hardcode the reference yourself, or even refer to them at all ... > +arch =3D rl78 looks like we should expand configure.tgt:SIM_ARCH to automatically export= =20 @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 you= r=20 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 . */ shouldn't these lines be indented ? seems to apply to most of the files in= =20 this patchset, so i won't comment again ... > +int verbose =3D 0; > +int trace =3D 0; > +int rl78_in_gdb =3D 1; > +int timer_enabled =3D 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[] =3D { 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 =3D ""; 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 miss= ed=20 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 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=20 common/ and give it a better name like cb_printf(). > +void > +rl78_load (bfd *prog, host_callback *callbacks) > +{ > ... > + phdrs =3D malloc (sizeof_phdrs); > + if (phdrs =3D=3D NULL) > + { > + fprintf (stderr, "Failed allocate memory to hold program > headers\n"); + return; > + } > ... > + buf =3D malloc (size); > + if (buf =3D=3D 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 > +#endif other places in this patchset you've included stdlib.h unconditionally.=20= =20 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 =3D strdup (optarg); does it need to be strdup-ed ? it's a pointer into argv, so it should be s= afe=20 to use as is ... otherwise, this should be xstrdup(). could also constify dump_counts_filename > + prog =3D 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 b= e a=20 little confusing where this error message is coming from when looking at=20 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 mapping= s=20 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 =3D NULL; > +static void > +init_stack_tty () (void) > +{ > + stack_tty =3D 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 l= ine=20 option specifying the output ... > +#define WILD_JUMP_CHECK(new_pc) \ > + if (new_pc =3D=3D 0 || new_pc > 0xfffff) \ > + { \ > + pc =3D 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 =3D fopen(filename, "w"); needs space before the "(" > + for (i=3D0; 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 fil= e,=20 so i've squashed other comments along these lines. > + time(&now); > + struct tm *tm =3D 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=3D0; 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 --nextPart2581149.KxL5sWQNxG Content-Type: application/pgp-signature; name=signature.asc Content-Description: This is a digitally signed message part. Content-length: 836 -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.17 (GNU/Linux) iQIcBAABAgAGBQJOxApfAAoJEEFjO5/oN/WBmzoP/Arv64VVUY+j/v6ceJXqMZh6 3rg3C17prP2k/AGLYBSPwvRbJZTV+dSlK9XxmOR7zWqw5u9dSdDdso56bJXvdab/ 2DFpbgtkETvuKFx90wnBpDe0KAXpeMn7551BOY6QBYG3SflpuE4iCIVr679FEEA/ sSQBjCeNBDRiGBFfKVea702RUEVGcfwjMTJgkT+xtlYp8PydYImk5R9bRwj2b6Cy 2v5GqMndqDGAeG8blmxoknRukw53uUj38fwoPJVvjSyo6Baf026AxXxD97R4GUUB xs92NufvXsIT7HFuI10cQglp0rVjrt3mjb/HR9j7yPXesh3h4SuRIf6a58HJ8NPE x7SGGvmMlRFSnNon6vzBky8e4XLOT9zRznGhUj7Z/m79q7oVFIUw2neRHJ9D2Se+ 6bTEsuE+9lK7KIqZi70t9EKOFBf/PpsypNMDbNQ2WJRAz2Yy7B6owHU27PQp2bHV FAL9YekZJV2T+GwWDgKjXtyrG2uJLyp7MAILAUN/g6oDAdCdfJtEo13q0X23/XK0 0TvNKoM4+gyctB3aCXxv7u+tfXFCqQQkoa0mkh2qxvurZ02zOFr9CUdAWlbN1Z53 QBwTO8yBwi66Hs01ORmMV2Jawf1+EHpPWCxcll6XKRZq6v2Vmu9VNqcJbXGxbknf nB2JAI+fNs+uoYOQjJbV =kkBE -----END PGP SIGNATURE----- --nextPart2581149.KxL5sWQNxG--