Re: [PATCH, libfortran] PR 47007, 61847 Locale failures in libgfortran

[Janne Blomqvist <blomqvist dot janne at gmail dot com>]

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.



-- 
Janne Blomqvist

Attachment: locale3_top2.diff
Description: Text document

Attachment: locale3.diff
Description: Text document