[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

gettimeofday seconds rollover problem?



It seems to me that the assembler statements in m68k.h for enabling and
disabling interrupts are never going to work with gcc optimisation.

Have a look at this article.

http://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html#Extended-Asm
 
Quoting 
"
The volatile keyword indicates that the instruction has important
side-effects. GCC will not delete a volatile asm if it is reachable.
(The instruction can still be deleted if GCC can prove that control-flow
will never reach the location of the instruction.) Note that even a
volatile asm instruction can be moved relative to other code, including
across jump instructions. For example, on many targets there is a system
register which can be set to control the rounding mode of floating point
operations. You might try setting it with a volatile asm, like this
PowerPC example: 

            asm volatile("mtfsf 255,%0" : : "f" (fpenv));
            sum = x + y;

This will not work reliably, as the compiler may move the addition back
before the volatile asm. To make it work you need to add an artificial
dependency to the asm referencing a variable in the code you don't want
moved, for example: 

         asm volatile ("mtfsf 255,%1" : "=X"(sum): "f"(fpenv));
         sum = x + y;
"

Nice trick, but how can we make it work for our situation? 

I am looking at setting values (eg. "memory") in the clobber field to
prevent movement of the instruction. 

-----Original Message-----
From: Ian Caddy [mailto:ianc at goanna.iinet.net.au] 
Sent: Thursday, 23 February 2006 3:01 PM
To: Andrew Sinclair
Cc: Chris Johns; Joel Sherrill; rtems-users
Subject: Re: gettimeofday seconds rollover problem?

Hmmm... looks like I will have to look deeper into our application.  I
had a quick look through this morning, but on some functions such as the
one you mentioned, the optimisation makes it a bit hard to follow on a
quick run through.

I confirm the problem for rtems_clock_get, although in our case, one of
the parameters is inside the disable interrupts, while the other part is
not.  This is just as bad as both not being in there.  Again, I would
say that changing tmp to be a volatile will fix the problem.

Throughout the rest of today I will try to look thoroughly through our
application to see if I can find anymore and compile a list.

regards,

Ian Caddy


Andrew Sinclair wrote:
> It looks like a similar issue exists with rtems_clock_get.
> 
> I just wonder where else it is an issue...
> 
> 
> -----Original Message-----
> From: Ian Caddy [mailto:ianc at goanna.iinet.net.au]
> Sent: Thursday, 23 February 2006 1:46 PM
> To: Chris Johns
> Cc: Joel Sherrill; Andrew Sinclair; rtems-users
> Subject: Re: gettimeofday seconds rollover problem?
> 
> On further analysis, it looks like the compiler is optimising out the 
> two local variables, seconds and microseconds in gettimeofday.
> 
> The reason for this is that the interrupt macros do not use these 
> variables and so the compiler has no reason to ensure they are created

> in the interrupt disabled state, since the compiler does not know that

> was what we were trying to do.
> 
> The simple solution is to make these two local variables volatile 
> which ensures no optimisation of them:
> 
> int gettimeofday(
>    struct timeval  *tp,
>    struct timezone *tzp
> )
> {
>    rtems_interrupt_level level;
>    volatile rtems_unsigned32      seconds;
>    volatile rtems_unsigned32      microseconds;
> 
> 
> This then creates the correct code, when -O3 is used:
> 
>    rtems_interrupt_disable(level);
>   8083f70:	203c 0000 0700 	movel #1792,%d0
>   8083f76:	40c1           	movew %sr,%d1
>   8083f78:	8081           	orl %d1,%d0
>   8083f7a:	46c0           	movew %d0,%sr
>      seconds      = _TOD_Seconds_since_epoch;
>   8083f7c:	2279 0810 f4e2 	moveal 810f4e2
> <_TOD_Seconds_since_epoch>,%a1
>   8083f82:	2f49 0004      	movel %a1,%sp@(4)
>      microseconds = _TOD_Current.ticks;
>   8083f86:	2eb9 0810 f5c6 	movel 810f5c6 <_TOD_Current+0x18>,%sp@
>    rtems_interrupt_enable(level);
>   8083f8c:	46c1           	movew %d1,%sr
> 
>    tp->tv_sec  = seconds + POSIX_TIME_SECONDS_1970_THROUGH_1988;
>   8083f8e:	222f 0004      	movel %sp@(4),%d1
>   8083f92:	0681 21da e500 	addil #567993600,%d1
>   8083f98:	2081           	movel %d1,%a0@
> 
> 
> I hope this helps.
> 
> regards,
> 
> Ian Caddy
> 
> 
> 
> 
> 
> Chris Johns wrote:
>> Joel Sherrill wrote:
>>> The seconds and nanoseconds values are grabbed/computed with 
>>> interrupts disabled so I don't think that would be the problem.
>> No they are not and it is the problem. The interrupts are being 
>> enable
> 
>> just after they are being disabled. Here is the code:
>>
>> 00000000 <gettimeofday>:
>> gettimeofday():
>> ../../../../../head/cpukit/libcsupport/src/__gettod.c:50
>>    0:   4e56 0000       linkw %fp,#0
>>    4:   206e 0008       moveal %fp@(8),%a0
>> ../../../../../head/cpukit/libcsupport/src/__gettod.c:55
>>    8:   4a88            tstl %a0
>>    a:   6736            beqs 42 <gettimeofday+0x42>
>> ../../../../../head/cpukit/libcsupport/src/__gettod.c:67
>>    c:   203c 0000 0700  movel #1792,%d0
>>   12:   2200            movel %d0,%d1
>>   14:   40c0            movew %sr,%d0
>>   16:   8280            orl %d0,%d1
>>   18:   46c1            movew %d1,%sr
>>                         ^^^^^^^^^^^^^ DISABLED 
>> ../../../../../head/cpukit/libcsupport/src/__gettod.c:70
>>   1a:   46c0            movew %d0,%sr
>>                         ^^^^^^^^^^^^^ ENABLED
>>
>> The remainder of the code is the reading of the time:
>>
>> ../../../../../head/cpukit/libcsupport/src/__gettod.c:72
>>   1c:   2039 0000 0000  movel 0 <gettimeofday>,%d0
>>   22:   0680 21da e500  addil #567993600,%d0
>>   28:   2080            movel %d0,%a0@
>> ../../../../../head/cpukit/libcsupport/src/__gettod.c:73
>>   2a:   2239 0000 0000  movel 0 <gettimeofday>,%d1
>>   30:   2039 0000 0000  movel 0 <gettimeofday>,%d0
>>   36:   4c00 1800       mulsl %d0,%d1
>>   3a:   2141 0004       movel %d1,%a0@(4)
>>   3e:   4280            clrl %d0
>>   40:   6010            bras 52 <gettimeofday+0x52>
>> ../../../../../head/cpukit/libcsupport/src/__gettod.c:56
>>   42:   4eb9 0000 0000  jsr 0 <gettimeofday>
>>   48:   2040            moveal %d0,%a0
>>   4a:   20bc 0000 000e  movel #14,%a0@
>>   50:   70ff            moveq #-1,%d0
>>   52:   4e5e            unlk %fp
>>   54:   4e75            rts
>>
>> I wonder where else in the kernel this is happening and to which 
>> other
> 
>> targets. I see the i386 also has problems. In the i386 only one value

>> is being read with interrupts masked:
>>
>>   rtems_interrupt_disable(level);
>>    c:   9c                      pushf
>>    d:   fa                      cli
>>    e:   58                      pop    %eax
>>  cpukit/libcsupport/src/__gettod.c:69
>>     seconds      = _TOD_Seconds_since_epoch;
>>     microseconds = _TOD_Current.ticks;
>>    f:   8b 15 18 00 00 00       mov    0x18,%edx
>>                                 ^^^^^^^^^^^^^^^^ ONLY ONE READ 
>> cpukit/libcsupport/src/__gettod.c:70
>>   rtems_interrupt_enable(level);
>>   15:   50                      push   %eax
>>   16:   9d                      popf
>> cpukit/libcsupport/src/__gettod.c:72
>>   tp->tv_sec  = seconds + POSIX_TIME_SECONDS_1970_THROUGH_1988;
>>   17:   a1 00 00 00 00          mov    0x0,%eax
>>                                 ^^^^^^^^^^^^^^^ THE OTHER READ
>>   1c:   05 00 e5 da 21          add    $0x21dae500,%eax
>>   21:   89 01                   mov    %eax,(%ecx)
>>
>>> The asm volatile statements
>>> should provide sync points for the compiler if a partial read of 
>>> memory were an issue.
>> If this is the case it would seem we have a problem with our asm 
>> statements or the compiler has a bug.
>>
>> Regards
>> Chris
>>
> 
> --
> Ian Caddy
> Goanna Technologies Pty Ltd
> +61 8 9221 1860
> 
> 
> 

--
Ian Caddy
Goanna Technologies Pty Ltd
+61 8 9221 1860