Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1407 +/- ##
=======================================
Coverage 69.33% 69.33%
=======================================
Files 6122 6124 +2
Lines 487027 487082 +55
Branches 23268 23268
=======================================
+ Hits 337686 337741 +55
Misses 148903 148903
Partials 438 438
Continue to review full report in Codecov by Harness.
|
|
Hi @mohawk2, I like the idea a lot, since it would solve all the problems we currently have with error testing (e.g., it doesn't work with shared Windows libs or two-level namespaces) and make it easier for users to use a custom xerbla on all platforms. A few things to consider:
|
|
There is also a possibility of defining it as a weak symbol so tests can override the regular one. that can limit the surgery to only xerbla. overall I don't think xerbla is a good way to report errors back but it is what it is for now. |
Yes, that's how it's currently done in the CBLAS test suite. Won't work on Windows, though. Also, doing that in Fortran would depend on the compilers supporting it (nagfor doesn't without major hacks, ifx & flang maybe). |
|
I do use it on windows albeit on a C translation and Clang.
ah then nevermind. Sorry for the noise |
Ah, you're right; Clang and Intel do support it on Windows for static libraries. Did you try it with a shared library? Because last time I checked, that was the blocker. |
|
Thank you all for your quick responses! As noted in my edit of the PR description, I'm intending to add CBLAS and LAPACKE equivalents. I'll ponder the names, I hadn't properly checked prior art and will do so now. I'll ponder the idea of different BLAS and LAPACK handlers - is it really true that the current implementation essentially has two similarly-named symbols that are each are getting overridden by the user-supplied one? If so, that seems horrible. I might need to make differently-named entry points for each that call each other, since the user expectation will be a full override for both. |
Yes, I think using the same REGISTER_XERBLA symbol name in BLAS and LAPACK could mean it only replaces one of the handlers (if we use separate shared libraries). My sugestion would be to have the separate symbols and then one symbol that does both and lives in LAPACK. Something like this: Edit: One issue would be that when we are compiling LAPACK with a vendor BLAS lib and they don't implement |
My pondering came up with a similar result, but not identical (see below). The above discussion included the point that prior art (
Edit 1: specify that
Yes, that sounds like a good approach. (Edit: now implemented, see below) |
|
My thinking so far on a CBLAS approach (The LAPACKE version will be mutatis mutandis, so will not be addressed separately). CBLAS has an actual
For the CBLAS versions of the 3 procedures plus
|
REGISTER_XERBLA - override the error-handler without linker stuffSET_XERBLA - override the error-handler without linker stuff
149c4ce to
2449269
Compare
@ACSimon33 I've had to adjust the plan a bit; a BLAS |
2449269 to
3d68c71
Compare
|
@mohawk2 Can you please merge the current master back into your branch? We now fixed numerical issues and errors on all platforms that are tested in CI, and the CI jobs are now configured to fail if new errors occur. |
Working on this now, thank you for highlighting this. |
34e2172 to
148a3b6
Compare
|
@ACSimon33 @langou I've just rebased this to latest |
|
@ACSimon33 @langou Sorry about this! I'm pretty sure the Makefile jobs in CI will fail, because I missed updating the Makefiles to add the 2 new files due to inept |
|
@ACSimon33 @langou Pushed. Works locally (and confirmed that without the new commit, it broke). |
|
Flang is failing with: Some preliminary searching indicates this is because it doesn't like the combination of procedure pointers, and @ACSimon33 How do you want to proceed? If you can't accept breaking Edit: also codecov is saying it's failing because this part doesn't get exercised: if (.not. associated(active_callback)) then
print *, 'Error: LAPACK_XERBLA called but no callback registered'
stop
end ifI had that in there because I thought it was better than a |
|
@ACSimon33 (sorry for all the highlights) I have opened #1411 further to my last comment, and will get started on instead using |
|
@mohawk2 Try removing the |
Right, we need it there. I don't have a specific preference about how we fix this, since I'm not a Fortran expert. Using a C function pointer with an internal indirection would work like this, I think: But as I said, I'm fine with either way. |
@ACSimon33 Great, I'm already figuring how to wend my way through the logic and regex stuff in the CMake code. Please could you look at #1411? That would really help. |
4acfc83 to
9e2c42d
Compare
|
Note to reviewers: This now incorporates part of #1411, which should be reviewed and merged first, else this change will include lots of noise. Other updates: I have changed the |
316f4f0 to
b41a779
Compare
b5b8e7f to
d7c905a
Compare
|
The above commit adds the probe for whether a user-supplied BLAS has cmake -B build \
-DBUILD_TESTING=ON \
-DCMAKE_BUILD_TYPE=Debug \
-DBLAS_LIBRARIES=/opt/homebrew/opt/openblas/lib/libopenblas.dylib \
-DBUILD_INDEX64_EXT_API=OFF && \
cmake --build build && \
ctest --parallel --build-dir build |
42a4586 to
ecde827
Compare
ecde827 to
36c4c7b
Compare
36c4c7b to
8aa71c8
Compare
|
Problems dealt with so far:
Now |
The four ifx failures come from an ifx bug, not from the new xerbla logic. They go away if What fails ctest fails the same four tests on every ifx job:
Cause When an external function returns a procedure pointer, and the caller declares it through an interface body, ifx treats the function name as a local procedure-pointer variable. For
I also tried writing the function without I reproduced this on Windows with 2026.1.1 (the version the Windows jobs install) and with 2025.1. Minimal reproducer
module cbmod
implicit none
abstract interface
subroutine cb_interface(srname, info)
character(*), intent(in) :: srname
integer, intent(in) :: info
end subroutine
end interface
procedure(cb_interface), pointer :: active_cb => null()
end module
function get_cb() result(cb_ret)
use cbmod
implicit none
procedure(cb_interface), pointer :: cb_ret
cb_ret => active_cb
end function
program main
implicit none
interface
subroutine cb_interface(srname, info)
character(*), intent(in) :: srname
integer, intent(in) :: info
end subroutine
function get_cb() result(cb_ret)
import :: cb_interface
implicit none
procedure(cb_interface), pointer :: cb_ret
end function
end interface
procedure(cb_interface), pointer :: p
p => get_cb()
print *, 'associated:', associated(p)
end programThe expected output is Possible fix Make the getters subroutines: diff --git a/BLAS/SRC/xerbla_blas.f90 b/BLAS/SRC/xerbla_blas.f90
--- a/BLAS/SRC/xerbla_blas.f90
+++ b/BLAS/SRC/xerbla_blas.f90
@@ -34,13 +34,14 @@
!> CHARACTER*(*), INTENT(IN) :: SRNAME
!> INTEGER, INTENT(IN) :: INFO
!> END SUBROUTINE
-!> FUNCTION GET_BLAS_XERBLA() RESULT(CB_RET)
+!> SUBROUTINE GET_BLAS_XERBLA(CB)
+!> IMPORT :: XERBLA_INTERFACE
!> IMPLICIT NONE
-!> PROCEDURE(XERBLA_INTERFACE), POINTER :: CB_RET
-!> END FUNCTION
+!> PROCEDURE(XERBLA_INTERFACE), POINTER, INTENT(OUT) :: CB
+!> END SUBROUTINE
!> END INTERFACE
!> PROCEDURE(XERBLA_INTERFACE), POINTER :: ALREADY_CB
-!> ALREADY_CB => GET_BLAS_XERBLA()
+!> CALL GET_BLAS_XERBLA(ALREADY_CB)
!> END PROGRAM HELLO
!> \endverbatim
!
@@ -94,9 +95,13 @@ subroutine set_blas_xerbla(cb)
active_callback => cb
end
-function get_blas_xerbla() result(cb_ret)
+! A subroutine, not a function returning a procedure pointer: ifx
+! miscompiles references to such a function when the caller declares
+! it through an interface body (ICE in 2025.1, a call through a null
+! pointer in 2026.1).
+subroutine get_blas_xerbla(cb)
use xerbla_blas
implicit none
- procedure(xerbla_interface), pointer :: cb_ret
- cb_ret => active_callback
+ procedure(xerbla_interface), pointer, intent(out) :: cb
+ cb => active_callback
end
diff --git a/BLAS/TESTING/cblat2.f b/BLAS/TESTING/cblat2.f
--- a/BLAS/TESTING/cblat2.f
+++ b/BLAS/TESTING/cblat2.f
@@ -170,16 +170,16 @@
CHARACTER*(*), INTENT(IN) :: SRNAME
INTEGER, INTENT(IN) :: INFO
END SUBROUTINE
- FUNCTION GET_BLAS_XERBLA() RESULT(CB_RET)
+ SUBROUTINE GET_BLAS_XERBLA(CB)
IMPORT :: XERBLA_INTERFACE
IMPLICIT NONE
- PROCEDURE(XERBLA_INTERFACE), POINTER :: CB_RET
- END FUNCTION
+ PROCEDURE(XERBLA_INTERFACE), POINTER, INTENT(OUT) :: CB
+ END SUBROUTINE
END INTERFACE
PROCEDURE(XERBLA_INTERFACE), POINTER :: ALREADY_CB
* .. Executable Statements ..
CALL CPU_TIME( S1 )
- ALREADY_CB => GET_BLAS_XERBLA()
+ CALL GET_BLAS_XERBLA(ALREADY_CB)
CALL SET_BLAS_XERBLA(XER_REPLACE)
*
* Read name and unit number for summary output file and open file.
diff --git a/SRC/xerbla_lapack.f90 b/SRC/xerbla_lapack.f90
--- a/SRC/xerbla_lapack.f90
+++ b/SRC/xerbla_lapack.f90
@@ -34,13 +34,14 @@
!> CHARACTER*(*), INTENT(IN) :: SRNAME
!> INTEGER, INTENT(IN) :: INFO
!> END SUBROUTINE
-!> FUNCTION GET_LAPACK_XERBLA() RESULT(CB_RET)
+!> SUBROUTINE GET_LAPACK_XERBLA(CB)
+!> IMPORT :: XERBLA_INTERFACE
!> IMPLICIT NONE
-!> PROCEDURE(XERBLA_INTERFACE), POINTER :: CB_RET
-!> END FUNCTION
+!> PROCEDURE(XERBLA_INTERFACE), POINTER, INTENT(OUT) :: CB
+!> END SUBROUTINE
!> END INTERFACE
!> PROCEDURE(XERBLA_INTERFACE), POINTER :: ALREADY_CB
-!> ALREADY_CB => GET_LAPACK_XERBLA()
+!> CALL GET_LAPACK_XERBLA(ALREADY_CB)
!> END PROGRAM HELLO
!> \endverbatim
!>
@@ -97,11 +98,15 @@ subroutine set_lapack_xerbla(cb)
active_callback => cb
end
-function get_lapack_xerbla() result(cb_ret)
+! A subroutine, not a function returning a procedure pointer: ifx
+! miscompiles references to such a function when the caller declares
+! it through an interface body (ICE in 2025.1, a call through a null
+! pointer in 2026.1).
+subroutine get_lapack_xerbla(cb)
use xerbla_lapack
implicit none
- procedure(xerbla_interface), pointer :: cb_ret
- cb_ret => active_callback
+ procedure(xerbla_interface), pointer, intent(out) :: cb
+ cb => active_callback
end
subroutine set_xerbla(cb)
diff --git a/TESTING/LIN/cchkrfp.f b/TESTING/LIN/cchkrfp.f
--- a/TESTING/LIN/cchkrfp.f
+++ b/TESTING/LIN/cchkrfp.f
@@ -118,17 +118,17 @@
CHARACTER*(*), INTENT(IN) :: SRNAME
INTEGER, INTENT(IN) :: INFO
END SUBROUTINE
- FUNCTION GET_LAPACK_XERBLA() RESULT(CB_RET)
+ SUBROUTINE GET_LAPACK_XERBLA(CB)
IMPORT :: XERBLA_INTERFACE
IMPLICIT NONE
- PROCEDURE(XERBLA_INTERFACE), POINTER :: CB_RET
- END FUNCTION
+ PROCEDURE(XERBLA_INTERFACE), POINTER, INTENT(OUT) :: CB
+ END SUBROUTINE
END INTERFACE
PROCEDURE(XERBLA_INTERFACE), POINTER :: ALREADY_CB
* ..
* .. Executable Statements ..
*
- ALREADY_CB => GET_LAPACK_XERBLA()
+ CALL GET_LAPACK_XERBLA(ALREADY_CB)
CALL SET_XERBLA(XER_REPLACE)
S1 = SECOND( )
FATAL = .FALSE.With the patch applied:
I haven't tried gfortran, flang or nagfor locally. |
Description
Currently, to override BLAS/LAPACK's handling of errors involves defining a
xerbla_(or sometimesxerbla) symbol in your library/executable. This may not work in 2-level namespace environments like macOS.Instead, this PR allows you to call
SET_XERBLA(Fortran) with a replacement handler, orNULL()to reset to default. There is also anGET_(library)_XERBLA. This does not address that the new handler must be a Fortran-compatible routine.There are CBLAS (
cblas_set_xerbla) and LAPACKE (LAPACKE_set_xerbla) handlers on the way, but I'm putting this up for review as I believe it already adds value.I have marked below that the documentation has been updated, because I added a paragraph in the doc-comments of
xerbla.f. If more is needed, please let me know.EDIT: updated function names.
Checklist