lapack icon indicating copy to clipboard operation
lapack copied to clipboard

Arguments of dsyevx, dgeevx changed and now the treekin package is broken

Open yurivict opened this issue 3 years ago • 11 comments

Recently there were some changes to these functions and now the biology/treekin port breaks:

In file included from calcpp.cpp:6:
./calcpp.h:317:74: error: too few arguments to function call, expected 23, have 20
              (double *)z, ldz, (double *)work, lwork, iwork, ifail, info);
                                                                         ^
/usr/local/include/lapacke/lapack.h:17248:6: note: 'dsyevx_' declared here
void LAPACK_dsyevx_base(
     ^
/usr/local/include/lapacke/lapack.h:17247:28: note: expanded from macro 'LAPACK_dsyevx_base'
#define LAPACK_dsyevx_base LAPACK_GLOBAL(dsyevx,DSYEVX)
                           ^
/usr/local/include/lapacke/lapacke_mangling.h:5:34: note: expanded from macro 'LAPACK_GLOBAL'
#define LAPACK_GLOBAL(name,NAME) name##_
                                 ^
<scratch space>:326:1: note: expanded from here
dsyevx_
^
In file included from calcpp.cpp:6:
./calcpp.h:373:33: error: too few arguments to function call, expected 27, have 23
              lwork, iwork, info);
                                ^
/usr/local/include/lapacke/lapack.h:1900:6: note: 'dgeevx_' declared here
void LAPACK_dgeevx_base(
     ^
/usr/local/include/lapacke/lapack.h:1899:28: note: expanded from macro 'LAPACK_dgeevx_base'
#define LAPACK_dgeevx_base LAPACK_GLOBAL(dgeevx,DGEEVX)
                           ^
/usr/local/include/lapacke/lapacke_mangling.h:5:34: note: expanded from macro 'LAPACK_GLOBAL'
#define LAPACK_GLOBAL(name,NAME) name##_
                                 ^
<scratch space>:377:1: note: expanded from here
dgeevx_
^

https://www.tbi.univie.ac.at/RNA/Treekin/

I am the maintainer of the FreeBSD port but I have no idea how to fix it now.

Were these functions changed intentionally or accidentally?

lapack-3.10.0

yurivict avatar Jul 26 '21 04:07 yurivict

What changed is the (addition of a) definition of LAPACK_FORTRAN_STRLEN_END in lapack.h for when you call LAPACK functions directly from your C code, instead of through LAPACKE. This was added to expose the hidden/implied string length arguments that most/all Fortran compilers expect to be present, as Fortran does not use NULL-termination of strings like C does. (The original issue was #339 - though what prompted it was largely a behaviour change in gfortran alone that got mitigated again in later releases. And while practically all Fortran compilers require additional length arguments for C strings, the controversy is/was about whether single-character arguments are exempt)

martin-frbg avatar Jul 26 '21 05:07 martin-frbg

(I do wonder what this means for compatibility with both past and alternative implementations (MKL?), and why it was not mentioned in the release notes for 3.10.0)

martin-frbg avatar Jul 26 '21 12:07 martin-frbg

I wonder who is going to be fixing software that used these functions (ex. treekin) which doesn't have active devs now.

yurivict avatar Jul 26 '21 14:07 yurivict

Legacy functions should be left in the library for the older software to use. They should be left at least with the "_legacy" suffix.

yurivict avatar Jul 26 '21 15:07 yurivict

Hi. The old interface should still work if you remove these lines:

https://github.com/Reference-LAPACK/lapack/blob/3b26987fc1206eb2899c6e4d4edd489294b37350/LAPACKE/include/lapack.h#L16-L19

Did you (Could you please) try it?

weslleyspereira avatar Jul 26 '21 16:07 weslleyspereira

Hi. The old interface should still work if you remove these lines:

This seems to require lapack to be rebuilt with these lines removed. Other packages need to use the pre-built lapack package, one for all.

yurivict avatar Jul 26 '21 16:07 yurivict

I'm not sure you need to rebuild LAPACK only because of those lines. We only use LAPACK_FORTRAN_STRLEN_END in lapack.h, which is a header file. Mind that this (optional) flag improves the compatibility between the C code and the library generated by most Fortran compilers. Please see #515 and the references therein for the discussion about it. Also, see #561, for the systematic use of this flag in LAPACKE.

weslleyspereira avatar Jul 27 '21 01:07 weslleyspereira

Having to modify a "system/distribution preinstalled" header depending on whose source you want to compile against LAPACK is not ideal either, I wonder if it would be possible to create a truly variadic prototype that would match both "traditional" and "pedantic" use ?

martin-frbg avatar Jul 27 '21 09:07 martin-frbg

This line:

      ::dsyevx_((char *)jobz, (char *)range, (char *)uplo, n, (double *)a, lda, (double *)vl,
              (double *)vu, il, iu, (double *)abstol, m, (double *)w,
              (double *)z, ldz, (double *)work, lwork, iwork, ifail, info);

breaks with:

./calcpp.h:317:74: error: too few arguments to function call, expected 23, have 20
              (double *)z, ldz, (double *)work, lwork, iwork, ifail, info);
                                                                         ^

and this line

      ::dgeevx_((char *)balanc, (char *)jobvl, (char *)jobvr, (char *)sense, n, (double *)a, lda,
              (double *)wr, (double *)wi, (double *)vl, ldvl,
              (double *)vr, ldvr, ilo, ihi, (double *)scale, (double *)abnrm, (double *)rconde,
              (double *)rcondv, (double *)work,
              lwork, iwork, info);

breaks with

./calcpp.h:373:33: error: too few arguments to function call, expected 27, have 23
              lwork, iwork, info);
                                ^

Any idea how to patch them?

yurivict avatar Oct 15 '21 07:10 yurivict

You'd need to add an "int" after the "info" parameter for each "char*" argument in the list. But I am not sure how portable that is with respect to earlier or alternative implementations of LAPACK:

martin-frbg avatar Oct 15 '21 07:10 martin-frbg

You'd need to add an "int" after the "info" parameter for each "char*" argument in the list.

This depends on the compiler and in the case of gfortran also on the gfortran version!

The GNU Fortran Compiler 11.2.0 Documentation: Argument passing conventions:

For arguments of CHARACTER type, the character length is passed as a hidden argument at the end of the argument list. For deferred-length strings, the value is passed by reference, otherwise by value. The character length has the C type size_t (or INTEGER(kind=C_SIZE_T) in Fortran). Note that this is different to older versions of the GNU Fortran compiler, where the type of the hidden character length argument was a C int.

gfortran 7 expected a hidden int argument, see the The GNU Fortran Compiler 7.5.0 Documentation: Argument passing conventions.

christoph-conrads avatar Oct 19 '21 18:10 christoph-conrads

So what's the best solution for the many C/C++ projects out there that expect the old API without the trailing string lengths? ~Always use LAPACKE?~ Use the LAPACK_[lapack function] macro everywhere?

haampie avatar Oct 05 '22 10:10 haampie

Yes, the best solution, in my opinion, is to use LAPACKE. If there are issues with LAPACKE, let us know. LAPACKE should work for great for C and C++.

If you are working in C++ and you want a wrapper to LAPACK, you might want to have a look at LAPACK++ which is a C++ interface to LAPACK (that uses LAPACKE).

langou avatar Oct 05 '22 13:10 langou

The treekin port was fixed by patching:

+- patch for latest breaking Lapack API changes, see https://github.com/Reference-LAPACK/lapack/issues/604#issuecomment-944069793
+
 --- src/calcpp.h.orig  2019-06-13 14:11:19 UTC
 +++ src/calcpp.h
 @@ -49,6 +49,9 @@
@@ -10,3 +12,23 @@
  #   include <lapacke/lapacke.h>
  # else
  #   ifdef HAVE_OPENBLAS_LAPACKE_H
+@@ -311,7 +314,8 @@ Calccpp::Mx_Dsyevx(const char *jobz,
+       /*default standard lapack */
+       ::dsyevx_((char *)jobz, (char *)range, (char *)uplo, n, (double *)a, lda, (double *)vl,
+               (double *)vu, il, iu, (double *)abstol, m, (double *)w,
+-              (double *)z, ldz, (double *)work, lwork, iwork, ifail, info);
++              (double *)z, ldz, (double *)work, lwork, iwork, ifail, info,
++              int(0) /*jobz_int*/, int(0) /*range_int*/, int(0) /*uplo_int*/); // "int" after the "info" parameter for each "char*" argument in the list
+       break;
+   }
+ }
+@@ -367,7 +371,8 @@ Calccpp::Mx_Dgeevx(const char *balanc,
+               (double *)wr, (double *)wi, (double *)vl, ldvl,
+               (double *)vr, ldvr, ilo, ihi, (double *)scale, (double *)abnrm, (double *)rconde,
+               (double *)rcondv, (double *)work,
+-              lwork, iwork, info);
++              lwork, iwork, info,
++              int(0) /*balanc_int*/, int(0) /*jobvl_int*/, int(0) /*jobvr_int*/, int(0) /*sense_int*/); // "int" after the "info" parameter for each "char*" argument in the list
+       break;
+   }
+ }

yurivict avatar Oct 05 '22 16:10 yurivict

@yurivict I suspect you can make it compatible with both old and new if you check if LAPACK_FORTRAN_STRLEN_END is defined (sits in lapack.h which gets included by lapacke.h) - at least as long as you can be sure that the lapacke.h came with the flavor of LAPACK library that you are linking against. (BTW not sure what the (truncated?) first chunk of the diff is about).

martin-frbg avatar Oct 05 '22 17:10 martin-frbg

Thanks @yurivict. I take from your message "The treekin port was fixed by patching . . ." that the issue is resolved. This is a good news. I am closing the issue. If I am misunderstanding, please feel welcome to reopen and explain more what needs to happen on our end. @langou

langou avatar Oct 06 '22 20:10 langou