Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
yurivict opened this issue Jul 26, 2021 · 16 comments
Closed

Comments

@yurivict
Copy link

yurivict commented Jul 26, 2021

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 yurivict changed the title Arguments of dsyevx, dgeevx changed and now the treekin oackage is broken Arguments of dsyevx, dgeevx changed and now the treekin package is broken Jul 26, 2021
@martin-frbg
Copy link
Collaborator

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
Copy link
Collaborator

martin-frbg commented Jul 26, 2021

(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)

@yurivict
Copy link
Author

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

@yurivict
Copy link
Author

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

@weslleyspereira
Copy link
Collaborator

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

/* It seems all current Fortran compilers put strlen at end.
* Some historical compilers put strlen after the str argument
* or make the str argument into a struct. */
#define LAPACK_FORTRAN_STRLEN_END

Did you (Could you please) try it?

@yurivict
Copy link
Author

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.

@weslleyspereira
Copy link
Collaborator

weslleyspereira commented Jul 27, 2021

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.

@martin-frbg
Copy link
Collaborator

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 ?

@yurivict
Copy link
Author

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?

@martin-frbg
Copy link
Collaborator

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:

@christoph-conrads
Copy link
Contributor

christoph-conrads commented Oct 19, 2021

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.

@haampie
Copy link
Contributor

haampie commented Oct 5, 2022

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?

@langou
Copy link
Contributor

langou commented Oct 5, 2022

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).

@yurivict
Copy link
Author

yurivict commented Oct 5, 2022

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;
+   }
+ }

@martin-frbg
Copy link
Collaborator

@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).

@langou
Copy link
Contributor

langou commented Oct 6, 2022

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

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

No branches or pull requests

6 participants