xtensor icon indicating copy to clipboard operation
xtensor copied to clipboard

xtest_extended_histogram.histogram_auto: violates bound checks in xtensor 0.24.6

Open SomeoneSerge opened this issue 1 year ago • 4 comments

Steps to reproduce

Build xtensor from refs/tags/0.24.6 with bound checks on:

          "-DXTENSOR_ENABLE_ASSERT=ON"
          "-DXTENSOR_CHECK_DIMENSION=ON"

Run tests and observe:

cd /build/source/build/test && ./test_xtensor_lib
[doctest] doctest version is "2.4.9"
[doctest] run with "--help" for options
terminate called after throwing an instance of 'std::runtime_error'
  what():  /build/source/include/xtensor/xscalar.hpp:569: check failed
        Number of arguments (1) is greater than the number of dimensions (0)
===============================================================================
/build/source/test/test_extended_xhistogram.cpp:34:
TEST CASE:  xtest_extended_histogram.histogram_auto

/build/source/test/test_extended_xhistogram.cpp:34: FATAL ERROR: test case CRASHED: SIGABRT - Abort (abnormal termination) signal

===============================================================================
[doctest] test cases:  6 |  5 passed | 1 failed | 1194 skipped
[doctest] assertions: 23 | 23 passed | 0 failed |
[doctest] Status: FAILURE!
make[3]: *** [test/CMakeFiles/xtest.dir/build.make:70: test/CMakeFiles/xtest] Aborted (core dumped)

SomeoneSerge avatar Apr 17 '23 02:04 SomeoneSerge

Thanks. At first glance I do not spot a typo in the tests. Is there a typo in the code? Do you have time to debug?

tdegeus avatar Apr 17 '23 15:04 tdegeus

Do you have time to debug?

Not during the week I'm afraid. But I also think that what we need to rule out first is the possibility that tests are correct and there's out of bounds access somewhere in the actual library

From what I see, the CI does run tests with -DXTENSOR_ENABLE_ASSERT=ON, but maybe not with -DXTENSOR_CHECK_DIMENSION=ON?

SomeoneSerge avatar Apr 17 '23 17:04 SomeoneSerge

Indeed, I think that there might be a 'typo' in the actual library. Indeed it seems that XTENSOR_CHECK_DIMENSION is stricter than XTENSOR_ENABLE_ASSERT. Yet is might incorrectly identify https://github.com/xtensor-stack/xtensor/blob/807aa88e005925f27031b885038e595e2c14117a/include/xtensor/xhistogram.hpp#L591 as problematic. Though is if perfectly ok to use xt::sum(n)() to let your test pass

A comment: per documentation, you should not use XTENSOR_CHECK_DIMENSION on all tests (or all codes) as it will fire on legal broadcasting.

tdegeus avatar Apr 17 '23 18:04 tdegeus

A comment: per documentation, you should not use XTENSOR_CHECK_DIMENSION on all tests (or all codes) as it will fire on legal broadcasting.

Oh, that's a relief. It means we can just merge https://github.com/NixOS/nixpkgs/pull/225904

SomeoneSerge avatar Apr 17 '23 19:04 SomeoneSerge