This is an automated email from the git hooks/post-receive script. It was generated because a ref change was pushed to the repository containing the project "MPICH primary repository". The branch, master has been updated via 3d96864e665e281a543383ec7e68b25d1861276a (commit) from d9a67f406163643fb86ab2d10e2853f20ad11a1d (commit) Those revisions listed above that are new to this repository have not appeared on any other notification email; so we list those revisions in full, below. - Log ----------------------------------------------------------------- http://git.mpich.org/mpich.git/commitdiff/3d96864e665e281a543383ec7e68b25d18... commit 3d96864e665e281a543383ec7e68b25d1861276a Author: William Gropp <[email protected]> Date: Thu May 2 12:49:26 2013 -0400 Fixes for #1804 and #1828, related to alignment issues with MPI_Status There are two alignment-related problems with MPI_STATUS arguments in Fortran. The first is that while MPI_STATUS is an INTEGER array in Fortran, in C there is an element of type MPI_Count. MPI_Count may be longer than an INTEGER and thus a C MPI_Status may have a stricter alignment requirement than a Fortran INTEGER array. The fixes to src/binding/f77/buildiface ensures that STATUS arguments that are provided to C by the Fortran wrappers are properly aligned, while avoiding copies when possible. The second fix deals with padding that the introduction of MPI_Count as the count type into the C structure for status will often introduce. diff --git a/.gitignore b/.gitignore index 1634a5a..ed5203c 100644 --- a/.gitignore +++ b/.gitignore @@ -159,6 +159,7 @@ Makefile.am-stamp /README.envvar /maint/extracterrmsgs /src/binding/f90/mpi_base.f90.in +/src/binding/f90/mpi_constants.f90.in /src/env/mpixxx_opts.conf /src/mpi/romio/include/mpio.h /src/mpi/romio/include/mpiof.h diff --git a/configure.ac b/configure.ac index 6b7237c..f3d3c2d 100644 --- a/configure.ac +++ b/configure.ac @@ -4079,9 +4079,10 @@ dnl len_doublecplx=$hexlen # PAC_PROG_FC_INT_KIND(INTEGER_KIND,$pac_cv_f77_sizeof_integer,$CROSS_F90_INTEGER_KIND) if test "$INTEGER_KIND" = "-1" ; then - # Wild guess; probably means that Fortran 90 is not available - AC_MSG_WARN([Unable to determine Fortran 90 KIND values for either address-sized integers or offset-sized integers. Using 4 in that case.]) - INTEGER_KIND=4 + # In our experience, this usually means that there is some + # problem building and/or running the f90 program. Fail + # in this case rather than attempt to continue + AC_MSG_ERROR([Unable to determine Fortran 90 KIND values for either address-sized integers or offset-sized integers.]) fi # # Some compilers won't allow a -1 kind (e.g., absoft). In this case, @@ -5792,6 +5793,8 @@ if test "$enable_f77" = yes -a -z "$MPI_STATUS_SIZE" ; then if test $MPI_SIZEOF_COUNT -gt $pac_cv_f77_sizeof_integer ; then # Alignment test looks at the low address bits, as long # as the sizeof(MPI_Count) is 2^j. + # If we can't tell, pick a value that forces the Fortran + # wrappers to use a local copy. MPIR_COUNT_ALIGNMENT=0 case $MPI_SIZEOF_COUNT in 4) MPIR_COUNT_ALIGNMENT="0x3";; @@ -5801,6 +5804,77 @@ if test "$enable_f77" = yes -a -z "$MPI_STATUS_SIZE" ; then esac AC_DEFINE_UNQUOTED([MPIR_COUNT_ALIGNMENT],[$MPIR_COUNT_ALIGNMENT],[Value that anded with an address will give 0 if the address is properly aligned for MPI_Count]) + + # Compare the size of MPI_Status to the sizes of the individual + # elements. If the sizeof(MPI_Status) > sum of the sizes, then + # determine if simple padding will work. + # FIXME: DETERMINED TO SEE IF THERE IS PROBLEM BUT NOT YET USED + AC_MSG_CHECKING([whether MPI_Status contains padding]) + sum_of_status_element_sizes=`expr 6 \* $ac_cv_sizeof_int + $MPI_SIZEOF_COUNT` + if test $sum_of_status_element_sizes -lt $pac_cv_sizeof_mpi_status ; then + AC_MSG_RESULT([yes]) + if test "$enable_fc" = "yes" ; then + # FIXME: We could compute the padding or delete the + # status definition from mpi_constants + AC_MSG_CHECKING([padding size before count field in MPI_Status]) + rm -f pac_mpi_status.h +dnl quote with [] since otherwise m4 strips the array declaration out of this +dnl structure +[ cat > pac_mpi_status.h <<_EOF +typedef struct { + int MPI_SOURCE; + int MPI_TAG; + int MPI_ERROR; + $MPI_COUNT count; + int cancelled; + int abi_slush_fund[2]; + $EXTRA_STATUS_DECL +} MPI_Status; +MPI_Status mya; +_EOF +] + AC_COMPUTE_INT(pac_cv_fc_pad1,[((char*)&mya.count-(((char*)&mya)+3*sizeof(int)))],[AC_INCLUDES_DEFAULT() +#include "pac_mpi_status.h"]) + AC_MSG_RESULT($pac_cv_fc_pad1) + AC_MSG_CHECKING([padding at end of MPI_Status]) + AC_COMPUTE_INT(pac_cv_fc_pad2,[((char*)(&mya + 1) - ((char *)(mya.abi_slush_fund+1) + sizeof(int)))],[AC_INCLUDES_DEFAULT() +#include "pac_mpi_status.h"]) + AC_MSG_RESULT($pac_cv_fc_pad2) + rm -f pac_mpi_status.h + pac_cv_pad1found=no + pac_cv_pad2found=no + FC_STATUS_PAD1="" + FC_STATUS_PAD2="" + AC_SUBST(FC_STATUS_PAD1) + AC_SUBST(FC_STATUS_PAD2) + if test $pac_cv_fc_pad1 -gt 0 ; then + if test $pac_cv_fc_pad1 -eq $ac_cv_sizeof_int ; then + pac_cv_pad1found=yes + FC_STATUS_PAD1="INTEGER :: PAD1" + fi + else + # No padding needed here, so successfully determined + # padding + pac_cv_pad1found=yes + fi + if test $pac_cv_fc_pad2 -gt 0 ; then + if test $pac_cv_fc_pad2 -eq $ac_cv_sizeof_int ; then + pac_cv_pad2found=yes + FC_STATUS_PAD2="INTEGER :: PAD2" + fi + else + # No padding needed here, so successfully determined + # padding + pac_cv_pad2found=yes + fi + if test $pac_cv_pad1found = "no" -o \ + $pac_cv_pad2found = "no" ; then + AC_MSG_ERROR([INTERNAL ERROR: definition of MPI_Status in C and Fortran 90 is incompatible due to padding in the C definition]) + fi + fi + else + AC_MSG_RESULT([no]) + fi fi fi AC_SUBST([MPI_STATUS_SIZE]) @@ -6063,6 +6137,7 @@ AC_OUTPUT(Makefile \ src/binding/f77/setbot.c \ src/binding/f90/mpi_sizeofs.f90 \ src/binding/f90/mpi_base.f90 \ + src/binding/f90/mpi_constants.f90 \ src/packaging/pkgconfig/mpich.pc \ src/packaging/envmods/mpich.module \ src/env/mpixxx_opts.conf \ diff --git a/src/binding/f77/buildiface b/src/binding/f77/buildiface index f6c6611..5f30c6d 100755 --- a/src/binding/f77/buildiface +++ b/src/binding/f77/buildiface @@ -2553,10 +2553,18 @@ sub status_out_ftoc { # casting to a pointer with greater alignment (and which the # specific runtime check is used to ensure the alignments are # ok). + # Horrible requirement: MPI Standard (see MPI-3, Section 3.2.5, + # page 30, lines 39-43) implies that the MPI_ERROR field needs + # to be *preserved*. Thus, if we have a temp status, we need to + # either copy it to the temp status (as here) or *not* copy over + # it when storing the result. This is simpler. print $OUTFD "\ - if (l$count != MPI_STATUS_IGNORE && - (((MPI_Aint)v$count) & MPIR_COUNT_ALIGNMENT) == 0) - l$count = (MPI_Status*)(void *)v$count;\n"; + if (l$count != MPI_STATUS_IGNORE) { + if ((((MPI_Aint)v$count) & MPIR_COUNT_ALIGNMENT) == 0) + l$count = (MPI_Status*)(void *)v$count; + else + l$count->MPI_ERROR = ((MPI_Status*)v$count)->MPI_ERROR; + }\n"; } } sub status_out_arg { @@ -2679,9 +2687,17 @@ sub status_array_out_fnulltoc { } sub status_array_out_ftoc { my $count = $_[0]; - + + # See discussion of status_out_ftoc - unfortunately, we need to + # copy *just* the MPI_ERROR field if ($within_fint) { - print $OUTFD " if (l$count != MPI_STATUSES_IGNORE) { l$count = (MPI_Status*)$malloc($Array_size * sizeof(MPI_Status)); }\n"; + print $OUTFD "\ + if (l$count != MPI_STATUSES_IGNORE) { + int li; + l$count = (MPI_Status*)$malloc($Array_size * sizeof(MPI_Status)); + for (li=0; li<$Array_size; li++) + l$count\[li].MPI_ERROR = ((MPI_Status*)v$count)[li].MPI_ERROR; + }\n"; } elsif ($fixStatusAlignment) { print $OUTFD "\ @@ -2689,8 +2705,11 @@ sub status_array_out_ftoc { if( (((MPI_Aint)v$count) & MPIR_COUNT_ALIGNMENT) == 0) l$count = (MPI_Status*)(void *)v$count; else { + int li; lalloc$count = 1; l$count = (MPI_Status*)$malloc($Array_size * sizeof(MPI_Status)); + for (li=0; li<$Array_size; li++) + l$count\[li].MPI_ERROR = ((MPI_Status*)v$count)[li].MPI_ERROR; } }\n"; } @@ -2714,7 +2733,7 @@ sub status_array_ctof { " if ($testFlag$coutvar != MPI_STATUSES_IGNORE) { int li; for (li=0; li<$ActSize; li++) { - MPI_Status_c2f($coutvar+li,$outvar+li*5); + MPI_Status_c2f($coutvar+li,$outvar+li*MPIF_STATUS_SIZE); } }\n"; $clean_up .= " if ($coutvar != MPI_STATUSES_IGNORE) { $free($coutvar); }\n"; @@ -2732,7 +2751,7 @@ sub status_array_ctof { " if (${testFlag}lalloc$count) { int li; for (li=0; li<$ActSize; li++) { - MPI_Status_c2f($coutvar+li,$outvar+li*5); + MPI_Status_c2f($coutvar+li,$outvar+li*MPIF_STATUS_SIZE); } }\n"; $clean_up .= " if (lalloc$count) { $free($coutvar); }\n"; diff --git a/src/binding/f90/buildiface b/src/binding/f90/buildiface index c5eb901..d84ef1f 100755 --- a/src/binding/f90/buildiface +++ b/src/binding/f90/buildiface @@ -603,7 +603,7 @@ print MPIBASEFD " END MODULE ${ucoutfile_prefix}_BASE\n"; close MPIBASEFD; &ReplaceIfDifferent( "${outfile_prefix}_base.f90.in", "${outfile_prefix}_base.f90.in.new" ); -open ( MPIFD, ">${outfile_prefix}_constants.f90.new" ) || die "Cannot open ${outfile_prefix}_constants.f90.new\n"; +open ( MPIFD, ">${outfile_prefix}_constants.f90.in.new" ) || die "Cannot open ${outfile_prefix}_constants.f90.in.new\n"; print MPIFD "! -*- Mode: Fortran; -*- ! (C) 2008 by Argonne National Laboratory. ! See COPYRIGHT in top-level directory. @@ -642,13 +642,21 @@ $PRIVATEVAR = ""; # #$BINDACCESS = ", BIND(C)"; #$BINDDEF =""; +# Yet another problem. +# Because MPI_Count may be longer than a single (Fortran) INTEGER, +# alignment restrictions may introduce padding in the structure +# And one more problem: If a Fortran INTEGER is not the same as a C int, +# then these are also wrong (see the "fint" option in +# src/binding/f77/buildiface print MPIFD <<EOT; TYPE$BINDACCESS :: MPI_Status $BINDDEF INTEGER$PUBLICVAR :: MPI_SOURCE, MPI_TAG, MPI_ERROR + \@FC_STATUS_PAD1\@ INTEGER$PRIVATEVAR(KIND=MPI_COUNT_KIND) :: count INTEGER$PRIVATEVAR :: cancelled INTEGER$PRIVATEVAR(2) :: abi_slush_fund + \@FC_STATUS_PAD2\@ END TYPE MPI_Status EOT @@ -708,7 +716,7 @@ foreach $handle (keys(%handles)) { print MPIFD " END MODULE ${ucoutfile_prefix}_CONSTANTS\n"; close MPIFD; -&ReplaceIfDifferent( "${outfile_prefix}_constants.f90", "${outfile_prefix}_constants.f90.new" ); +&ReplaceIfDifferent( "${outfile_prefix}_constants.f90.in", "${outfile_prefix}_constants.f90.in.new" ); # # Generate the choice argument routines ----------------------------------------------------------------------- Summary of changes: .gitignore | 1 + configure.ac | 81 ++++++++++++++++++++++++++++++++++++++++++-- src/binding/f77/buildiface | 33 ++++++++++++++---- src/binding/f90/buildiface | 12 +++++- 4 files changed, 115 insertions(+), 12 deletions(-) hooks/post-receive -- MPICH primary repository