Is it dangerous to rely on overflow?

Question

The following is a delay function I found in our firmware. It looks a little dangerous or, at the least, confusing to the reader.

Global Variable

static int32u masterTimerCounter; // 32-bit unsigned timer counter

System Tick Interrupt Handler

/* Called Every Millisecond */
void sysTickIrqHandler(void)
{
    masterTimerCounter++;
}

Set Timer Expiration Function

void setTimerExpiration(int32u *timerExpiration, int32u delay)
{
    *timerExpiration = masterTimerCounter + delay;
}

Check If Timer Expired Function

boolean timerExpired(int32u timerExpiration, int32u delay)
{
    if((masterTimerCounter - timerExpiration) < delay)
        return TRUE; // Timer has expired
    else
        return FALSE; // Timer still active
}

Set Timer Expriation And Block Until Timer Expired

int32u timerExpiration;
setTimerExpiration(&timerExpiration, 15); // Set expiration timer to 15 milliseconds
while(!timerExpired(timerExpiration, 15) // Block until timer has expired
    continue; 

Question

As you can see in timerExpired(), masterTimerCounter is subtracted by timerExpiration. If the timer hasn't expired yet, the computation will result in a very large number (because both of the operands are unsigned numbers). When the timer has expired the computation will result in a value less than the delay amount.

Though this seems to work fine, it seems like it can be dangerous or, at the least, be confusing to the reader (I had to read it several times to understand the original programmer's intent).

If I had to write something similar to this, I would define the timerExpired function as follows:

boolean timerExpired(int32u timerExpiration)
{
    if(timerExpiration > masterTimerCounter)
        return FALSE; // Timer still active
    else
        return TRUE; // Timer has expired
}

Should I redefine 'timerExpired()`?

Note: Function and variable names have been changed to protect the innocent.

Answer 1

The issue with your way is that if masterTimerCounter + delay causes a rollover of the 32 bit int, than the timerExpired test passes right away.

I think the most straightforward way to do integer timers in the presence of possible rollover is like this:

void startTimer(int32u *timerValue)
{
    *timerValue = masterTimerCounter;
}

Check If Timer Expired Function

boolean timerExpired(int32u timerVal, int32u delay)
{
    if ((masterTimerCounter - timerVal) >= delay)
        return TRUE; // Timer has expired
    else
        return FALSE; // Timer still active
}

Usage:

int32u timer;
startTimer(&timer); // Start timing
while(!timerExpired(timer, 15) // Block for 15 ticks
    continue;

Even if the subtraction in timerExpired underflows this returns the correct results.