gettimeofday seconds rollover problem?

[Till Straumann]

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.

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
>
>