Skip to content

Conversation

lostbard
Copy link
Collaborator

@lostbard lostbard commented Apr 2, 2025

v0.2.4

@mmuetzel
Copy link
Member

mmuetzel commented Apr 4, 2025

It looks like the tests for that package crashed Octave in the CI:

  Testing functions in package 'tisean':
  
  Integrated test scripts:
  
                                                                                    [ CPU    /  CLOCK ]
    ..packages/tisean-0.2.4/x86_64-pc-linux-gnu-api-v59/lazy.cc-tst  pass    4/4    [ 0.008s /  0.008s]
    ..ckages/tisean-0.2.4/x86_64-pc-linux-gnu-api-v59/mutual.cc-tst  pass    5/5    [ 0.013s /  0.013s]
  fatal: caught signal Segmentation fault -- stopping myself...
    /usr/share/octave/packages/tisean-0.2.4/av_d2.m ................
  Error: Process completed with exit code 139.

Is this something that needs to be fixed? Or should we just go ahead with this version anyway?

@lostbard
Copy link
Collaborator Author

lostbard commented Apr 4, 2025

yeah would be nice to fix but isnt happenig on my work machine - i was going to try a couple of others to see if I could recreate it

@mmuetzel
Copy link
Member

mmuetzel commented Apr 4, 2025

Could that be the same issue that Dmitri reported on Savannah with address sanitizer?
https://savannah.gnu.org/bugs/?61583

  ..ri/.local/share/octave/api-v60/packages/tisean-0.2.4/lyap_r.m =================================================================
==669628==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x7c1bbf9b4f68 at pc 0x7bfaa4e36cde bp 0x7bfa9da1c8d0 sp 0x7bfa9da1c8c8
READ of size 8 at 0x7c1bbf9b4f68 thread T10 (QThread)
    #0 0x7bfaa4e36cdd in F__lyap_r__(octave_value_list const&, int) /tmp/oct-FMgS89/tisean-0.2.4/src/__lyap_r__.cc:173:48
    #1 0x7ffbc7fb869d in octave::tree_evaluator::execute_builtin_function(octave_builtin&, int, octave_value_list const&) /home/dmitri/src/dev/octave/clang_asan/../libinterp/parse-tree/pt-eval.cc:3551:14

But then, the tests for lyap_r.m would probably have run after the tests for av_d2.m for which it apparently crashed in the CI...

@lostbard
Copy link
Collaborator Author

lostbard commented Apr 4, 2025

I can duplicate a crash in windows for av_d2
I think I know the problem so will see what need to do to fix

@siko1056 siko1056 added the package release New package release label Apr 5, 2025
@lostbard
Copy link
Collaborator Author

lostbard commented Apr 5, 2025

Using this patch:

diff -r f93e77808612 src/configure.ac
--- a/src/configure.ac  Fri Apr 04 10:00:42 2025 -0400
+++ b/src/configure.ac  Sat Apr 05 08:06:44 2025 -0400
@@ -41,6 +41,14 @@
     [AC_MSG_RESULT([no])]
     [AC_MSG_ERROR([*** A fortran compiler with support for -freal-4-real-8 flag is required.])]
 )
+FFLAGS=-finteger-4-integer-8
+AC_MSG_CHECKING([whether $F77 supports -finteger-4-integer-8])
+AC_COMPILE_IFELSE([AC_LANG_PROGRAM([])],
+    [AC_MSG_RESULT([yes])]
+    [AM_FFLAGS="$AM_FFLAGS -finteger-4-integer-8"],
+    [AC_MSG_RESULT([no])]
+    [AC_MSG_ERROR([*** A fortran compiler with support for -finteger-4-integer-8 flag is required.])]
+)
 FFLAGS="$my_save_fflags"
 AC_SUBST([AM_FFLAGS])

I stop getting crashes at least on the windows machie that I saw them on.
However more tests fail which seems to be the otherr issue raise in the bug report.

@siko1056
Copy link
Member

siko1056 commented Apr 5, 2025

The CI Octave container image is using BLAS configured 64 Bit indices entirely:

Does the tisean package detect and respect the Octave configuration?

@siko1056
Copy link
Member

siko1056 commented Apr 5, 2025

gfortran also states a warning about the suitability of the flag -finteger-4-integer-8:

This option should be used with care and may not be suitable for your codes. Areas of possible concern include calls to external procedures [...]

--- https://gcc.gnu.org/onlinedocs/gfortran/Fortran-Dialect-Options.html#index-finteger-4-integer-8

Where I assume "external procedures" could be calls to BLAS procedures configured with 8 bit indices explicitly in this case?

@lostbard
Copy link
Collaborator Author

lostbard commented Apr 5, 2025

yeah i dont use fortran so cant comment much on what it is or isnt doing.

Apart from the flags set in configure for real-4-8 and patch above for int-4-8, it is using whatever mkoctfile uses.

Without the int-4 patch, I see warnings in windows for values of INT(4) and INT(8) conflicts

@mmuetzel
Copy link
Member

mmuetzel commented Apr 6, 2025

Fwiw, MXE Octave uses the Fortran flag -fdefault-integer-8 when linking to Fortran libraries with 64-bit integers. I don't know how that differs from -finteger-4-integer-8 though.

In general: Should we really use a Docker image which contains Fortran libraries with (fairly uncommon) 64-bit default Fortran integer size? Would it be better to use the default 32-bit Fortran integer size in the CI instead?
Is support for 64-bit Fortran integers a requirement for Octave packages?

@mmuetzel
Copy link
Member

mmuetzel commented Aug 5, 2025

I'm also able to reproduce the crash.

The top of the backtrace looks like this:

(gdb) bt
#0  0x00007ffc74c82745 in mneigh (nmax=<optimized out>, mmax=<optimized out>, nxx=<optimized out>, y=...,
    n=-103295133, nlast=1000, id=1, m=9, jh=..., jpntr=..., eps=0.354788046951234, nlist=..., nfound=0)
    at source_f/neigh.f:165
#1  0x00007ffc74c8191e in d1 (nmax=1000, mmax=2, nxx=1000, y=..., id=1, m=9, ncmin=100, pr=0, pln=-6.4799584020685925,
    eln=-44.052167321973606, nmin=0, kmax=100, iverb=0) at source_f/d1.f:103
#2  0x00007ffc74c86be9 in F__c1__ (args=..., nargout=<optimized out>) at __c1__.cc:121
#3  0x00007ffc84080083 in liboctinterp-13!_ZN6octave14tree_evaluator24execute_builtin_functionER14octave_builtiniRK17octave_value_list () from C:\PROGRA~1\GNUOCT~1\OCF8BC~1.0\mingw64\bin\liboctinterp-13.dll
#4  0x00007ffc83f2cb03 in liboctinterp-13!_ZN14octave_builtin7executeERN6octave14tree_evaluatorEiRK17octave_value_list
    () from C:\PROGRA~1\GNUOCT~1\OCF8BC~1.0\mingw64\bin\liboctinterp-13.dll
#5  0x00007ffc83f74eb0 in liboctinterp-13!_ZN15octave_function4callERN6octave14tree_evaluatorEiRK17octave_value_list ()
   from C:\PROGRA~1\GNUOCT~1\OCF8BC~1.0\mingw64\bin\liboctinterp-13.dll
#6  0x00007ffc840a3572 in liboctinterp-13!_ZN6octave21tree_index_expression10evaluate_nERNS_14tree_evaluatorEi ()
   from C:\PROGRA~1\GNUOCT~1\OCF8BC~1.0\mingw64\bin\liboctinterp-13.dll
#7  0x00007ffc847068aa in liboctinterp-13!_ZN6octave21tree_index_expression8evaluateERNS_14tree_evaluatorEi ()
   from C:\PROGRA~1\GNUOCT~1\OCF8BC~1.0\mingw64\bin\liboctinterp-13.dll
#8  0x00007ffc8405a11f in liboctinterp-13!_ZN6octave22tree_simple_assignment8evaluateERNS_14tree_evaluatorEi ()
   from C:\PROGRA~1\GNUOCT~1\OCF8BC~1.0\mingw64\bin\liboctinterp-13.dll
#9  0x00007ffc8408ee16 in liboctinterp-13!_ZN6octave14tree_evaluator15visit_statementERNS_14tree_statementE ()
   from C:\PROGRA~1\GNUOCT~1\OCF8BC~1.0\mingw64\bin\liboctinterp-13.dll

n has a suspicious value. It is negative and has a large magnitude. That is probably not intentional.

Looking at d1.f, there is a suspicious expression which probably misses parenthesis.
I don't know how to install a package from a checked out repository. So, I haven't tested that. But does the following change help?

diff --git a/src/source_f/d1.f b/src/source_f/d1.f
--- a/src/source_f/d1.f
+++ b/src/source_f/d1.f
@@ -89,7 +89,7 @@ c     computations
       do 10 i=1,nmax-(mt-1)*id
  10      ju(i)=i+(mt-1)*id
       do 20 i=1,nmax-(mt-1)*id
-         iperm=min(int(rand(0.0)*nmax-(mt-1)*id)+1,nmax-(mt-1)*id)
+         iperm=min(int(rand(0)*(nmax-(mt-1)*id))+1,nmax-(mt-1)*id)
          ih=ju(i)
          ju(i)=ju(iperm)
  20      ju(iperm)=ih

This also fixes the input argument type for the RAND intrinsic which according to its documentation should be called with an integer:
https://gcc.gnu.org/onlinedocs/gfortran/RAND.html

Since the existing code only randomly leads to negative indices iperm, this might not crash every time...

@mmuetzel
Copy link
Member

mmuetzel commented Aug 5, 2025

To test, I decompressed the tarball, made that modification in src/source_f/d1.f and zipped the folder again.
Then, I installed the package from the zip-file using pkg install -verbose tisean-0.2.4.zip.

With that change, pkg test tisean no longer crashes for me. (Repeated 5 times with consistent results.) All tests run through with the following summary:

Failure Summary:

  ..\AppData\Roaming\octave\api-v60\packages\tisean-0.2.4\av_d2.m  pass    6/8
                                                                   FAIL    2
  ..kus\AppData\Roaming\octave\api-v60\packages\tisean-0.2.4\c1.m  pass    1/3
                                                                   FAIL    2
  ..us\AppData\Roaming\octave\api-v60\packages\tisean-0.2.4\c2d.m  pass    2/3
                                                                   FAIL    1
  ..us\AppData\Roaming\octave\api-v60\packages\tisean-0.2.4\c2g.m  pass    1/2
                                                                   FAIL    1
  ..\AppData\Roaming\octave\api-v60\packages\tisean-0.2.4\ikeda.m  pass    0/1
                                                                   FAIL    1
  ..Data\Roaming\octave\api-v60\packages\tisean-0.2.4\lyap_spec.m  pass    3/5

                                                                                                                                      
Summary:

  PASS                              153
  FAIL                                9

I've no idea what these tests are actually doing. So, I can't tell how bad those failing tests actually are. Maybe, good enough for a release still?
The created log file:
fntests-tisean-0.2.4.zip

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package release New package release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants