Date: Sun, 28 Aug 2016 18:49:45 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Garrett Cooper <ngie@freebsd.org> 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 Message-ID: <20160828174315.E1696@besplex.bde.org> In-Reply-To: <201608280710.u7S7Am7X057748@repo.freebsd.org> References: <201608280710.u7S7Am7X057748@repo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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. <sys/acct.h> now
devfines AHZV1 and this has value 64. This is for a legacy API. Not
very compatible.
This file doesn't include <sys/acct.h> 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20160828174315.E1696>