Test failure

[opoplawski]

I'm looking at updating the octave-statistics package in Fedora to 1.7.1 but hitting the following:

  fitcnet.m .................................................../usr/include/c++/15/bits/stl_vector.h:1262: std::vector<_Tp, _Alloc>::reference std::vector<_Tp, _Alloc>::operator[](size_type) [with _Tp = double; _Alloc = std::allocator<double>; reference = double&; size_type = long unsigned int]: Assertion '__n < this->size()' failed.
fatal: caught signal Aborted -- stopping myself...
/var/tmp/rpm-tmp.77dj2o: line 48: 744869 Aborted                 (core dumped) octave -H -q --no-window-system --no-site-file --eval 'pkg("prefix","/home/orion/fedora/octave-statistics/octave-statistics-1.7.1-build/BUILDROOT/usr/share/octave/packages","/home/orion/fedora/octave-statistics/octave-statistics-1.7.1-build/BUILDROOT/usr/lib64/octave/packages");pkg("local_list",fullfile("/home/orion/fedora/octave-statistics/octave-statistics-1.7.1-build/BUILDROOT/usr/share/octave","octave_packages"));pkg("list");pkg("install","-verbose",glob("/home/orion/fedora/octave-statistics/octave-statistics-1.7.1-build/statistics-release-1.7.1/build/statistics-1.7.1-*.tar.gz"){1,1});pkg("load","statistics");pkg("list");oruntests("/home/orion/fedora/octave-statistics/octave-statistics-1.7.1-build/BUILDROOT/usr/share/octave/packages/statistics-1.7.1");unlink(pkg("local_list"));'

I'll try to collect some more info when I have some time.

[dasergatskov]

/usr/include/c++/15/

Is this Fedora Rawhide ?
It works for me on Fedora 41 (with gcc 14).
Is octave compiled with the same compiler?

p.s.: I just noticed that in Fedora 41 octave is still on 8.4.
p.p.s.: Trying to rebuild 9.3 srpm, noticed that it still requires mesa-libOSMesa and ftgl-devel . Octave does not use those for years.

[opoplawski]

This is rawhide with gcc 15.

[dasergatskov]

This is rawhide with gcc 15.

I would recommend to rebuild octave with your current toolchain.

[opoplawski]

octave has been rebuilt with the current toolchain.

p.s.: 8.4 -> 9 was a soname/API update. We don't update those in stable releases.
p.p.s.: Thanks - I've dropped those requirements.

[opoplawski]

This is what I've been able to glean from gdb:

#4  0x00007ffff580b0c2 in std::__glibcxx_assert_fail (file=file@entry=0x7ffff28e9f48 "/usr/include/c++/15/bits/stl_vector.h", line=line@entry=1262,
    function=function@entry=0x7ffff28ea028 "std::vector<_Tp, _Alloc>::reference std::vector<_Tp, _Alloc>::operator[](size_type) [with _Tp = double; _Alloc = std::allocator<double>; reference = double&; size_type = long unsigned int]", condition=condition@entry=0x7ffff28e9d92 "__n < this->size()")
    at ../../../../../libstdc++-v3/src/c++11/assert_fail.cc:41
#5  0x00007ffff28d6ddf in std::vector<double, std::allocator<double> >::operator[] (__n=<optimized out>, this=<optimized out>)
    at /usr/include/c++/15/bits/stl_vector.h:1262
#6  0x00007ffff28d7cd4 in std::vector<double, std::allocator<double> >::operator[] (this=<optimized out>, __n=<optimized out>)
    at /usr/include/octave-9.3.0/octave/../octave/MArray.h:79
#7  Ffcnntrain (args=..., nargout=<optimized out>)
    at /home/orion/fedora/octave-statistics/octave-statistics-1.7.1-build/statistics-release-1.7.1/build/statistics-release-1.7.1/src/fcnntrain.cc:158
#8  0x00007ffff7468890 in octave::tree_evaluator::execute_builtin_function (this=0x5555555c93e8, builtin_function=..., nargout=1, args=...)
    at libinterp/parse-tree/pt-eval.cc:3470
#9  0x00007ffff72d149b in octave_builtin::execute (this=<optimized out>, tw=..., nargout=<optimized out>, args=...) at libinterp/octave-value/ov-builtin.cc:49
#10 0x00007ffff7335b03 in octave_function::call (this=0x5555557d20a0, tw=..., nargout=1, args=...) at libinterp/octave-value/ov-fcn.cc:63
#7  Ffcnntrain (args=..., nargout=<optimized out>)
    at /home/orion/fedora/octave-statistics/octave-statistics-1.7.1-build/statistics-release-1.7.1/build/statistics-release-1.7.1/src/fcnntrain.cc:158
158           data[i][j] = X(i,j);
(gdb) list
153       Matrix X = args(0).matrix_value ();
154       for (int i = 0; i < n; i++)
155       {
156         for (int j = 0; j < d; j++)
157         {
158           data[i][j] = X(i,j);
159         }
160       }
161       // Construct 1D vector from labels in Y
162       vector<int> labels(n, 0);
(gdb) print i
$1 = <optimized out>
(gdb) print n
$2 = 150
(gdb) print d
$3 = <optimized out>
(gdb) print j
$4 = <optimized out>
(gdb) up
#10 0x00007ffff7335b03 in octave_function::call (this=0x5555557d20a0, tw=..., nargout=1, args=...) at libinterp/octave-value/ov-fcn.cc:63
63        return execute (tw, nargout, args);
(gdb) print args
$10 = (const octave_value_list &) @0x7fffffff9510: {m_data = std::vector of length 7, capacity 7 = {{m_rep = 0x555555dc6b30}, {m_rep = 0x555555e005a0}, {
      m_rep = 0x555555d208d0}, {m_rep = 0x555555dbbc90}, {m_rep = 0x555555d12300}, {m_rep = 0x555555d885c0}, {m_rep = 0x555555ddb9b0}}, m_names = {m_data = {
      _vptr.Array = 0x7ffff7e32960 <vtable for Array<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > >+16>, m_dimensions = {m_num_dims = 2, m_dims = 0x555555cd19d0}, m_rep = 0x555555dcbe00,
      m_slice_data = 0x555555c78fc0, m_slice_len = 7}}}

[dasergatskov]

I build it with ASAN and I do see a problem:

octave:1> pkg load statistics
octave:2> test fitcnet
=================================================================
==453703==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x515005ce4780 at pc 0x7f1601d27797 bp 0x7ffc9fc3a290 sp 0x7ffc9fc3a288
READ of size 8 at 0x515005ce4780 thread T0
    #0 0x7f1601d27796 in Ffcnntrain(octave_value_list const&, int) /tmp/oct-CFpmYB/statistics-release-1.7.1/src/fcnntrain.cc:364:21
    #1 0x7f160a31488b in octave::tree_evaluator::execute_builtin_function(octave_builtin&, int, octave_value_list const&) /home/dima/src/octave/clang_asan/../libinterp/parse-tree/pt-eval.cc:3470:14
    #2 0x7f1609eab925 in octave_builtin::execute(octave::tree_evaluator&, int, octave_value_list const&) /home/dima/src/octave/clang_asan/../libinterp/octave-value/ov-builtin.cc:49:13
    #3 0x7f1609f9f2ec in octave_function::call(octave::tree_evaluator&, int, octave_value_list const&) /home/dima/src/octave/clang_asan/../libinterp/octave-value/ov-fcn.cc
...

Full trace attached. This is on Centos Stream 9 / octave 9.3.1 / clang-19.1.5

fitcnet_asan.txt.gz

[dasergatskov]

Tried current git snapshot:

octave:1> pkg load statistics
octave:2> test fitcnet
=================================================================
==456184==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x5150015a0680 at pc 0x7fc90e8275d1 bp 0x7ffc52ef6ad0 sp 0x7ffc52ef6ac8
READ of size 8 at 0x5150015a0680 thread T0
    #0 0x7fc90e8275d0 in Ffcnntrain(octave_value_list const&, int) /tmp/oct-7yU7Gq/statistics/src/fcnntrain.cc:393:21
    #1 0x7fc916f1488b in octave::tree_evaluator::execute_builtin_function(octave_builtin&, int, octave_value_list const&) /home/dima/src/octave/clang_asan/../libinterp/parse-tree/pt-eval.cc:3470:14
    #2 0x7fc916aab925 in octave_builtin::execute(octave::tree_evaluator&, int, octave_value_list const&) /home/dima/src/octave/clang_asan/../libinterp/octave-value/ov-builtin.cc:49:13
    #3 0x7fc916b9f2ec in octave_function::call(octave::tree_evaluator&, int, octave_value_list const&) /home/dima/src/octave/clang_asan/../libinterp/octave-value/ov-fcn.cc:63:10
...

[dasergatskov]

The following diff seems to fix this particular crash for me (ASAN build).

$ git diff
diff --git a/src/fcnntrain.cc b/src/fcnntrain.cc
index 4c659cde..9b34b531 100644
--- a/src/fcnntrain.cc
+++ b/src/fcnntrain.cc
@@ -271,9 +271,8 @@ neural network model's training process. \
   }
 
   // Define return variables
-  vector<double> Accuracy;
-  vector<double> Loss;
-
+  vector<double> Accuracy(n);
+  vector<double> Loss(n);
   // Start training
   int epoch = 0;
   for (; epoch < args(7).uint_value (); epoch++)

I really do not understand it -- I thought vector<> is a dynamic array and pre-allocating helps with performance, but not required.

[dasergatskov]

It took about an hour, but ASAN compiled statistics (with the above diff) passed all the tests:

Fixed test scripts:

                                                        total time (CPU / CLOCK)  [3136.8s / 3165.5s]
Failure Summary:

  ..share/octave/api-v59/packages/statistics-1.7.1/shadow9/mean.m  pass   79/80  
                                                    (reported bug) XFAIL   1
  ../share/octave/api-v59/packages/statistics-1.7.1/fillmissing.m  pass  379/380 
                                                    (reported bug) XFAIL   1
  ..a/.local/share/octave/api-v59/packages/statistics-1.7.1/pca.m  pass   31/32  
                                                (expected failure) XFAIL   1

Summary:

  PASS                            11186
  FAIL                                0
  XFAIL (reported bug)                2
  XFAIL (expected failure)            1

[mmuetzel]

It's surprising that your proposed change fixes this issue. But it is probably a good idea anyway to let the compiler know the size of these vectors ahead of time so that it has an easier time figuring out how much memory it should preferably pre-allocate for them.

Looks good to me. 👍

[mmuetzel]

Ah. I think I see which issue this might be fixing:
Maybe, args(7).uint_value () is not necessarily args(0).rows ().
Maybe, it would be even better/clearer to use the two-argument constructor in that case. (Maybe with 0.0 as the default value?)

Also, is there a check that limits args(7).uint_value () to a maximum of args(0).rows ()? (I haven't checked that.)

Edit: Alternatively, the returned A and L should be shorter in that case. That's something that @pr0m1th3as probably knows best.

[pr0m1th3as]

args(7).uint_value is always a positive scalar. The checks are in lines 270-277.

EDIT, the proposed changes will crash the function if the samples in the data set are less than the number of epochs used in training.

So,

   // Define return variables
-  vector<double> Accuracy;
-  vector<double> Loss;
-
+  vector<double> Accuracy(n);
+  vector<double> Loss(n);

is a disaster waiting to happen. The tests pass simple because the fisheriris dataset has 150 samples and the iteration limit in the fitcnet BIST is set to 50. By default, it is 1000.

If we need to preallocate Accuracy and Loss vectors, this should be to the number of epochs used in training.

[dasergatskov]

I am a simple man. I see it was crashing in the loop

for (int sample_idx = 0; sample_idx < n; sample_idx++)
  {
    A(sample_idx) = Accuracy[sample_idx];
    L(sample_idx) = Loss[sample_idx];
  }

where sample_idx is less than n...

[pr0m1th3as]

Yep!, While writing that n cannot be used for preallocating Accuracy and Loss, I realized that I was using it after the training to populate the returning RowVectors. My bad. Thanks to all for spotting this bug!!
Please, give it another go to see if the issue has been resolved.