Date: Thu, 19 Dec 2013 22:15:54 +0100 From: Stefan Esser <se@freebsd.org> To: Andreas Tobler <andreast@FreeBSD.org>, Bruce Evans <brde@optusnet.com.au> Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org Subject: Re: svn commit: r259609 - head/sys/kern Message-ID: <52B3620A.8050603@freebsd.org> In-Reply-To: <52B35B37.5070809@FreeBSD.org> References: <201312190901.rBJ91ko3036881@svn.freebsd.org> <20131219204903.V24189@besplex.bde.org> <52B32647.2030008@freebsd.org> <52B35B37.5070809@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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 :(
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?52B3620A.8050603>