[Patch, Fortran] Add caf_runtime_error to libgfortran/caf/single.c

[Patch, Fortran] Add caf_runtime_error to libgfortran/caf/single.c

[Daniel Carrera]

[-- Attachment #1: Type: text/plain, Size: 542 bytes --]

This patch adds a caf_runtime_error function to the non-MPI 
implementation of Coarray Fortran. It is based on the MPI function of 
the same name in mpi.c.

Ok to commit?

ChangeLog:

2011-07-14  Daniel Carrera  <dcarrera@gmail.com>

	* caf/single.c:  Include stdarg.h header.
	(caf_runtime_error): New function based on the function in
	mpi.c with the same name.
	(_gfortran_caf_init): Use caf_runtime_error.
	* caf/mpi.c (caf_runtime_error): Add a note to keep in sync
	with the function in single.c.
-- 
I'm not overweight, I'm undertall.

[-- Attachment #2: caf-runtime-error.diff --]
[-- Type: text/x-patch, Size: 2373 bytes --]

Index: libgfortran/caf/single.c
===================================================================
--- libgfortran/caf/single.c	(revision 176230)
+++ libgfortran/caf/single.c	(working copy)
@@ -28,6 +28,7 @@ see the files COPYING3 and COPYING.RUNTI
 #include <stdio.h>  /* For fputs and fprintf.  */
 #include <stdlib.h> /* For exit and malloc.  */
 #include <string.h> /* For memcpy and memset.  */
+#include <stdarg.h> /* For variadic arguments.  */
 
 /* Define GFC_CAF_CHECK to enable run-time checking.  */
 /* #define GFC_CAF_CHECK  1  */
@@ -40,6 +41,21 @@ see the files COPYING3 and COPYING.RUNTI
 caf_static_t *caf_static_list = NULL;
 
 
+/* Keep in sync with mpi.c.  */
+static void
+caf_runtime_error (int error, const char *message, ...)
+{
+  va_list ap;
+  fprintf (stderr, "Fortran runtime error.");
+  va_start (ap, message);
+  fprintf (stderr, message, ap);
+  va_end (ap);
+  fprintf (stderr, "\n");
+
+  /* FIXME: Shutdown the Fortran RTL to flush the buffer.  PR 43849.  */
+  exit (error);
+}
+
 void
 _gfortran_caf_init (int *argc __attribute__ ((unused)),
 		    char ***argv __attribute__ ((unused)),
@@ -73,12 +89,12 @@ _gfortran_caf_register (ptrdiff_t size, 
 
   if (unlikely (local == NULL || token == NULL))
     {
+      const char msg[] = "Failed to allocate coarray";
       if (stat)
 	{
 	  *stat = 1;
 	  if (errmsg_len > 0)
 	    {
-	      const char msg[] = "Failed to allocate coarray";
 	      int len = ((int) sizeof (msg) > errmsg_len) ? errmsg_len
 							  : (int) sizeof (msg);
 	      memcpy (errmsg, msg, len);
@@ -88,10 +104,7 @@ _gfortran_caf_register (ptrdiff_t size, 
 	  return NULL;
 	}
       else
-	{
-	  fprintf (stderr, "ERROR: Failed to allocate coarray");
-	  exit (1);
-	}
+	  caf_runtime_error (1, msg);
     }
 
   if (stat)
Index: libgfortran/caf/mpi.c
===================================================================
--- libgfortran/caf/mpi.c	(revision 176230)
+++ libgfortran/caf/mpi.c	(working copy)
@@ -47,6 +47,7 @@ static int caf_is_finalized;
 caf_static_t *caf_static_list = NULL;
 
 
+/* Keep in sync with single.c.  */
 static void
 caf_runtime_error (int error, const char *message, ...)
 {
@@ -62,7 +63,7 @@ caf_runtime_error (int error, const char
   MPI_Abort (MPI_COMM_WORLD, error);
 
   /* Should be unreachable, but to make sure also call exit.  */
-  exit (2);
+  exit (error);
 }
 
 

Re: [Patch, Fortran] Add caf_runtime_error to libgfortran/caf/single.c

[Tobias Burnus]

On 07/14/2011 11:48 AM, Daniel Carrera wrote:
> This patch adds a caf_runtime_error function to the non-MPI 
> implementation of Coarray Fortran. It is based on the MPI function of 
> the same name in mpi.c.
>
> Ok to commit?

I was wondering - based on the discussion - whether one should remove 
the "int error" argument from caf_runtime_error and simply use "exit 
(EXIT_FAILURE)" for all exit() calls in mpi.c/single.c, cf. 
http://gcc.gnu.org/ml/fortran/2011-07/msg00140.html
But one can also do so as follow up patch.

Thus, OK for the patch as is - or with replacing all exit(...) by 
"exit(EXIT_FAILURE)", which  uses stdlib.h's EXIT_FAILURE. One then can 
also drop the "int error" argument to the caf_runtime_error function.

Thanks for the patch!

Tobias

> ChangeLog:
>
> 2011-07-14  Daniel Carrera <dcarrera@gmail.com>
>
>     * caf/single.c:  Include stdarg.h header.
>     (caf_runtime_error): New function based on the function in
>     mpi.c with the same name.
>     (_gfortran_caf_init): Use caf_runtime_error.
>     * caf/mpi.c (caf_runtime_error): Add a note to keep in sync
>     with the function in single.c.

Re: [Patch, Fortran] Add caf_runtime_error to libgfortran/caf/single.c

[Daniel Carrera]

On 07/14/2011 12:04 PM, Tobias Burnus wrote:
> I was wondering - based on the discussion - whether one should remove
> the "int error" argument from caf_runtime_error and simply use "exit
> (EXIT_FAILURE)" for all exit() calls in mpi.c/single.c, cf.
> http://gcc.gnu.org/ml/fortran/2011-07/msg00140.html
> But one can also do so as follow up patch.

You are the boss. The message I got from Nick's post is that it doesn't 
matter much and that you could even get surprising behaviour because 
EXIT_SUCCESS is not required to be zero and EXIT_FAILURE is not required 
to be non-zero. But maybe I missed the point. So it's up to you.

Cheers,
Daniel.
-- 
I'm not overweight, I'm undertall.

Re: [Patch, Fortran] Add caf_runtime_error to libgfortran/caf/single.c

[Tobias Burnus]

On 07/14/2011 12:14 PM, Daniel Carrera wrote:
> On 07/14/2011 12:04 PM, Tobias Burnus wrote:
>> I was wondering - based on the discussion - whether one should remove
>> the "int error" argument from caf_runtime_error and simply use "exit
>> (EXIT_FAILURE)" for all exit() calls in mpi.c/single.c, cf.
>> http://gcc.gnu.org/ml/fortran/2011-07/msg00140.html
>> But one can also do so as follow up patch.
>
> You are the boss. The message I got from Nick's post is that it 
> doesn't matter much and that you could even get surprising behaviour 
> because EXIT_SUCCESS is not required to be zero and EXIT_FAILURE is 
> not required to be non-zero. But maybe I missed the point. So it's up 
> to you.

Well, for "exit(3)" one knows that one will get a surprising in Windows: 
An abort. While EXIT_FAILURE [but also exit(1)] have a very high change 
to be working (nearly) everywhere. At the end, as long as the operating 
system does not react in a completely odd way, the exit status does not 
matter - it's the problem of the caller (e.g. some script) to make use 
of the result. If EXIT_FAILURE is 0 and the script assumes that it is a 
success, it's either a fault of the script writer, or of the one putting 
0 for EXIT_FAILURE into the header, or a fault of the OS designer.

Thus, I think the chance that one avoids surprises is higher with 
exit(EXIT_FAILURE) and one avoids having some random number (for the 
exit status) in the code. Users usually do not check the exact exit 
status value but only its 0- vs non-0-ness.

Hence, I think exit(EXIT_FAILURE) is better than the current version.

Tobias

Re: [Patch, Fortran] Add caf_runtime_error to libgfortran/caf/single.c

[Daniel Carrera]

Hi Tobias,

As per your suggestion, I'm copying to gfotran and gcc-patches. I've 
made the change your asked and I'm going to commit the patch.

Cheers,
Daniel.


On 07/14/2011 05:31 PM, Tobias Burnus wrote:
> On 07/14/2011 05:05 PM, Daniel Carrera wrote:
>> +caf_runtime_error (const char *message, ...)
>> +{
>> + va_list ap;
>> + fprintf (stderr, "Fortran runtime error.");
>
> Could you replace "." by ": " (colon, space), I think "Fortran runtime
> error: Could not ..." looks better than "Fortran runtime error.Could not
> ...".
>
> Otherwise, the patch is OK. Could you then send the patch as to fortran@
> and gcc-patches@?
>
> Tobias
>
> PS: You could directly commit the modified patch, simply reply to this
> email, add a CC to fortran@/gcc-patches@ and attach the patch and
> changelog. There is no need that I approve the patch again.


-- 
I'm not overweight, I'm undertall.

Re: [Patch, Fortran] Add caf_runtime_error to libgfortran/caf/single.c

[Daniel Carrera]

[-- Attachment #1: Type: text/plain, Size: 1622 bytes --]

And of course, here is the patch and ChangeLog.

2011-07-14  Daniel Carrera  <dcarrera@gmail.com>

	* caf/single.c:  Include stdarg.h header.
	(caf_runtime_error): New function. Use "exit(EXIT_FAILURE)".
	(_gfortran_caf_register): Use caf_runtime_error.
	(_gfortran_caf_sync_images): Use "exit(EXIT_FAILURE)".
	* caf/mpi.c (caf_runtime_error): Remove "error" parameter.
	Return EXIT_FAILURE instead.
	(_gfortran_caf_register): Update call to caf_runtime_error.
	(_gfortran_caf_sync_all): Ditto.
	(_gfortran_caf_sync_images): Ditto.
	(_gfortran_caf_error_stop_str): Use "exit(EXIT_FAILURE)".


Now I'm just waiting for SVN update before I commit...


On 07/14/2011 05:34 PM, Daniel Carrera wrote:
> Hi Tobias,
>
> As per your suggestion, I'm copying to gfotran and gcc-patches. I've
> made the change your asked and I'm going to commit the patch.
>
> Cheers,
> Daniel.
>
>
> On 07/14/2011 05:31 PM, Tobias Burnus wrote:
>> On 07/14/2011 05:05 PM, Daniel Carrera wrote:
>>> +caf_runtime_error (const char *message, ...)
>>> +{
>>> + va_list ap;
>>> + fprintf (stderr, "Fortran runtime error.");
>>
>> Could you replace "." by ": " (colon, space), I think "Fortran runtime
>> error: Could not ..." looks better than "Fortran runtime error.Could not
>> ...".
>>
>> Otherwise, the patch is OK. Could you then send the patch as to fortran@
>> and gcc-patches@?
>>
>> Tobias
>>
>> PS: You could directly commit the modified patch, simply reply to this
>> email, add a CC to fortran@/gcc-patches@ and attach the patch and
>> changelog. There is no need that I approve the patch again.
>
>


-- 
I'm not overweight, I'm undertall.

[-- Attachment #2: caf-runtime-error.diff --]
[-- Type: text/x-patch, Size: 3570 bytes --]

Index: libgfortran/caf/single.c
===================================================================
--- libgfortran/caf/single.c	(revision 176230)
+++ libgfortran/caf/single.c	(working copy)
@@ -28,6 +28,7 @@ see the files COPYING3 and COPYING.RUNTI
 #include <stdio.h>  /* For fputs and fprintf.  */
 #include <stdlib.h> /* For exit and malloc.  */
 #include <string.h> /* For memcpy and memset.  */
+#include <stdarg.h> /* For variadic arguments.  */
 
 /* Define GFC_CAF_CHECK to enable run-time checking.  */
 /* #define GFC_CAF_CHECK  1  */
@@ -40,6 +41,21 @@ see the files COPYING3 and COPYING.RUNTI
 caf_static_t *caf_static_list = NULL;
 
 
+/* Keep in sync with mpi.c.  */
+static void
+caf_runtime_error (const char *message, ...)
+{
+  va_list ap;
+  fprintf (stderr, "Fortran runtime error: ");
+  va_start (ap, message);
+  fprintf (stderr, message, ap);
+  va_end (ap);
+  fprintf (stderr, "\n");
+
+  /* FIXME: Shutdown the Fortran RTL to flush the buffer.  PR 43849.  */
+  exit (EXIT_FAILURE);
+}
+
 void
 _gfortran_caf_init (int *argc __attribute__ ((unused)),
 		    char ***argv __attribute__ ((unused)),
@@ -73,12 +89,12 @@ _gfortran_caf_register (ptrdiff_t size, 
 
   if (unlikely (local == NULL || token == NULL))
     {
+      const char msg[] = "Failed to allocate coarray";
       if (stat)
 	{
 	  *stat = 1;
 	  if (errmsg_len > 0)
 	    {
-	      const char msg[] = "Failed to allocate coarray";
 	      int len = ((int) sizeof (msg) > errmsg_len) ? errmsg_len
 							  : (int) sizeof (msg);
 	      memcpy (errmsg, msg, len);
@@ -88,10 +104,7 @@ _gfortran_caf_register (ptrdiff_t size, 
 	  return NULL;
 	}
       else
-	{
-	  fprintf (stderr, "ERROR: Failed to allocate coarray");
-	  exit (1);
-	}
+	  caf_runtime_error (msg);
     }
 
   if (stat)
@@ -140,7 +153,7 @@ _gfortran_caf_sync_images (int count __a
       {
 	fprintf (stderr, "COARRAY ERROR: Invalid image index %d to SYNC "
 		 "IMAGES", images[i]);
-	exit (1);
+	exit (EXIT_FAILURE);
       }
 #endif
 
Index: libgfortran/caf/mpi.c
===================================================================
--- libgfortran/caf/mpi.c	(revision 176230)
+++ libgfortran/caf/mpi.c	(working copy)
@@ -47,8 +47,9 @@ static int caf_is_finalized;
 caf_static_t *caf_static_list = NULL;
 
 
+/* Keep in sync with single.c.  */
 static void
-caf_runtime_error (int error, const char *message, ...)
+caf_runtime_error (const char *message, ...)
 {
   va_list ap;
   fprintf (stderr, "Fortran runtime error on image %d: ", caf_this_image);
@@ -59,10 +60,10 @@ caf_runtime_error (int error, const char
 
   /* FIXME: Shutdown the Fortran RTL to flush the buffer.  PR 43849.  */
   /* FIXME: Do some more effort than just MPI_ABORT.  */
-  MPI_Abort (MPI_COMM_WORLD, error);
+  MPI_Abort (MPI_COMM_WORLD, EXIT_FAILURE);
 
   /* Should be unreachable, but to make sure also call exit.  */
-  exit (2);
+  exit (EXIT_FAILURE);
 }
 
 
@@ -179,7 +180,7 @@ error:
 	  }
       }
     else
-      caf_runtime_error (caf_is_finalized ? STAT_STOPPED_IMAGE : 1, msg);
+      caf_runtime_error (msg);
   }
 
   return NULL;
@@ -223,7 +224,7 @@ _gfortran_caf_sync_all (int *stat, char 
 	    memset (&errmsg[len], ' ', errmsg_len-len);
 	}
       else
-	caf_runtime_error (caf_is_finalized ? STAT_STOPPED_IMAGE : ierr, msg);
+	caf_runtime_error (msg);
     }
 }
 
@@ -291,7 +292,7 @@ _gfortran_caf_sync_images (int count, in
 	    memset (&errmsg[len], ' ', errmsg_len-len);
 	}
       else
-	caf_runtime_error (caf_is_finalized ? STAT_STOPPED_IMAGE : ierr, msg);
+	caf_runtime_error (msg);
     }
 }
 

Re: [Patch, Fortran] Add caf_runtime_error to libgfortran/caf/single.c

[Janne Blomqvist]

On Thu, Jul 14, 2011 at 18:40, Daniel Carrera <dcarrera@gmail.com> wrote:
> And of course, here is the patch and ChangeLog.
>
> 2011-07-14  Daniel Carrera  <dcarrera@gmail.com>

>        * caf/mpi.c (caf_runtime_error): Remove "error" parameter.
>        Return EXIT_FAILURE instead.

Well, this changelog entry is incorrect; you're calling
exit(EXIT_FAILURE) and not returning a value.

From the patch:

 /* FIXME: Shutdown the Fortran RTL to flush the buffer.  PR 43849.  */

This is unnecessary, as the call to exit() will call the libgfortran
destructor which will close all units, as explained in comment #3 in
the PR.



-- 
Janne Blomqvist

Re: [Patch, Fortran] Add caf_runtime_error to libgfortran/caf/single.c

[Daniel Carrera]

On 07/14/2011 09:48 PM, Janne Blomqvist wrote:
>> 2011-07-14  Daniel Carrera<dcarrera@gmail.com>
>
>>         * caf/mpi.c (caf_runtime_error): Remove "error" parameter.
>>         Return EXIT_FAILURE instead.
>
> Well, this changelog entry is incorrect; you're calling
> exit(EXIT_FAILURE) and not returning a value.

Ok. What term should I have used? I was thinking that the program itself 
returns a value, but I can see that what I wrote was not optimal.


>  From the patch:
>
>   /* FIXME: Shutdown the Fortran RTL to flush the buffer.  PR 43849.  */
>
> This is unnecessary, as the call to exit() will call the libgfortran
> destructor which will close all units, as explained in comment #3 in
> the PR.

Ok. I didn't want to mess with comments that I didn't fully understand.

-- 
I'm not overweight, I'm undertall.

Re: [Patch, Fortran] Add caf_runtime_error to libgfortran/caf/single.c

[Tobias Burnus]

Janne Blomqvist wrote:
>         * caf/mpi.c (caf_runtime_error): Remove "error" parameter.
>         Return EXIT_FAILURE instead.
> > From the patch:
>
>   /* FIXME: Shutdown the Fortran RTL to flush the buffer.  PR 43849.  */
>
> This is unnecessary, as the call to exit() will call the libgfortran
> destructor which will close all units, as explained in comment #3 in
> the PR.

While I think it should be sufficient for single-processor usage, I am 
not sure that that way all I/O buffers gets written before one calls 
MPI_Finalize - nor am I sure how one would handle ERROR STOP with 
regards to I/O.

In terms of I/O, there are three kinds of I/O, which might need to be 
treated differently:
* I/O to stdout (OUTPUT_UNIT): Here, all output should be collected by 
MPI - I am not sure whether it will come to later in a destructor
* I/O to (local) files
* I/O via the communication library: Here, I see the greatest problems, 
but that's not in F2008's coarrays, but might well be added with the 
Technical Report.

I somehow would feel better if I could ensure that the buffers are 
flushed and the files closed before I pull the MPI plug (MPI_Finalize, 
MPI_Abort).

For reference, the comment Janne is referring to is the one at the 
bottom (comment 3) of http://gcc.gnu.org/bugzilla/show_bug.cgi?id=43849

Tobias

Re: [Patch, Fortran] Add caf_runtime_error to libgfortran/caf/single.c

[Janne Blomqvist]

On Thu, Jul 14, 2011 at 23:34, Tobias Burnus <burnus@net-b.de> wrote:
> Janne Blomqvist wrote:
>>
>>        * caf/mpi.c (caf_runtime_error): Remove "error" parameter.
>>        Return EXIT_FAILURE instead.
>> > From the patch:
>>
>>  /* FIXME: Shutdown the Fortran RTL to flush the buffer.  PR 43849.  */
>>
>> This is unnecessary, as the call to exit() will call the libgfortran
>> destructor which will close all units, as explained in comment #3 in
>> the PR.
>
> While I think it should be sufficient for single-processor usage, I am not
> sure that that way all I/O buffers gets written before one calls
> MPI_Finalize - nor am I sure how one would handle ERROR STOP with regards to
> I/O.

Ah, I read that comment from caf/single.c, but now I see it in the
caf/mpi.c part of the patch as well. Yeah, in that case it might
matter, depending on how the MPI library implements MPI_Abort(). That
is, does it call exit() (in which case the destructor will be called)
or will it call abort() or raise some other fatal signal.

At least Open MPI seems to implement it by sending SIGTERM

http://www.open-mpi.org/doc/v1.4/man3/MPI_Abort.3.php

Come to think of it, in this case, if you want to use MPI_Abort()
rather than MPI_Finalize() + exit(), you should probably reset the
fatal signal handlers to the default one unless you want the backtrace
to be printed.


-- 
Janne Blomqvist