MPICH Fortran test `greqf` uses `extrastate` wrong

[eschnett]

The greqf.f test in MPICH 5.0.1 (directory test/mpi/f77/pt2pt/) contains the line

      extrastate = extrastate - 1

I think this is wrong. extrastate is conceptually a pointer (it's a void* in C, and MPI_Aint in Fortran), and the free_fn callback must not modify it.

greqf90 and greqf08 contain equivalent code which is also wrong.

The Fortran 2008 MPI standard shows this argument as

INTEGER(KIND=MPI_ADDRESS_KIND), INTENT(IN) :: extra_state

which implies that the callee must not modify it. Instead, MPICH captures this argument by reference and modifies it in free_fn. I think that the Fortran subroutine is a transcription of the C function, and that extrastate would usually be a pointer in both cases.

[hzhou]

Was the INTENT(IN) attribute from the signature of MPI_Grequest_start? It is a input argument for MPI_Grequest_start, but not for the callbacks. Otherwise, what's the point of extra_state if nobody can update it?

I think that the Fortran subroutine is a transcription of the C function,

You'll discover that was not historically true. Fortran comes before C and the MPI standard was originally designed to work with both Fortran and C natively.

[eschnett]

I think that extra_state is a pointer to the state (same as in C). While the state can be modified, the address in memory of the state remains fixed. That explains why extra_state is MPI_Aint, an address.

[hzhou]

I think that extra_state is a pointer to the state (same as in C).

I see where you come from. It is a bit ambiguous.

With C, void * can be cast to anything. The actual type is determined by the user code. extra_state passes through MPI as a context. MPI implementation is guaranteed not change extra_state, as the INTENT(IN) signifies. I think that is all what the MPI standard says. How the user code uses extra_state is up to the user. Even in C, if user want to cast an int to void * is a valid usage.

The greqf.f test in MPICH 5.0.1 (directory test/mpi/f77/pt2pt/) contains the line

     extrastate = extrastate - 1

This is similar, IMO, as in c that we cast an int to void *, passed to MPI, and the later modify the int value. It will likely cause issue if the modification happens while MPI is holding the context as it will be invalid context if the reference to "context" got changed in-between. However, one can argue that the free callback marks the end of MPI holding this context. Since MPI will no longer hold this context, the user code can change it with no consequence what-so-ever.

What actual issue are you seeing ?

[eschnett]

I am implementing Fortran MPI bindings mpif based on the new MPI ABI. I am running the MPICH Fortran test suite on these bindings, and this test is failing. The reason is that mpif captures extra_state by value, not by reference, so the change that happens in the test case never makes it back to the caller. Internally, mpif converts extra_state from MPI_Aint to void* and then forwards the call to underlying MPI implementation (e.g. MPICH).

[hzhou]

How do you handle the callback? Do you let the underlying MPI directly call the callback as a C function or do you intercept with a proxy and call the Fortran callback in turn? I think you have to do that latter since there is no direct API tell MPI what is the language of the callbacks. If you do the latter, then you should call the user callback by Fortran convention, which is to pass extra_state by reference, right?

[eschnett]

Yes, there is a trampoline, and extra_state is passed by reference:

struct grequest_box {
  mpif_fortran_procedure query_fn;
  mpif_fortran_procedure free_fn;
  mpif_fortran_procedure cancel_fn;
  // The registered value, copied rather than aliased; see the header
  MPI_Aint extra_state;
};

int mpif_grequest_free_trampoline(void *extra_state) {
  struct grequest_box *const box = extra_state;
  MPI_Fint f_ierror = MPI_SUCCESS;
  ((fortran_grequest_free_fn)box->free_fn)(&box->extra_state, &f_ierror);
  // The last callback this request will make, so the box goes with it
  free(box);
  return f_ierror;
}

[hzhou]

I think you need store the reference of extra_state in box rather than value. Fortran user functions rely on the pass-by-ref convention. The address to your box field is meaningless to Fortran users.

[eschnett]

Yes, if I want to use your interpretation of the MPI standard, then I need to capture the address of extra_state (I assume that is what MPICH is doing). However, I did interpret the standard differently when writing this function (and when opening this issue).