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

[Ngie Cooper]

> On Aug 28, 2016, at 01:49, Bruce Evans  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.cSun Aug 28 07:09:45 2016
>> (r304946)
>> +++ stable/11/tests/sys/kern/acct/acct_test.cSun 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:
> 
> Xfor (k = 1; k < 100L; k++) {
> Xtv.tv_sec = random();
> Xtv.tv_usec = (random() % 100L);
> Xv.c = encode_timeval(tv);
> Xcheck_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 1000 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 100 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.  100 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 = 99
> (20 bits).  That is exactly representable in 53 bits (double precision)
> so we 

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

[Bruce Evans]

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 < 100L; k++) {
X   tv.tv_sec = random();
X   tv.tv_usec = (random() % 100L);
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 1000 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 100 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.  100 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 = 99
(20 bits).  That is exactly representable in 53 bits (double precision)
so we should use that.

Bruce
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to 

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

[Garrett Cooper]

Author: ngie
Date: Sun Aug 28 07:10:48 2016
New Revision: 304947
URL: https://svnweb.freebsd.org/changeset/base/304947

Log:
  MFC r304238:
  
  Only expect :encode_tv_random_million to fail on 64-bit platforms
  
  It passes on i386

Modified:
  stable/11/tests/sys/kern/acct/acct_test.c
Directory Properties:
  stable/11/   (props changed)

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);
 
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"