Some pointer handling in OpenIFS seems to rely on compiler behaviour which is not guaranteed by the Fortran standard, in particular when a function creates a pointer to an array that was passed to it as an argument, and the pointer is expected to be valid beyond the end of the function.
One example is in src/ifs/phys_ec/local_state_ini.F90:
SUBROUTINE LOCAL_STATE_INI(KDIM, keymask, state_t0, state_tmp, & & tendency_dyn, tendency_cml, tendency_tmp, tendency_vdf, tendency_loc, & & PSTATE_ARRG,PSTATE_ARRL, PSTATE_CLD, & & PU, PV, PT ,PGFL, PTENGMV, PTENGFL) ... TYPE (STATE_TYPE) ,INTENT(OUT) :: state_t0 , tendency_dyn, tendency_cml REAL(KIND=JPRB) ,INTENT(IN), TARGET :: PT(KDIM%KLON,KDIM%KLEV) ... state_t0%T => PT ! here a pointer to an input argument is assigned and returned in an output argument
When the dummy argument is declared with an explicit size (as in the function above), it is system-dependent whether or not pointers associated with the dummy argument are valid after the subroutine finishes.
If, however, the dummy argument is declared as an assumed-shape array (i.e. real :: a(:, ; ) ), and both the dummy argument and the actual argument are declared as target, the pointer is guaranteed to be valid also outside the scope of the subroutine.
Source: Modern Fortran: Style and Usage, p. 70,71
To make LOCAL_STATE_INI safe, we believe all the input arrays should be declared with colons for size (assumed-shape).
We have not seen the official OpenIFS misbehave because of this, but with a modified version, we found a case where the pointers set up in LOCAL_STATE_INI became invalid while the data they should point to still was in scope. This occured with the intel compier, but not with gfortran. The reason was that in a function call, a temporary copy of the array was made and passed to the function. The pointers then got associated with this temporary copy, and became invalid when the copy went out of scope. In our case the temporary copy was made when passing a slice of an array as a function argument.
When we changed the declarations, PT(KDIM%KLON,KDIM%KLEV) → PT(:,:) and similar for the other parameters, this problem disappeared.
In general, passing assumed-shape arrays seem to be recommended over explicit-size arrays. This could have some performance benefits, if they avoid temporary copies of the data. Where EC_PHYS_DRV() calls EC_PHYS() several sliced arrays are passed, and intel fortran seems to create copies here.
Fredrik Jansson & Gijs van den Oord
3 Comments
Unknown User (nagc)
Hi Fredrik & Gijs,
Thanks for the report. It appears this may also affect later versions of IFS. This has been passed on to our IFS team to look at.
From the code you've looked at, does this only affect EC_PHYS & LOCAL_STATE_INI?
Glenn
Unknown User (jansson)
Hi Glenn,
We have mostly looked at the EC_PHYS code. There we only saw the problem in LOCAL_STATE_INI, we quickly looked for it in other places but haven't searched systematically.
We found another confusing array passing convention, though. There are many subroutines where array parameters are defined with fixed ranges. Sometimes, in function calls arrays are passed by referencing only the first element, like here, callpar.f90, line 896, in particular the PSAVTEND argument.
CALL CLOUD_LAYER(KDIM, LLSLPHY, LLMAINCALL, PAUX, state_t0, tendency_cml, tendency_tmp, tendency_dyn, && tendency_vdf, PRAD, PSAVTEND(1,1,MSAT_SAVTEND),PSURF, LLKEYS, AUXL, FLUX, PDIAG, tendency_loc)As far as we understand, this is allowed by the fortran standard, and means the full first and second dimension of the array will be passed, since the subroutine expects an array of a certain size. But this could also be written as
PSAVTEND(:,:,MSAT_SAVTEND), which is clearer. Changing this might influence wether the compiler is allowed to make a temporary copy, maybe the subroutine could be changed to accept assumed-shape arrays at the same time. Here is a promise that no copying is done if the array parameter is declared assumed shape.Fredrik
Unknown User (nagc)
Hi Fredrik,
The (1,1,..) approach is used to ensure passing by reference to stop the compiler making temporary copies where we find it happening, particularly in parts of the code where it has an impact on the timing. Unfortunately, it's more prevalent in the code than it needs to be because this approach gets copied. So it's probably more common than it needs but we'd have to do some internal testing to ensure removing it with our latest compilers doesn't have an impact.
Glenn