From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 6589 invoked by alias); 7 Jan 2016 05:53:36 -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 6567 invoked by uid 89); 7 Jan 2016 05:53:35 -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=integrate, formats, sprinkle, unwinding 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; Thu, 07 Jan 2016 05:53:34 +0000 Received: from vapier.lan (localhost [127.0.0.1]) by smtp.gentoo.org (Postfix) with SMTP id 3F7E5340813; Thu, 7 Jan 2016 05:53:31 +0000 (UTC) Date: Thu, 07 Jan 2016 05:53:00 -0000 From: Mike Frysinger To: Joel Brobecker Cc: Michael Frysinger , gdb-patches@sourceware.org, Olivier Hainque Subject: Re: new sim: Visium Message-ID: <20160107055330.GV25548@vapier.lan> Mail-Followup-To: Joel Brobecker , Michael Frysinger , gdb-patches@sourceware.org, Olivier Hainque References: <20160106044754.GC23304@adacore.com> <20160106193601.GR25548@vapier.lan> <20160107033505.GA4505@adacore.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="zvmqw4jX2vbPsMQB" Content-Disposition: inline In-Reply-To: <20160107033505.GA4505@adacore.com> X-IsSubscribed: yes X-SW-Source: 2016-01/txt/msg00107.txt.bz2 --zvmqw4jX2vbPsMQB Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Content-length: 6813 On 07 Jan 2016 07:35, Joel Brobecker wrote: > > > +SIM_AC_COMMON > >=20 > > please add at least: > > SIM_AC_OPTION_WARNINGS > >=20 > > and then fix all the warnings :) >=20 > I didn't know about SIM_AC_OPTION_WARNINGS. FTR, during development/ > cleanup, I modified the visium Makefile to add all the compilation > warnings that we use for GDB, expect pointer signedness, IIRC, which > was creating a lot more noise than what I felt had the time to handle. > Dealing with those warnings was a very useful exercise because it > found a couple of bugs, and allowed a fair amount of cleanup. > I'll followup with pointer signedness in another of your comments... SIM_AC_OPTION_WARNINGS is based on the gdb flags. i don't think we add anything in the sim that isn't in gdb. it sometimes gets out of date, but then someone just resyncs them ;). > > > +++ 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 facili= tate > > > + 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. */ > >=20 > > erm, the origin is pretty clear -- it was duplicated from > > sim/common/sim-fpu.c. this needs to be rectified. >=20 > Does "this" mean the comment, or moving visium to the common sim-fpu? moving to the common code > I see that many of the small differences are comments and formatting, > so I will work towards normalizing. But there seems to be an important > difference in: >=20 > const sim_fpu sim_fpu_qnan =3D { > - sim_fpu_class_qnan, 0, 0, 0 > + sim_fpu_class_qnan, 1, 1152921367167893504, 1986400654 >=20 > I am not sure what to do for that one... >=20 > I was hoping we could start with visium having its own copy for now, > with the understanding that we should find a solution to avoid it > in the future. is this the only difference ? iiuc, it's not unheard of for arches (either in the hardware or the ABI) to define different values for NaN. as such, letting a target override this makes sense. maybe for now introduce a define like: #ifndef SIM_FPU_QNAN_VALUE # define SIM_FPU_QNAN_VALUE 0, 0, 0 #endif const sim_fpu sim_fpu_qnan =3D { sim_fpu_class_qnan, SIM_FPU_QNAN_VALUE }; and then in your sim-main.h do: #define SIM_FPU_QNAN_VALUE 1, UNSIGNED64(0xfffffe000000000), 0x7666118e while you're at it, use hex values to make it more readable :) > > > +/* 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 architec= ture, > > > + one day, in which case this macro will be useful to help supporti= ng > > > + 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)) > >=20 > > 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. >=20 > I tried to differentiate between the data which is CPU-specific > (eg. registers) and the data which is shared between all CPUs > (eg. devices). The former was part of the sim_cpu structure, while > on the other hand, the latter was placed inside the sim_desc. > Because you nearly always need access to stuff like the memory > device, I was naturally pushed towards passing the sim_desc rather > than the sim_cpu. To pass the sim_cpu instead, I think I would have > to move a lot of the stuff in struct sim_state to the sim_cpu, > which feels wrong to me. you can get to the state from the cpu: SIM_DESC sd =3D CPU_STATE (cpu); so i don't think you need to do any structure shuffling > > > +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) > >=20 > > 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, y= ou > > should be using the common memory functions to attach it. if you want = to > > simulate hardware, you should be using the WITH_HW framework. >=20 > For the read/write functions, we have a feature read-before-write > feature which I don't think the current sim provides. i don't know what this feature is. could you elaborate ? > There is > also a pre-initialization feature of the RAM to a certain value > to make execution more reliable when the program reads undefined > memory. What would you suggest we do? when you attach memory, the default is to be zero filled. we do this for all ports. that sounds pretty reliable to me :). if you want to use a diff value, you can do this from the command line: $ run --memory-fill 0xff --memory-size 10Mb ... did you need something else ? > > > +++ b/sim/visium/visium-trace.c > > > +++ b/sim/visium/visium-trace.h > >=20 > > 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). si= nce > > it's all software based, you should throw away the visium trace logic a= nd > > switch to the common sim-trace module. the sim-trace.h header includes= a > > lot of macros to quickly instrument your code. >=20 > The traces have to have the the format that visium-trace generates. > This is because the format is then exploited by other tools which > expect that format, and so we cannot change that. I don't think > the sim-trace module allows us to generate the data in the format > we need, does it? If that's not the case, then we have two options: > 1. leave the visum-trace module as it; 2. yank it out. I don't think > that (1) will make global maintenance of the sim project harder, but > if you think (2) is best, then we'll keep this as an AdaCore-only > piece of code. currently the sim-trace module does not have output formats. i'm open to extending this so ports can add custom hooks to control it. can you provide a few sample lines ? would hooking at trace_generic be all you needed ? my concern isn't so much about global maintenance (although that's always part of it), but about users having consistent behavior across targets. i've been trying to integrate sim-trace into more common places, and so targets can easily sprinkle one or two lines around, and then get access to a lot of useful data. -mike --zvmqw4jX2vbPsMQB Content-Type: application/pgp-signature; name="signature.asc" Content-Description: Digital signature Content-length: 819 -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJWjf1aAAoJEEFjO5/oN/WBFo8P/3s3Mmylgj6vRvf9yRNMtXT/ AO5Rj84EXyqQ9Lq+c48pMqi1ZWvTN9PQ7P+4H3sCKLe2Vgq9KLiCuZmXyDDo6MW6 8lLi6gORpfM5rq9mVNirHXyj18R3olfTz6iz3op2IA0EGNsurlJwW0H9Mqm8p/5i E/JMyl68o+seegXR14LtPiodxzqdmrQyXkchkOZNb561hu61unHcH/M9nluM4E+/ SO/tauNKgV70ZqbkSfS5AeXtscTFiiNyuK0wxvnwSqIdob2mIEie9VQrJkPIwEsc 5dZcbIeZps00io6MainvktHqnIrBAqefeW9YGUhPIxOuWFfOa4Q1o/CInet0NFj9 3bp4ibMP40GyeDvcwtawZn21MCUIAbiZ4hc8fTuaPOu2ZWzhZ1VZKfBf7Brl1m2d QOVfy7UsBeMe9J5rwiVe0p57qnl65zouB7i11/cQ9fo1CkQbnk+Y6xDGgMwNgRMC qAG/sx3gHQ1UtBEVwoa1KXgBKVGOhTHBQi3VzMbkLWO9DJN8O2ArQQh6vxWSqmZG eAKpUPP0/7Rnh68P4P/OVbcb0e9Nx10X56nnRSBlTl1mb2ofcHRU0OhxEItF3oNO V/+bztS2tJnvMVaVlAH7dIOwr4r1wejG7iySyN1zq012Y9ZG7df7DNPwKcAzbK71 NtYrdEcFhFrMhRb1AN4t =N9Qt -----END PGP SIGNATURE----- --zvmqw4jX2vbPsMQB--