Clean 'make check' against gcc -fsanitize=address,undefined

[Richard Kettlewell]

On 23/06/2015 21:40, Julien ÉLIE wrote:
> Hi Richard,
> 
>> Make sure timer arithmetic is done in unsigned types
>>
>> Some care is required.  The divison must be done in the signed type
>> (otherwise the answer is wrong if usec<0) but any operation that
>> can overflow (+ and *) must be done in an unsigned type.
>>
>> +    long usec, msec;
>>
>> -    now = (tv.tv_sec - base.tv_sec) * 1000;
>> -    now += (tv.tv_usec - base.tv_usec) / 1000;
>> +    now = (unsigned long)(tv.tv_sec - base.tv_sec) * 1000u;
>> +    usec = tv.tv_usec - base.tv_usec; /* maybe -ve */
>> +    msec = usec / 1000;               /* still maybe -ve */
>> +    now += (unsigned long)msec;
> 
> Are we obliged to use both usec and msec?
> 
> As I understand the operation, there is an implicit conversion
> of signed long to unsigned long when the numerator and the denominator
> are not both signed or both unsigned.
> 
> The current code is:
> now = (tv.tv_sec - base.tv_sec) * 1000;
> now += (tv.tv_usec - base.tv_usec) / 1000;
> 
> As now is unsigned, the result of the * of the 2 signed numbers
> is cast to unsigned in the first line.

That doesn't help.  In the current code the multiplication is done in
the signed type.  If it overflows (as it will after a few weeks, on
32-bit platforms) then we are already into undefined behavior (C99 s6.5#5).

The subsequent conversion to an unsigned type does not make any
difference - the 'working type' for the multiplication is determined by
the operands alone, not by the treatment of the result.

> The / of the 2 signed numbers is a signed number that is then added
> to now.
> I do not understand what is wrong here.  Please tell where I am wrong.
> 
> 
> 
> If I follow your suggestion with tv=2,2s and base=1,4s:
> 
> now = (unsigned long)(tv.tv_sec - base.tv_sec) * 1000u;
> usec = tv.tv_usec - base.tv_usec; /* maybe -ve */
> msec = usec / 1000; /* still maybe -ve */
> now += (unsigned long)msec;
> I believe it would result in:
> now = 1000 + (-200+UINT_MAX+1) which would overflow because
> of the negative msec being converted to unsigned, instead of:

Overflow behavior for unsigned types (including conversion of negative
values to unsigned types) is defined: the result is the residue modulo
2^n (C99 s6.2.5#9).  The result with the patch is therefore 800.

ttfn/rjk