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.

Sign up for GitHub

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

Jump to bottom

Some tests fail when compiled with -fcheck=bounds #512

Closed
weslleyspereira opened this issue Mar 12, 2021 · 8 comments · Fixed by #515
Closed

Some tests fail when compiled with -fcheck=bounds #512

weslleyspereira opened this issue Mar 12, 2021 · 8 comments · Fixed by #515
Milestone
LAPACK 3.9.1

Comments

@weslleyspereira
Copy link
Collaborator

Some tests fail when compiled with -fcheck=bounds.

The following tests FAILED:

	 14 - CBLAS-xscblat2 (Failed)

	 15 - CBLAS-xscblat3 (Failed)

	 17 - CBLAS-xdcblat2 (Failed)

	 18 - CBLAS-xdcblat3 (Failed)

	 20 - CBLAS-xccblat2 (Failed)

	 21 - CBLAS-xccblat3 (Failed)

	 23 - CBLAS-xzcblat2 (Failed)

	 24 - CBLAS-xzcblat3 (Failed)

	 25 - example1_CBLAS (Failed)

	 26 - example2_CBLAS (Failed)

	120 - example_DGELS_rowmajor (Failed)

	121 - example_DGELS_colmajor (Failed)

The errors are:

At line 183 of file /home/travis/build/weslleyspereira/lapack/SRC/dgels.f
Fortran runtime error: Actual string length is shorter than the declared one for dummy argument 'trans' (-681041696/1)

At line 188 of file /home/travis/build/weslleyspereira/lapack/BLAS/SRC/dgemm.f
Fortran runtime error: Actual string length is shorter than the declared one for dummy argument 'transb' (0/1)

At line 157 of file /home/travis/build/weslleyspereira/lapack/BLAS/SRC/sgemv.f
Fortran runtime error: Actual string length is shorter than the declared one for dummy argument 'trans' (0/1)

At line 157 of file /home/travis/build/weslleyspereira/lapack/BLAS/SRC/dgemv.f
Fortran runtime error: Actual string length is shorter than the declared one for dummy argument 'trans' (0/1)

At line 188 of file /home/travis/build/weslleyspereira/lapack/BLAS/SRC/dgemm.f
Fortran runtime error: Actual string length is shorter than the declared one for dummy argument 'transb' (0/1)

At line 188 of file /home/travis/build/weslleyspereira/lapack/BLAS/SRC/sgemm.f
Fortran runtime error: Actual string length is shorter than the declared one for dummy argument 'transa' (0/1)

At line 188 of file /home/travis/build/weslleyspereira/lapack/BLAS/SRC/cgemm.f
Fortran runtime error: Actual string length is shorter than the declared one for dummy argument 'transb' (0/1)

At line 183 of file /home/travis/build/weslleyspereira/lapack/SRC/dgels.f
Fortran runtime error: Actual string length is shorter than the declared one for dummy argument 'trans' (0/1)

At line 159 of file /home/travis/build/weslleyspereira/lapack/BLAS/SRC/cgemv.f
Fortran runtime error: Actual string length is shorter than the declared one for dummy argument 'trans' (0/1)

At line 188 of file /home/travis/build/weslleyspereira/lapack/BLAS/SRC/zgemm.f
Fortran runtime error: Actual string length is shorter than the declared one for dummy argument 'transb' (-1406718144/1)

At line 159 of file /home/travis/build/weslleyspereira/lapack/BLAS/SRC/zgemv.f
Fortran runtime error: Actual string length is shorter than the declared one for dummy argument 'trans' (-2068420400/1)

At line 157 of file /home/travis/build/weslleyspereira/lapack/BLAS/SRC/dgemv.f
Fortran runtime error: Actual string length is shorter than the declared one for dummy argument 'trans' (0/1)

So far I couldn't find any problems during the debug.

Travis results: https://github.com/weslleyspereira/lapack/runs/2098841918
Branch: https://github.com/weslleyspereira/lapack/tree/try-flag--fcheck=bounds
@langou
Copy link
Contributor

langou commented Mar 13, 2021 •
edited
Loading

Ah, the joy of passing character from C to Fortran.
We start with LAPACKE/example/example_DGELS_colmajor.c

lapack/LAPACKE/example/example_DGELS_colmajor.c

Line 89 in 1827da0

info = LAPACKE_dgels(LAPACK_COL_MAJOR,'N',m,n,nrhs,*A,lda,*b,ldb);

then we use LAPACKE

lapack/LAPACKE/src/lapacke_dgels.c

Line 36 in 1827da0

lapack_int LAPACKE_dgels( int matrix_layout, char trans, lapack_int m,

that calls the work layer at line

lapack/LAPACKE/src/lapacke_dgels.c

Line 73 in 1827da0

info = LAPACKE_dgels_work( matrix_layout, trans, m, n, nrhs, a, lda, b, ldb,

the work layer is here:

lapack/LAPACKE/src/lapacke_dgels_work.c

Line 36 in 1827da0

lapack_int LAPACKE_dgels_work( int matrix_layout, char trans, lapack_int m,

Finally we call LAPACK at:

lapack/LAPACKE/src/lapacke_dgels_work.c

Line 86 in 1827da0

LAPACK_dgels( &trans, &m, &n, &nrhs, a_t, &lda_t, b_t, &ldb_t, work,

And here is LAPACK:

lapack/SRC/dgels.f

Line 183 in 1827da0

SUBROUTINE DGELS( TRANS, M, N, NRHS, A, LDA, B, LDB, WORK, LWORK,

Seems like the &trans is not doing what we expect it to do.

@martin-frbg
Copy link
Collaborator

probably just #339 (#440) again - the GCC folks "fixed" the behavior of their compiler during code generation to allow"traditional" single-character argument passing again but they did not disarm the warning.

@weslleyspereira
Copy link
Collaborator Author

Yes, I agree. I forgot about #440. Maybe we can continue discussing from there.

@weslleyspereira weslleyspereira mentioned this issue Mar 13, 2021
Closed
@mgates3
Copy link
Contributor

mgates3 commented Mar 15, 2021

Most Fortran compilers (all current ones that I have encountered) add a strlen argument for each character string to the end of the argument list. For instance, in LAPACK++ we have:

#define LAPACK_stptrs LAPACK_GLOBAL(stptrs,STPTRS)
void LAPACK_stptrs(
    char const* uplo, char const* trans, char const* diag,         // <= note
    lapack_int const* n,
    lapack_int const* nrhs,
    float const* ap,
    float* b, lapack_int const* ldb,
    lapack_int* info
    #ifdef LAPACK_FORTRAN_STRLEN_END
    , unsigned uplo_len, unsigned trans_len, unsigned diag_len    // <= note
    #endif
    );

The wrapper calls this with a strlen of 1:

    LAPACK_stptrs(
        &uplo_, &trans_, &diag_, &n_, &nrhs_,
        AP,
        B, &ldb_, &info_
        #ifdef LAPACK_FORTRAN_STRLEN_END
        , 1, 1, 1                                                // <= note
        #endif
    );

This has been fixed in LAPACK++ but not yet in BLAS++.

The better fix would be to write a Fortran routine using the Fortran 2003 bind(C) interface, which handles the C => Fortran string conversion in a Fortran-sanctioned way. E.g., adding an LAPACKE_stptrs_fortran wrapper written in Fortran 2003:

LAPACKE_stptrs => LAPACKE_stptrs_work => LAPACKE_stptrs_fortran => stptrs_

In the past, I think some Fortran compilers stuck the strlen as an argument immediately after the string, e.g.,

void LAPACK_stptrs(
    char const* uplo, unsigned uplo_len,     // <= note
    char const* trans, unsigned trans_len,   // <= note
    char const* diag, unsigned diag_len,     // <= note
    lapack_int const* n,
    lapack_int const* nrhs,
    float const* ap,
    float* b, lapack_int const* ldb,
    lapack_int* info
    );

Also in the past, there were some Fortran compilers that had a struct, maybe something like:

void LAPACK_stptrs(
    struct fortran_string uplo,     // <= note
    struct fortran_string trans,    // <= note
    struct fortran_string diag,     // <= note
    lapack_int const* n,
    lapack_int const* nrhs,
    float const* ap,
    float* b, lapack_int const* ldb,
    lapack_int* info
    );

I can't find references for these last 2 conventions at the moment, and I haven't run into them in practice.

Mark

@mgates3
Copy link
Contributor

mgates3 commented Mar 15, 2021

One caution: with the added strlen arguments, your Fortran prototypes won't match other folks' strlen arguments (say, in MKL, ESSL). So be careful to not put them in public headers that would get included along with, say, mkl.h.

For instance, in LAPACK++
#include <lapack.hh>
does not include
#include <lapack/fortran.h>
which has to be included explicitly if desired. fortran.h is basically viewed as an internal header.

Mark

@mgates3
Copy link
Contributor

mgates3 commented Mar 15, 2021

For CBLAS, it seems cblas_f77.h is fine for that purpose — as far as I can see, it isn't included via cblas.h, and is only included in CBLAS/src/*.c
See:

grep cblas_f77.h `git ls-files`

@weslleyspereira
Copy link
Collaborator Author

Thanks @mgates3 ! I will try your suggestions in the following commit here.

@weslleyspereira weslleyspereira mentioned this issue Mar 16, 2021
Merged
1 task
@weslleyspereira
Copy link
Collaborator Author

I am writing #515 to try solving this issue.

@julielangou julielangou added this to the LAPACK 3.9.1 milestone Mar 25, 2021
@weslleyspereira weslleyspereira mentioned this issue Jul 27, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
LAPACK 3.9.1
Development

Successfully merging a pull request may close this issue.

Try flag -fcheck=all weslleyspereira/lapack
5 participants
@langou @martin-frbg @weslleyspereira @julielangou @mgates3