New ForTrilinos.h and forerror module, prefixed ierr and serr #169
Conversation
aprokop
force-pushed
the
ierr_fix
branch
2 times, most recently
from
23322a4
to
6aae78a
Compare
Codecov Report
@@ Coverage Diff @@
## develop #169 +/- ##
===========================================
- Coverage 38.21% 38.12% -0.09%
===========================================
Files 31 33 +2
Lines 13454 13462 +8
===========================================
- Hits 5141 5133 -8
- Misses 8313 8329 +16
Continue to review full report at Codecov.
|
|
Rebased on top of |
|
I'm pretty OK with merging this as it is. The main goal of removing the global space pollution has been achieved. It could be further improved, though, to simplify users life:
@tjfulle Could move the |
aprokop
changed the title
|
Ya, I can move Do you want to merge first and then I’ll make changes on top? User code should mostly use the check error macro, so adding extra getter/setter is probably superfluous. And, I don’t want to modify the swig... |
Don't know, all this sound pretty bad to me. But I can't find a better one. I guess
Sure. But right now there is a hidden problem with this PR somewhere. Everything is fine and dandy building with gcc, but flang build fails for Tpetra tutorials with a linker error: I really have trouble figuring out why. |
|
The dependence of CHECK_IERR on To the other concern, I believe that if the macro is called, the code should stop and give an error message. If a user wants to query |
|
Weird, compiling but in flang it gives |
|
Even more fun: removing initialization of So, somehow, initialization of |
|
Just noticed that flang produces a warning which seems to indicate that maybe the code is wrong: |
|
Stupid thought, Do you get the same error if the variables are named ierr and serr? |
|
The following patch seems to fix the issue: diff --git a/src/utils/src/forerror.f90 b/src/utils/src/forerror.f90
index 7d698ea..5e29ef0 100644
--- a/src/utils/src/forerror.f90
+++ b/src/utils/src/forerror.f90
@@ -23,8 +23,8 @@ module forerror
! PARAMETERS
integer(C_INT), parameter, public :: SWIG_FORTRAN_ERROR_STRLEN = 1024_C_INT
- integer(C_INT), bind(C) :: fortrilinos_ierr = 0
- character(kind=C_CHAR, len=1024), bind(C) :: fortrilinos_serr = ""
+ integer(C_INT), bind(C) :: fortrilinos_ierr
+ character(kind=C_CHAR, len=1024), bind(C) :: fortrilinos_serr
end module
diff --git a/src/utils/src/forerrorFORTRAN_wrap.cxx b/src/utils/src/forerrorFORTRAN_wrap.cxx
index 2c45228..8a08fa4 100644
--- a/src/utils/src/forerrorFORTRAN_wrap.cxx
+++ b/src/utils/src/forerrorFORTRAN_wrap.cxx
@@ -217,8 +217,8 @@ template <typename T> T SwigValueInit() {
extern "C" {
-extern int fortrilinos_ierr;
-extern char fortrilinos_serr[1024];
+int fortrilinos_ierr = 0;
+char fortrilinos_serr[1024] = "";
}@sethrj Thoughts? |
|
@tjfulle No, but that also without the separate |
|
According to this: It seems that we were trying to bind 2 Fortran variables to C variables with external linkage, and the compiler caught us doing the naughty. |
|
What????? This is good reason to use many compilers before releasing a product... Alternatively, can |
I guess it could. I just don't want to spend too much time on this, and want to see what @sethrj says. |
|
@aprokop @tjfulle I still don't understand the motivation for these changes. I considered and tried multiple ways of handling this when developing the error handling. Do you want each module to have separate error variables? That would break the interface because a function in one module wouldn't know if an upstream module had thrown an unhandled exception on a previous call. If the purpose is not to have #define SWIG_FORTRAN_ERROR_INT fortrilinos_ierr
#define SWIG_FORTRAN_ERROR_STR fortrilinos_serr
#define SWIG_FORTRAN_ERROR_STRLEN 1024
%include <exception.i>and then alias the variables as needed downstream: program test_foo
use forerror, only : ierr => fortrilinos_ierr, serr => fortrilinos_serr
... |
|
My motivation in bringing it up originally was to discuss whether the global error handling should be done independent of |
|
OK @aprokop I've got a new version of SWIG that only binds the one value |
|
@aprokop this looks much better! There seems to be one inconsistency, but only for the tests. |
|
@tjfulle Good point! I'll fix that. |
|
@aprokop also, to save space on macro length, the FORTRILINOS_TEST_IERR macro doesn't need to take the error message, it can call it itself from within and only if FORTRILINOS_IERR /= 0 |
|
@tjfulle |
|
Pushed the update. One thing that remains to be done is to actually test that we get a proper string of the exception. I am actually not sure. |
|
@aprokop, ya, the macro TEST_IERR calls the subroutine fortrilinos_test_ierr (in fortest.f90). If fortest.f90 Typing with thumbs, hopefully that made sense |
|
If we’ve taken |
- Modify fortest_ierr to not pass ierr and serr; - include ForTrilinos.h in FortranTestUtilities.h
|
@tjfulle I think I did what you wanted me to, take a look. Independently, should we add printing of serr to other testing macros? Otherwise, you will get failures but it's going to be hard to figure out why. Are the macros supposed to work only with I also checked that indeed we can get and print proper Trilinos exception in Fortran. So I think we are good here. |
|
@aprokop , it took me a minute to think of what you were saying, but if I understand, you are saying any test function that queries |
|
Actually, most of the other test macros just check for correctness and operate independent of |
Compiles but setfaults.