gettimeofday seconds rollover problem?

Thu Feb 23 20:33:10 UTC 2006

Till Straumann wrote:

> One moment, folks -- I'm not sure the "memory" approach is best
> for all architectures. On CPUs with many registers, making
> rtems_interrupt_enable()/disable a real subroutine might be cheaper.
>
> In any case, I believe that declaring the relevant global variables
> 'volatile' is the correct way to deal with this problem.
>
> The standard says:
>
> "An object that has volatile-qualified type may be modified in ways 
> unknown to the
> implementation or have other unknown side effects. Therefore any 
> expression referring
> to such an object shall be evaluated strictly according to the rules 
> of the abstract machine,
> as described in 5.1.2.3. Furthermore, at every sequence point the 
> value last stored in the
> object shall agree with that prescribed by the abstract machine, 
> except as modified by the
> unknown factors mentioned previously.114) What constitutes an access 
> to an object that
> has volatile-qualified type is implementation-defined.
>
> -----114) A volatile declaration may be used to describe an object 
> corresponding to a memory-mapped
> input/output port or an object accessed by an asynchronously 
> interrupting function. Actions on
> objects so declared shall not be ‘‘optimized out’’ by an 
> implementation or reordered except as
> permitted by the rules for evaluating expressions."
>
> Seems exactly what we want and indeed, while the powerpc port 
> (gcc-4.0.2) currently
> also exhibits the problem, when recompiled with both _TOD_Current and 
> _TOD_Seconts_since_epoch
> declared 'volatile' the respective read operations are not moved out 
> of the protected
> section anymore.
>
> -- Till
>
> BTW: I also checked m68k/coldfire/gcc-4.0.2 -- declaring the relevant 
> globals 'volatile' produces
> correctly protected code.
>

Does anyone object to this solution? 

Is there any other variable inside RTEMS that should be volatile.  I 
have a sick feeling that there are
a handful that may need it.  At the top of my list would be 
_Thread_Executing and _Thread_Heir and
a few others that are related. 

Till, can you check the generated code for this code from the top of 
_Thread_Dispatch?  It may hint
that more  volatiles are needed.

  _ISR_Disable( level );
  while ( _Context_Switch_necessary == TRUE ) {
    heir = _Thread_Heir;
    _Thread_Dispatch_disable_level = 1;
    _Context_Switch_necessary = FALSE;
    _Thread_Executing = heir;
    executing->rtems_ada_self = rtems_ada_self;
    rtems_ada_self = heir->rtems_ada_self;
    if ( heir->budget_algorithm == 
THREAD_CPU_BUDGET_ALGORITHM_RESET_TIMESLICE )
      heir->cpu_time_budget = _Thread_Ticks_per_timeslice;
    _ISR_Enable( level );

BTW: I also checked m68k/coldfire/gcc-4.0.2 -- declaring the relevant 
globals 'volatile' produces
correctly protected code.

Once this gets settled, it needs a patch. :)

--joel

> Eric Norum wrote:
>
>> Ack!
>> Looks like gcc really is getting aggressive.
>>
>> Here's the code for a ColdFire MCF5282:
>>
>> (gdb) disassem gettimeofday
>> Dump of assembler code for function gettimeofday:
>> 0x00127d8c <gettimeofday+0>: moveal %sp@(4),%a0
>> 0x00127d90 <gettimeofday+4>: tstl %a0
>> 0x00127d92 <gettimeofday+6>: beqs 0x127dca <gettimeofday+62>
>> 0x00127d94 <gettimeofday+8>: movel #1792,%d0
>> 0x00127d9a <gettimeofday+14>: movel %d0,%d1
>> 0x00127d9c <gettimeofday+16>: movew %sr,%d0
>> 0x00127d9e <gettimeofday+18>: orl %d0,%d1
>> 0x00127da0 <gettimeofday+20>: movew %d1,%sr
>> 0x00127da2 <gettimeofday+22>: movel 0x219f22 <_TOD_Current+24>,%d1
>> 0x00127da8 <gettimeofday+28>: movew %d0,%sr
>> 0x00127daa <gettimeofday+30>: movel 0x219e46 
>> <_TOD_Seconds_since_epoch>,%d0
>> 0x00127db0 <gettimeofday+36>: addil #567993600,%d0
>> 0x00127db6 <gettimeofday+42>: movel %d0,%a0@
>> 0x00127db8 <gettimeofday+44>: movel 0x219f72 
>> <_TOD_Microseconds_per_tick>,%d0
>> 0x00127dbe <gettimeofday+50>: mulsl %d1,%d0
>> 0x00127dc2 <gettimeofday+54>: movel %d0,%a0@(4)
>> 0x00127dc6 <gettimeofday+58>: clrl %d0
>> 0x00127dc8 <gettimeofday+60>: rts
>>
>>
>> As you can see, the 'ticks' value is read with interrupts disabled, 
>> but the 'seconds since epoch' is not.
>>
>>
>> norume at pyramidion 221> m68k-rtems4.7-gcc --version
>> m68k-rtems4.7-gcc (GCC) 4.0.2
>>
>>
>>
>> I made the following change:
>> Index: cpukit/score/cpu/m68k/rtems/score/m68k.h
>> ===================================================================
>> RCS file: /usr1/CVS/rtems/cpukit/score/cpu/m68k/rtems/score/m68k.h,v
>> retrieving revision 1.24
>> diff -u -r1.24 m68k.h
>> --- cpukit/score/cpu/m68k/rtems/score/m68k.h 7 Jun 2005 16:44:18 
>> -0000 1.24
>> +++ cpukit/score/cpu/m68k/rtems/score/m68k.h 23 Feb 2006 16:29:19 -0000
>> @@ -280,17 +280,19 @@
>> asm volatile ( "move.w %%sr,%0\n\t" \
>> "or.l %0,%1\n\t" \
>> "move.w %1,%%sr" \
>> - : "=d" (_level), "=d"(_tmpsr) : "1"(_tmpsr) ); \
>> + : "=d" (_level), "=d"(_tmpsr) : "1"(_tmpsr) \
>> + : "memory", "cc" ); \
>> } while( 0 )
>> #else
>> #define m68k_disable_interrupts( _level ) \
>> asm volatile ( "move.w %%sr,%0\n\t" \
>> "or.w #0x0700,%%sr" \
>> - : "=d" (_level))
>> + : "=d" (_level) \
>> + : : "memory", "cc" )
>> #endif
>> #define m68k_enable_interrupts( _level ) \
>> - asm volatile ( "move.w %0,%%sr " : : "d" (_level));
>> + asm volatile ( "move.w %0,%%sr " : : "d" (_level) : "memory", "cc");
>> #if ( M68K_COLDFIRE_ARCH == 1 )
>> #define m68k_flash_interrupts( _level ) \
>> @@ -298,13 +300,15 @@
>> asm volatile ( "move.w %2,%%sr\n\t" \
>> "or.l %2,%1\n\t" \
>> "move.w %1,%%sr" \
>> - : "=d"(_tmpsr) : "0"(_tmpsr), "d"(_level) ); \
>> + : "=d"(_tmpsr) : "0"(_tmpsr), "d"(_level) \
>> + : "memory", "cc"); \
>> } while( 0 )
>> #else
>> #define m68k_flash_interrupts( _level ) \
>> asm volatile ( "move.w %0,%%sr\n\t" \
>> "or.w #0x0700,%%sr" \
>> - : : "d" (_level))
>> + : : "d" (_level) \
>> + : "memory", "cc" )
>> #endif
>> #define m68k_get_interrupt_level( _level ) \
>>
>>
>>
>> Now the code looks good:
>>
>> (gdb) disassem gettimeofday
>> Dump of assembler code for function gettimeofday:
>> 0x00127d8c <gettimeofday+0>: movel %d2,%sp at -
>> 0x00127d8e <gettimeofday+2>: moveal %sp@(8),%a0
>> 0x00127d92 <gettimeofday+6>: tstl %a0
>> 0x00127d94 <gettimeofday+8>: beqs 0x127dcc <gettimeofday+64>
>> 0x00127d96 <gettimeofday+10>: movel #1792,%d0
>> 0x00127d9c <gettimeofday+16>: movel %d0,%d1
>> 0x00127d9e <gettimeofday+18>: movew %sr,%d0
>> 0x00127da0 <gettimeofday+20>: orl %d0,%d1
>> 0x00127da2 <gettimeofday+22>: movew %d1,%sr
>> 0x00127da4 <gettimeofday+24>: movel 0x219e06 
>> <_TOD_Seconds_since_epoch>,%d1
>> 0x00127daa <gettimeofday+30>: movel 0x219ee2 <_TOD_Current+24>,%d2
>> 0x00127db0 <gettimeofday+36>: movew %d0,%sr
>> 0x00127db2 <gettimeofday+38>: addil #567993600,%d1
>> 0x00127db8 <gettimeofday+44>: movel %d1,%a0@
>> 0x00127dba <gettimeofday+46>: movel 0x219f32 
>> <_TOD_Microseconds_per_tick>,%d0
>> 0x00127dc0 <gettimeofday+52>: mulsl %d2,%d0
>> 0x00127dc4 <gettimeofday+56>: movel %d0,%a0@(4)
>> 0x00127dc8 <gettimeofday+60>: clrl %d0
>> 0x00127dca <gettimeofday+62>: bras 0x127ddc <gettimeofday+80>
>> 0x00127dcc <gettimeofday+64>: jsr 0x185270 <__errno>
>> 0x00127dd2 <gettimeofday+70>: moveal %d0,%a0
>> 0x00127dd4 <gettimeofday+72>: movel #14,%a0@
>> 0x00127dda <gettimeofday+78>: moveq #-1,%d0
>> 0x00127ddc <gettimeofday+80>: movel %sp at +,%d2
>> 0x00127dde <gettimeofday+82>: rts
>>
>>
>