From patchwork Fri Nov 7 10:15:56 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Janne Blomqvist X-Patchwork-Id: 408028 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 7F9F21400E9 for ; Fri, 7 Nov 2014 21:16:10 +1100 (AEDT) DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :mime-version:in-reply-to:references:date:message-id:subject :from:to:content-type; q=dns; s=default; b=tqKPithVb12Wq5vwD53Mb FAntSOYsvwgEfTMrhr/7l0aJ2lRey/tVwDWbFYZZ4fEXmZyyfIji2UFpHwYvkhvR 5YGoRx9fEeUl1T/KlgddDzDyYVjAazvfTBi6j+6RJSWqq4m8rPipPtCchDbA71sb XtQSlOCyJGxtzXSQChciNE= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :mime-version:in-reply-to:references:date:message-id:subject :from:to:content-type; s=default; bh=XhbpF9C8m2Ge6gsqJ4CJ2nN2EKs =; b=Zr08SPrU180Wq1J1MQGCw3BICEJ41dtUni6VWkO/5PAgCUM08CmppXLn/NX +nIjiNj2IfrqBSLvFn/g2kGqVE8kWioP3BEPK5btUhXOYWgUxRsMAd56Sygpk3YY IlNrJBX71efzU2ekXDa1Q7WAkrQC0K+Dg5Wvg6276XQPHsnQ= Received: (qmail 31614 invoked by alias); 7 Nov 2014 10:16:03 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 31550 invoked by uid 89); 7 Nov 2014 10:16:01 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.5 required=5.0 tests=AWL, BAYES_00, FREEMAIL_FROM, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=ham version=3.3.2 X-Spam-User: qpsmtpd, 2 recipients X-HELO: mail-qa0-f52.google.com Received: from mail-qa0-f52.google.com (HELO mail-qa0-f52.google.com) (209.85.216.52) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Fri, 07 Nov 2014 10:15:59 +0000 Received: by mail-qa0-f52.google.com with SMTP id u7so2039655qaz.39 for ; Fri, 07 Nov 2014 02:15:56 -0800 (PST) MIME-Version: 1.0 X-Received: by 10.229.37.70 with SMTP id w6mr15933678qcd.11.1415355356638; Fri, 07 Nov 2014 02:15:56 -0800 (PST) Received: by 10.140.94.213 with HTTP; Fri, 7 Nov 2014 02:15:56 -0800 (PST) In-Reply-To: References: Date: Fri, 7 Nov 2014 12:15:56 +0200 Message-ID: Subject: Re: [PATCH, libfortran] PR 47007, 61847 Locale failures in libgfortran From: Janne Blomqvist To: Fortran List , GCC Patches On Thu, Nov 6, 2014 at 12:38 PM, Janne Blomqvist wrote: > On Wed, Nov 5, 2014 at 12:48 PM, Janne Blomqvist > 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 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 PR libfortran/47007 PR libfortran/61847 * gfortran.texi: Add note about locale issues to thread-safety section. 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 }