gettimeofday seconds rollover problem?

[Till Straumann]

Joel Sherrill wrote:

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

Not sure I understand exactly what you mean. Assume a (global) variable X
that is not touched by an ISR. If some operation does

  _Thread_Disable_dispatch();
  X++;
  _Thread_Enable_dispatch();

Then, for sure, X is not modified by any ISR. However, the compiler could
still move X outside of the safe region (because the dispatch-disable/enable
ops are inlines) so it effectively looks like this

  X++
  _Thread_Disable_dispatch();
  _Thread_Enable_dispatch();

with obvious consequences if a context switch occurs while X++ is in 
progress.

T.

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