Message ID  20191204205241.56911david.marchand@redhat.com 

State  Superseded 
Delegated to:  David Marchand 
Headers  show 
Series 

Related  show 
Context  Check  Description 

ci/travisrobot  success  Travis build: passed 
ci/iolmellanoxPerformance  success  Performance Testing PASS 
ci/ioltesting  success  Testing PASS 
ci/Intelcompilation  success  Compilation OK 
ci/iolintelPerformance  success  Performance Testing PASS 
ci/checkpatch  success  coding style OK 
David Marchand <david.marchand@redhat.com> writes: > We recently started to get random failures on the common_autotest ut with > clang on Ubuntu 16.04.6. > > Example: https://travisci.com/DPDK/dpdk/jobs/263177424 > > Wrong rte_log2_u64(0) val 0, expected ffffffff > Test Failed > > The ut passes 0 to log2() to get an expected value. > > Quoting log2 / log(3) manual: > If x is zero, then a pole error occurs, and the functions return > HUGE_VAL, HUGE_VALF, or HUGE_VALL, respectively. > > rte_log2_uXX helpers handle 0 as a special value and return 0. > Let's have dedicated tests for this case. > > Fixes: 05c4345ef5c2 ("test: add unit test for integer log2 function") > Cc: stable@dpdk.org > > Signedoffby: David Marchand <david.marchand@redhat.com> >  Ackedby: Aaron Conole <aconole@redhat.com> Somethings that concern me: 1. A log2(0) should probably be an undetermined value, but this effectively makes log2(0) == log2(1) so that if anyone uses these for some mathematical work, it will have an exceptional behavior. I know it's documented from a programmer perspective, but I am all for documenting the mathematical side effect as well. 2. Why hasn't this been complaining for so long? Or has it and we just haven't noticed? Were some compiler flags changed recently? (maybe funsafemath was always set or something?) Aaron
On Wed, Dec 4, 2019 at 10:20 PM Aaron Conole <aconole@redhat.com> wrote: > > David Marchand <david.marchand@redhat.com> writes: > > > We recently started to get random failures on the common_autotest ut with > > clang on Ubuntu 16.04.6. > > > > Example: https://travisci.com/DPDK/dpdk/jobs/263177424 > > > > Wrong rte_log2_u64(0) val 0, expected ffffffff > > Test Failed > > > > The ut passes 0 to log2() to get an expected value. > > > > Quoting log2 / log(3) manual: > > If x is zero, then a pole error occurs, and the functions return > > HUGE_VAL, HUGE_VALF, or HUGE_VALL, respectively. > > > > rte_log2_uXX helpers handle 0 as a special value and return 0. > > Let's have dedicated tests for this case. > > > > Fixes: 05c4345ef5c2 ("test: add unit test for integer log2 function") > > Cc: stable@dpdk.org > > > > Signedoffby: David Marchand <david.marchand@redhat.com> > >  > > Ackedby: Aaron Conole <aconole@redhat.com> > > Somethings that concern me: > > 1. A log2(0) should probably be an undetermined value, but this > effectively makes log2(0) == log2(1) so that if anyone uses these > for some mathematical work, it will have an exceptional behavior. I > know it's documented from a programmer perspective, but I am all for > documenting the mathematical side effect as well. What do you have in mind, adding a big warning in the doxygen "THIS IS NOT REAL MATH STUFF" ? :) > > 2. Why hasn't this been complaining for so long? Or has it and we just > haven't noticed? Were some compiler flags changed recently? (maybe > funsafemath was always set or something?) Yes, I wondered too how we did not see this earlier. Even now, it happens randomly. libc log2(0) is not undefined itself, it returns infinity. Looking at the test code, ceilf (I would expect ceil) returns infinity when getting it passed. So I'd say the problem lies in the cast to uint32_t. Both gcc and clang do not seem to bother with standard compilation flags, and the cast gives 0 on my RHEL. #include <stdio.h> #include <inttypes.h> #include <math.h> int main(int argc, char *argv[]) { printf("%lf %f %"PRIu32"\n", log2(0), (float)log2(0), (uint32_t)log2(0)); return 0; } $ ./log2 inf inf 0 Now, if I use UBSAN with clang, I get a complaint at runtime: $ ./log2 (/home/dmarchan/log2+0x41dfa5): runtime error: value inf is outside the range of representable values of type 'unsigned int' inf inf 0 Not sure if it explains the random failures, but won't undefined behavior eat your babies?
David Marchand <david.marchand@redhat.com> writes: > On Wed, Dec 4, 2019 at 10:20 PM Aaron Conole <aconole@redhat.com> wrote: >> >> David Marchand <david.marchand@redhat.com> writes: >> >> > We recently started to get random failures on the common_autotest ut with >> > clang on Ubuntu 16.04.6. >> > >> > Example: https://travisci.com/DPDK/dpdk/jobs/263177424 >> > >> > Wrong rte_log2_u64(0) val 0, expected ffffffff >> > Test Failed >> > >> > The ut passes 0 to log2() to get an expected value. >> > >> > Quoting log2 / log(3) manual: >> > If x is zero, then a pole error occurs, and the functions return >> > HUGE_VAL, HUGE_VALF, or HUGE_VALL, respectively. >> > >> > rte_log2_uXX helpers handle 0 as a special value and return 0. >> > Let's have dedicated tests for this case. >> > >> > Fixes: 05c4345ef5c2 ("test: add unit test for integer log2 function") >> > Cc: stable@dpdk.org >> > >> > Signedoffby: David Marchand <david.marchand@redhat.com> >> >  >> >> Ackedby: Aaron Conole <aconole@redhat.com> >> >> Somethings that concern me: >> >> 1. A log2(0) should probably be an undetermined value, but this >> effectively makes log2(0) == log2(1) so that if anyone uses these >> for some mathematical work, it will have an exceptional behavior. I >> know it's documented from a programmer perspective, but I am all for >> documenting the mathematical side effect as well. > > What do you have in mind, adding a big warning in the doxygen "THIS IS > NOT REAL MATH STUFF" ? :) Is such a warning not reasonable? :) >> >> 2. Why hasn't this been complaining for so long? Or has it and we just >> haven't noticed? Were some compiler flags changed recently? (maybe >> funsafemath was always set or something?) > > Yes, I wondered too how we did not see this earlier. > Even now, it happens randomly. > > libc log2(0) is not undefined itself, it returns infinity. > Looking at the test code, ceilf (I would expect ceil) returns > infinity when getting it passed. > So I'd say the problem lies in the cast to uint32_t. > > Both gcc and clang do not seem to bother with standard compilation > flags, and the cast gives 0 on my RHEL. > > #include <stdio.h> > #include <inttypes.h> > #include <math.h> > > int main(int argc, char *argv[]) > { > printf("%lf %f %"PRIu32"\n", log2(0), (float)log2(0), (uint32_t)log2(0)); > return 0; > } > > $ ./log2 > inf inf 0 > > > Now, if I use UBSAN with clang, I get a complaint at runtime: > $ ./log2 > (/home/dmarchan/log2+0x41dfa5): runtime error: value inf is outside > the range of representable values of type 'unsigned int' > inf inf 0 > > Not sure if it explains the random failures, but won't undefined > behavior eat your babies? Possibly. I would still expect it to be consistent when it eats babies, but maybe it doesn't have to be.
diff git a/app/test/test_common.c b/app/test/test_common.c index 2b856f8ba5..12bd1cad90 100644  a/app/test/test_common.c +++ b/app/test/test_common.c @@ 216,7 +216,19 @@ test_log2(void) const uint32_t max = 0x10000; const uint32_t step = 1;  for (i = 0; i < max; i = i + step) { + compare = rte_log2_u32(0); + if (compare != 0) { + printf("Wrong rte_log2_u32(0) val %x, expected 0\n", compare); + return TEST_FAILED; + } + + compare = rte_log2_u64(0); + if (compare != 0) { + printf("Wrong rte_log2_u64(0) val %x, expected 0\n", compare); + return TEST_FAILED; + } + + for (i = 1; i < max; i = i + step) { uint64_t i64; /* extend range for 64bit */
We recently started to get random failures on the common_autotest ut with clang on Ubuntu 16.04.6. Example: https://travisci.com/DPDK/dpdk/jobs/263177424 Wrong rte_log2_u64(0) val 0, expected ffffffff Test Failed The ut passes 0 to log2() to get an expected value. Quoting log2 / log(3) manual: If x is zero, then a pole error occurs, and the functions return HUGE_VAL, HUGE_VALF, or HUGE_VALL, respectively. rte_log2_uXX helpers handle 0 as a special value and return 0. Let's have dedicated tests for this case. Fixes: 05c4345ef5c2 ("test: add unit test for integer log2 function") Cc: stable@dpdk.org Signedoffby: David Marchand <david.marchand@redhat.com>  app/test/test_common.c  14 +++++++++++++ 1 file changed, 13 insertions(+), 1 deletion()