From 36583df68945af4ab718294fdf6fa063e6996199 Mon Sep 17 00:00:00 2001 From: Mark Allen Date: Mon, 4 Mar 2019 17:57:53 -0500 Subject: [PATCH] in-place conversion macro writes into INPUT argument In fint_2_int.h there are some conversion macros for logicals. It has one path for OMPI_SIZEOF_FORTRAN_LOGICAL != SIZEOF_INT where a new array would be allocated and the conversions then might expand to c_array[i] = (array[i] == 0 ? 0 : 1) and another path for OMPI_SIZEOF_FORTRAN_LOGICAL == SIZEOF_INT where it does things "in place", so the same conversion there would just be array[i] = (array[i] == 0 ? 0 : 1) The problem is some of the logical arrays being converted are INPUT arguments. And it's possible for some compilers to even put the argument in read-only memory so the above "in place" conversion SEGV's. A testcase I have used call MPI_CART_SUB(oldcomm, (/.true.,.false./), newcomm, ierr) and gfortran put the second arg in read-only mem. In cart_sub_f.c you can trace the ompi_fortran_logical_t *remain_dims arg. remain_dims[] is for input only, but the file uses OMPI_LOGICAL_ARRAY_NAME_DECL(remain_dims); OMPI_ARRAY_LOGICAL_2_INT(remain_dims, ndims); PMPI_Cart_sub(..., OMPI_LOGICAL_ARRAY_NAME_CONVERT(remain_dims), ...); OMPI_ARRAY_INT_2_LOGICAL(remain_dims, ndims); to convert it to c-ints make a C call then restore it to Fortran logicals before returning. It's not always wrong to convert purely in-place, eg cart_get_f.c has a periods[] that's exclusively for OUTPUT and it would be fine with the macros as they were. But I still say the macros are invalid because they don't distinguish whether they're being used on INPUT or OUTPUT args and thus they can't be used in a way that's legal for both cases. It might be possible to fix the macros by adding more of them so that cart_create_f.c and cart_get_f.c would use different macros that give more context. But my fix here is just to turn off the first block and make all paths run as if OMPI_SIZEOF_FORTRAN_LOGICAL != SIZEOF_INT. The main macros that get enlarged by this change are define OMPI_ARRAY_LOGICAL_2_INT_ALLOC : mallocs now define OMPI_ARRAY_LOGICAL_2_INT : also mallocs now But these are only used in 4 places, three of which are the purpose of this checkin, to avoid the former in-place expansion of an INPUT arg: cart_create_f.c cart_map_f.c cart_sub_f.c and one of which is an OUPUT arg that was fine and that gets unnecessarily expanded into a separate array by this checkin. cart_get_f.c So I think an unnecessary malloc in cart_get_f.c is the only downside to this change, where the logicals array argument could have been used and converted in place. Signed-off-by: Mark Allen Update provided by Gilles Gouaillardet to keep the in-place option if OMPI_FORTRAN_VALUE_TRUE == 1 where no conversion is needed. Signed-off-by: Gilles Gouaillardet (cherry picked from commit 0a7f1e3cc58dabe536df00ae9c97f7e9d27103ad) --- ompi/mpi/fortran/base/fint_2_int.h | 59 +++++++++++-------------- ompi/mpi/fortran/mpif-h/cart_create_f.c | 3 +- ompi/mpi/fortran/mpif-h/cart_map_f.c | 3 +- ompi/mpi/fortran/mpif-h/cart_sub_f.c | 3 +- 4 files changed, 32 insertions(+), 36 deletions(-) diff --git a/ompi/mpi/fortran/base/fint_2_int.h b/ompi/mpi/fortran/base/fint_2_int.h index 5971694eb9b..44ce1289567 100644 --- a/ompi/mpi/fortran/base/fint_2_int.h +++ b/ompi/mpi/fortran/base/fint_2_int.h @@ -11,8 +11,8 @@ * All rights reserved. * Copyright (c) 2011-2012 Cisco Systems, Inc. All rights reserved. * Copyright (c) 2012 Oracle and/or its affiliates. All rights reserved. - * Copyright (c) 2014 Research Organization for Information Science - * and Technology (RIST). All rights reserved. + * Copyright (c) 2014-2019 Research Organization for Information Science + * and Technology (RIST). All rights reserved. * $COPYRIGHT$ * * Additional copyrights may follow @@ -161,9 +161,24 @@ /* * Define MACROS to take account of different size of logical from int + * + * There used to be an in-place option for the below conversions of + * logical arrays. So if mpi_cart_create(..., periods, ...) took an + * input array of Fortran logicals, it would walk the array converting + * the elements to C-logical values, then at the end it would restore + * the values back to Fortran logicals. + * + * The problem with that is periods is an INPUT argument and some + * Fortran compilers even put it in read-only memory because of that. + * So writing to it wasn't generally okay, even though we were restoring it + * before returning. + * + * The in-place option is hence only valid if no conversion is ever needed + * (e.g. Fortran logical and C int have the same size *and** Fortran logical + * .TRUE. value is 1 in C. */ -#if OMPI_SIZEOF_FORTRAN_LOGICAL == SIZEOF_INT +#if (OMPI_SIZEOF_FORTRAN_LOGICAL == SIZEOF_INT) && (OMPI_FORTRAN_VALUE_TRUE == 1) # define OMPI_LOGICAL_NAME_DECL(in) /* Not needed for int==logical */ # define OMPI_LOGICAL_NAME_CONVERT(in) in /* Not needed for int==logical */ # define OMPI_LOGICAL_SINGLE_NAME_CONVERT(in) in /* Not needed for int==logical */ @@ -172,37 +187,15 @@ # define OMPI_ARRAY_LOGICAL_2_INT_ALLOC(in,n) /* Not needed for int==logical */ # define OMPI_ARRAY_LOGICAL_2_INT_CLEANUP(in) /* Not needed for int==logical */ -# if OMPI_FORTRAN_VALUE_TRUE == 1 -# define OMPI_FORTRAN_MUST_CONVERT_LOGICAL_2_INT 0 -# define OMPI_LOGICAL_2_INT(a) a -# define OMPI_INT_2_LOGICAL(a) a -# define OMPI_ARRAY_LOGICAL_2_INT(in, n) -# define OMPI_ARRAY_INT_2_LOGICAL(in, n) -# define OMPI_SINGLE_INT_2_LOGICAL(a) /* Single-OUT variable -- Not needed for int==logical, true=1 */ -# else -# define OMPI_FORTRAN_MUST_CONVERT_LOGICAL_2_INT 1 -# define OMPI_LOGICAL_2_INT(a) ((a)==0? 0 : 1) -# define OMPI_INT_2_LOGICAL(a) ((a)==0? 0 : OMPI_FORTRAN_VALUE_TRUE) -# define OMPI_SINGLE_INT_2_LOGICAL(a) *a=OMPI_INT_2_LOGICAL(OMPI_LOGICAL_NAME_CONVERT(*a)) -# define OMPI_ARRAY_LOGICAL_2_INT(in, n) do { \ - int converted_n = (int)(n); \ - OMPI_ARRAY_LOGICAL_2_INT_ALLOC(in, converted_n + 1); \ - while (--converted_n >= 0) { \ - OMPI_LOGICAL_ARRAY_NAME_CONVERT(in)[converted_n]=OMPI_LOGICAL_2_INT(in[converted_n]); \ - } \ - } while (0) -# define OMPI_ARRAY_INT_2_LOGICAL(in, n) do { \ - int converted_n = (int)(n); \ - while (--converted_n >= 0) { \ - in[converted_n]=OMPI_INT_2_LOGICAL(OMPI_LOGICAL_ARRAY_NAME_CONVERT(in)[converted_n]); \ - } \ - OMPI_ARRAY_LOGICAL_2_INT_CLEANUP(in); \ - } while (0) - -# endif +# define OMPI_FORTRAN_MUST_CONVERT_LOGICAL_2_INT 0 +# define OMPI_LOGICAL_2_INT(a) a +# define OMPI_INT_2_LOGICAL(a) a +# define OMPI_ARRAY_LOGICAL_2_INT(in, n) +# define OMPI_ARRAY_INT_2_LOGICAL(in, n) +# define OMPI_SINGLE_INT_2_LOGICAL(a) /* Single-OUT variable -- Not needed for int==logical, true=1 */ #else /* - * For anything other than Fortran-logical == C-int, we have to convert + * For anything other than Fortran-logical == C-int or some .TRUE. is not 1 in C, we have to convert */ # define OMPI_FORTRAN_MUST_CONVERT_LOGICAL_2_INT 1 # define OMPI_LOGICAL_NAME_DECL(in) int c_##in @@ -238,7 +231,7 @@ } \ OMPI_ARRAY_LOGICAL_2_INT_CLEANUP(in); \ } while (0) -#endif /* OMPI_SIZEOF_FORTRAN_LOGICAL */ +#endif /* OMPI_SIZEOF_FORTRAN_LOGICAL && OMPI_FORTRAN_VALUE_TRUE */ #endif /* OMPI_FORTRAN_BASE_FINT_2_INT_H */ diff --git a/ompi/mpi/fortran/mpif-h/cart_create_f.c b/ompi/mpi/fortran/mpif-h/cart_create_f.c index d1bafcc0b62..a617ef0c2e4 100644 --- a/ompi/mpi/fortran/mpif-h/cart_create_f.c +++ b/ompi/mpi/fortran/mpif-h/cart_create_f.c @@ -12,6 +12,7 @@ * Copyright (c) 2011-2012 Cisco Systems, Inc. All rights reserved. * Copyright (c) 2015 Research Organization for Information Science * and Technology (RIST). All rights reserved. + * Copyright (c) 2019 IBM Corporation. All rights reserved. * $COPYRIGHT$ * * Additional copyrights may follow @@ -96,5 +97,5 @@ void ompi_cart_create_f(MPI_Fint *old_comm, MPI_Fint *ndims, MPI_Fint *dims, * Need to convert back into Fortran, to not surprise the user */ OMPI_ARRAY_FINT_2_INT_CLEANUP(dims); - OMPI_ARRAY_INT_2_LOGICAL(periods, size); + OMPI_ARRAY_LOGICAL_2_INT_CLEANUP(periods); } diff --git a/ompi/mpi/fortran/mpif-h/cart_map_f.c b/ompi/mpi/fortran/mpif-h/cart_map_f.c index 3ad10c341e8..6557c89191e 100644 --- a/ompi/mpi/fortran/mpif-h/cart_map_f.c +++ b/ompi/mpi/fortran/mpif-h/cart_map_f.c @@ -12,6 +12,7 @@ * Copyright (c) 2011-2012 Cisco Systems, Inc. All rights reserved. * Copyright (c) 2015 Research Organization for Information Science * and Technology (RIST). All rights reserved. + * Copyright (c) 2019 IBM Corporation. All rights reserved. * $COPYRIGHT$ * * Additional copyrights may follow @@ -89,6 +90,6 @@ void ompi_cart_map_f(MPI_Fint *comm, MPI_Fint *ndims, MPI_Fint *dims, if (NULL != ierr) *ierr = OMPI_INT_2_FINT(c_ierr); OMPI_ARRAY_FINT_2_INT_CLEANUP(dims); - OMPI_ARRAY_INT_2_LOGICAL(periods, size); + OMPI_ARRAY_LOGICAL_2_INT_CLEANUP(periods); OMPI_SINGLE_INT_2_FINT(newrank); } diff --git a/ompi/mpi/fortran/mpif-h/cart_sub_f.c b/ompi/mpi/fortran/mpif-h/cart_sub_f.c index be51488e30e..463c96df10e 100644 --- a/ompi/mpi/fortran/mpif-h/cart_sub_f.c +++ b/ompi/mpi/fortran/mpif-h/cart_sub_f.c @@ -12,6 +12,7 @@ * Copyright (c) 2011-2012 Cisco Systems, Inc. All rights reserved. * Copyright (c) 2015 Research Organization for Information Science * and Technology (RIST). All rights reserved. + * Copyright (c) 2019 IBM Corporation. All rights reserved. * $COPYRIGHT$ * * Additional copyrights may follow @@ -101,5 +102,5 @@ void ompi_cart_sub_f(MPI_Fint *comm, ompi_fortran_logical_t *remain_dims, *new_comm = PMPI_Comm_c2f(c_new_comm); } - OMPI_ARRAY_INT_2_LOGICAL(remain_dims, ndims); + OMPI_ARRAY_LOGICAL_2_INT_CLEANUP(remain_dims); }