| Message ID | AANLkTik9-NJ93hozTxpgasrO=v=qJBJcTtW3zoNe23Jy@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Jan 20, 2011 at 20:11, Janne Blomqvist <blomqvist.janne@gmail.com> wrote: > Hello, > > POSIX does not require strerror() to be thread-safe (although some > implementations are), using it is thus a potential error in > libgfortran. The attached patch fixes this in the case that > strerror_r() is available; thanks to Jakub for the hint about > __builtin_classify_type. > > Regtested on x86_64-unknown-linux-gnu, Ok for trunk? Some googling suggest TLS is not supported on all targets, so I'm withdrawing this patch and coming up with another solution.
On Fri, Jan 21, 2011 at 03:00:11PM +0200, Janne Blomqvist wrote: > On Thu, Jan 20, 2011 at 20:11, Janne Blomqvist > <blomqvist.janne@gmail.com> wrote: > > Hello, > > > > POSIX does not require strerror() to be thread-safe (although some > > implementations are), using it is thus a potential error in > > libgfortran. The attached patch fixes this in the case that > > strerror_r() is available; thanks to Jakub for the hint about > > __builtin_classify_type. > > > > Regtested on x86_64-unknown-linux-gnu, Ok for trunk? > > Some googling suggest TLS is not supported on all targets, so I'm > withdrawing this patch and coming up with another solution. > Janne, Which targets don't have any form of tls? I thought all of the targets currently had either tls or emutls support. Jack > -- > Janne Blomqvist
On Fri, Jan 21, 2011 at 08:55:29AM -0500, Jack Howarth wrote: > On Fri, Jan 21, 2011 at 03:00:11PM +0200, Janne Blomqvist wrote: > > On Thu, Jan 20, 2011 at 20:11, Janne Blomqvist > > <blomqvist.janne@gmail.com> wrote: > > > POSIX does not require strerror() to be thread-safe (although some > > > implementations are), using it is thus a potential error in > > > libgfortran. The attached patch fixes this in the case that > > > strerror_r() is available; thanks to Jakub for the hint about > > > __builtin_classify_type. > > > > > > Regtested on x86_64-unknown-linux-gnu, Ok for trunk? > > > > Some googling suggest TLS is not supported on all targets, so I'm > > withdrawing this patch and coming up with another solution. > > > > Which targets don't have any form of tls? I thought all of the > targets currently had either tls or emutls support. Even if all of them do now, unless public API of libgfortran requires it, using TLS for this is a big waste of resources when it is much better to change the interla gfc_strerror API to something similar to either POSIX or GNU strerror_r and adjust the callers to supply a buffer on their stacks. Jakub
On Fri, Jan 21, 2011 at 15:58, Jakub Jelinek <jakub@redhat.com> wrote: > On Fri, Jan 21, 2011 at 08:55:29AM -0500, Jack Howarth wrote: >> On Fri, Jan 21, 2011 at 03:00:11PM +0200, Janne Blomqvist wrote: >> > On Thu, Jan 20, 2011 at 20:11, Janne Blomqvist >> > <blomqvist.janne@gmail.com> wrote: >> > > POSIX does not require strerror() to be thread-safe (although some >> > > implementations are), using it is thus a potential error in >> > > libgfortran. The attached patch fixes this in the case that >> > > strerror_r() is available; thanks to Jakub for the hint about >> > > __builtin_classify_type. >> > > >> > > Regtested on x86_64-unknown-linux-gnu, Ok for trunk? >> > >> > Some googling suggest TLS is not supported on all targets, so I'm >> > withdrawing this patch and coming up with another solution. >> > >> >> Which targets don't have any form of tls? I thought all of the >> targets currently had either tls or emutls support. My quick googling came up with a number of posts that suggested TLS doesn't work with Apple gcc on OSX. Perhaps FSF gcc works, I don't know, nor do I know if other targets have similar problems. In any case, considering that this is just a libgfortran internal API with relatively few callers, IMHO it's safer to change the API rather than risk causing problems on targets for which I have neither the means nor any particular interest to fix. > Even if all of them do now, unless public API of libgfortran requires > it, using TLS for this is a big waste of resources when it is much > better to change the interla gfc_strerror API to something similar to either > POSIX or GNU strerror_r and adjust the callers to supply a buffer on their > stacks. Yes, this was exactly what I had in mind; the GNU strerror_r API should work nicely both for the strerror_r() backed versions and the strerror() fallback.
On Fri, Jan 21, 2011 at 04:22:59PM +0200, Janne Blomqvist wrote: > On Fri, Jan 21, 2011 at 15:58, Jakub Jelinek <jakub@redhat.com> wrote: > > On Fri, Jan 21, 2011 at 08:55:29AM -0500, Jack Howarth wrote: > >> On Fri, Jan 21, 2011 at 03:00:11PM +0200, Janne Blomqvist wrote: > >> > On Thu, Jan 20, 2011 at 20:11, Janne Blomqvist > >> > <blomqvist.janne@gmail.com> wrote: > >> > > POSIX does not require strerror() to be thread-safe (although some > >> > > implementations are), using it is thus a potential error in > >> > > libgfortran. The attached patch fixes this in the case that > >> > > strerror_r() is available; thanks to Jakub for the hint about > >> > > __builtin_classify_type. > >> > > > >> > > Regtested on x86_64-unknown-linux-gnu, Ok for trunk? > >> > > >> > Some googling suggest TLS is not supported on all targets, so I'm > >> > withdrawing this patch and coming up with another solution. > >> > > >> > >> Which targets don't have any form of tls? I thought all of the > >> targets currently had either tls or emutls support. > > My quick googling came up with a number of posts that suggested TLS > doesn't work with Apple gcc on OSX. Perhaps FSF gcc works, I don't > know, nor do I know if other targets have similar problems. In any > case, considering that this is just a libgfortran internal API with > relatively few callers, IMHO it's safer to change the API rather than > risk causing problems on targets for which I have neither the means > nor any particular interest to fix. Janne, There is complete support for emutls on darwin for emutls. The tls testsuite was revised to test exhaustively on emutls as well. Also, emutls was reworked in gcc 4.6 to be fully compatible with lto. Jack > > > Even if all of them do now, unless public API of libgfortran requires > > it, using TLS for this is a big waste of resources when it is much > > better to change the interla gfc_strerror API to something similar to either > > POSIX or GNU strerror_r and adjust the callers to supply a buffer on their > > stacks. > > Yes, this was exactly what I had in mind; the GNU strerror_r API > should work nicely both for the strerror_r() backed versions and the > strerror() fallback. > > > -- > Janne Blomqvist
diff --git a/libgfortran/configure.ac b/libgfortran/configure.ac index 7b28f12..4f137e4 100644 --- a/libgfortran/configure.ac +++ b/libgfortran/configure.ac @@ -249,7 +249,7 @@ AC_CHECK_FUNCS(chdir strerror getlogin gethostname kill link symlink perror) AC_CHECK_FUNCS(sleep time ttyname signal alarm ctime 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) +AC_CHECK_FUNCS(localtime_r gmtime_r strerror_r) # Check for glibc backtrace functions AC_CHECK_FUNCS(backtrace backtrace_symbols) diff --git a/libgfortran/intrinsics/gerror.c b/libgfortran/intrinsics/gerror.c index ccb5c3e..44873f9 100644 --- a/libgfortran/intrinsics/gerror.c +++ b/libgfortran/intrinsics/gerror.c @@ -45,14 +45,13 @@ PREFIX(gerror) (char * msg, gfc_charlen_type msg_len) memset (msg, ' ', msg_len); /* Blank the string. */ - p = strerror (errno); + p = gf_strerror (errno); if (p == NULL) return; p_len = strlen (p); if (msg_len < p_len) - memcpy (msg, p, msg_len); - else - memcpy (msg, p, p_len); + p_len = msg_len; + memcpy (msg, p, p_len); } #endif diff --git a/libgfortran/io/unix.c b/libgfortran/io/unix.c index fa64e20..b7eba0b 100644 --- a/libgfortran/io/unix.c +++ b/libgfortran/io/unix.c @@ -262,7 +262,7 @@ flush_if_preconnected (stream * s) const char * get_oserror (void) { - return strerror (errno); + return gf_strerror (errno); } diff --git a/libgfortran/libgfortran.h b/libgfortran/libgfortran.h index ac86492..d092a54 100644 --- a/libgfortran/libgfortran.h +++ b/libgfortran/libgfortran.h @@ -1,5 +1,6 @@ /* Common declarations for all of libgfortran. - Copyright (C) 2002, 2003, 2004, 2005, 2006, 2007, 2008, 2009, 2010 + Copyright (C) 2002, 2003, 2004, 2005, 2006, 2007, 2008, 2009, 2010, + 2011 Free Software Foundation, Inc. Contributed by Paul Brook <paul@nowt.org>, and Andy Vaught <andy@xena.eas.asu.edu> @@ -756,6 +757,9 @@ internal_proto(notify_std); extern notification notification_std(int); internal_proto(notification_std); +extern char *gf_strerror (int); +internal_proto(gf_strerror); + /* fpu.c */ extern void set_fpu (void); diff --git a/libgfortran/runtime/error.c b/libgfortran/runtime/error.c index 1baf9d3..a04f2c5 100644 --- a/libgfortran/runtime/error.c +++ b/libgfortran/runtime/error.c @@ -1,4 +1,4 @@ -/* Copyright (C) 2002, 2003, 2005, 2006, 2007, 2009, 2010 +/* Copyright (C) 2002, 2003, 2005, 2006, 2007, 2009, 2010, 2011 Free Software Foundation, Inc. Contributed by Andy Vaught @@ -141,6 +141,36 @@ gfc_xtoa (GFC_UINTEGER_LARGEST n, char *buffer, size_t len) return p; } + +/* Hopefully thread-safe wrapper for a strerror() style function. */ + +char * +gf_strerror (int errnum) +{ +#ifdef HAVE_STRERROR_R +#define GF_STRERROR_R_MAXSZ 256 + static __thread char msg[GF_STRERROR_R_MAXSZ]; + /* TODO: How to prevent the compiler warning due to strerror_r of + the untaken branch having the wrong return type? */ + if (__builtin_classify_type (strerror_r (0, msg, 0)) == 5) + { + /* GNU strerror_r() */ + return strerror_r (errnum, msg, GF_STRERROR_R_MAXSZ); + } + else + { + /* POSIX strerror_r () */ + strerror_r (errnum, msg, GF_STRERROR_R_MAXSZ); + return msg; + } +#else + /* strerror () is not necessarily thread-safe, but should at least + be available everywhere. */ + return strerror (errnum); +#endif +} + + /* show_locus()-- Print a line number and filename describing where * something went wrong */