[Bug fortran/51073] New: _gfortran_caf_register incorrectly assumes malloc(0) returns non-NULL

[Bug fortran/51073] New: _gfortran_caf_register incorrectly assumes malloc(0) returns non-NULL

[joel at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51073

             Bug #: 51073
           Summary: _gfortran_caf_register incorrectly assumes malloc(0)
                    returns non-NULL
    Classification: Unclassified
           Product: gcc
           Version: 4.7.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: fortran
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: joel@gcc.gnu.org


Revision: Wed Nov  9 13:24:05 UTC 2011 (revision 181203)
Target: mips-rtems4.11 but impacts all where malloc(0) returns NULL

I am debugging why some of the gfortran tests are failing.  I have tracked NN
failures down to this code in caf/mpi.c around line 155.


  /* Token contains only a list of pointers.  */
  local = malloc (size);
  token = malloc (sizeof (void*) * caf_num_images);

  if (unlikely (local == NULL || token == NULL))
    goto error;

>From http://pubs.opengroup.org/onlinepubs/009695399/functions/malloc.html

Upon successful completion with size not equal to 0, malloc() shall return a
pointer to the allocated space. If size is 0, either a null pointer or a unique
pointer that can be successfully passed to free() shall be returned. Otherwise,
it shall return a null pointer [CX]   and set errno to indicate the error. 

The code uses the memory pointed to by local to pass to MPI_Allgather() and
then returns it to the caller.  I suspect that the line "local = malloc (size)"
should be something like:

  local = malloc ((size == 0)? sizeof(XXX) : size);

Where XXX needs to be the type of one entity of sufficient size to pass into
MPI_Allgather.

[Bug fortran/51073] _gfortran_caf_register incorrectly assumes malloc(0) returns non-NULL

[burnus at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51073

--- Comment #1 from Tobias Burnus <burnus at gcc dot gnu.org> 2011-11-10 08:13:54 UTC ---
(In reply to comment #0)
> I am debugging why some of the gfortran tests are failing.  I have tracked NN
> failures down to this code in caf/mpi.c around line 155.

I think you are looking at the wrong file and the cause is due to
libgfortran/caf/single.c, which contains similar code.

With coarrays, gfortran can either use single.c ("libcaf_single.a") or mpi.c.
While "libcaf_single.a" is build on all systems and tested in
gcc/testsuite/gfortran.dg/coarrary/, mpi.c is not build at all. The reason is
that building mpi.c strongly depends on the used Message Passing Interface
implementation. Thus, it is currently left to the user to fetch mpi.c and
libcaf.h and run "mpicc -O2 -c mpi.c". Hence, it is also not automatically
tested.

I am considering to allow building mpi.c by passing some flags to configure. I
am also thinking of supporting it in the testsuite (with some environment
variables for the mpi-library path and the running command). However, I have
not yet done so.

>   /* Token contains only a list of pointers.  */
>   local = malloc (size);

> Upon successful completion with size not equal to 0, malloc() shall return a
> pointer to the allocated space. If size is 0, either a null pointer or a
> unique pointer that can be successfully passed to free() shall be returned.

Good point - I have think about how to fix that correctly.

[Bug fortran/51073] _gfortran_caf_register incorrectly assumes malloc(0) returns non-NULL

[burnus at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51073

--- Comment #2 from Tobias Burnus <burnus at gcc dot gnu.org> 2011-11-10 10:01:36 UTC ---
I think the size == 0 issue can only happen for static ("save") coarrays such
as 
  integer, save :: caf(1:0)[*]
  print *, size(caf)
  end
For allocatable coarrays, one always allocates at least a single byte.

Thus, it should be done in the front end for performance reasons (size is known
at the compile time and it is a rather special case).

Regarding the RTEMS failures: I somehow doubt that they are related to this
issue. I think there should be no size-zero test case for coarrays.

[Bug fortran/51073] _gfortran_caf_register incorrectly assumes malloc(0) returns non-NULL

[burnus at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51073

--- Comment #3 from Tobias Burnus <burnus at gcc dot gnu.org> 2011-11-10 11:02:06 UTC ---
Patch which makes sure that for static coarrays the minimal size is 1.

--- a/gcc/fortran/trans-decl.c
+++ b/gcc/fortran/trans-decl.c
@@ -4234,6 +4238,11 @@ generate_coarray_sym_init (gfc_symbol *sym)

   size = TYPE_SIZE_UNIT (gfc_get_element_type (TREE_TYPE (decl)));

+  /* Ensure that we do not have size=0 for zero-sized arrays.  */
+  size = fold_build2_loc (input_location, MAX_EXPR, size_type_node,
+                         fold_convert (size_type_node, size)
+                         build_int_cst (size_type_node, 1));
+
   if (GFC_TYPE_ARRAY_RANK (TREE_TYPE (decl)))
     {
       tmp = GFC_TYPE_ARRAY_SIZE (TREE_TYPE (decl));

[Bug fortran/51073] _gfortran_caf_register incorrectly assumes malloc(0) returns non-NULL

[joel at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51073

--- Comment #4 from Joel Sherrill <joel at gcc dot gnu.org> 2011-11-10 13:10:43 UTC ---
I double checked the test log.  This looks to have only caused one test
failure.  I thought it caused more.  

Do I need to test your patch locally? Or are you committing it?

[Bug fortran/51073] _gfortran_caf_register incorrectly assumes malloc(0) returns non-NULL

[burnus at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51073

--- Comment #5 from Tobias Burnus <burnus at gcc dot gnu.org> 2011-11-10 13:33:20 UTC ---
(In reply to comment #4)
> Do I need to test your patch locally? Or are you committing it?

I plan to commit it - but I am not sure how soon; it might take some days.

[Bug fortran/51073] _gfortran_caf_register incorrectly assumes malloc(0) returns non-NULL

[joel at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51073

--- Comment #6 from Joel Sherrill <joel at gcc dot gnu.org> 2011-11-10 17:49:19 UTC ---
Patch doesn't compile.

/users/joel/test-gcc/gcc-svn/gcc/fortran/trans-decl.c: In function
‘generate_coarray_sym_init’:
/users/joel/test-gcc/gcc-svn/gcc/fortran/trans-decl.c:4240:59: error: macro
"fold_build2_loc" requires 5 arguments, but only 4 given
/users/joel/test-gcc/gcc-svn/gcc/fortran/trans-decl.c:4238:10: error:
‘fold_build2_loc’ undeclared (first use in this function)
/users/joel/test-gcc/gcc-svn/gcc/fortran/trans-decl.c:4238:10: note: each
undeclared identifier is reported only once for each function it appears in
make[2]: *** [fortran/trans-decl.o] Error 1
make[2]: Leaving directory `/home2/joel/build/b-mips-fortran/gcc'

[Bug fortran/51073] _gfortran_caf_register incorrectly assumes malloc(0) returns non-NULL

[burnus at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51073

--- Comment #7 from Tobias Burnus <burnus at gcc dot gnu.org> 2011-11-14 08:15:29 UTC ---
Author: burnus
Date: Mon Nov 14 08:15:09 2011
New Revision: 181348

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=181348
Log:
2011-11-14  Tobias Burnus  <burnus@net-b.de>

        PR fortran/51073
        * trans-decl.c (generate_coarray_sym_init): Handle zero-sized
        * arrays.


Modified:
    trunk/gcc/fortran/ChangeLog
    trunk/gcc/fortran/trans-decl.c

[Bug fortran/51073] _gfortran_caf_register incorrectly assumes malloc(0) returns non-NULL

[burnus at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51073

Tobias Burnus <burnus at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
         Resolution|                            |FIXED

--- Comment #8 from Tobias Burnus <burnus at gcc dot gnu.org> 2011-11-14 08:17:32 UTC ---
(In reply to comment #6)
> Patch doesn't compile.
there was a trailing comma missing.

FIXED on the trunk (4.7) [well, 4.6 didn't even include the code].

Thanks for the bug report and for tracing down the issue.