From owner-svn-src-head@FreeBSD.ORG Tue Feb 3 16:09:09 2015 Return-Path: Delivered-To: svn-src-head@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 7878DEDD for ; Tue, 3 Feb 2015 16:09:09 +0000 (UTC) Received: from nm8-vm0.bullet.mail.bf1.yahoo.com (nm8-vm0.bullet.mail.bf1.yahoo.com [98.139.213.95]) (using TLSv1 with cipher ECDHE-RSA-RC4-SHA (128/128 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 338AD31F for ; Tue, 3 Feb 2015 16:09:09 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=yahoo.com; s=s2048; t=1422979741; bh=EGu/nK5yAQbD1nGemgWHQPvmBLgxdPMSrnJxv0jOHHc=; h=Date:From:To:CC:Subject:References:In-Reply-To:From:Subject; b=aM5HBhaGRthC1vDYFArtxdP/9zl4nPdG7KA+DvjL+SCIYpJMq0sGLXWsHRZuVFsvKdyoWbuG7MCn73LD3BP4SJ5SzOXFiEBcs4CEAWFg7eMVldooitXpfEklUcF1GGVyw31oQC4z7as1YdTgbcSsh52fpThR62aQ8PSg0M45i5A4kwgLAJY8lrLy+8nfkIhTRHUXm4Mo+BtoxjTFUzzQ5pSe52VeRuAziYbbKQg5R8vqwK3Aa0sWgOIt/rkVW6CzmK0y11n46MDcJhOUBWGH5DdiyPvsdaS8t6MEJgdgnZwVJ0pkQCUs/+u9lw5l4aOMdYV9NdQgTpxG04bB2X1npA== Received: from [98.139.215.140] by nm8.bullet.mail.bf1.yahoo.com with NNFMP; 03 Feb 2015 16:09:01 -0000 Received: from [98.139.211.198] by tm11.bullet.mail.bf1.yahoo.com with NNFMP; 03 Feb 2015 16:09:01 -0000 Received: from [127.0.0.1] by smtp207.mail.bf1.yahoo.com with NNFMP; 03 Feb 2015 16:09:01 -0000 X-Yahoo-Newman-Id: 754830.35050.bm@smtp207.mail.bf1.yahoo.com X-Yahoo-Newman-Property: ymail-3 X-YMail-OSG: HOwVN7EVM1n3urV2NC5wBcxFnnEK3f_8BXvw31lJDzOjxXU K17T7.UiitBCh80iPvI6KHS_jtBVF.ndL2eWFtv9REEslwL7cULOEztu2YP6 pkiVcvknRC4ngFMP..E1tZ8OmhuqMliKgy1ZZVRdYs1h_DD2mMY_lyLqZ33z GSBqzUNiDNxpH.vyIsmMezTNcPiaBRS9gc7YLHDFPr5s7S.QcXbJqvy2ND4F GCZz375FoXNVEQYV1EBV5qn3IzwnWNfktlqg1HJq1nz4tFjHb1GbXXywc1Gz 65mp0pyzgMuA155PVwa1_tijldEPqD7mEOjaVBoiTX1k1BpWqeHyOv2tEZd7 O8zr7.WrMg68EfzQD1xwenqTD6i85BrO4y.3N5FRcu8h.OxbP14V3zKiRSb9 pNcRxerKCbPw3kB31wSLboCr6RhRC8TvAXt5ivGDgD5EMBnJ2PIBDyDKQDfi fcnbpdu.mbIA2sto2VJR1jNlxWh7hY4UGEshx6EuD2yPGiGMP.pkaB_yj1PN AkzSFp0RPxWfRohxRGp.ycf6kthVuKNUL X-Yahoo-SMTP: xcjD0guswBAZaPPIbxpWwLcp9Unf Message-ID: <54D0F29C.2090401@FreeBSD.org> Date: Tue, 03 Feb 2015 11:09:00 -0500 From: Pedro Giffuni Organization: FreeBSD Project User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Thunderbird/31.4.0 MIME-Version: 1.0 To: Bruce Evans Subject: Re: svn commit: r278154 - head/lib/msun/src References: <201502031417.t13EHU9g074687@svn.freebsd.org> <20150204012614.T1972@besplex.bde.org> In-Reply-To: <20150204012614.T1972@besplex.bde.org> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 03 Feb 2015 16:09:09 -0000 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 >