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 <[email protected]>

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 <[email protected]>

Author: markalle

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 */

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); 
 }

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);
 }

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); 
 }