I pushed this change on stable and merged with default:

http://hg.savannah.gnu.org/hgweb/octave/rev/45d958bc3437

gcc builds 9beec32ba3d6 without any complaints. did now also a clang build and got same warning as mentioned in comment #64. but it comes many times in other .cc files as well. Anyway BISTs pass in both worlds without errors and my own test cases also pass.

I pushed the change here:
https://hg.savannah.gnu.org/hgweb/octave/rev/9beec32ba3d6

I hope there won't be too many changes necessary (in case the build bot are complaining). I also hope it is ok to do those in the repository if they were needed.

Marking as ready for test again.

No problem ;-) Just for confirmation the patch file #49783 builds without errors and the BISTs are fine on Fedora 31.

The example in comment #57 might not have been the best to demonstrate the possible implications. It was just the first one I found with a (possibly) narrowing assignment that is un-checked.

Thanks for the test.
Sorry, I wasn't clear what I wanted to have tested. I actually meant to ask for testing the attached patch. The verbatim block was just to show what I changed with respect to the patch that failed on the buildbots.

My Ubuntu 20.04.1 is 64bit. But still sunindextype is 32bit wide  on that system.

I might be misunderstanding how this is supposed to work. But afaics, sunindextype should be set to the same size as the integer type used in the BLAS/LAPACK libraries. That is still 32bit on most systems.
Otherwise, bad things could happen somewhere when unavoidably sunindextype has to be cast to "int" for calling the BLAS/LAPACK functions.

For example in sundials 4.1.0 in sunlinsol_lapackband.c:

int SUNLinSolSetup_LapackBand(SUNLinearSolver S, SUNMatrix A)
{
  int n, ml, mu, ldim, ier;

  /* check for valid inputs */
  if ( (A == NULL) || (S == NULL) )
    return(SUNLS_MEM_NULL);

  /* Ensure that A is a band matrix */
  if (SUNMatGetID(A) != SUNMATRIX_BAND) {
    LASTFLAG(S) = SUNLS_ILL_INPUT;
    return(LASTFLAG(S));
  }

  /* Call LAPACK to do LU factorization of A */
  n = SUNBandMatrix_Rows(A);
  ml = SUNBandMatrix_LowerBandwidth(A);
  mu = SUNBandMatrix_UpperBandwidth(A);
  ldim = SUNBandMatrix_LDim(A);
  xgbtrf_f77(&n, &n, &ml, &mu, SUNBandMatrix_Data(A),
             &ldim, PIVOTS(S), &ier);

  LASTFLAG(S) = (long int) ier;
  if (ier > 0)
    return(SUNLS_LUFACT_FAIL);
  if (ier < 0)
    return(SUNLS_PACKAGE_FAIL_UNREC);
  return(SUNLS_SUCCESS);
}

With among other the following declarations in sunmatrix_band.h:

SUNDIALS_EXPORT sunindextype SUNBandMatrix_Rows(SUNMatrix A);
SUNDIALS_EXPORT sunindextype SUNBandMatrix_Columns(SUNMatrix A);
SUNDIALS_EXPORT sunindextype SUNBandMatrix_LowerBandwidth(SUNMatrix A);
SUNDIALS_EXPORT sunindextype SUNBandMatrix_UpperBandwidth(SUNMatrix A);

That seems to have been fixed in the meantime by efectively forcing sunindextype to match the BLAS index type.
But it is only safe in older version if "sunindextype" was correctly configured when sundials was compiled.
That seems to be not the case for the sundials that is packaged for Fedora.
So to avoid a possible issue here, we need to make sure that the sunindextype variables we pass to sundials can safely be cast to the BLAS integer type.

Thanks for the info.
That version of Sundials uses "int" in the BLAS/LAPACK wrappers (in sundials_lapack.h).
It might be that that could lead to an integer overflow sizeof(sunindextype)>sizeof(int). It would if not checked carefully (like we try to do with octave::to_f77_int).
Presumably, that was fixed in a later version effectively forcing sunindextype to match the integer type used by the BLAS/LAPACK libraries. At least in sundials 5.3.0, the wrappers use sunindextype.

Compiling at hg id 0c9a5eae6c27 works for me on Ubuntu with sundials (3.1.2+dfsg-3ubuntu2).
In /usr/include/sundials/sundials_lapack.h, the BLAS/LAPACK wrappers also use "int". But I have "#define SUNDIALS_INT32_T 1" in sundials_config.h. IIUC, that means that sunindextype is "int".
So everything is fine.

It's getting more probable that this is a packaging/configuration error on Fedora.

If we want to support such systems, I guess we could continue to use "sunindextype *" for the variables in the error messages and still be careful to not pass integers that are larger than what the BLAS/LAPACK libraries can cope with:

    SUNMatZero_Sparse (Jac);
    sunindextype *colptrs = SUNSparseMatrix_IndexPointers (Jac);
    sunindextype *rowvals = SUNSparseMatrix_IndexValues (Jac);

    for (octave_f77_int_type i = 0; i < m_num + 1; i++)
      colptrs[i] = to_f77_int (jac.cidx (i));

    double *d = SUNSparseMatrix_Data (Jac);
    for (octave_f77_int_type i = 0; i < to_f77_int (jac.nnz ()); i++)
      {
        rowvals[i] = to_f77_int (jac.ridx (i));
        d[i] = jac.data (i);
      }

Could someone with an affected system please test the attached patch? Does it compile with those changes? Do we need an additional cast (from octave_f77_int_type to sunindextype), or will the compiler do that for us?

(file #49780)

The types seem to match on the Debian buildbots (and also on my Ubuntu). But they seem to be different on Fedora. E.g.:
http://buildbot.octave.org:8010/#/builders/11/builds/1732

There is still this note in the IDA manual (which isn't very enlightening to me) [1]:

> A user program which uses sunindextype to handle indices will work with both index storage types except for any calls to index storage-specific external libraries.

And jwe observed the following:

> The sundials wrappers for BLAS functions use sunindextype, so it appears that it should match the size used by BLAS.
> See the file sundials-5.3.0/include/sundials/sundials_lapack.h, for example.

And:

> Since it is used in the prototypes for calling BLAS/Lapack functions inside sundials, then it seems we are forced to make it match the size of the type used for BLAS integers (octave_f77_int_type).

So is this a packaging issue in Fedora? (Probably not very likely.)
Or an error in Sunlinsol?
Or is there more to it?

[1]: https://computing.llnl.gov/sites/default/files/public/ida_guide.pdf

The new bug for the possible integer overflow is bug #59094.

Gnulib has macros for checking whether integer operations overflow, or we could (conditionally) use things like __builtin_mul_overflow to multiply dimensions and check for overflow.

I recently noticed that one of the Octave dependencies (maybe it was Sundials?) was using the portable snippets library for this job.  Perhaps we could use that or at least borrow ideas from it.  See, for example,  https://github.com/nemequ/portable-snippets/tree/master/safe-math

I hope we can refactor so that we limit the number of places where checks like this are needed.

See also bug #58790.

To convert an octave_idx_type value to an octave_f77_int_type value, you can use octave::to_f77_int, declared in f77_fcn.h.  It throws an error if the value is too large.

Is sunindextype always supposed to match the integer type used by the BLAS and Lapack libraries?  If so, then Octave's F77_INT typedef should always the the same as sunindextype and using to_f77_int should be OK.  Otherwise, we may want to define another function that does the same job, but for sunindextype.

I no longer see any errors for "test ode15i" on a Windows 32bit build from the stable branch (hg id f044b9951df5).

I also went through the integer type usage in "__ode15__.cc". Where according to the API of SUNDIALS IDA "sunindextype" should be used, I used "octave_f77_int_type".
In some places, the API uses "int" or "long int". In these places, I kept these types.
Where to me it seemed to be the most appropriate type, I used "octave_idx_type".


(file #49735)

I pushed the following change to MXE Octave:
https://hg.octave.org/mxe-octave/rev/9f9ed6a325bd

With that changes, KLU is detected by SUNDIALS IDA also for native Linux.

Only one file needs to change to update SuiteSparse.

I pushed the change here:
https://hg.octave.org/mxe-octave/rev/1541641533a6

I'd say go ahead and push the change to mxe-octave as it seems we should have sunindextype match the BLAS/fortran integer size.

I also see some places in Octave's _ode15_.cc file where "int" and "long int" are used where I suspect octave_idx_type or octave_f77_int_type should be used.

Is sunindextype something internal to sundials (like Octave's octave_f77_int_type) or is it something that appears in the interface to sundials functions that we call?

Since it is used in the prototypes for calling BLAS/Lapack functions inside sundials, then it seems we are forced to make it match the size of the type used for BLAS integers (octave_f77_int_type).

If it also appears in the external interface of sundials functions, then we have to be careful in Octave to call sundials functions with octave_f77_int_type variables, same as we do for BLAS/Lapack and some other libraries.

SUNDIALS IDA depends on LAPACK and SuiteSparse.
I'm not sure if sunindextype should (or must) match the index type of one or the other.
On [1], they write the following about v3.0.0:

> Addition of sunindextype option for 32- or 64-bit integer data index types within all SUNDIALS structures.
> > Sunindextype can be int64_t or int32_t or long long int and int depending on machine support for portable types.
> > The Fortran interfaces continue to use long_int for indices, except for their sparse matrix interface that now uses the new sunindextype.
> > Includes interfaces to PETSc, hypre, SuperLU_MT, and KLU with either 64-bit or 32-bit capabilities depending how the user configures SUNDIALS.

In their manual [2], they write:

> A user program which uses sunindextype to handle indices will work with both index storage types except for any calls to index storage-specific external libraries.

So that kind of sounds like it should match something. But I'm not sure if that is SuiteSparse_long or what SuiteSparse calls LONGBLAS and we call F77_INT.

It is currently only failing for Windows 32bit (where SuiteSparse_long is 32bit and F77_INT is 32bit). It didn't fail for hg200 when he tested on Windows 64bit (where SuiteSparse_long is 64bit and F77_INT was 32bit). (It was also independent of the size of octave_idx_type in his tests.)
So I'm guessing, sunindextype might need to match the size of SuiteSparse_long?

But it might also be caused by the code snippet, you showed in comment #31.
IIUC, SUNDIALS_INT64_T is defined if SUNDIALS_INDEX_SIZE is 64. That seems to be the default. So by default, sunindextype is 64bit. But KLU_INDEXTYPE would be 32bit wide on Windows (32bit and 64bit). That indeed looks suspicious. But if that is the problem, why does it work on Windows 64bit?
I wonder why KLU_INDEXTYPE isn't always defined as sunindextype?

Or those are two different (though quite similar) bugs.

[1]: https://computing.llnl.gov/projects/sundials/ida
[2]: https://computing.llnl.gov/sites/default/files/public/ida_guide.pdf

It appears to me that sundials can call BLAS or Lapack functions and it uses sunindextype for the integer dimensions and work arrays that it passes.  So that type should match what is used by the BLAS and Lapack libraries on the system (i.e., it should follow the ENABLE_FORTRAN_INT64 option for mxe-octave.

I also found the following lines in sundials-5.3.0/src/sunlinsol/klu/sunlinsol_klu.c:

#if defined(SUNDIALS_INT64_T)
#define KLU_INDEXTYPE long int
#else
#define KLU_INDEXTYPE int
#endif

That looks wrong for Windows, but shouldn't be a problem for w32 systems, only for 64-bit systems when using 64-bit fortran integers?

For those systems (the mxe-octave w64-64 builds), the buildbot log file for sundials shows warning like this:

/scratch/buildbot/workers/jwe-debian-x86_64-3/w64-64-on-debian/src/tmp-sundials-ida/sundials-5.3.0/src/sunlinsol/klu/sunlinsol_klu.c: In function 'SUNLinSolSetup_KLU':
/scratch/buildbot/workers/jwe-debian-x86_64-3/w64-64-on-debian/src/tmp-sundials-ida/sundials-5.3.0/src/sunlinsol/klu/sunlinsol_klu.c:281:35: warning: passing argument 2 of 'klu_l_analyze' from incompatible pointer type [-Wincompatible-pointer-types]
  281 |                                   (KLU_INDEXTYPE*) SUNSparseMatrix_IndexPointers(A),
      |                                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      |                                   |
      |                                   long int *
In file included from /scratch/buildbot/workers/jwe-debian-x86_64-3/w64-64-on-debian/src/tmp-sundials-ida/sundials-5.3.0/include/sunlinsol/sunlinsol_klu.h:34,
                 from /scratch/buildbot/workers/jwe-debian-x86_64-3/w64-64-on-debian/src/tmp-sundials-ida/sundials-5.3.0/src/sunlinsol/klu/sunlinsol_klu.c:22:
/scratch/buildbot/workers/jwe-debian-x86_64-3/w64-64-on-debian/src/usr/x86_64-w64-mingw32/include/suitesparse/klu.h:253:17: note: expected 'long long int *' but argument is of type 'long int *'
  253 | klu_l_symbolic *klu_l_analyze (SuiteSparse_long, SuiteSparse_long *,
      |                 ^~~~~~~~~~~~~

RE comment #25, I haven't looked at the sundials configuration and build system, but it seems wrong that we would need to specify full path names including SO versions when linking to shared libraries.

Got that wrong.
Sorry for the noise.

If I make the following change to the build rule of SUNDIALS IDA, it seems to correctly link to these libraries:

diff -r 2993dbd2acea -r 0bc7ea7264d9 src/sundials-ida.mk
--- a/src/sundials-ida.mk        Sun Jul 05 12:21:53 2020 +0200
+++ b/src/sundials-ida.mk        Thu Jul 30 14:08:20 2020 +0200
@@ -52,6 +52,11 @@
         -DKLU_ENABLE=ON \
         -DKLU_INCLUDE_DIR=$(HOST_INCDIR)/suitesparse \
         -DKLU_LIBRARY_DIR=$(HOST_LIBDIR) \
+        -DKLU_LIBRARY=$(HOST_LIBDIR)/libklu.so.1 \
+        -DAMD_LIBRARY=$(HOST_LIBDIR)/libamd.so.2 \
+        -DBTF_LIBRARY=$(HOST_LIBDIR)/libbtf.so.1 \
+        -DCOLAMD_LIBRARY=$(HOST_LIBDIR)/libcolamd.so.2 \
+        -DSUITESPARSECONFIG_LIBRARY=$(HOST_LIBDIR)/libsuitesparseconfig.so.5 \
         -DCMAKE_TOOLCHAIN_FILE='$(CMAKE_TOOLCHAIN_FILE)' \
         -DBUILD_ARKODE=OFF \
         -DBUILD_CVODE=OFF \

I don't know why the build tools wouldn't find and use those libraries automatically. They seem to be successfully found for Windows cross builds.
Maybe it has something to do with the trailing version? The Windows libraries seem to end in ".a" (without version postfix).

Sounds like a good test of our current working assumption.

To give you a starting point for a native Linux build with MXE Octave (I don't remember if I sent that before), the buildbot configuration uses switches similar to these:

./configure --with-pkg-dir=../mxe-octave-pkg --with-ccache --enable-octave=default --enable-binary-packages --enable-native-build --enable-qt5 --enable-lib64-directory --enable-pic-flag --disable-devel-tools --enable-system-x11-libs --enable-system-fontconfig --enable-system-gcc gnu-linux

For a test, you could add "--disable-64" (and don't use the patch you provided here).

Same for me. I have no idea whats going on in _ode15_.

Anyway, the attached patch v03 is verified with complete new & fresh windows builds, for 32 and 64 bit targets either. The testsuite / ODE-BISTS complete all without errors for both targets on Windows 7.

I didn't verify the "else path" so far (i.e. the non-windows build variants).

> Or use MXE Octave for a native Linux build.

Yeah - thats a good idea! I can build a Windows 64 target but with --disable-64 and see if the problem can be forced. If so then go ahead with Linux.



(file #49564)

I'm not yet sure about the exact root cause. As long as Octave's ode15 uses the IDA-headers to create the data structures that are exchanged with the IDA library, i would not expect casting problems, provided that sunindextype is int64_t and octave_idx_type is int32_t. I.e. if a structure is packed, or padded and how it is casted should not depend on the type of an index?

Another observation: Since I can't debug under windows, I made a build on my 64-bit Linux, with octave_idx_type set to int32_t, based on the ENABLE_64 switch, which in turn is set in configure.ac based on the configuration option --disable-64. My expectation would have been to see the problem if the root cause is a mismatch between octave_idx_type and sunindextype. But the ode tests run smoothly. One could argue that i have Sundials 4.1.0 on Linux versus 5.3.0 on Windows MXE Octave, so it is not a proof. But it is a "try and error".

However, if the root cause is a compatibility mismatch between the octave / sundial indices: The sunindextype can be either int32_t or int64_t (default), but it can't be unsigned. Fortunately, octave_idx_type is signed, although I know there have been discussions about switching to unsigned (see bug #54405). So up to know, the signedness is equal but the size can differ.

As you said, everything has changed since 5.2.0: Version number, _oct15_.cc implementation, etc. So i am bit stuck what i could do next.

Regarding the patch: Yes, it feels "better" to have the same CMAKE switches for other OS than Windows too. I will create an update.

Is there a reason, you set SUNDIALS_INDEX_SIZE only for Windows? Wouldn't that also be necessary on other platforms?
Can signed-ness be an issue? AFAICT, octave_idx_type is unsigned (either int32_t or int64_t). What type is sunindextype if SUNDIALS_INDEX_SIZE is set to 32 (or 64?)?

I'm also adapting the bug title to what is most likely the cause for the issue. It probably affects all platforms and isn't limited to Windows.

Good finding!

IIRC, the API of SUNDIALS IDA changed between versions. Octave 5 was using the API of version 2.7. Octave 6 and newer switched to a newer incompatible API. The older API is no longer available in newer versions of SUNDIALS IDA.

I haven't tested your patch. But it looks good at a quick view.

While that change is probably something we want to have in MXE Octave, maybe Octave should also use SUNDIALS IDA correctly independent on how SUNDIALS_INDEX_TYPE was configured.
I guess there might be places in Octave's code for SUDIALS IDA where octave_idx_type is used when it should use sunindextype instead.

The tests were done with the reference BLAS library.
For a test, I switched to OpenBLAS (in the SSE/SSE2 build). But the results are the same.

All text that would be displayed in the shell should be stored in the log for each package. All logs are in the "log" folder in the MXE build(?) tree.
I'm not sure if SUNDIALS IDA can be more verbose. If it can, you could change the build rule in src/sundials-ida.mk accordingly and re-build. AFAICS, "VERBOSE=1" is already passed to "make".

MXE Octave is used to build Octave and all of its dependencies. It also even builds the (cross-)compiler and other build tools to build the rest.
The patch I used in bug #58807 changes the gcc cross compilers to compile with SSE and SSE2 instructions by default also for the i686-w64-mingw32 target (setting the flags you showed here to be used by default).

I did not change any other build rules. But the other precision issues have disappeared. So I guess the patch did what it was meant to do.

I re-built Octave and all of its dependencies from scratch (with that modified cross-compiler). That includes all libraries that are packaged for Octave for Windows.

There is probably something else that is causing the issue here.

The precision bugs are solved by using the SSE and SSE2 instructions (see bug #58807).

The tests for ode15i and ode15s still fail with the same error.

That's not an "official" source. But it is probably good enough for us to know that Windows 7 with all available updates won't run on CPUs without SSE2.

Thanks for re-posting the lost message. (I mis-spelled "verbatim" in the closing tag of the first block.)


Do you have a source where MS specifies that Windows 7 requires a CPU with SSE2?

On their system requirements page [1], they don't specify any specific CPU instruction set.

The only thing I could find with a quick Google search is [2] where they recommend to update to a CPU supporting the SSE2 instruction set as a workaround for a known issue. (How bold is that?!)
But since Windows 7 is no longer supported by MS and an update for that issue is very unlikely, I guess that workaround might be a requirement now...


Anyway, any half-decent CPU that was bought in the last 20 years most likely supports SSE2. So it is probably ok to have that as a requirement for Octave on Windows.
It would probably solve (some of) the precision bugs for which I haven't opened a bug report yet. Not sure about this bug here though.
If it turns out that that would fix this bug (or others), we should probably ask on the mailing list about raising the minimum hardware requirements.


We would probably need to compile Octave itself and most (or all) of its dependencies with these switches. Is that also what you think?


[1]: https://support.microsoft.com/en-us/help/10737/windows-7-system-requirements
[2]: https://support.microsoft.com/en-us/help/4103718/windows-7-update-kb4103718

Somehow savannah does not display all the message.
Reposting from the email:

Details:

MXE Octave (hg id a65cacd05892) was configured to cross-compile Windows 32bit
executables with:

./configure --with-pkg-dir=../mxe-octave-pkg --with-ccache
--enable-octave=default --enable-binary-packages --enable-qt5
--enable-devel-tools --disable-windows-64 --disable-64 --disable-fortran-int64
--disable-system-opengl
-vebatim-

On Windows 10 2004 64bit (and according to bug #58790 also on Windows 7), the
tests for ode15i and ode15s fail with the following errors:

>> test ode15i

[IDA ERROR]  IDASolve
  At t = 0, the linear solver setup failed unrecoverably.

***** testif HAVE_SUNDIALS_SUNLINSOL_KLU
 opt = odeset ("Jacobian", @jacfunsparse, "AbsTol", 1e-7, "RelTol", 1e-7);
 [t, y] = ode15i (@rob, [0, 100], [1; 0; 0], [-1e-4; 1e-4; 0], opt);
 assert ([t(end), y(end,:)], fref, 1e-3);
!!!!! test failed
IDASolve failed
>> test ode15s

[IDA ERROR]  IDASolve
  At t = 0, the linear solver setup failed unrecoverably.

***** testif HAVE_SUNDIALS_SUNLINSOL_KLU
 opt = odeset ("MStateDependence", "none",
               "Mass", sparse ([1, 0, 0; 0, 1, 0; 0, 0, 0]),
               "Jacobian", @jacfunsparse);
 [t, y] = ode15s (@rob, [0, 100], [1; 0; 0], opt);
 assert ([t(end), y(end,:)], frefrob, 1e-3);
!!!!! test failed
IDASolve failed


This might be a problem with SUNDIALS IDA 32bit itself or with how we are
using it.

In case this should matter, MXE Octave bundles SUNDIALS IDA version 5.3.0 and
SuiteSparse version 5.7.2.

(That was Markus Mützel <mmuetzel> )

Dmitri.