From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 44315 invoked by alias); 6 Jan 2016 19:36:08 -0000 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 Received: (qmail 44219 invoked by uid 89); 6 Jan 2016 19:36:07 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.8 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 spammy=will!, SMP, smp, subset X-Spam-User: qpsmtpd, 2 recipients X-HELO: smtp.gentoo.org Received: from smtp.gentoo.org (HELO smtp.gentoo.org) (140.211.166.183) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Wed, 06 Jan 2016 19:36:04 +0000 Received: from vapier.lan (localhost [127.0.0.1]) by smtp.gentoo.org (Postfix) with SMTP id E2592340A15; Wed, 6 Jan 2016 19:36:01 +0000 (UTC) Date: Wed, 06 Jan 2016 19:36:00 -0000 From: Mike Frysinger To: Joel Brobecker Cc: Michael Frysinger , gdb-patches@sourceware.org, Olivier Hainque Subject: Re: new sim: Visium Message-ID: <20160106193601.GR25548@vapier.lan> Mail-Followup-To: Joel Brobecker , Michael Frysinger , gdb-patches@sourceware.org, Olivier Hainque References: <20160106044754.GC23304@adacore.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="2qwbT0JTInWqknst" Content-Disposition: inline In-Reply-To: <20160106044754.GC23304@adacore.com> X-IsSubscribed: yes X-SW-Source: 2016-01/txt/msg00103.txt.bz2 --2qwbT0JTInWqknst Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-length: 6571 On 06 Jan 2016 08:47, Joel Brobecker wrote: > What's still left to do on our end is create a testsuite, which > we haven't gotten around to doing yet. But we will! :-) yeah, i don't think i want to allow any more ports w/out tests. it makes it too hard for me to adjust code w/out breaking things. > +++ b/sim/visium/configure.ac > > +dnl This file is part of GDB. i think all these lines should be changed: dnl This file is part of the GNU Simulators. comes up in a bunch of files in this patch > +SIM_AC_COMMON please add at least: SIM_AC_OPTION_WARNINGS and then fix all the warnings :) > +# BFD conditionally uses zlib, so we must link it in if libbfd does, by > +# using the same condition. > +AM_ZLIB > + > +# BFD uses libdl when when plugins enabled. > +AC_PLUGINS you should not need either of these. they should be handled for you already. > +++ b/sim/visium/sim-fpu.c > > +/* Note: Although the origin of this file has not been researched, > + we know this is not the master copy of this code, and therefore > + we try to do as few modifications as possible, in order to facilitate > + possible coordination with that original, if it is every found. > + This explains why no apparent effort is made to improve this file's > + style to better match our usual standards. */ erm, the origin is pretty clear -- it was duplicated from sim/common/sim-fpu.c. this needs to be rectified. > +++ b/sim/visium/sim-main.h > > + int32_t regs[VISIUM_GENERAL_REGS]; do you want all the regs to be signed ? usually ports have them be unsigned as it makes coding simpler. > +/* A small macro to return the sim_cpu from the sim descriptor. > + We only support one CPU at the moment, so the CPU index is > + always 0. But perhaps we'll need to support SMP on this architecture, > + one day, in which case this macro will be useful to help supporting > + that (easy to find all locations, and perhaps CPU selection could > + be automated inside this macro itself). */ > +#define VISIUM_STATE_CPU(sd) (STATE_CPU (sd, 0)) usually you shouldn't need this. if you have a reference to the state but not a cpu, it tends to indicate the API isn't correctly passing down the cpu as an argument. so those funcs should be adjusted instead. > +++ b/sim/visium/sim_calls.c > > +sim_open (SIM_OPEN_KIND kind, struct host_callback_struct *callback, > + struct bfd *abfd, char **argv) looks like this forgets to call: sim_config sim_post_argv_init > +void > +sim_close (SIM_DESC sd, int quitting) > +{ > + sim_cpu *cpu = VISIUM_STATE_CPU (sd); > + > + visium_core_destroy (sd); > + visium_config_delete (sd); > + > + if (sd->trace_data != NULL) > + visium_trace_close (sd); > +} you should define SIM_CLOSE_HOOK in sim-main.h instead and use the common sim-close.c file. then delete this. > +sim_load (SIM_DESC sd, const char *prog, bfd *abfd, int from_tty) > +sim_read (SIM_DESC sd, SIM_ADDR mem, unsigned char *buf, int length) > +sim_write (SIM_DESC sd, SIM_ADDR mem, const unsigned char *buf, int length) this looks hairy and will require a good bit of unwinding. you shouldn't be defining your own sim_read/sim_write anymore. if you want memory, you should be using the common memory functions to attach it. if you want to simulate hardware, you should be using the WITH_HW framework. > +sim_fetch_register (SIM_DESC sd, int regno, unsigned char *buf, int length) > +sim_store_register (SIM_DESC sd, int regno, unsigned char *buf, int length) don't define these yourself. see commit e1211e55062594679697d2175b7ea77d as an example of converting to the new callbacks. > +sim_stop_reason (SIM_DESC sd, enum sim_stop *reason, int *sigrc) > +sim_stop (SIM_DESC sd) > +sim_resume (SIM_DESC sd, int step, int siggnal) you should be using the common core for these. instead, define sim_engine_run (see bfin/interp.c as a simple example), and instead of setting sd->stop_reason, call sim_engine_halt to halt simulation. that call will not return as it uses setjmp/longjmp. it tends to make design of your sim simpler. > +++ b/sim/visium/visium-defs.h > > +/* Display an error message. */ > +#define err(msg, args ...) \ > + do { \ > + fprintf (stderr, "[ERROR] (%s:%d: errno: %s) " msg "\n", \ > + __FILE__, __LINE__, visium_get_errno_msg (), ## args); \ > + } while (0) you should be using sim_io_eprintf instead of fprintf > +#define info(msg, args ...) \ > + do { \ > + fprintf (stdout, "[INFO] (%s:%d) " msg "\n", __FILE__, __LINE__, \ > + ## args); \ > + } while (0) > + > +/* Display a message. */ > +#define msg(msg, args ...) \ > + do { \ > + fprintf (stdout, msg "\n", ## args); \ > + fflush (stdout); \ > + } while (0) these should use sim_io_printf instead of fprintf > +/* All as "checks" except not requesting the simulation to stop. */ > +#define check(val, msg, args ...) \ > + do { \ > + if (!(val)) \ > + { \ > + err (msg, ## args); \ > + errno = 0; \ > + goto error; \ > + } \ > + } while (0) > + > +/* Check that Addr is not NULL. */ > +#define check_memory(addr) check (addr, "Out of memory") > + > +/* Unconditionally display an error message and jump to an error > + handler. */ > +#define fatal(msg, args ...) \ > + do { \ > + err (msg, ## args); \ > + errno = 0; \ > + goto error; \ > + } while (0) we have sim_io_error for throwing an error and exiting > --- /dev/null > +++ b/sim/visium/visium-dev-syscall.c > > +/* Handle a subset of system calls via a modified newlib C library. > + List of supported functions: > + - open() > + - close() > + - read() > + - write() > + - lseek() > + - rename() > + - unlink() > + - stat() > + - fstat() > + - gettimeofday() > + - isatty() > + - system() > + */ you shouldn't have to implement all this yourself. we have sim-syscall.c to deal with these. all your sim needs to do is handle the trap and then unpack all the regs before passing them to sim_syscall(). > +++ b/sim/visium/visium-ranges.c > +++ b/sim/visium/visium-ranges.h glancing at the API, i think the common sim-arange module provides the same logic already. > +++ b/sim/visium/visium-trace.c > +++ b/sim/visium/visium-trace.h i glanced through the trace logic ... it doesn't seem like it's hardware specific (like you've got a hardware module that is handling this). since it's all software based, you should throw away the visium trace logic and switch to the common sim-trace module. the sim-trace.h header includes a lot of macros to quickly instrument your code. -mike --2qwbT0JTInWqknst Content-Type: application/pgp-signature; name="signature.asc" Content-Description: Digital signature Content-length: 819 -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJWjWyhAAoJEEFjO5/oN/WB6A0P/iPo9yODtR7YpAn6SwKz33Pk MevNLr6ECC1kaIodjTuCoUN2MjuTnyg8hYUz4nbbIJ2ORm3fm70imRLerIckWFVo xdDxQb/e540LqmMWX9OA01DKYjMBLV6W1JFZveQiNAGK+fb0XCMGLKOeE2C+zZca ydSt29QWXHNo8oSo+r7mpXKcYof9QYjroHy/a7eWLPHuZn+6hk/uRx4FCX868NK3 AnZ+WL9y043mTSZt2RKycP0LKTic4qmuvLAByV5v0jhMesyQIgjtGhctl5R0T5VC zRXQjQgxFxv5duEx5VUGzqsx+1nAXJae9btFhBtjSFZvJX0sPcBsnF1CnjLsgVE0 dqHXobyUkNyjsZ2FYmwCKCEfUratAIR3Ut1UcnDVWCEnEiGUTzSVZVe2177/MBlU tUwyz482Jxw4D/Lt3UcDnWiJ/8RgNHKE1UxyYdJ0fra0WbsSzYSCOJeJU05rjV1t vlkmA6uht6lG77HHlw6M8kFyJbNz0lnZ3zy9WybA6wpMqFdXEE3Bba4URmyw1oFG NDGCl6hZ69Tdi09X2jVvB6DfH1pYU7/jytZXc+LX3XYVYQlYL1hpsJImWqPgXfaI 2G6Xm3Qcl2FT6bNLFVzJGthfjXhQ2vQDJ+50pH111kVANKVLTv0rfsT/eD9Q9cUX U1gGrFH7OcQ8j1q+H9aG =O+1R -----END PGP SIGNATURE----- --2qwbT0JTInWqknst--