diff mbox

[libfortran] PR 47007, 61847 Locale failures in libgfortran

Message ID CAO9iq9FpnK_1c6usiN_VzX6Bnzs8Mren81yn7boH0xa+RiQ8Xg@mail.gmail.com
State New
Headers show

Commit Message

Janne Blomqvist Nov. 7, 2014, 10:15 a.m. UTC 
On Thu, Nov 6, 2014 at 12:38 PM, Janne Blomqvist
<blomqvist.janne@gmail.com> wrote:
> On Wed, Nov 5, 2014 at 12:48 PM, Janne Blomqvist
> <blomqvist.janne@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?

2014-11-07  Janne Blomqvist  <jb@gcc.gnu.org>

    PR libfortran/47007
    PR libfortran/61847
    * config.h.in: Regenerated.
    * configure: Regenerated.
    * configure.ac (AC_CHECK_HEADERS_ONCE): Check for xlocale.h.
    (AC_CHECK_FUNCS_ONCE): Check for newlocale, freelocale, uselocale,
    strerror_l.
    * io/io.h (locale.h): Include.
    (xlocale.h): Include if present.
    (c_locale): New variable.
    (st_parameter_dt): Add old_locale member.
    * io/transfer.c (data_transfer_init): Set locale to "C" if doing
    formatted transfer.
    (finalize_transfer): Reset locale to previous.
    * io/unit.c (c_locale): New variable.
    (init_units): Init c_locale.
    (close_units): Free c_locale.
    * runtime/error.c (locale.h): Include.
    (xlocale.h): Include if present.
    (gf_strerror): Use strerror_l if available. Reset locale to
    LC_GLOBAL_LOCALE for strerror_r branch.


2014-11-07  Janne Blomqvist  <jb@gcc.gnu.org>

    PR libfortran/47007
    PR libfortran/61847
    * gfortran.texi: Add note about locale issues to thread-safety
    section.

Comments

Jerry DeLisle Nov. 8, 2014, 12:21 a.m. UTC | #1 
On 11/07/2014 02:15 AM, Janne Blomqvist wrote:
> On Thu, Nov 6, 2014 at 12:38 PM, Janne Blomqvist
> <blomqvist.janne@gmail.com> wrote:
>> On Wed, Nov 5, 2014 at 12:48 PM, Janne Blomqvist
>> <blomqvist.janne@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.  ;)
diff mbox

Patch

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..b6a60fa 100644
--- a/libgfortran/io/io.h
+++ b/libgfortran/io/io.h
@@ -468,6 +468,8 @@  typedef struct st_parameter_dt
 	  namelist_info *ionml;
 #ifdef HAVE_NEWLOCALE
 	  locale_t old_locale;
+#else
+	  char *old_locale;
 #endif
 	  /* Current position within the look-ahead line buffer.  */
 	  int line_buffer_pos;
diff --git a/libgfortran/io/transfer.c b/libgfortran/io/transfer.c
index 203f516..1c37fc8 100644
--- a/libgfortran/io/transfer.c
+++ b/libgfortran/io/transfer.c
@@ -2874,6 +2874,9 @@  data_transfer_init (st_parameter_dt *dtp, int read_flag)
     {
 #ifdef HAVE_USELOCALE
       dtp->u.p.old_locale = uselocale (c_locale);
+#else
+      dtp->u.p.old_locale = setlocale (LC_NUMERIC, NULL);
+      setlocale (LC_NUMERIC, "C");
 #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 +3617,12 @@  finalize_transfer (st_parameter_dt *dtp)
       uselocale (dtp->u.p.old_locale);
       dtp->u.p.old_locale = (locale_t) 0;
     }
+#else
+  if (dtp->u.p.old_locale)
+    {
+      setlocale (LC_NUMERIC, dtp->u.p.old_locale);
+      dtp->u.p.old_locale = NULL;
+    }
 #endif
 }