Fortran: MPI_Alltoallw/MPI_Ialltoallw using MPI_IN_PLACE do not ignore sendtypes

[geimer]

According to the standard, MPI_Alltoallw and MPI_Ialltoallw should ignore the sendcounts, sdispls, and sendtypes arguments if sendbuf is MPI_IN_PLACE. Open MPI (tested with 3.0.0) takes this into account for sendcounts and sdispls, but not for sendtypes. This leads to a segmentation fault with the following test code:

PROGRAM main
    USE mpi
    IMPLICIT NONE

    INTEGER :: size
    INTEGER :: rank
    INTEGER :: ierr
    INTEGER :: i

    INTEGER, ALLOCATABLE :: recvbuf(:)
    INTEGER, ALLOCATABLE :: recvcounts(:)
    INTEGER, ALLOCATABLE :: recvdispls(:)
    INTEGER, ALLOCATABLE :: recvtypes(:)
    INTEGER, ALLOCATABLE :: NULL(:)

    CALL MPI_Init(ierr)
    CALL MPI_Comm_size(MPI_COMM_WORLD, size, ierr)
    CALL MPI_Comm_rank(MPI_COMM_WORLD, rank, ierr)

    ALLOCATE(recvbuf(size))
    ALLOCATE(recvcounts(size))
    ALLOCATE(recvdispls(size))
    ALLOCATE(recvtypes(size))
    DO i = 1, size
        recvcounts(i) = 1
        recvdispls(i) = i * (storage_size(recvbuf) / 8)
        recvtypes(i)  = MPI_INTEGER
        recvbuf(i)    = rank
    END DO

    CALL MPI_Alltoallw(MPI_IN_PLACE, NULL, NULL, NULL, &
                       recvbuf, recvcounts, recvdispls, recvtypes, &
                       MPI_COMM_WORLD, ierr)

    CALL MPI_Finalize(ierr)
END PROGRAM

It seems that the loops at

ompi/ompi/mpi/fortran/mpif-h/alltoallw_f.c

Lines 95 to 99 in e172849

	while (size > 0) {
	c_sendtypes[size - 1] = PMPI_Type_f2c(sendtypes[size - 1]);
	c_recvtypes[size - 1] = PMPI_Type_f2c(recvtypes[size - 1]);
	--size;
	}

and

ompi/ompi/mpi/fortran/mpif-h/ialltoallw_f.c

Lines 96 to 100 in e172849

	while (size > 0) {
	c_sendtypes[size - 1] = PMPI_Type_f2c(sendtypes[size - 1]);
	c_recvtypes[size - 1] = PMPI_Type_f2c(recvtypes[size - 1]);
	--size;
	}

are the culprit.

[ggouaillardet]

thanks for the bug report !

do you want to issue a PR by yourself ?
If not, I will be happy to fix this bug and credit you for reporting it.

[geimer]

Thinking about a potential solution, I'm not quite sure why the conversions of sendcounts and sdispls don't segfault. I don't see any handling of NULL or MPI_IN_PLACE in the OMPI_ARRAY_FINT_2_INT macro... Feels like a more general issue that might require a deeper understanding of how things are usually tackled in Open MPI. Or am I missing something here?

[ggouaillardet]

there is no conversion when MPI_Fint and int have the same size (and this is the most common case), feel free to refer to the (#ifdef'ed) macro definition(s) in ompi/mpi/fortran/base/fint_2_int.h.

If you build Open MPI with 8 bytes integer (-i8 option with Intel compilers, -fdefault-integer-8 with GNU compilers), then it will very likely crash. Since c_sendcounts will be ignored, I think the easiest way is not to invoke OMPI_ARRAY_FINT_2_INT when MPI_IN_PLACE is used.

note you also need to fix ompi/mpi/fortran/mpif-h/[i]neighbor_alltoallw (even if MPI_IN_PLACE is not allowed here, this error case should not result in a crash, but should be passed and handled to PMPI_{In,N}eighbor_alltoallw().

you will also need to apply similar fixes to the persistent collectives extensions in ompi/mpiext/pcollreq/mpif-h

last but not least, your commits have to be signed-off in order to be accepted,
make sure you understand the implications (IANAL, at the very least, make sure your management
is OK with you contributing to Open MPI)

[geimer]

Ah, OK, I've missed that there are multiple definitions of OMPI_ARRAY_FINT_2_INT.

Regarding MPI_{In,N}eighbor_alltoallw: I'm not entirely sure I understand what you are suggesting. IMHO these functions aren't affected, as all arguments are significant on all ranks. And if one passes in an invalid pointer, the program is erroneous. I also quickly checked some other routines that need to do handle conversion on arrays (MPI_Waitall, MPI_Waitsome); they don't handle this case either.

[jsquyres]

I didn't see a pull request go by -- did this get fixed?

[jsquyres]

Sorry -- I should clarify: we're making the v2.1.4rc in the immediate future (probably Monday). It will likely be the last release in the v2.1.x series. If we get a fix for this right quick, we might be able to include it in v2.1.4.

More releases are coming shortly after that (see https://github.com/open-mpi/ompi/milestones?direction=asc&sort=due_date&state=open), so the sooner we get a PR for this, the better.

[geimer]

@jsquyres I haven't found the time yet to work on a PR. And it is very unlikely that it will happen over the weekend (which starts for me in a couple of minutes)...

[jsquyres]

No worries -- have a great weekend!

(if this missed the v2.1.4 release, that's not a tragedy -- we can always slot it in the upcoming v3.0.x / v3.1.x / v4.0.0 releases)

[jsquyres]

@geimer Were you ever able to make progress on this, perchance?

[geimer]

@jsquyres I got distracted by vacation ;-) And now I'm behind schedule on my primary project... In other words: I don't believe I can find the time to look into this in the near future. Sorry!

[gpaulsen]

@geimer What is the status of this issue?

[geimer]

@gpaulsen The state on my side is still unchanged: I couldn't find time to work on this. Sorry. Note that I hit this issue just by accident - and since I'm a C/C++ programmer, Fortran stuff gets very low priority on my TODO list. That is, if you want to see this fixed anytime soon, I suggest that someone else takes care of it.

[ggouaillardet]

@geimer i fixed this in #6813. Meanwhile, you can manually download and apply the patch at https://github.com/open-mpi/ompi/pull/6813.patch

[ggouaillardet]

note to myself :
MPI_Ialltoallw() and MPI_Ineighbor_alltoallw() mpif-h bindings are not correct since some parameters passed to the C bindings are free'd before the request completes.
A similar mechanism to #2154 can be implemented to solve this correctly.

[gpaulsen]

@ggouaillardet Hi. In #6813 (comment) (master only, not yet backported to v4.1.x, v4.0.x, or earlier), you mentioned that this is a bigger issue than you thought, and that you were working on a PR for release branches, but that it kept growing.

Could you summarize the status in this issue? Cherry-picking 6813 looks straight forward enough, but it sounds like that's just the tip of the iceberg. Thoughts?

[ggouaillardet]

The commit can be backported, but does not address all the issues described in my previous comments:

1. the MPI_Ialltoallw() Fortran binding explicitly dynamically allocates c_sendtypes and c_recvtypes, and might allocate under the hood c_sdispls, c_sendcounts, c_rdispls and c_recvcounts. These pointers are then passed to the C PMPI_Ialltoallw() binding and free'd right away. This is not correct since according to the standard, these pointers can only be free'd after the non blocking collective completes. I guess the persistent collectives have the same issue.
   In an other (large) PR, I added some mechanism to automatically free some data once a non blocking collective completes, similar mechanism could/should be used here.
   IIRC coll/libnbc does not access these pointers once ialltoallw() returns, so that could hide the bug.
2. In the case of neighbor collectives, MPI_IN_PLACE is not a valid parameter. C bindings do check that and error with a user friendly error message. IIRC, the Fortran binding won't pass MPI_IN_PLACE, resulting into a non user friendly crash.

[hppritcha]

fixed in v4.0.x via #6838. closing since we're not going to patch v3.1.x and v3.0.x