From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 82664 invoked by alias); 7 Jan 2016 03:35:19 -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 82648 invoked by uid 89); 7 Jan 2016 03:35:18 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.2 required=5.0 tests=AWL,BAYES_00,KAM_LAZY_DOMAIN_SECURITY,RCVD_IN_DNSWL_NONE autolearn=no version=3.3.2 spammy=unwinding, felt, our X-Spam-User: qpsmtpd, 2 recipients X-HELO: rock.gnat.com Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-SHA encrypted) ESMTPS; Thu, 07 Jan 2016 03:35:16 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id E0D9711679E; Wed, 6 Jan 2016 22:35:14 -0500 (EST) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id vTZhVcl7HbxR; Wed, 6 Jan 2016 22:35:14 -0500 (EST) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 6F51611679B; Wed, 6 Jan 2016 22:35:14 -0500 (EST) Received: by joel.gnat.com (Postfix, from userid 1000) id 5632145019; Thu, 7 Jan 2016 07:35:05 +0400 (RET) Date: Thu, 07 Jan 2016 03:35:00 -0000 From: Joel Brobecker To: Michael Frysinger , gdb-patches@sourceware.org, Olivier Hainque Subject: Re: new sim: Visium Message-ID: <20160107033505.GA4505@adacore.com> References: <20160106044754.GC23304@adacore.com> <20160106193601.GR25548@vapier.lan> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160106193601.GR25548@vapier.lan> User-Agent: Mutt/1.5.23 (2014-03-12) X-SW-Source: 2016-01/txt/msg00106.txt.bz2 Hi Mike, Thanks for the quick review. A few questions below, before I get cracking on it again... > > +++ 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 Easy to do. > > +SIM_AC_COMMON > > please add at least: > SIM_AC_OPTION_WARNINGS > > and then fix all the warnings :) 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... > > +++ 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. Does "this" mean the comment, or moving visium to the common sim-fpu? 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: const sim_fpu sim_fpu_qnan = { - sim_fpu_class_qnan, 0, 0, 0 + sim_fpu_class_qnan, 1, 1152921367167893504, 1986400654 I am not sure what to do for that one... 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. > > +++ 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. That's the pointer-signedness warnings I was talking about above. Good eyes! I think you are right; and I actually tried to change it, but it had ripple effects, and I didn't want to get distracted by that, while I was working on numerous other cleanups. I will work on it now... > > +/* 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. 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. > > +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. For the read/write functions, we have a feature read-before-write feature which I don't think the current sim provides. 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? I will look at the WITH_HW framework. > > +++ 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. 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. Thanks again for the super-fast review! -- Joel