This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [patch] sim/erc32/ max simulation time extended by using 64bit ints
- From: Doug Evans <dje at google dot com>
- To: Joel Sherrill <joel dot sherrill at oarcorp dot com>
- Cc: Tiemen Schut <T dot Schut at sron dot nl>, "gdb-patches at sourceware dot org" <gdb-patches at sourceware dot org>
- Date: Fri, 7 May 2010 10:48:29 -0700
- Subject: Re: [patch] sim/erc32/ max simulation time extended by using 64bit ints
- References: <4BD1BBE3020000520000FC62@pluto.sron.nl> <m2ze394668d1004231328h55952dfpdea88e2fd17b4c2d@mail.gmail.com> <4BE08E95.5040500@oarcorp.com>
On Tue, May 4, 2010 at 2:16 PM, Joel Sherrill <joel.sherrill@oarcorp.com> wrote:
>> 1)
>>
>> -#define ? ? ? ?VAL(x) ?strtol(x,(char **)NULL,0)
>> +#define ? ? ? ?VAL(x) ?strtoull(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. ?But 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 @@
>>
>> ?int
>> ?sim_fetch_register(sd, regno, buf, length)
>> - ? ? SIM_DESC sd;
>> - ? ?int ? ? ? ? ? ? regno;
>> - ? ?unsigned char ?*buf;
>> - ? ? int length;
>> + ? ?SIM_DESC sd;
>> + ? ?int regno;
>> + ? ?unsigned char *buf;
>> + ? ?int length;
>> ?{
>> ? ? ?get_regi(&sregs, regno, buf);
>> ? ? ?return -1;
>> @@ -349,10 +349,10 @@
>>
>> ?int
>> ?sim_write(sd, mem, buf, length)
>> - ? ? SIM_DESC sd;
>> - ? ?SIM_ADDR ? ? ? ? ? ? mem;
>> + ? ?SIM_DESC sd;
>> + ? ?SIM_ADDR mem;
>> ? ? ?const unsigned char ?*buf;
>> - ? ?int ? ? ? ? ? ? length;
>> + ? ?int length;
>> ?{
>> ? ? ?return (sis_memory_write(mem, buf, length));
>> ?}
>>
>> Generally, formatting changes/fixes should be separate. ?I noticed a
>> few, can you remove them?
>>
>
> The ones you are citing above are my fault. ?I used a
> very new version of gcc to compile this when checking
> his patch and it complained. ?I 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<stdint.h>
>>
>
> Fixed.
Thanks.
>>
>> 4)
>>
>> - ? ?uint32 ? ? ? ? ?ildreg; ? ?/* Destination of last load instruction */
>> + ? ?uint64 ? ? ? ? ?ildreg; ? ?/* Destination of last load instruction */
>>
>> No point in making this uint64, leave it as uint32.
>>
>>
>
> Fixed.
Thanks.
>>
>> 5)
>>
>> + ? ? ? * sis.c, func.c, sis.h, interf.c: Increase max simulation time
>> + ? ? ? 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. ?It'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 <dje@google.com>
* 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. ?Look
> at this one from sim/ChangeLog:
>
> 2010-02-11 ?Doug Evans <dje@sebabeach.org>
>
> ? ?* cris/cpuv10.h, * cris/cpuv32.h, * cris/cris-desc.c,
> ? ?* cris/cris-desc.h, * cris/decodev10.c, * cris/decodev32.c,
> ? ?* cris/modelv10.c, * cris/modelv32.c, * cris/semcrisv10f-switch.c,
> ? ?* 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.