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

[Doug Evans <dje@google.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.