On Sat, Nov 8, 2014 at 2:21 AM, Jerry DeLisle <jvdeli...@charter.net> wrote:
> On 11/07/2014 02:15 AM, Janne Blomqvist wrote:
>>
>> On Thu, Nov 6, 2014 at 12:38 PM, Janne Blomqvist
>> <blomqvist.ja...@gmail.com> wrote:
>>>
>>> On Wed, Nov 5, 2014 at 12:48 PM, Janne Blomqvist
>>> <blomqvist.ja...@gmail.com> wrote:
>>>>
>>>> Hi,
>>>>
>>>> the attached patch fixes a few locale related failures in libgfortran,
>>>> in the case where the POSIX 2008 extended locale functionality and
>>>> extensions strto{f,d,ld}_l are present.
>>>>
>>>> These failures typically occur when libgfortran is used from a program
>>>> which has set the locale with setlocale(), and the locale uses a
>>>> different decimal separator than the C locale. The patch fixes this by
>>>> creating a C locale which is then used by strto{f,d,ld}_l, and also is
>>>> installed as the per-thread locale when starting a formatted IO, then
>>>> reset to the previous value when the IO is done. I have chosen to not
>>>> fallback to calling setlocale() in case the POSIX 2008 locale stuff
>>>> isn't available, as that could create nasty hard to debug race
>>>> conditions in a multi-threaded program.
>>>>
>>>> (I think Jerry's proposed patch which checks the locale for the
>>>> decimal separator is still useful as a fallback in case the POSIX 2008
>>>> locale stuff isn't available)
>>>
>>>
>>> Hi,
>>>
>>> updated patch attached. Since the patch sets the per-thread locale
>>> with uselocale, using the non-standard strto{f,d,ld}_l functions isn't
>>> necessary. When getting rid of this part of the original patch, I
>>> noticed a few failures due to the uselocale() calls being in the wrong
>>> places. These are fixed in the updated patch. Also Jakub's suggestion
>>> has been incorporated. Further, investigation revealed that some
>>> targets (Darwin and Freebsd) have the extended locale functionality in
>>> xlocale.h rather than locale.h as POSIX 2008 specifies. So check for
>>> that header. Finally, as we set the per-thread locale to "C", we'd
>>> lose localized error messages. So the updated patch fixes this by
>>> updating the gf_strerror() function as well.
>>
>>
>> Hi again!
>>
>> Well, for all my ranting against using setlocale() in a library
>> potentially used by multi-threaded programs, here's a patch that does
>> exactly that, as a fallback in case the POSIX 2008 per-thread locale
>> stuff isn't available. So this can be seen as an alternative to the
>> approach Jerry suggested in the patch attached in PR 61847.
>>
>> - Jerry's patch (use localeconv() to figure out the decimal separator)
>> - Race condition if locale is set between OPEN (where the separator
>> is checked) and READ/WRITE.
>> - Potential breakage in weird locales where the decimal separator
>> isn't "." or ","? See
>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61847#c21
>> - Other potential issues with weird locales? E.g. are there locales
>> which use grouping characters, e.g. 1e6 is 1,000,000.0?
>>
>> - My patch (use setlocale(LC_NUMERIC, "C") when starting formatted
>> I/O, change back when I/O statement is done.)
>> - Race condition if locale is set concurrently in another thread
>> while a formatted I/O is in progress.
>> - Potential problem if another thread does something dependent on
>> LC_NUMERIC while a formatted I/O is in progress in one thread.
>> - Should be robust against weird locales.
>>
>> In both cases IMHO these approaches should be used only if the POSIX
>> 2008 per-thread locale stuff isn't available. I have no strong
>> opinions which is preferable, comments?
>>
>> Attached is locale3_top2.diff, which is on top of my previous patch,
>> and locale3.diff which includes the previous patch and applies against
>> trunk.
>>
>> Ok for trunk?
>
>
> OK, thanks for doing all the work. ;)
I realized my setlocale fallback wouldn't work correctly if we have
multiple threads doing formatted I/O concurrently. Committed the patch
with the fallback part per the attached part.
--
Janne Blomqvist
diff --git a/gcc/fortran/gfortran.texi b/gcc/fortran/gfortran.texi
index 41d6559..0d19e7a 100644
--- a/gcc/fortran/gfortran.texi
+++ b/gcc/fortran/gfortran.texi
@@ -1223,10 +1223,26 @@ implemented with the @code{system} function, which need
not be
thread-safe. It is the responsibility of the user to ensure that
@code{system} is not called concurrently.
-Finally, for platforms not supporting thread-safe POSIX functions,
-further functionality might not be thread-safe. For details, please
-consult the documentation for your operating system.
-
+For platforms not supporting thread-safe POSIX functions, further
+functionality might not be thread-safe. For details, please consult
+the documentation for your operating system.
+
+The GNU Fortran runtime library uses various C library functions that
+depend on the locale, such as @code{strtod} and @code{snprintf}. In
+order to work correctly in locale-aware programs that set the locale
+using @code{setlocale}, the locale is reset to the default ``C''
+locale while executing a formatted @code{READ} or @code{WRITE}
+statement. On targets supporting the POSIX 2008 per-thread locale
+functions (e.g. @code{newlocale}, @code{uselocale},
+@code{freelocale}), these are used and thus the global locale set
+using @code{setlocale} or the per-thread locales in other threads are
+not affected. However, on targets lacking this functionality, the
+global LC_NUMERIC locale is set to ``C'' during the formatted I/O.
+Thus, on such targets it's not safe to call @code{setlocale}
+concurrently from another thread while a Fortran formatted I/O
+operation is in progress. Also, other threads doing something
+dependent on the LC_NUMERIC locale might not work correctly if a
+formatted I/O operation is in progress in another thread.
@node Data consistency and durability
@section Data consistency and durability
diff --git a/libgfortran/io/io.h b/libgfortran/io/io.h
index 0ff5dcc..a75177f 100644
--- a/libgfortran/io/io.h
+++ b/libgfortran/io/io.h
@@ -54,6 +54,14 @@ struct gfc_unit;
#ifdef HAVE_NEWLOCALE
/* We have POSIX 2008 extended locale stuff. */
extern locale_t c_locale;
+internal_proto(c_locale);
+#else
+extern char* old_locale;
+internal_proto(old_locale);
+extern int old_locale_ctr;
+internal_proto(old_locale_ctr);
+extern __gthread_mutex_t old_locale_lock;
+internal_proto(old_locale_lock);
#endif
diff --git a/libgfortran/io/transfer.c b/libgfortran/io/transfer.c
index 203f516..87b8c05 100644
--- a/libgfortran/io/transfer.c
+++ b/libgfortran/io/transfer.c
@@ -2874,6 +2874,14 @@ data_transfer_init (st_parameter_dt *dtp, int read_flag)
{
#ifdef HAVE_USELOCALE
dtp->u.p.old_locale = uselocale (c_locale);
+#else
+ __gthread_mutex_lock (&old_locale_lock);
+ if (!old_locale_ctr++)
+ {
+ old_locale = setlocale (LC_NUMERIC, NULL);
+ setlocale (LC_NUMERIC, "C");
+ }
+ __gthread_mutex_unlock (&old_locale_lock);
#endif
/* Start the data transfer if we are doing a formatted transfer. */
if ((cf & (IOPARM_DT_LIST_FORMAT | IOPARM_DT_HAS_NAMELIST_NAME)) == 0
@@ -3614,6 +3622,14 @@ finalize_transfer (st_parameter_dt *dtp)
uselocale (dtp->u.p.old_locale);
dtp->u.p.old_locale = (locale_t) 0;
}
+#else
+ __gthread_mutex_lock (&old_locale_lock);
+ if (!--old_locale_ctr)
+ {
+ setlocale (LC_NUMERIC, old_locale);
+ old_locale = NULL;
+ }
+ __gthread_mutex_unlock (&old_locale_lock);
#endif
}
diff --git a/libgfortran/io/unit.c b/libgfortran/io/unit.c
index e4bc60b..277c7a1 100644
--- a/libgfortran/io/unit.c
+++ b/libgfortran/io/unit.c
@@ -93,8 +93,23 @@ static char stderr_name[] = "stderr";
#ifdef HAVE_NEWLOCALE
locale_t c_locale;
+#else
+/* If we don't have POSIX 2008 per-thread locales, we need to use the
+ traditional setlocale(). To prevent multiple concurrent threads
+ doing formatted I/O from messing up the locale, we need to store a
+ global old_locale, and a counter keeping track of how many threads
+ are currently doing formatted I/O. The first thread saves the old
+ locale, and the last one restores it. */
+char *old_locale;
+int old_locale_ctr;
+#ifdef __GTHREAD_MUTEX_INIT
+__gthread_mutex_t old_locale_lock = __GTHREAD_MUTEX_INIT;
+#else
+__gthread_mutex_t old_locale_lock;
+#endif
#endif
+
/* This implementation is based on Stefan Nilsson's article in the
* July 1997 Doctor Dobb's Journal, "Treaps in Java". */
@@ -568,6 +583,10 @@ init_units (void)
#ifdef HAVE_NEWLOCALE
c_locale = newlocale (0, "C", 0);
+#else
+#ifndef __GTHREAD_MUTEX_INIT
+ __GTHREAD_MUTEX_INIT_FUNCTION (&old_locale_lock);
+#endif
#endif
#ifndef __GTHREAD_MUTEX_INIT
