svn commit: r259609 - head/sys/kern

[Stefan Esser]

Am 19.12.2013 21:46, schrieb Andreas Tobler:
> On 19.12.13 18:00, Stefan Esser wrote:
> 
>> I'd replace the two occurances of LLONG_MAX with INT64_MAX and add the
>> missing empty line:
>>
>> static __inline sbintime_t
>> timer2sbintime(intptr_t data)
>> {
>>
>>         if (data > INT64_MAX / SBT_1MS)
>>                 return INT64_MAX;
>>         return (SBT_1MS * data);
>> }
>>
>> If you can show evidence that a limit of INT64_MAX/2 is more appropriate
>> (2^30 seconds or 34 years), the limit could be of course be reduced to
>> that value.
>>
>> I could not find any code that would not tolerate INT64_MAX, though ...
> 
> Aehm, what about 32-bit systems where intptr_t == __int32_t?
> 
> cc1: warnings being treated as errors
> /export/devel/fbsd/src/sys/kern/kern_event.c: In function 'timer2sbintime':
> /export/devel/fbsd/src/sys/kern/kern_event.c:529: warning: comparison is
> always false due to limited range of data type

You are right, this needs to be fixed, too :(

I see two possibilities:

1) Conditional compilation: There is no need to check an upper bound on
   ILP32 architectures. INT32_MAX seconds can be expressed as sbintime.
   Architectures where intptr_t has at least 48 bits will support the
   maximum time that can be expressed in sbintime and the comparison
   can be left as is for them.

2) Calculate the upper bound in such a way, that it is guaranteed to be
   lower than the highest value that can be expressed as intptr_t. This
   value is (INT32_MAX - 1) on 32 bit architectures, while it remains
   (INT64_MAX / SBT_1MS) on 64 bit architectures.

I'm afraid that the warning emitted for 32 bit architectures will cause
tinderbox build failures, but I haven't seen a failure message, yet.

Should I back-out the commit that added the range check?

As long as -fno-strict-overflow is not put back into CFLAGS, the test
is not strictly required (only for the extremely unlikely case that a
number > 1000 * MAX32_INT results in an extremely short remainder after
multiplication with SBT_1MS modulo 32 ...).

Regards, STefan

NB: I should have known better and should have asked for a review of
    this change before it wa committed. Sorry for the inconvenience
    caused :(