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>