svn commit: r304947 - stable/11/tests/sys/kern/acct

[Ngie Cooper]

> On Aug 28, 2016, at 01:49, Bruce Evans <brde at optusnet.com.au> wrote:

...

I agree. This commit in effect papered over the problem. More investigation will be done with the PR that introduced the expected failure.

Thanks!
-Ngie
> 
> 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