Date: Sat, 3 Sep 2016 17:04:43 -0700 From: Ngie Cooper <yaneurabeya@gmail.com> To: Bruce Evans <brde@optusnet.com.au> Cc: Garrett Cooper <ngie@freebsd.org>, 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: <FE62A5F9-3528-452A-A30C-0F27E5A3CBBA@gmail.com> In-Reply-To: <20160828174315.E1696@besplex.bde.org> References: <201608280710.u7S7Am7X057748@repo.freebsd.org> <20160828174315.E1696@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
> On Aug 28, 2016, at 01:49, Bruce Evans <brde@optusnet.com.au> wrote:
...
I agree. This commit in effect papered over the problem. More investigation w=
ill be done with the PR that introduced the expected failure.
Thanks!
-Ngie
>=20
> 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.
>=20
> This is probably a bug in the tests that shows up on arches with extra
> precision. Perhaps just a complier bug.
>=20
>> Modified: stable/11/tests/sys/kern/acct/acct_test.c
>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D
>> --- 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;
>>=20
>> - 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
>>=20
>> ATF_REQUIRE_MSG(unsetenv("TZ") =3D=3D 0, "unsetting TZ failed; errno=3D=
%d", errno);
>=20
> The rest of the function is:
>=20
> X for (k =3D 1; k < 1000000L; k++) {
> X tv.tv_sec =3D random();
> X tv.tv_usec =3D (random() % 1000000L);
> X v.c =3D encode_timeval(tv);
> X check_result(atf_tc_get_ident(tc),
> X (float)tv.tv_sec * AHZ + tv.tv_usec, v);
> X }
>=20
> 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.
>=20
> 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.
>=20
> 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.
>=20
> 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.
>=20
> The float expression may be evaluated in extra precision, and is on
> i386. So we expect smaller inaccuracies on i386.
>=20
> 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.
>=20
> 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.
>=20
> 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 =3D 2**31-1 (31 bits) and tv_usec =3D 999999
> (20 bits). That is exactly representable in 53 bits (double precision)
> so we should use that.
>=20
> Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?FE62A5F9-3528-452A-A30C-0F27E5A3CBBA>