On Tue, Sep 1, 2015 at 1:04 AM, FX <fxcoud...@gmail.com> wrote:
>> the attached patch improves the error handling for backtrace failing,
>> by printing the error number or the error string in addition to the
>> message. It also fixes a potential null pointer crash in gf_strerror.
>>
>> Regtested on x86_64-pc-linux-gnu, Ok for trunk?
>
> Mostly OK. I have one question, though: in the specific case from the PR, the
> error in the backtrace is an OS error (allocating memory), which had set the
> errno variable. But in other cases, it is (I think) possible that
> libbacktrace itself fails and call error_callback() without errno being set.
> In that case, the errno message will be confusing.
Hmm, in that case errnum must be set to 0. What about the attached
patch, which prints the existing message if errnum == 0, and the new
and improved only for errnum > 0?
>
> So, for that part of the patch, I think it would make more sense to change
> libbacktrace to give out a reasonable error message (and not “mmap”). A quick
> grep through the libbacktrace sources reveal that most calls to
> error_callback() actually provide insightful error messages. The ones that
> need to be improved are:
>
> alloc.c: error_callback (data, "malloc", errno);
> alloc.c: error_callback (data, "realloc", errno);
> alloc.c: error_callback (data, "realloc", errno);
> mmap.c: error_callback (data, "mmap", errno);
> mmapio.c: error_callback (data, "mmap", errno);
> mmapio.c: error_callback (data, "munmap", errno);
> posix.c: error_callback (data, "close", errno);
> read.c: error_callback (data, "lseek", errno);
> read.c: error_callback (data, "read", errno);
This seems to be the unix tradition of terse error messages, I suppose
it depends on other libbacktrace users whether such a change would be
seen as desirable..
--
Janne Blomqvist
diff --git a/libgfortran/io/write.c b/libgfortran/io/write.c
index 7599659..e226236 100644
--- a/libgfortran/io/write.c
+++ b/libgfortran/io/write.c
@@ -1032,47 +1032,6 @@ ztoa_big (const char *s, char *buffer, int len,
GFC_UINTEGER_LARGEST *n)
return buffer;
}
-/* gfc_itoa()-- Integer to decimal conversion.
- The itoa function is a widespread non-standard extension to standard
- C, often declared in <stdlib.h>. Even though the itoa defined here
- is a static function we take care not to conflict with any prior
- non-static declaration. Hence the 'gfc_' prefix, which is normally
- reserved for functions with external linkage. */
-
-static const char *
-gfc_itoa (GFC_INTEGER_LARGEST n, char *buffer, size_t len)
-{
- int negative;
- char *p;
- GFC_UINTEGER_LARGEST t;
-
- assert (len >= GFC_ITOA_BUF_SIZE);
-
- if (n == 0)
- return "0";
-
- negative = 0;
- t = n;
- if (n < 0)
- {
- negative = 1;
- t = -n; /*must use unsigned to protect from overflow*/
- }
-
- p = buffer + GFC_ITOA_BUF_SIZE - 1;
- *p = '\0';
-
- while (t != 0)
- {
- *--p = '0' + (t % 10);
- t /= 10;
- }
-
- if (negative)
- *--p = '-';
- return p;
-}
-
void
write_i (st_parameter_dt *dtp, const fnode *f, const char *p, int len)
diff --git a/libgfortran/libgfortran.h b/libgfortran/libgfortran.h
index 553cef1..3eb0d85 100644
--- a/libgfortran/libgfortran.h
+++ b/libgfortran/libgfortran.h
@@ -651,7 +651,7 @@ export_proto(store_exe_path);
/* backtrace.c */
-extern void show_backtrace (int);
+extern void show_backtrace (bool);
internal_proto(show_backtrace);
@@ -838,6 +838,9 @@ internal_proto(fc_strdup);
extern char *fc_strdup_notrim(const char *, gfc_charlen_type);
internal_proto(fc_strdup_notrim);
+extern const char *gfc_itoa(GFC_INTEGER_LARGEST, char *, size_t);
+internal_proto(gfc_itoa);
+
/* io/intrinsics.c */
extern void flush_all_units (void);
diff --git a/libgfortran/runtime/backtrace.c b/libgfortran/runtime/backtrace.c
index 0d7c1fc..12ad76a 100644
--- a/libgfortran/runtime/backtrace.c
+++ b/libgfortran/runtime/backtrace.c
@@ -26,6 +26,7 @@ see the files COPYING3 and COPYING.RUNTIME respectively. If
not, see
#include <string.h>
#include <stdlib.h>
+#include <errno.h>
#ifdef HAVE_UNISTD_H
#include <unistd.h>
@@ -38,8 +39,9 @@ see the files COPYING3 and COPYING.RUNTIME respectively. If
not, see
/* Store our own state while backtracing. */
struct mystate
{
- int try_simple;
int frame;
+ bool try_simple;
+ bool in_signal_handler;
};
@@ -65,15 +67,35 @@ static void
error_callback (void *data, const char *msg, int errnum)
{
struct mystate *state = (struct mystate *) data;
+#define ERRHDR "\nCould not print backtrace: "
+
if (errnum < 0)
{
- state->try_simple = 1;
+ state->try_simple = true;
return;
}
-
- estr_write ("\nSomething went wrong while printing the backtrace: ");
- estr_write (msg);
- estr_write ("\n");
+ else if (errnum == 0)
+ {
+ estr_write (ERRHDR);
+ estr_write (msg);
+ estr_write ("\n");
+ }
+ else
+ {
+ char errbuf[256];
+ if (state->in_signal_handler)
+ {
+ estr_write (ERRHDR);
+ estr_write (msg);
+ estr_write (", errno: ");
+ const char *p = gfc_itoa (errnum, errbuf, sizeof (errbuf));
+ estr_write (p);
+ estr_write ("\n");
+ }
+ else
+ st_printf (ERRHDR "%s: %s\n", msg,
+ gf_strerror (errnum, errbuf, sizeof (errbuf)));
+ }
}
static int
@@ -110,10 +132,10 @@ full_callback (void *data, uintptr_t pc, const char
*filename,
/* Display the backtrace. */
void
-show_backtrace (int in_signal_handler)
+show_backtrace (bool in_signal_handler)
{
struct backtrace_state *lbstate;
- struct mystate state = { 0, 0 };
+ struct mystate state = { 0, false, in_signal_handler };
lbstate = backtrace_create_state (NULL, 1, error_callback, NULL);
@@ -147,6 +169,6 @@ export_proto (backtrace);
void
backtrace (void)
{
- show_backtrace (0);
+ show_backtrace (false);
}
diff --git a/libgfortran/runtime/compile_options.c
b/libgfortran/runtime/compile_options.c
index f44256b..087c070 100644
--- a/libgfortran/runtime/compile_options.c
+++ b/libgfortran/runtime/compile_options.c
@@ -126,7 +126,7 @@ backtrace_handler (int signum)
show_signal (signum);
estr_write ("\nBacktrace for this error:\n");
- show_backtrace (1);
+ show_backtrace (true);
/* Now reraise the signal. We reactivate the signal's
default handling, which is to terminate the process.
diff --git a/libgfortran/runtime/error.c b/libgfortran/runtime/error.c
index 9eb0764..4aabe4a 100644
--- a/libgfortran/runtime/error.c
+++ b/libgfortran/runtime/error.c
@@ -173,7 +173,7 @@ sys_abort (void)
|| (options.backtrace == -1 && compile_options.backtrace == 1))
{
estr_write ("\nProgram aborted. Backtrace:\n");
- show_backtrace (0);
+ show_backtrace (false);
signal (SIGABRT, SIG_DFL);
}
@@ -221,8 +221,16 @@ gf_strerror (int errnum,
#ifdef HAVE_STRERROR_L
locale_t myloc = newlocale (LC_CTYPE_MASK | LC_MESSAGES_MASK, "",
(locale_t) 0);
- char *p = strerror_l (errnum, myloc);
- freelocale (myloc);
+ char *p;
+ if (myloc)
+ {
+ p = strerror_l (errnum, myloc);
+ freelocale (myloc);
+ }
+ else
+ /* newlocale might fail e.g. due to running out of memory, fall
+ back to the simpler strerror. */
+ p = strerror (errnum);
return p;
#elif defined(HAVE_STRERROR_R)
#ifdef HAVE_USELOCALE
diff --git a/libgfortran/runtime/string.c b/libgfortran/runtime/string.c
index 3c339da..5bd0f61 100644
--- a/libgfortran/runtime/string.c
+++ b/libgfortran/runtime/string.c
@@ -1,7 +1,7 @@
/* Copyright (C) 2002-2015 Free Software Foundation, Inc.
Contributed by Paul Brook
-This file is part of the GNU Fortran 95 runtime library (libgfortran).
+This file is part of the GNU Fortran runtime library (libgfortran).
Libgfortran is free software; you can redistribute it and/or modify
it under the terms of the GNU General Public License as published by
@@ -167,3 +167,48 @@ find_option (st_parameter_common *cmp, const char *s1,
gfc_charlen_type s1_len,
return -1;
}
+
+
+/* gfc_itoa()-- Integer to decimal conversion.
+ The itoa function is a widespread non-standard extension to
+ standard C, often declared in <stdlib.h>. Even though the itoa
+ defined here is a static function we take care not to conflict with
+ any prior non-static declaration. Hence the 'gfc_' prefix, which
+ is normally reserved for functions with external linkage. Notably,
+ in contrast to the *printf() family of functions, this ought to be
+ async-signal-safe. */
+
+const char *
+gfc_itoa (GFC_INTEGER_LARGEST n, char *buffer, size_t len)
+{
+ int negative;
+ char *p;
+ GFC_UINTEGER_LARGEST t;
+
+ if (len < GFC_ITOA_BUF_SIZE)
+ sys_abort ();
+
+ if (n == 0)
+ return "0";
+
+ negative = 0;
+ t = n;
+ if (n < 0)
+ {
+ negative = 1;
+ t = -n; /*must use unsigned to protect from overflow*/
+ }
+
+ p = buffer + GFC_ITOA_BUF_SIZE - 1;
+ *p = '\0';
+
+ while (t != 0)
+ {
+ *--p = '0' + (t % 10);
+ t /= 10;
+ }
+
+ if (negative)
+ *--p = '-';
+ return p;
+}
