Patchwork [libfortran] PR 48931 Async-signal-safety of backtrace signal handler

login
register
mail settings
Submitter Janne Blomqvist
Date May 14, 2011, 7:40 p.m.
Message ID <BANLkTimighoVZpe2Z02-titfhqmyvPWckw@mail.gmail.com>
Download mbox | patch
Permalink /patch/95579/
State New
Headers show

Comments

Janne Blomqvist - May 14, 2011, 7:40 p.m.
Hi,

the current version of showing the backtrace is not async-signal-safe
as it uses backtrace_symbols() which, in turn, uses malloc(). The
attached patch changes the backtrace printing functionality to instead
use backtrace_symbols_fd() and pipes.

Also, it does some other work on backtrace printing:

- Nowadays the main program has the same debug symbol name as whatever
the name of the main program is, rather than MAIN__. Therefore remove
special case logic related to that.

- Don't filter out stack frames from inside libgfortran, as this might
lose information in case the reason for the crash is in the library.

- Reformat the output slightly, so the each stack frame fits on one
line, and begins with #NUM, similar to GDB.

For instance, the small program

subroutine c
  call abort ()
end subroutine c

subroutine b
  call c
end subroutine b

subroutine a
  call b
end subroutine a

program bt
  call a
end program bt


compiled with -g -fno-whole-file now generates


Backtrace for this error:
  #0  /home/janne/src/gfortran/trunk/install/lib64/libgfortran.so.3(+0x182b7)[0x7f9c8a2c42b7]
  #1  /home/janne/src/gfortran/trunk/install/lib64/libgfortran.so.3(+0x19d07)[0x7f9c8a2c5d07]
  #2  /home/janne/src/gfortran/trunk/install/lib64/libgfortran.so.3(+0xe1e49)[0x7f9c8a38de49]
  #3  in b_ at bt.f90:5 (0x400612)
  #4  in b_ at bt.f90:7 (0x400620)
  #5  in a_ at bt.f90:11 (0x400630)
  #6  in bt at bt.f90:15 (0x400640)
Aborted

In this case the 3 first frames are the output from
backtrace_symbols_fd() since addr2line can't get the symbols from
libgfortran. With static linking addr2line can see it:


Backtrace for this error:
  #0  in _gfortrani_show_backtrace at backtrace.c:85 (0x405427)
  #1  in _gfortrani_sys_abort at error.c:176 (0x4007B7)
  #2  in _gfortran_abort (0x404469)
  #3  in b_ at bt.f90:5 (0x400402)
  #4  in b_ at bt.f90:7 (0x400410)
  #5  in a_ at bt.f90:11 (0x400420)
  #6  in bt at bt.f90:15 (0x400430)
Aborted

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

2011-05-14  Janne Blomqvist  <jb@gcc.gnu.org>

	PR libfortran/48931
	* configure.ac: Check for backtrace_symbols_fd instead of
	backtrace_symbols.
	* config.h.in: Regenerated.
	* configure: Regenerated.
	* runtime/backtrace.c (local_strcasestr): Remove.
	(bt_header): New function.
	(dump_glibc_backtrace): Remove.
	(show_backtrace): Rework to use backtrace_symbols_fd and pipes,
	reformat output.
Toon Moene - May 17, 2011, 5:50 p.m.
On 05/14/2011 09:40 PM, Janne Blomqvist wrote:

> Hi,
>
> the current version of showing the backtrace is not async-signal-safe
> as it uses backtrace_symbols() which, in turn, uses malloc(). The
> attached patch changes the backtrace printing functionality to instead
> use backtrace_symbols_fd() and pipes.

Great - this would solve a problem I filed a bugzilla report for years 
ago (unfortunately, I do not know the number of it).

I closed it WONTFIX, because neither FX nor I could come up with an 
alternative way *not* using malloc.

[ The problem was getting a traceback after corruption of the
   malloc arena, which just hangs under the current implementation. ]
Toon Moene - May 17, 2011, 7:34 p.m.
On 05/17/2011 07:50 PM, Toon Moene wrote:

> On 05/14/2011 09:40 PM, Janne Blomqvist wrote:
>
>> Hi,
>>
>> the current version of showing the backtrace is not async-signal-safe
>> as it uses backtrace_symbols() which, in turn, uses malloc(). The
>> attached patch changes the backtrace printing functionality to instead
>> use backtrace_symbols_fd() and pipes.
>
> Great - this would solve a problem I filed a bugzilla report for years
> ago (unfortunately, I do not know the number of it).

It was 33905 (2007-10-26).

Patch

diff --git a/libgfortran/configure.ac b/libgfortran/configure.ac
index cf38fb0..9bb6210 100644
--- a/libgfortran/configure.ac
+++ b/libgfortran/configure.ac
@@ -267,7 +267,7 @@  AC_CHECK_FUNCS(localtime_r gmtime_r strerror_r getpwuid_r ttyname_r)
 AC_CHECK_FUNCS(clock_gettime strftime)
 
 # Check for glibc backtrace functions
-AC_CHECK_FUNCS(backtrace backtrace_symbols)
+AC_CHECK_FUNCS(backtrace backtrace_symbols_fd)
 
 # Check libc for getgid, getpid, getuid
 AC_CHECK_LIB([c],[getgid],[AC_DEFINE([HAVE_GETGID],[1],[libc includes getgid])])
diff --git a/libgfortran/runtime/backtrace.c b/libgfortran/runtime/backtrace.c
index 10917d3..ce1a0c7 100644
--- a/libgfortran/runtime/backtrace.c
+++ b/libgfortran/runtime/backtrace.c
@@ -54,56 +54,19 @@  see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
 #define CAN_FORK (defined(HAVE_FORK) && defined(HAVE_EXECVP) \
 		  && defined(HAVE_WAIT))
 #define GLIBC_BACKTRACE (defined(HAVE_BACKTRACE) \
-			 && defined(HAVE_BACKTRACE_SYMBOLS))
+			 && defined(HAVE_BACKTRACE_SYMBOLS_FD))
 #define CAN_PIPE (CAN_FORK && defined(HAVE_PIPE) \
 		  && defined(HAVE_DUP2) && defined(HAVE_FDOPEN) \
 		  && defined(HAVE_CLOSE))
 
 
-#if GLIBC_BACKTRACE && CAN_PIPE
-static char *
-local_strcasestr (const char *s1, const char *s2)
+/* GDB style #NUM index for each stack frame.  */
+static void 
+bt_header (int num)
 {
-#ifdef HAVE_STRCASESTR
-  return strcasestr (s1, s2);
-#else
-
-  const char *p = s1;
-  const size_t len = strlen (s2);
-  const char u = *s2, v = isupper((int) *s2) ? tolower((int) *s2)
-				  : (islower((int) *s2) ? toupper((int) *s2)
-							: *s2);
-
-  while (1)
-    {
-      while (*p != u && *p != v && *p)
-	p++;
-      if (*p == 0)
-	return NULL;
-      if (strncasecmp (p, s2, len) == 0)
-	return (char *)p;
-    }
-#endif
+  st_printf ("  #%d  ", num);
 }
-#endif
-
-
-#if GLIBC_BACKTRACE
-static void
-dump_glibc_backtrace (int depth, char *str[])
-{
-  int i;
-
-  for (i = 0; i < depth; i++)
-    {
-      estr_write ("  + ");
-      estr_write (str[i]);
-      estr_write ("\n");
-    }
 
-  free (str);
-}
-#endif
 
 /* show_backtrace displays the backtrace, currently obtained by means of
    the glibc backtrace* functions.  */
@@ -116,63 +79,49 @@  show_backtrace (void)
 #define BUFSIZE 1024
 
   void *trace[DEPTH];
-  char **str;
   int depth;
 
   depth = backtrace (trace, DEPTH);
   if (depth <= 0)
     return;
 
-  str = backtrace_symbols (trace, depth);
-
 #if CAN_PIPE
 
-#ifndef STDIN_FILENO
-#define STDIN_FILENO 0
-#endif
-
-#ifndef STDOUT_FILENO
-#define STDOUT_FILENO 1
-#endif
-
-#ifndef STDERR_FILENO
-#define STDERR_FILENO 2
-#endif
-
   /* We attempt to extract file and line information from addr2line.  */
   do
   {
     /* Local variables.  */
-    int f[2], pid, line, i;
+    int f[2], pid, line, bt[2], inp[2];
     FILE *output;
-    char addr_buf[DEPTH][GFC_XTOA_BUF_SIZE], func[BUFSIZE], file[BUFSIZE];
+    char addr_buf[GFC_XTOA_BUF_SIZE], func[BUFSIZE], file[BUFSIZE];
     char *p, *end;
-    const char *addr[DEPTH];
-
-    /* Write the list of addresses in hexadecimal format.  */
-    for (i = 0; i < depth; i++)
-      addr[i] = gfc_xtoa ((GFC_UINTEGER_LARGEST) (intptr_t) trace[i], addr_buf[i],
-		      sizeof (addr_buf[i]));
 
     /* Don't output an error message if something goes wrong, we'll simply
        fall back to the pstack and glibc backtraces.  */
     if (pipe (f) != 0)
       break;
+    if (pipe (inp) != 0)
+      break;
     if ((pid = fork ()) == -1)
       break;
 
     if (pid == 0)
       {
 	/* Child process.  */
-#define NUM_FIXEDARGS 5
-	char *arg[DEPTH+NUM_FIXEDARGS+1];
+#define NUM_FIXEDARGS 7
+	char *arg[NUM_FIXEDARGS];
 
 	close (f[0]);
-	close (STDIN_FILENO);
+
+	close (inp[1]);
+	if (dup2 (inp[0], STDIN_FILENO) == -1)
+	  _exit (1);
+	close (inp[0]);
+
 	close (STDERR_FILENO);
 
 	if (dup2 (f[1], STDOUT_FILENO) == -1)
-	  _exit (0);
+	  _exit (1);
 	close (f[1]);
 
 	arg[0] = (char *) "addr2line";
@@ -180,112 +129,133 @@  show_backtrace (void)
 	arg[2] = full_exe_path ();
 	arg[3] = (char *) "-f";
 	arg[4] = (char *) "-s";
-	for (i = 0; i < depth; i++)
-	  arg[NUM_FIXEDARGS+i] = (char *) addr[i];
-	arg[NUM_FIXEDARGS+depth] = NULL;
+	arg[5] = (char *) "-C";
+	arg[6] = NULL;
 	execvp (arg[0], arg);
-	_exit (0);
+	_exit (1);
 #undef NUM_FIXEDARGS
       }
 
     /* Father process.  */
     close (f[1]);
-    wait (NULL);
+    close (inp[0]);
+    if (pipe (bt) != 0)
+      break;
+    backtrace_symbols_fd (trace, depth, bt[1]);
+    close (bt[1]);
     output = fdopen (f[0], "r");
-    i = -1;
 
-    if (fgets (func, sizeof(func), output))
+    estr_write ("\nBacktrace for this error:\n");
+    for (int j = 0; j < depth; j++)
       {
-	estr_write ("\nBacktrace for this error:\n");
-
-	do
+	const char *addr = gfc_xtoa 
+	  ((GFC_UINTEGER_LARGEST) (intptr_t) trace[j], 
+	   addr_buf, sizeof (addr_buf));
+
+	write (inp[1], addr, strlen (addr));
+	write (inp[1], "\n", 1);
+	
+	if (! fgets (func, sizeof(func), output))
+	  goto fallback;
+	if (! fgets (file, sizeof(file), output))
+	  goto fallback;
+	    
+	for (p = func; *p != '\n' && *p != '\r'; p++)
+	  ;
+	*p = '\0';
+	
+	/* If we only have the address, use the glibc backtrace.  */
+	if (func[0] == '?' && func[1] == '?' && file[0] == '?'
+	    && file[1] == '?')
 	  {
-	    if (! fgets (file, sizeof(file), output))
-	      goto fallback;
-
-	    i++;
-
-	    for (p = func; *p != '\n' && *p != '\r'; p++)
-	      ;
-
-	    *p = '\0';
-
-	    /* Try to recognize the internal libgfortran functions.  */
-	    if (strncasecmp (func, "*_gfortran", 10) == 0
-		|| strncasecmp (func, "_gfortran", 9) == 0
-		|| strcmp (func, "main") == 0 || strcmp (func, "_start") == 0
-		|| strcmp (func, "_gfortrani_backtrace_handler") == 0)
-	      continue;
-
-	    if (local_strcasestr (str[i], "libgfortran.so") != NULL
-		|| local_strcasestr (str[i], "libgfortran.dylib") != NULL
-		|| local_strcasestr (str[i], "libgfortran.a") != NULL)
-	      continue;
-
-	    /* If we only have the address, use the glibc backtrace.  */
-	    if (func[0] == '?' && func[1] == '?' && file[0] == '?'
-		&& file[1] == '?')
+	    bt_header (j);
+	    while (1)
 	      {
-		estr_write ("  + ");
-		estr_write (str[i]);
-		estr_write ("\n");
-	        continue;
+		char bc;
+		ssize_t nread = read (bt[0], &bc, 1);
+		if (nread != 1 || bc == '\n')
+		  break;
+		write (STDERR_FILENO, &bc, 1);
 	      }
-
-	    /* Extract the line number.  */
-	    for (end = NULL, p = file; *p; p++)
-	      if (*p == ':')
-		end = p;
-	    if (end != NULL)
-	      {
-		*end = '\0';
-		line = atoi (++end);
-	      }
-	    else
-	      line = -1;
-
-	    if (strcmp (func, "MAIN__") == 0)
-	      estr_write ("  + in the main program\n");
-	    else
-	      {
-		estr_write ("  + function ");
-		estr_write (func);
-		estr_write (" (0x");
-		estr_write (addr[i]);
-		estr_write (")\n");
-	      }
-
-	    if (line <= 0 && strcmp (file, "??") == 0)
-	      continue;
-
-	    if (line <= 0)
+	    estr_write ("\n");
+	    continue;
+	  }
+	else
+	  {
+	    /* Forward to the next entry in the backtrace. */
+	    while (1)
 	      {
-		estr_write ("    from file ");
-		estr_write (file);
-		estr_write ("\n");
+		char bc;
+		ssize_t nread = read (bt[0], &bc, 1);
+		if (nread != 1 || bc == '\n')
+		  break;
 	      }
-	    else
-	      st_printf ("    at line %d of file %s\n", line, file);
 	  }
-	while (fgets (func, sizeof(func), output));
 
-	free (str);
-	return;
+	/* _start is a setup routine that calls main(), and main() is
+	   the frontend routine that calls some setup stuff and then
+	   calls MAIN__, so at this point we should stop.  */
+	if (strcmp (func, "_start") == 0 || strcmp (func, "main") == 0)
+	  break;
+	
+	/* Extract the line number.  */
+	for (end = NULL, p = file; *p; p++)
+	  if (*p == ':')
+	    end = p;
+	if (end != NULL)
+	  {
+	    *end = '\0';
+	    line = atoi (++end);
+	  }
+	else
+	  line = -1;
+	
+	bt_header (j);
+	estr_write ("in ");
+	estr_write (func);
+	
+	if (line <= 0 && strcmp (file, "??") == 0)
+	  {
+	    estr_write (" (0x");
+	    estr_write (addr);
+	    estr_write (")\n");
+	    continue;
+	  }
+	
+	if (line <= 0)
+	  {
+	    estr_write (" at ");
+	    estr_write (file);
+	  }
+	else
+	  st_printf (" at %s:%d", file, line);
+
+	estr_write (" (0x");
+	estr_write (addr);
+	estr_write (")\n");
+      } /* Loop over each hex address.  */
+    close (inp[1]);
+    close (bt[0]);
+    wait (NULL);
+    return;
 
 fallback:
-	estr_write ("** Something went wrong while running addr2line. **\n"
-		    "** Falling back  to a simpler  backtrace scheme. **\n");
-      }
-    }
+    estr_write ("** Something went wrong while running addr2line. **\n"
+		"** Falling back  to a simpler  backtrace scheme. **\n");
+  }
   while (0);
 
 #undef DEPTH
 #undef BUFSIZE
 
-#endif
-#endif
+#endif /* CAN_PIPE */
 
-#if CAN_FORK && defined(HAVE_GETPPID)
+  /* Fallback to the glibc backtrace.  */
+  estr_write ("\nBacktrace for this error:\n");
+  backtrace_symbols_fd (trace, depth, STDERR_FILENO);
+  return;
+
+#elif defined(CAN_FORK) && defined(HAVE_GETPPID)
   /* Try to call pstack.  */
   do
   {
@@ -312,15 +282,9 @@  fallback:
 	execvp (arg[0], arg);
 #undef NUM_ARGS
 
-	/* pstack didn't work, so we fall back to dumping the glibc
-	   backtrace if we can.  */
-#if GLIBC_BACKTRACE
-	dump_glibc_backtrace (depth, str);
-#else
+	/* pstack didn't work.  */
 	estr_write ("  unable to produce a backtrace, sorry!\n");
-#endif
-
-	_exit (0);
+	_exit (1);
       }
 
     /* Father process.  */
@@ -328,13 +292,7 @@  fallback:
     return;
   }
   while(0);
-#endif
-
-#if GLIBC_BACKTRACE
-  /* Fallback to the glibc backtrace.  */
-  estr_write ("\nBacktrace for this error:\n");
-  dump_glibc_backtrace (depth, str);
-  return;
-#endif
+#else
   estr_write ("\nBacktrace not yet available on this platform, sorry!\n");
+#endif
 }