This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [patch] sim/erc32/ max simulation time extended by using 64bit ints


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.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]