gettimeofday seconds rollover problem?

[Ian Caddy]

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