public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [Patch, libfortran] Thread safety and simplification of error printing
@ 2011-05-07 17:07 Janne Blomqvist
  2011-05-08 13:44 ` Janne Blomqvist
  0 siblings, 1 reply; 9+ messages in thread
From: Janne Blomqvist @ 2011-05-07 17:07 UTC (permalink / raw)
  To: Fortran List, GCC Patches

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

Hi,

the error printing functionality (in io/unix.c) st_printf and
st_vprintf are not thread-safe as they use a static buffer. However,
since these routines are used when something has gone wrong, we
shouldn't use malloc() to allocate thread-specific buffers or anything
fancy like that. The way the buffer is used is that it is filled with
vs(n)printf() and then the buffer is written using write(); the
simplest way to fix this is to just print directly using vfprintf(),
which is what this patch does. Also, the patch uses
flockfile/funlockfile in a few places to increase the likelihood of
multiple related messages to end up together on the output.

As stderr is unbuffered and stdout is buffered, this patch also gets
rid of the functionality to choose where to send the error output, as
we don't want to deal with buffering in the error handling path. IMHO
this is no loss, as all targets/shells/batch schedulers/ etc. where
this might be relevant, offer functionality to redirect stderr to
wherever stdout goes. So there is no need for a gfortran-specific
mechanism for this.

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

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

        * gfortran.texi: Remove GFORTRAN_USE_STDERR documentation.

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

        * config.h.in: Regenerated.
        * configure: Likewise.
        * configure.ac: Check for flockfile, funlockfile.
        * io/unix.c (st_vprintf): Remove.
        (st_printf): Use vfprintf() instead of st_vprintf.
        * libgfortran.h (flockfile,funlockfile): Define to empty if
        functions not available.
        (struct options_t): Remove use_stderr field.
        (st_vprintf): Remove prototype.
        * runtime/environ.c (variable_table[]): Remove GFORTRAN_USE_STDERR
        entry.
        * runtime/error.c (runtime_error): Use vfprintf instead of
        st_vprintf.
        (runtime_error_at): Likewise.
        (runtime_warning_at): Likewise.


-- 
Janne Blomqvist

[-- Attachment #2: st_printf.diff --]
[-- Type: text/x-patch, Size: 8071 bytes --]

diff --git a/gcc/fortran/gfortran.texi b/gcc/fortran/gfortran.texi
index 1284c3d..c810fe2 100644
--- a/gcc/fortran/gfortran.texi
+++ b/gcc/fortran/gfortran.texi
@@ -579,7 +579,6 @@ Malformed environment variables are silently ignored.
 * GFORTRAN_STDIN_UNIT:: Unit number for standard input
 * GFORTRAN_STDOUT_UNIT:: Unit number for standard output
 * GFORTRAN_STDERR_UNIT:: Unit number for standard error
-* GFORTRAN_USE_STDERR:: Send library output to standard error
 * GFORTRAN_TMPDIR:: Directory for scratch files
 * GFORTRAN_UNBUFFERED_ALL:: Don't buffer I/O for all units.
 * GFORTRAN_UNBUFFERED_PRECONNECTED:: Don't buffer I/O for preconnected units.
@@ -613,14 +612,6 @@ This environment variable can be used to select the unit number
 preconnected to standard error.  This must be a positive integer.
 The default value is 0.
 
-@node GFORTRAN_USE_STDERR
-@section @env{GFORTRAN_USE_STDERR}---Send library output to standard error
-
-This environment variable controls where library output is sent.
-If the first letter is @samp{y}, @samp{Y} or @samp{1}, standard
-error is used.  If the first letter is @samp{n}, @samp{N} or
-@samp{0}, standard output is used.
-
 @node GFORTRAN_TMPDIR
 @section @env{GFORTRAN_TMPDIR}---Directory for scratch files
 
diff --git a/libgfortran/config.h.in b/libgfortran/config.h.in
index 30da5fa..a48381d 100644
--- a/libgfortran/config.h.in
+++ b/libgfortran/config.h.in
@@ -393,6 +393,9 @@
 /* Define to 1 if you have the <float.h> header file. */
 #undef HAVE_FLOAT_H
 
+/* Define to 1 if you have the `flockfile' function. */
+#undef HAVE_FLOCKFILE
+
 /* libm includes floor */
 #undef HAVE_FLOOR
 
@@ -441,6 +444,9 @@
 /* Define to 1 if you have the `ftruncate' function. */
 #undef HAVE_FTRUNCATE
 
+/* Define to 1 if you have the `funlockfile' function. */
+#undef HAVE_FUNLOCKFILE
+
 /* Define to 1 if you have the `getcwd' function. */
 #undef HAVE_GETCWD
 
diff --git a/libgfortran/configure b/libgfortran/configure
index 833a0e1..9fa2b7f 100755
--- a/libgfortran/configure
+++ b/libgfortran/configure
@@ -16458,7 +16458,7 @@ _ACEOF
 fi
 done
 
-for ac_func in clock_gettime strftime
+for ac_func in clock_gettime strftime flockfile funlockfile
 do :
   as_ac_var=`$as_echo "ac_cv_func_$ac_func" | $as_tr_sh`
 ac_fn_c_check_func "$LINENO" "$ac_func" "$as_ac_var"
diff --git a/libgfortran/configure.ac b/libgfortran/configure.ac
index cf38fb0..e39ae17 100644
--- a/libgfortran/configure.ac
+++ b/libgfortran/configure.ac
@@ -264,7 +264,7 @@ AC_CHECK_FUNCS(sleep time ttyname signal alarm clock access fork execl)
 AC_CHECK_FUNCS(wait setmode execvp pipe dup2 close fdopen strcasestr getrlimit)
 AC_CHECK_FUNCS(gettimeofday stat fstat lstat getpwuid vsnprintf dup getcwd)
 AC_CHECK_FUNCS(localtime_r gmtime_r strerror_r getpwuid_r ttyname_r)
-AC_CHECK_FUNCS(clock_gettime strftime)
+AC_CHECK_FUNCS(clock_gettime strftime flockfile funlockfile)
 
 # Check for glibc backtrace functions
 AC_CHECK_FUNCS(backtrace backtrace_symbols)
diff --git a/libgfortran/io/unix.c b/libgfortran/io/unix.c
index 4e4bc3b..3b80e36 100644
--- a/libgfortran/io/unix.c
+++ b/libgfortran/io/unix.c
@@ -1353,48 +1353,7 @@ error_stream (void)
 }
 
 
-/* st_vprintf()-- vprintf function for error output.  To avoid buffer
-   overruns, we limit the length of the buffer to ST_VPRINTF_SIZE.  2k
-   is big enough to completely fill a 80x25 terminal, so it shuld be
-   OK.  We use a direct write() because it is simpler and least likely
-   to be clobbered by memory corruption.  Writing an error message
-   longer than that is an error.  */
-
-#define ST_VPRINTF_SIZE 2048
-
-int
-st_vprintf (const char *format, va_list ap)
-{
-  static char buffer[ST_VPRINTF_SIZE];
-  int written;
-  int fd;
-
-  fd = options.use_stderr ? STDERR_FILENO : STDOUT_FILENO;
-#ifdef HAVE_VSNPRINTF
-  written = vsnprintf(buffer, ST_VPRINTF_SIZE, format, ap);
-#else
-  written = vsprintf(buffer, format, ap);
-
-  if (written >= ST_VPRINTF_SIZE-1)
-    {
-      /* The error message was longer than our buffer.  Ouch.  Because
-	 we may have messed up things badly, report the error and
-	 quit.  */
-#define ERROR_MESSAGE "Internal error: buffer overrun in st_vprintf()\n"
-      write (fd, buffer, ST_VPRINTF_SIZE-1);
-      write (fd, ERROR_MESSAGE, strlen(ERROR_MESSAGE));
-      sys_exit(2);
-#undef ERROR_MESSAGE
-
-    }
-#endif
-
-  written = write (fd, buffer, written);
-  return written;
-}
-
-/* st_printf()-- printf() function for error output.  This just calls
-   st_vprintf() to do the actual work.  */
+/* st_printf()-- printf() function for error output.  */
 
 int
 st_printf (const char *format, ...)
@@ -1402,7 +1361,7 @@ st_printf (const char *format, ...)
   int written;
   va_list ap;
   va_start (ap, format);
-  written = st_vprintf(format, ap);
+  written = vfprintf (stderr, format, ap);
   va_end (ap);
   return written;
 }
diff --git a/libgfortran/libgfortran.h b/libgfortran/libgfortran.h
index 6cccaca..ca1f462 100644
--- a/libgfortran/libgfortran.h
+++ b/libgfortran/libgfortran.h
@@ -125,6 +125,13 @@ extern int __mingw_snprintf (char *, size_t, const char *, ...)
 #define snprintf(str, size, ...) sprintf (str, __VA_ARGS__)
 #endif
 
+#ifndef HAVE_FLOCKFILE
+#define flockfile(fhandle)
+#endif
+
+#ifndef HAVE_FUNLOCKFILE
+#define funlockfile(fhandle)
+#endif
 
 /* For a library, a standard prefix is a requirement in order to partition
    the namespace.  IPREFIX is for symbols intended to be internal to the
@@ -508,7 +515,7 @@ typedef struct
   int separator_len;
   const char *separator;
 
-  int use_stderr, all_unbuffered, unbuffered_preconnected, default_recl;
+  int all_unbuffered, unbuffered_preconnected, default_recl;
   int fpe, dump_core, backtrace;
 }
 options_t;
@@ -796,9 +803,6 @@ extern int st_printf (const char *, ...)
   __attribute__ ((format (gfc_printf, 1, 2)));
 internal_proto(st_printf);
 
-extern int st_vprintf (const char *, va_list);
-internal_proto(st_vprintf);
-
 extern char * filename_from_unit (int);
 internal_proto(filename_from_unit);
 
diff --git a/libgfortran/runtime/environ.c b/libgfortran/runtime/environ.c
index a6ce645..d70103c 100644
--- a/libgfortran/runtime/environ.c
+++ b/libgfortran/runtime/environ.c
@@ -281,10 +281,6 @@ static variable variable_table[] = {
    "Unit number that will be preconnected to standard error\n"
    "(No preconnection if negative)", 0},
 
-  {"GFORTRAN_USE_STDERR", 1, &options.use_stderr, init_boolean,
-   show_boolean,
-   "Sends library output to standard error instead of standard output.", 0},
-
   {"GFORTRAN_TMPDIR", 0, NULL, init_string, show_string,
    "Directory for scratch files.  Overrides the TMP environment variable\n"
    "If TMP is not set " DEFAULT_TEMPDIR " is used.", 0},
diff --git a/libgfortran/runtime/error.c b/libgfortran/runtime/error.c
index 06c144a..694d668 100644
--- a/libgfortran/runtime/error.c
+++ b/libgfortran/runtime/error.c
@@ -249,11 +249,13 @@ runtime_error (const char *message, ...)
   va_list ap;
 
   recursion_check ();
+  flockfile (stderr);
   st_printf ("Fortran runtime error: ");
   va_start (ap, message);
-  st_vprintf (message, ap);
+  vfprintf (stderr, message, ap);
   va_end (ap);
   st_printf ("\n");
+  funlockfile (stderr);
   sys_exit (2);
 }
 iexport(runtime_error);
@@ -267,12 +269,14 @@ runtime_error_at (const char *where, const char *message, ...)
   va_list ap;
 
   recursion_check ();
+  flockfile (stderr);
   st_printf ("%s\n", where);
   st_printf ("Fortran runtime error: ");
   va_start (ap, message);
-  st_vprintf (message, ap);
+  vfprintf (stderr, message, ap);
   va_end (ap);
   st_printf ("\n");
+  funlockfile (stderr);
   sys_exit (2);
 }
 iexport(runtime_error_at);
@@ -283,12 +287,14 @@ runtime_warning_at (const char *where, const char *message, ...)
 {
   va_list ap;
 
+  flockfile (stderr);
   st_printf ("%s\n", where);
   st_printf ("Fortran runtime warning: ");
   va_start (ap, message);
-  st_vprintf (message, ap);
+  vfprintf (stderr, message, ap);
   va_end (ap);
   st_printf ("\n");
+  funlockfile (stderr);
 }
 iexport(runtime_warning_at);
 

^ permalink raw reply	[flat|nested] 9+ messages in thread
end of thread, other threads:[~2011-05-14  0:53 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-05-07 17:07 [Patch, libfortran] Thread safety and simplification of error printing Janne Blomqvist
2011-05-08 13:44 ` Janne Blomqvist
2011-05-08 17:35   ` N.M. Maclaren
2011-05-08 22:50     ` N.M. Maclaren
2011-05-09  2:31     ` Janne Blomqvist
2011-05-09  4:00       ` N.M. Maclaren
2011-05-08 23:01   ` Janne Blomqvist
2011-05-13 11:00     ` Janne Blomqvist
2011-05-14 10:26     ` Steve Kargl

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).