From owner-svn-src-all@freebsd.org Sun Aug 28 08:49:57 2016 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 34E5CB778CD; Sun, 28 Aug 2016 08:49:57 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail106.syd.optusnet.com.au (mail106.syd.optusnet.com.au [211.29.132.42]) by mx1.freebsd.org (Postfix) with ESMTP id D9DB4779; Sun, 28 Aug 2016 08:49:56 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from c122-106-149-109.carlnfd1.nsw.optusnet.com.au (c122-106-149-109.carlnfd1.nsw.optusnet.com.au [122.106.149.109]) by mail106.syd.optusnet.com.au (Postfix) with ESMTPS id 913863C82ED; Sun, 28 Aug 2016 18:49:46 +1000 (AEST) Date: Sun, 28 Aug 2016 18:49:45 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Garrett Cooper cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-stable@freebsd.org, svn-src-stable-11@freebsd.org Subject: Re: svn commit: r304947 - stable/11/tests/sys/kern/acct In-Reply-To: <201608280710.u7S7Am7X057748@repo.freebsd.org> Message-ID: <20160828174315.E1696@besplex.bde.org> References: <201608280710.u7S7Am7X057748@repo.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.1 cv=CoZCCSMD c=1 sm=1 tr=0 a=R/f3m204ZbWUO/0rwPSMPw==:117 a=L9H7d07YOLsA:10 a=9cW_t1CCXrUA:10 a=s5jvgZ67dGcA:10 a=kj9zAlcOel0A:10 a=E_Nap1b-OaeuWiuqp0EA:9 a=-WCYPtOOOc7DIrD1:21 a=M3j2CLTmEwHoSjr9:21 a=CjuIK1q_8ugA:10 X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 28 Aug 2016 08:49:57 -0000 On Sun, 28 Aug 2016, Garrett Cooper wrote: > Log: > MFC r304238: > > Only expect :encode_tv_random_million to fail on 64-bit platforms > > It passes on i386 This can't depend on 64-bitness. It might depend on FLT_EPSILON, but IEEE might require a specific representation of floats and we only have and only support one. This is probably a bug in the tests that shows up on arches with extra precision. Perhaps just a complier bug. > Modified: stable/11/tests/sys/kern/acct/acct_test.c > ============================================================================== > --- stable/11/tests/sys/kern/acct/acct_test.c Sun Aug 28 07:09:45 2016 (r304946) > +++ stable/11/tests/sys/kern/acct/acct_test.c Sun Aug 28 07:10:48 2016 (r304947) > @@ -204,7 +204,10 @@ ATF_TC_BODY(encode_tv_random_million, tc > struct timeval tv; > long k; > > - atf_tc_expect_fail("the testcase violates FLT_EPSILON"); > +#ifdef __LP64__ > + atf_tc_expect_fail("the testcase violates FLT_EPSILON on 64-bit " > + "platforms, e.g. amd64"); > +#endif > > ATF_REQUIRE_MSG(unsetenv("TZ") == 0, "unsetting TZ failed; errno=%d", errno); The rest of the function is: X for (k = 1; k < 1000000L; k++) { X tv.tv_sec = random(); X tv.tv_usec = (random() % 1000000L); X v.c = encode_timeval(tv); X check_result(atf_tc_get_ident(tc), X (float)tv.tv_sec * AHZ + tv.tv_usec, v); X } AHZ here is less than an obfuscation of literal 10000000 or just 1e6 or 1e6F. It doesn't even have the style bug of an L suffix like the nearby literals. Types are important here, but the L isn't. AHZ used to be a constant related to fixed-point conversions in acct.h. It used to have value 1000. Note much like the AHZ. now devfines AHZV1 and this has value 64. This is for a legacy API. Not very compatible. This file doesn't include except possibly via namespace pollution, so it doesn't get any AHZ*. It only uses AHZ to convert tv_sec to usec. This was magical and may be broken. The file convert.c is included. This is a converted copy of kern_acct.c. Back when AHZ existed in acct.h, kern_acct.c used to use AHZ with its different value. I don't see how overriding that value either didn't cause a redefinition error or inconsistencies. Now kern_acct.c doesn't use either AHZ* so this definition is not magical. So AHZ should be replaced by literal 1000000 except possibly for type problems. IIRC, C99 specifies the dubious behaviour of converting integers to float in float expressions to support the dubious behaviour of evaluating float expressions in float precision. 1000000 is even exactly representable in float precision. But the result of the mutliplication isn't in general. Adding a small tv_usec to a not very large tv_sec converted to usec is almost certain to not be representable in a 32-bit float after a few random choices. So we expect innacuracies. The float expression may be evaluated in extra precision, and is on i386. So we expect smaller inaccuracies on i386. It is not clear if remaining bugs are in the test or the compiler. Probably both. The test asks for inaccuracies and gets them in the expression sometimes. It doesn't try to force the inaccuracies by casting to float, and only C90+ compilers do this cast as specified since the specification specifies behaviour that is too pessimal to use. C90+ compilers are in short supply, but gcc later than aout 4.6 properlay pessimizes the cast when instructed to by certain compiler flags. But the test it calls a function which should do the conversion. It takes excessive inlining combined with the de-pessimization to not do the conversion. Apparently, clang does do the excessive inlining. Otherwise the result would be the same on i386 as on amd64. The test seems to be quite broken. encode_timeval() does some conversion which is presumably correct. We calculate a value in usec to compare against. This is only done in float precision (possibly higher, but we don't control this). We expect a relative error of about FLT_EPSILON in this. Later we convert to a relative error, so this is only slightly broken. encode_timeval() must have a rounding error, and our calculation has one and the scaling has more. So we should expect errors of several times FLT_EPSILON. So the test only seems to be slightly broken. Strictly less than FLT_EPSILON is too strict if the calculations are actually done in float precision since it is too difficult to calculate the reference and do the scaling without increasing the error. The worst case for the reference is tv_sec = 2**31-1 (31 bits) and tv_usec = 999999 (20 bits). That is exactly representable in 53 bits (double precision) so we should use that. Bruce