Date: Tue, 03 Feb 2015 11:09:00 -0500 From: Pedro Giffuni <pfg@FreeBSD.org> To: 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: r278154 - head/lib/msun/src Message-ID: <54D0F29C.2090401@FreeBSD.org> In-Reply-To: <20150204012614.T1972@besplex.bde.org> References: <201502031417.t13EHU9g074687@svn.freebsd.org> <20150204012614.T1972@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On 03/02/2015 10:38 a.m., Bruce Evans wrote:
> On Tue, 3 Feb 2015, Pedro F. Giffuni wrote:
>
>> Log:
>> Reduce confusion in scalbnl() family of functions.
>>
>> The changes unrelated to the bug in r277948 made
>> the code very difficult to understand to both
>> coverity and regular humans so take a step back
>> to something that is much easier to understand
>> for both and follows better the original code.
>>
>> CID: 1267992, 1267993, 1267994
>> Discussed with: kargl
>
> You mean, take a step backwards to something that is harder to
> understand.
>
> You also added style bugs (excessive parentheses) in only 1 of 3 places.
>
>> Modified: head/lib/msun/src/s_scalbln.c
>> ==============================================================================
>>
>> --- head/lib/msun/src/s_scalbln.c Tue Feb 3 13:45:06 2015 (r278153)
>> +++ head/lib/msun/src/s_scalbln.c Tue Feb 3 14:17:29 2015 (r278154)
>> @@ -35,7 +35,9 @@ scalbln (double x, long n)
>> {
>> int in;
>>
>> - in = (n > INT_MAX) ? INT_MAX : (n < INT_MIN) ? INT_MIN : n;
>
> It is a feature that this detected a bug in Coverity. I intentionally
> left out the cast of n at the end, after even more intentionally leaving
> it out at the beginning. It is obvious to humans^WC programmers, that
> n has been checked to fit in an int; thus any compiler warning about n
> possibly being truncated is a compiler defect. It is especially
> important
> that compilers understand explicit range checks and don't warn about
> conversions that cannot change the value when the range checks have been
> satisfied.
>
I noticed the implicit cast, but then the fact that you triggered a bug
in coverity,
could be an indication that you could also trigger the same bug in a
compiler
optimizer so some bogus compiler may end up optimizing the above check into
/*do nothing */.
>> + in = (int)n;
>
> This restores a hard-to understand cast. Compilers cannot reasonably
> understand this at all. Without the cast, they should (optionally)
> warn about possible truncation. But the truncation is intentional.
> The programmer is using the cast to inhibit the warning (the case has no
> other effect). A higher level of warnings would still warn about it,
> and my rewrite was to prevent this and to make the code easier to
> understand.
>
> The second point that is hard to understand for this cast is that its
> behaviour is only implementation-defined. Thus it delivers a result
> and has no bad side effects.
>
> The third point that is hard to understand about this cast is that its
> result doesn't matter. The usual result is the original n truncated,
> but it can be complete garbage. In both cases, truncation is detected
> by (in != n), and we proceed to adjust the original n in the truncated
> cases. Implementation-defined behaviour is usually much harder to
> understand than this. Some entity has to read the system documentation
> for all supported systems (and sometimes first file bug reports to get
> it written) and write ifdefs for all cases. This is hard for compilers
> to do. Here they only have to read the C standard to check that the
> bahaviour is not undefined. Even language lawyers might need to check
> this.
>
> The fourth point that is (not so) hard to understand about this cast is
> that after detecting truncation, we don't use the truncated n again to
> complete the adjustment.
>
>> + if (in != n)
>> + in = (n > 0) ? INT_MAX: INT_MIN;
>
> Instead of an idiomatic conditional ladder, this now uses a
> combination of
> an if clause and simpler conditional clause. Before this, many
> complications are introduced by the implementation-defined behaviour.
> We use the else clause to separate the truncated case from the truncated
> case. Then in the truncated case, we still use a conditional clause
> which non-C programmers might find confusing, and it is essentially the
> first part of the conditional ladder written in an obfuscated way.
> Compare:
>
> first part of ladder:
> (n > INT_MAX) ? INT_MAX : (n < INT_MIN) ? INT_MIN
> replacement:
> (n > 0) ? INT_MAX: INT_MIN
>
> (n > 0) is an obfuscated/manually microoptimized way of writing
> (n > INT_MAX). It means the same since we are in the truncated case,
> so n cannot be between 0 and INT_MAX. The negation of (n > 0) means
> (n < INT_MIN), similarly.
>
> As pointed out in my review, the range checks could be further
> unobfuscated
> to check the limits on the exponents instead of INT_MIN and INT_MAX.
> These limits are significantly smaller. Using them, the magic cast
> to int just gets in the way. Magic still occurs in the final conversion,
> and it is best to leave the final n in my version uncast so that the
> compiler can detect problems in the algorithm or implementation. The
> magic is that the natural limits are smaller than INT_MIN and INT_MAX;
> this if we restrict to them we automatically restrict to values that can
> be represented as an int. Then if we make a mistake in this, the
> compiler
> has a chance of warning provided we don't kill the warning using the
> cast.
> We are only using INT_MIN and INT_MAX in the first place since they are
> slightly easier to spell than the natural limits (but harder to spell
> hard-coded limits that should be enough for any FP format). We know by
> non-magic that larger-than necessary limits give the same results.
>
I honestly only wanted to clean the original bug but all in all, I find our
implementation of this functions somewhat disappointing: shouldn't we
be doing real calculations with the requested precision instead of wrapping
them around the int counterparts? It looks like NetBSD does implement them.
>> return (scalbn(x, in));
>> }
>>
>
> Similarly in the other functions, except scalblnl() has excessive braces.
>
Oh yes, I will fix that.
> Bruce
>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?54D0F29C.2090401>