From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 21878 invoked by alias); 7 May 2010 17:48:41 -0000 Received: (qmail 21867 invoked by uid 22791); 7 May 2010 17:48:40 -0000 X-SWARE-Spam-Status: No, hits=-1.9 required=5.0 tests=AWL,BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,SPF_HELO_PASS,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from smtp-out.google.com (HELO smtp-out.google.com) (216.239.44.51) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 07 May 2010 17:48:35 +0000 Received: from hpaq7.eem.corp.google.com (hpaq7.eem.corp.google.com [172.25.149.7]) by smtp-out.google.com with ESMTP id o47HmWMZ005595 for ; Fri, 7 May 2010 10:48:32 -0700 Received: from pvg12 (pvg12.prod.google.com [10.241.210.140]) by hpaq7.eem.corp.google.com with ESMTP id o47Hm557024233 for ; Fri, 7 May 2010 10:48:30 -0700 Received: by pvg12 with SMTP id 12so594987pvg.21 for ; Fri, 07 May 2010 10:48:30 -0700 (PDT) MIME-Version: 1.0 Received: by 10.141.131.13 with SMTP id i13mr135664rvn.121.1273254509899; Fri, 07 May 2010 10:48:29 -0700 (PDT) Received: by 10.141.89.17 with HTTP; Fri, 7 May 2010 10:48:29 -0700 (PDT) In-Reply-To: <4BE08E95.5040500@oarcorp.com> References: <4BD1BBE3020000520000FC62@pluto.sron.nl> <4BE08E95.5040500@oarcorp.com> Date: Fri, 07 May 2010 17:48:00 -0000 Message-ID: Subject: Re: [patch] sim/erc32/ max simulation time extended by using 64bit ints From: Doug Evans To: Joel Sherrill Cc: Tiemen Schut , "gdb-patches@sourceware.org" Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable X-System-Of-Record: true 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: 2010-05/txt/msg00194.txt.bz2 On Tue, May 4, 2010 at 2:16 PM, Joel Sherrill w= rote: >> 1) >> >> -#define =A0 =A0 =A0 =A0VAL(x) =A0strtol(x,(char **)NULL,0) >> +#define =A0 =A0 =A0 =A0VAL(x) =A0strtoull(x,(char **)NULL,0) >> >> I realize VAL is only used once in interf.c but it's also defined in >> other files as well. >> While one could consolidate them, having the macro at all is probably >> less preferable to just calling strtoul{,l} directly. >> I would just remove it from interf.c and call strtoull directly. >> > > I have fixed this in interf.c. =A0But didn't touch the other files. > Is this OK? Sure. > I can do a second patch after this is merged if you want VAL killed. Naaa, no need. >> 2) >> >> @@ -338,10 +338,10 @@ >> >> =A0int >> =A0sim_fetch_register(sd, regno, buf, length) >> - =A0 =A0 SIM_DESC sd; >> - =A0 =A0int =A0 =A0 =A0 =A0 =A0 =A0 regno; >> - =A0 =A0unsigned char =A0*buf; >> - =A0 =A0 int length; >> + =A0 =A0SIM_DESC sd; >> + =A0 =A0int regno; >> + =A0 =A0unsigned char *buf; >> + =A0 =A0int length; >> =A0{ >> =A0 =A0 =A0get_regi(&sregs, regno, buf); >> =A0 =A0 =A0return -1; >> @@ -349,10 +349,10 @@ >> >> =A0int >> =A0sim_write(sd, mem, buf, length) >> - =A0 =A0 SIM_DESC sd; >> - =A0 =A0SIM_ADDR =A0 =A0 =A0 =A0 =A0 =A0 mem; >> + =A0 =A0SIM_DESC sd; >> + =A0 =A0SIM_ADDR mem; >> =A0 =A0 =A0const unsigned char =A0*buf; >> - =A0 =A0int =A0 =A0 =A0 =A0 =A0 =A0 length; >> + =A0 =A0int length; >> =A0{ >> =A0 =A0 =A0return (sis_memory_write(mem, buf, length)); >> =A0} >> >> Generally, formatting changes/fixes should be separate. =A0I noticed a >> few, can you remove them? >> > > The ones you are citing above are my fault. =A0I used a > very new version of gcc to compile this when checking > his patch and it complained. =A0I got near the code and > fixed it. Sorry. I'm not sure what gcc complained about, but it's not important. I noticed the whitespace changes are still there. Am I looking at the wrong patch? btw, there's no need for another iteration of approvals just for that. >> 3) >> >> +#include "stdint.h" >> >> That should be >> > > Fixed. Thanks. >> >> 4) >> >> - =A0 =A0uint32 =A0 =A0 =A0 =A0 =A0ildreg; =A0 =A0/* Destination of last= load instruction */ >> + =A0 =A0uint64 =A0 =A0 =A0 =A0 =A0ildreg; =A0 =A0/* Destination of last= load instruction */ >> >> No point in making this uint64, leave it as uint32. >> >> > > Fixed. Thanks. >> >> 5) >> >> + =A0 =A0 =A0 * sis.c, func.c, sis.h, interf.c: Increase max simulation = time >> + =A0 =A0 =A0 by using uint64 for relevant counters. >> >> I realize sim/erc32/ChangeLog doesn't always follow the GNU >> conventions for ChangeLog entries - it's software obtained from >> elsewhere. =A0It's ok with me to leave as is, but I defer to someone >> else with an opinion. >> >> > > What specifically are you noticing here? > > I looked around and didn't spot anything too offensive. For reference sake, compare that entry with, for example, this one: 2010-04-19 Doug Evans * ser-base.c (generic_readchar): Watch for EOF in read of error_fd. * ser-pipe.c (pipe_open): Fix file descriptor leaks. (pipe_close): Ditto. Note that there's a separate entry for each function changed. But as I say, I wouldn't insist on this for erc32. > FWIW Doug I think you have a script which is helping with > your ChangeLog entries and it has (or had) a bug. =A0Look > at this one from sim/ChangeLog: > > 2010-02-11 =A0Doug Evans > > =A0 =A0* cris/cpuv10.h, * cris/cpuv32.h, * cris/cris-desc.c, > =A0 =A0* cris/cris-desc.h, * cris/decodev10.c, * cris/decodev32.c, > =A0 =A0* cris/modelv10.c, * cris/modelv32.c, * cris/semcrisv10f-switch.c, > =A0 =A0* cris/semcrisv32f-switch.c: Regenerate. > > I don't think there should be a ", *" between the files. :-D I loathe having one line per file, where each line says the same thing. Too low an S/N ratio for my tastes, especially with a lot of files. :-( So I copied a style I saw in opcodes. If Alan [Modra] can do that in opcodes, I can do that in the sims. [And thank goodness. :-)] > > My helper script has its own oddities. LOL >> >> 6) I'm assuming this change has been well tested. >> > > I've used it for RTEMS testing. Cool.