[PATCH, fortran] PR 53379 Backtrace on error termination

[PATCH, fortran] PR 53379 Backtrace on error termination

[David Edelsohn]

libbacktrace is not supported on AIX.  This patch breaks bootstrap on AIX.

It's okay if Fortran backtrace does not work on AIX, but not all
targets support libbacktrace.

Thanks, David

Re: [PATCH, fortran] PR 53379 Backtrace on error termination

[FX]

> libbacktrace is not supported on AIX.  This patch breaks bootstrap on AIX.
> It's okay if Fortran backtrace does not work on AIX, but not all
> targets support libbacktrace.

libbacktrace is designed to be compiled on all targets. Some targets offer full support, some offer nothing, but libbacktrace is compiled and its headers provided in all cases.

Can you please give us something to investigate? Like, the error message you’re seeing.

FX

Re: [PATCH, fortran] PR 53379 Backtrace on error termination

[David Edelsohn]

On Tue, Sep 8, 2015 at 9:15 AM, FX <fxcoudert@gmail.com> wrote:
>> libbacktrace is not supported on AIX.  This patch breaks bootstrap on AIX.
>> It's okay if Fortran backtrace does not work on AIX, but not all
>> targets support libbacktrace.
>
> libbacktrace is designed to be compiled on all targets. Some targets offer full support, some offer nothing, but libbacktrace is compiled and its headers provided in all cases.
>
> Can you please give us something to investigate? Like, the error message you’re seeing.

/home/dje/src/src/libbacktrace/posix.c: In function 'backtrace_open':
/home/dje/src/src/libbacktrace/posix.c:67:32: error: overflow in
implicit constant conversion [-Werror=overflow]
   descriptor = open (filename, O_RDONLY | O_BINARY | O_CLOEXEC);
                                ^

Thanks, David

Re: [PATCH, fortran] PR 53379 Backtrace on error termination

[David Edelsohn]

On Tue, Sep 8, 2015 at 9:40 AM, David Edelsohn <dje.gcc@gmail.com> wrote:
> On Tue, Sep 8, 2015 at 9:15 AM, FX <fxcoudert@gmail.com> wrote:
>>> libbacktrace is not supported on AIX.  This patch breaks bootstrap on AIX.
>>> It's okay if Fortran backtrace does not work on AIX, but not all
>>> targets support libbacktrace.
>>
>> libbacktrace is designed to be compiled on all targets. Some targets offer full support, some offer nothing, but libbacktrace is compiled and its headers provided in all cases.
>>
>> Can you please give us something to investigate? Like, the error message you’re seeing.
>
> /home/dje/src/src/libbacktrace/posix.c: In function 'backtrace_open':
> /home/dje/src/src/libbacktrace/posix.c:67:32: error: overflow in
> implicit constant conversion [-Werror=overflow]
>    descriptor = open (filename, O_RDONLY | O_BINARY | O_CLOEXEC);
>                                 ^

AIX system headers do not define O_BINARY, so posix.c defines it as 0.

#define _FCLOEXEC       0x0000001000000000L
#define O_CLOEXEC       _FCLOEXEC       /* sets FD_CLOEXEC on open      */

Thanks, David

Re: [PATCH, fortran] PR 53379 Backtrace on error termination

[FX]

> #define _FCLOEXEC       0x0000001000000000L
> #define O_CLOEXEC       _FCLOEXEC       /* sets FD_CLOEXEC on open      */

That’s weird, and definitely an AIX bug: http://pubs.opengroup.org/onlinepubs/9699919799/functions/open.html
How does that even work? open() takes int as second arg.

FX

Re: [PATCH, fortran] PR 53379 Backtrace on error termination

[David Edelsohn]

On Tue, Sep 8, 2015 at 9:51 AM, FX <fxcoudert@gmail.com> wrote:
>> #define _FCLOEXEC       0x0000001000000000L
>> #define O_CLOEXEC       _FCLOEXEC       /* sets FD_CLOEXEC on open      */
>
> That’s weird, and definitely an AIX bug: http://pubs.opengroup.org/onlinepubs/9699919799/functions/open.html

Welcome to AIX :-/

> How does that even work? open() takes int as second arg.

No one else uses it? ;-)

The following kluge works:

Index: posix.c
===================================================================
--- posix.c     (revision 227528)
+++ posix.c     (working copy)
@@ -45,6 +45,10 @@
 #define O_BINARY 0
 #endif

+#ifdef _AIX
+#undef O_CLOEXEC
+#endif
+
 #ifndef O_CLOEXEC
 #define O_CLOEXEC 0
 #endif

Thanks, David

Re: [PATCH, fortran] PR 53379 Backtrace on error termination

[Mike Stump]

On Sep 8, 2015, at 6:53 AM, David Edelsohn <dje.gcc@gmail.com> wrote:
> On Tue, Sep 8, 2015 at 9:51 AM, FX <fxcoudert@gmail.com> wrote:
>>> #define _FCLOEXEC       0x0000001000000000L
>>> #define O_CLOEXEC       _FCLOEXEC       /* sets FD_CLOEXEC on open      */
>> 
>> That’s weird, and definitely an AIX bug: http://pubs.opengroup.org/onlinepubs/9699919799/functions/open.html
> 
> Welcome to AIX :-/
> 
>> How does that even work? open() takes int as second arg.
> 
> No one else uses it? ;-)
> 
> The following kluge works:
> 
> Index: posix.c
> ===================================================================
> --- posix.c     (revision 227528)
> +++ posix.c     (working copy)
> @@ -45,6 +45,10 @@
> #define O_BINARY 0
> #endif
> 
> +#ifdef _AIX
> +#undef O_CLOEXEC
> +#endif
> +

Not a big issue, but slightly better if (O_CLOEXEC>>32) != 0 is also true.  See, if AIX should ever define this to a sensible value, the above would disappear the feature.  However, if they did, then this expression should then be false.

Re: [PATCH, fortran] PR 53379 Backtrace on error termination

[FX]

> Not a big issue, but slightly better if (O_CLOEXEC>>32) != 0 is also true.  See, if AIX should ever define this to a sensible value, the above would disappear the feature.  However, if they did, then this expression should then be false.

Sounds good.
This being a libbacktrace patch, you need Ian to approve it, it’s his area.

FX

Re: [PATCH, fortran] PR 53379 Backtrace on error termination

[Ian Lance Taylor]

Mike Stump <mikestump@comcast.net> writes:

> Not a big issue, but slightly better if (O_CLOEXEC>>32) != 0 is also
> true.  See, if AIX should ever define this to a sensible value, the
> above would disappear the feature.  However, if they did, then this
> expression should then be false.

Yes, I think this might be even better in code.  How about something
like

  /* On some versions of AIX O_CLOEXEC does not fit in int, so use a
     cast to force it.  */
  descriptor = open (filename, (int) (O_RDONLY | O_BINARY | O_CLOEXEC));

Does that work on AIX?

Ian

Re: [PATCH, fortran] PR 53379 Backtrace on error termination

[David Edelsohn]

On Wed, Sep 9, 2015 at 12:12 AM, Ian Lance Taylor <ian@airs.com> wrote:
> Mike Stump <mikestump@comcast.net> writes:
>
>> Not a big issue, but slightly better if (O_CLOEXEC>>32) != 0 is also
>> true.  See, if AIX should ever define this to a sensible value, the
>> above would disappear the feature.  However, if they did, then this
>> expression should then be false.
>
> Yes, I think this might be even better in code.  How about something
> like
>
>   /* On some versions of AIX O_CLOEXEC does not fit in int, so use a
>      cast to force it.  */
>   descriptor = open (filename, (int) (O_RDONLY | O_BINARY | O_CLOEXEC));
>
> Does that work on AIX?

Yes, that approach also fixes the warning and the build failure.

Thanks, David

Re: [PATCH, fortran] PR 53379 Backtrace on error termination

[Mike Stump]

On Sep 8, 2015, at 9:12 PM, Ian Lance Taylor <ian@airs.com> wrote:
> Yes, I think this might be even better in code.  How about something
> like
> 
>  /* On some versions of AIX O_CLOEXEC does not fit in int, so use a
>     cast to force it.  */
>  descriptor = open (filename, (int) (O_RDONLY | O_BINARY | O_CLOEXEC));

How about:

  descriptor = open (filename, /* AIX */ (int) (O_RDONLY | O_BINARY | O_CLOEXEC));

?  It makes it removable in the nearer term.  :-)

Re: [PATCH, fortran] PR 53379 Backtrace on error termination

[David Edelsohn]

On Wed, Sep 9, 2015 at 12:12 AM, Ian Lance Taylor <ian@airs.com> wrote:
> Mike Stump <mikestump@comcast.net> writes:
>
>> Not a big issue, but slightly better if (O_CLOEXEC>>32) != 0 is also
>> true.  See, if AIX should ever define this to a sensible value, the
>> above would disappear the feature.  However, if they did, then this
>> expression should then be false.
>
> Yes, I think this might be even better in code.  How about something
> like
>
>   /* On some versions of AIX O_CLOEXEC does not fit in int, so use a
>      cast to force it.  */
>   descriptor = open (filename, (int) (O_RDONLY | O_BINARY | O_CLOEXEC));
>
> Does that work on AIX?

Any decision about this patch?

Thanks, David

Re: [PATCH, fortran] PR 53379 Backtrace on error termination

[Ian Lance Taylor]

On Thu, Sep 17, 2015 at 7:08 AM, David Edelsohn <dje.gcc@gmail.com> wrote:
> On Wed, Sep 9, 2015 at 12:12 AM, Ian Lance Taylor <ian@airs.com> wrote:
>> Mike Stump <mikestump@comcast.net> writes:
>>
>>> Not a big issue, but slightly better if (O_CLOEXEC>>32) != 0 is also
>>> true.  See, if AIX should ever define this to a sensible value, the
>>> above would disappear the feature.  However, if they did, then this
>>> expression should then be false.
>>
>> Yes, I think this might be even better in code.  How about something
>> like
>>
>>   /* On some versions of AIX O_CLOEXEC does not fit in int, so use a
>>      cast to force it.  */
>>   descriptor = open (filename, (int) (O_RDONLY | O_BINARY | O_CLOEXEC));
>>
>> Does that work on AIX?
>
> Any decision about this patch?

This patch is OK if it works.

Ian

Re: [PATCH, fortran] PR 53379 Backtrace on error termination

[FX]

> /home/dje/src/src/libbacktrace/posix.c: In function 'backtrace_open':
> /home/dje/src/src/libbacktrace/posix.c:67:32: error: overflow in
> implicit constant conversion [-Werror=overflow]
>   descriptor = open (filename, O_RDONLY | O_BINARY | O_CLOEXEC);

?? I have a hard time understanding how the non-constant filename can give an overflow error. Or maybe the error is wrong, and it’s O_RDONLY | O_BINARY | O_CLOEXEC that have crazy values?

This looks like a valid POSIX construct to me.

FX

[PATCH, fortran] PR 53379 Backtrace on error termination

[Janne Blomqvist]

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

Hi,

the consesus in PR 53379 seems to be that a backtrace is desired in
case of error termination. The attached patch implements this.

Regtested on x86_64-pc-linux-gnu, Ok for trunk?

2015-09-05  Janne Blomqvist  <jb@gcc.gnu.org>

    PR fortran/53579
    * libgfortran.h (exit_error): New prototype.
    * runtime/error.c (exit_error): New function.
    (os_error): Call exit_error instead of exit.
    (runtime_error): Likewise.
    (runtime_error_at): Likewise.
    (internal_error): Likewise.
    (generate_error): Likewise.
    (notify_std): Likewise.
    * runtime/stop.c (error_stop_string): Likewise.
    (error_stop_numeric): Likewise.


-- 
Janne Blomqvist

[-- Attachment #2: bt.diff --]
[-- Type: text/plain, Size: 4391 bytes --]

diff --git a/libgfortran/libgfortran.h b/libgfortran/libgfortran.h
index 3eb0d85..81240e5 100644
--- a/libgfortran/libgfortran.h
+++ b/libgfortran/libgfortran.h
@@ -675,6 +675,9 @@ internal_proto(show_backtrace);
 extern _Noreturn void sys_abort (void);
 internal_proto(sys_abort);
 
+extern _Noreturn void exit_error (int);
+internal_proto(exit_error);
+
 extern ssize_t estr_write (const char *);
 internal_proto(estr_write);
 
diff --git a/libgfortran/runtime/error.c b/libgfortran/runtime/error.c
index 4aabe4a..d357edb 100644
--- a/libgfortran/runtime/error.c
+++ b/libgfortran/runtime/error.c
@@ -74,15 +74,17 @@ see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
 
    2.3.5 also explains how co-images synchronize during termination.
 
-   In libgfortran we have two ways of ending a program. exit(code) is
-   a normal exit; calling exit() also causes open units to be
-   closed. No backtrace or core dump is needed here. When something
-   goes wrong, we have sys_abort() which tries to print the backtrace
-   if -fbacktrace is enabled, and then dumps core; whether a core file
-   is generated is system dependent. When aborting, we don't flush and
-   close open units, as program memory might be corrupted and we'd
-   rather risk losing dirty data in the buffers rather than corrupting
-   files on disk.
+   In libgfortran we have three ways of ending a program. exit(code)
+   is a normal exit; calling exit() also causes open units to be
+   closed. No backtrace or core dump is needed here.  For error
+   termination, we have exit_error(status), which prints a backtrace
+   if backtracing is enabled, then exits.  Finally, when something
+   goes terribly wrong, we have sys_abort() which tries to print the
+   backtrace if -fbacktrace is enabled, and then dumps core; whether a
+   core file is generated is system dependent. When aborting, we don't
+   flush and close open units, as program memory might be corrupted
+   and we'd rather risk losing dirty data in the buffers rather than
+   corrupting files on disk.
 
 */
 
@@ -181,6 +183,23 @@ sys_abort (void)
 }
 
 
+/* Exit in case of error termination. If backtracing is enabled, print
+   backtrace, then exit.  */
+
+void
+exit_error (int status)
+{
+  if (options.backtrace == 1
+      || (options.backtrace == -1 && compile_options.backtrace == 1))
+    {
+      estr_write ("\nError termination. Backtrace:\n");
+      show_backtrace (false);
+    }
+  exit (status);
+}
+
+
+
 /* gfc_xtoa()-- Integer to hexadecimal conversion.  */
 
 const char *
@@ -326,7 +345,7 @@ os_error (const char *message)
   estr_write ("\n");
   estr_write (message);
   estr_write ("\n");
-  exit (1);
+  exit_error (1);
 }
 iexport(os_error);
 
@@ -345,7 +364,7 @@ runtime_error (const char *message, ...)
   st_vprintf (message, ap);
   va_end (ap);
   estr_write ("\n");
-  exit (2);
+  exit_error (2);
 }
 iexport(runtime_error);
 
@@ -364,7 +383,7 @@ runtime_error_at (const char *where, const char *message, ...)
   st_vprintf (message, ap);
   va_end (ap);
   estr_write ("\n");
-  exit (2);
+  exit_error (2);
 }
 iexport(runtime_error_at);
 
@@ -402,7 +421,7 @@ internal_error (st_parameter_common *cmp, const char *message)
      because hopefully it doesn't happen too often).  */
   stupid_function_name_for_static_linking();
 
-  exit (3);
+ exit_error (3);
 }
 
 
@@ -574,7 +593,7 @@ generate_error (st_parameter_common *cmp, int family, const char *message)
   estr_write ("Fortran runtime error: ");
   estr_write (message);
   estr_write ("\n");
-  exit (2);
+  exit_error (2);
 }
 iexport(generate_error);
 
@@ -636,7 +655,7 @@ notify_std (st_parameter_common *cmp, int std, const char * message)
       estr_write ("Fortran runtime error: ");
       estr_write (message);
       estr_write ("\n");
-      exit (2);
+      exit_error (2);
     }
   else
     {
diff --git a/libgfortran/runtime/stop.c b/libgfortran/runtime/stop.c
index 5c5483b..8b8a41f 100644
--- a/libgfortran/runtime/stop.c
+++ b/libgfortran/runtime/stop.c
@@ -145,7 +145,7 @@ error_stop_string (const char *string, GFC_INTEGER_4 len)
   (void) write (STDERR_FILENO, string, len);
   estr_write ("\n");
 
-  exit (1);
+  exit_error (1);
 }
 
 
@@ -159,5 +159,5 @@ error_stop_numeric (GFC_INTEGER_4 code)
 {
   report_exception ();
   st_printf ("ERROR STOP %d\n", (int) code);
-  exit (code);
+  exit_error (code);
 }

Re: [PATCH, fortran] PR 53379 Backtrace on error termination

[FX]

> 2015-09-05  Janne Blomqvist  <jb@gcc.gnu.org>
> 
>    PR fortran/53579
>    * libgfortran.h (exit_error): New prototype.
>    * runtime/error.c (exit_error): New function.
>    (os_error): Call exit_error instead of exit.
>    (runtime_error): Likewise.
>    (runtime_error_at): Likewise.
>    (internal_error): Likewise.
>    (generate_error): Likewise.
>    (notify_std): Likewise.
>    * runtime/stop.c (error_stop_string): Likewise.
>    (error_stop_numeric): Likewise.

OK, except that the PR number is wrong in the ChangeLog).
(Related: I’ve just closed PR 63930 as WONTFIX.)

Thanks for the patch!

FX