svn commit: r278154 - head/lib/msun/src

[Pedro Giffuni]

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
>