gettimeofday seconds rollover problem?

[Joel Sherrill]

Till Straumann wrote:

> Eric Norum wrote:
>
>> On Feb 24, 2006, at 1:16 AM, Andrew Sinclair wrote:
>>
>>>
>>> 1) interesting...
>>> 2) I think an alternative here is adding all the registers to the
>>> clobber list in the "barrier", we can force the compiler to reload any
>>> data into registers, and perform stores within the critical sections.
>>>
>>> 3) Yes, on second thoughts I think you have something here. It is  
>>> safer
>>> as one single asm volatile statement.
>>>
>>>
>>> For example on the Coldfire...
>>>
>>> #define m68k_disable_interrupts( _level ) \
>>>     do { register unsigned32   _tmpsr = 0x0700; \
>>>          asm __volatile__ ( "move.w %%sr,%0\n\t" \
>>>                  "or.l   %0,%1\n\t" \
>>>                  "move.w %1,%%sr" \
>>>                  : "=d" (_level), "=d"(_tmpsr) : "1"(_tmpsr) \
>>>                  :"cc","d0","d1","d2","d3","d4","d5", \
>>>                 "a0","a1","a2","a3","a4","a5","a6","memory" ); \
>>>     } while( 0 )
>>>
>>> #define m68k_enable_interrupts( _level ) \
>>>     do { asm __volatile__ ( "move.w  %0,%%sr " : : "d" (_level) \
>>>        :"cc","d0","d1","d2","d3","d4","d5", \
>>>        "a0","a1","a2","a3","a4","a5","a6","memory" ); \
>>>     } while( 0 )
>>>
>>> #define m68k_flash_interrupts( _level ) \
>>>    do { register unsigned32 _tmpsr = 0x0700; \
>>>         asm __volatile__ ( "move.w %2,%%sr\n\t" \
>>>                "or.l   %2,%1\n\t" \
>>>                "move.w %1,%%sr" \
>>>                : "=d"(_tmpsr) : "0"(_tmpsr), "d"(_level) \
>>>                 :"cc","d0","d1","d2","d3","d4","d5", \
>>>                 "a0","a1","a2","a3","a4","a5","a6","memory" ); \
>>>    } while( 0 )
>>
>>
>>
>> I *really* don't like this.
>> Given that the assembly instructions in question don't affect d0-d5/ 
>> a0-a6 why would you want to place them in the clobber list?
>>
>> To summarize my position:
>> 1) The _TOD_* variables must be declared as volatile since their  
>> values can change in a fashion (Deus ex machina) not apparent to  
>> compiler.
>> 2) The M68K_BARRIER macro, or its equivalent addition to the enable/ 
>> disable macros, is required to keep the optimizer from hoisting/ 
>> sinking any code beyond the interrupt disable/enable operations.   
>> My  feeling is that it's clearer and simpler to just add the 
>> "memory",  "cc" to the disable/enable macros, but I'm not dogmatic 
>> about this.
>>
>> These two steps seem very clear to me since they impart to the  
>> compiler exactly the information needed.  Both steps are necessary
>
>
> This is not true. Only the 'volatile' declaration of global variables
> that are accessed during any kind of 'inlined' protection mechanism
> (be it interrupts, thread-dispatching, mutex, ...)  is necessary
> (and sufficient).
>
> Nobody has addressed my point # 1: adding a memory barrier
> to rtems_interrupt_disable/enable does not address 
> thread-dispatch-disable
> protected code. If we're going to add this safe-guard, it should
> be introduced to thread_dispatch_disable/enable, too.
>
Nearly every operation in RTEMS is protected by one of those mechanisms.
I think you are only concerned with asynchronous modifications relative to
the current thread of execution.  That should mean only variables modified
during an interrupt handler.  This would eliminate _Thread_Executing which
is only modified during startup and in _Thread_Dispatch.  That should
narrow it down to mostly:

_Thread_Heir
_Context_Switch_necessary
_Thread_Dispatch_disable_level
priority bit maps
whatever clock tick hits

>
> BTW: I don't think we should modify all the ports! 

Well I for one am glad to hear a solution that doesn't involve mucking 
around
with each port. :)

> We should just
>      change score/isr.h:
>
>      #define _ISR_Disable( _level ) \
>              _CPU_ISR_Disable( _level ) \
>              asm volatile("":::"memory")
>
>      #define _ISR_Enable( _level ) \
>              asm volatile("":::"memory") \
>              _CPU_ISR_Enable()
>
>      #define _ISR_Flash( _level ) \
>              asm volatile("":::"memory") \
>              _CPU_ISR_Flash( _level ) \
>              asm volatile("":::"memory")
>
> _Thread_Disable_dispatch() should be treated alike.
>
> -- Till
>
>>  and together they should be sufficient.
>>
>
>
>
>