Message ID | ora5ykpaj2.fsf@lxoliva.fsfla.org |
---|---|
State | Committed |
Delegated to: | Patrick Palka |
Headers |
Return-Path: <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id E7E5F3857009 for <patchwork@sourceware.org>; Thu, 4 May 2023 12:05:53 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org E7E5F3857009 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1683201953; bh=u2qNLRUHslsiCgkwFXyS/fc+NebWxThk+v9JC6vkdEY=; h=To:Subject:Date:List-Id:List-Unsubscribe:List-Archive:List-Post: List-Help:List-Subscribe:From:Reply-To:From; b=omcaDSPCujoN/RspOHIjvXPcr21ODyb4FSrasUBUY1eXw/KIIUqPq/08cTUrbfU3m SO9SQG7PW2yM1bq957XuJhQYUAQSU/i3u4zfWWK09IqeZAO3YpQH7Gvy5IscswriH2 XutAo15YRXwrsh+EV9okHWL8cpMI0djkVGi7jZtc= X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from mail-oa1-x35.google.com (mail-oa1-x35.google.com [IPv6:2001:4860:4864:20::35]) by sourceware.org (Postfix) with ESMTPS id EA3453858D33 for <gcc-patches@gcc.gnu.org>; Thu, 4 May 2023 12:05:23 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org EA3453858D33 Received: by mail-oa1-x35.google.com with SMTP id 586e51a60fabf-1928860f63eso274119fac.0 for <gcc-patches@gcc.gnu.org>; Thu, 04 May 2023 05:05:23 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1683201919; x=1685793919; h=mime-version:user-agent:message-id:date:errors-to:organization :subject:to:from:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=u2qNLRUHslsiCgkwFXyS/fc+NebWxThk+v9JC6vkdEY=; b=QKuaubptU2n85Ah2N95glzeKtjZPOdNsCl4Tj9fOL+vsO1Vjp26bqbuhaUiAWDMSCM iXhkT2Rh4JZH9duHZGEl9LTnFv5UgnzO9iOgXs9E+ahqPOjji74aE0360bVVjLs93i3F Fqkk0uex18EQTttuONLQa60whI8x/J3Ec4uEoez6aSlgGbb8mFkCHdEgXIEzk1zeBgwP jGa8noRoxiBKEeZEprOyITqBdT8mcWDoQDAqtoichclsuSIq7j/JJDvufWu7iuqVNF9v 8yfdpIXNDVP18Ih+sAM3oeY1s6BjAulOsT87ffrF/zl4ijM60Ia+4EgisI3QisDari9M C/eQ== X-Gm-Message-State: AC+VfDyU4F5LkEu6Q1TZf9v+o9sHVIyEIzYXLgAt7LkjsnvSuy9oWHrH jzGN9G4gxeIpPSPWcDoyVk8c4dCcOQtJmrLwl54= X-Google-Smtp-Source: ACHHUZ4iU0ev3cTo8iRqV9rT/Qpmyycyd2nQIJes5KiVxV8yoB53HFAMV/9kKVNCxVvWSRZC1wWGJg== X-Received: by 2002:a05:6870:4c3:b0:18b:1926:43eb with SMTP id u3-20020a05687004c300b0018b192643ebmr693352oam.48.1683201918936; Thu, 04 May 2023 05:05:18 -0700 (PDT) Received: from free.home ([2804:7f1:2080:c4e6:caf1:179:3909:b1dd]) by smtp.gmail.com with ESMTPSA id d13-20020a056871040d00b0019296ee9606sm572686oag.4.2023.05.04.05.05.17 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 04 May 2023 05:05:18 -0700 (PDT) Received: from livre (livre.home [172.31.160.2]) by free.home (8.15.2/8.15.2) with ESMTPS id 344C55wF2687770 (version=TLSv1.3 cipher=TLS_AES_256_GCM_SHA384 bits=256 verify=NOT); Thu, 4 May 2023 09:05:06 -0300 To: gcc-patches@gcc.gnu.org, libstdc++@gcc.gnu.org Subject: [libstdc++] use strtold for from_chars even without locale Organization: Free thinker, does not speak for AdaCore Date: Thu, 04 May 2023 09:05:05 -0300 Message-ID: <ora5ykpaj2.fsf@lxoliva.fsfla.org> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-Scanned-By: MIMEDefang 2.84 X-Spam-Status: No, score=-12.3 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list <gcc-patches.gcc.gnu.org> List-Unsubscribe: <https://gcc.gnu.org/mailman/options/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=unsubscribe> List-Archive: <https://gcc.gnu.org/pipermail/gcc-patches/> List-Post: <mailto:gcc-patches@gcc.gnu.org> List-Help: <mailto:gcc-patches-request@gcc.gnu.org?subject=help> List-Subscribe: <https://gcc.gnu.org/mailman/listinfo/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=subscribe> From: Alexandre Oliva via Gcc-patches <gcc-patches@gcc.gnu.org> Reply-To: Alexandre Oliva <oliva@adacore.com> Errors-To: gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org Sender: "Gcc-patches" <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> |
Series |
[libstdc++] use strtold for from_chars even without locale
|
|
Commit Message
Alexandre Oliva
May 4, 2023, 12:05 p.m. UTC
When we're using fast_float for 32- and 64-bit floating point, use strtold for wider long double, even if locales are unavailable. On vxworks, test for strtof's and strtold's declarations, so that they can be used even when cross compiling. Include stdlib.h in the decl-checking macro, so that it can find them. Regstrapped on x86_64-linux-gnu. Also tested on aarch64-vx7r2 with gcc-12, where uselocale is not available, and using strtold rather than fast_math's double fallback avoids a couple of from_chars-related testsuite fails (from_chars/4.cc and to_chars/long_double.cc). Ok to install? for libstdc++-v3/ChangeLog * src/c++17/floating_from_chars.cc (USE_STRTOD_FOR_FROM_CHARS): Define when using fast_float if long double is not as wide as double and strtold is not broken. * crossconfig.m4: Test for strtof and strtold declarations on vxworks. (GLIBCXX_CHECK_MATH_DECL): Include stdlib.h too. * configure: Rebuilt. --- libstdc++-v3/configure | 131 +++++++++++++++++++++++++ libstdc++-v3/crossconfig.m4 | 3 - libstdc++-v3/src/c++17/floating_from_chars.cc | 10 ++ 3 files changed, 143 insertions(+), 1 deletion(-)
Comments
On Thu, 4 May 2023 at 13:06, Alexandre Oliva via Libstdc++ < libstdc++@gcc.gnu.org> wrote: > > When we're using fast_float for 32- and 64-bit floating point, use > strtold for wider long double, even if locales are unavailable. > > On vxworks, test for strtof's and strtold's declarations, so that they > can be used even when cross compiling. > > Include stdlib.h in the decl-checking macro, so that it can find them. > > Regstrapped on x86_64-linux-gnu. Also tested on aarch64-vx7r2 with > gcc-12, where uselocale is not available, and using strtold rather than > fast_math's double fallback avoids a couple of from_chars-related > testsuite fails (from_chars/4.cc and to_chars/long_double.cc). Ok to > install? > The reason we don't use strtod (or strtold) without uselocale is that it is locale-dependent, and so doesn't have the correct semantics for from_chars. Using fast_float's binary64 implementation for 80-bit or 128-bit long double might give inaccurate results, but using the global locale can give completely incorrect results. For example, if the global locale is set to "fr_FR" then from_chars would parse "1.23" as 1.0L and would parse "1,23" as 1.23L, both of which are wrong. We could use strtod for a single-threaded target (i.e. !defined(_GLIBCXX_HAS_GTHREADS) by changing the global locale using setlocale, instead of changing the per-thread locale using uselocale. And we could use strtod for a target that doesn't support locales *at all* (so strtod always behaves as specified for LANG=C). But unless I'm missing something, your change applies to multi-threaded targets that support locales. I think it needs to be more specific, maybe via some "really use strtod for from_chars, I know it's wrong in the general case" target-specific macro that you could define for vxworks. The attached (lightly-tested) patch uses RAII to set/restore the locale and the FE rounding mode, and extends the use of strtod to single-threaded targets. That removes some of the repetition in the preprocessor conditions, which should make it simpler to extend with a "really use strtod" macro if we want to do that. Patrick, could you please review Alex's patch and my one attached here, in case we've missed anything else w.r.t from_chars, thanks. commit b9733838ba64a748745b9aac640a35417a36dc0e Author: Jonathan Wakely <jwakely@redhat.com> Date: Thu May 4 15:22:07 2023 libstdc++: Use RAII types in strtod-based std::from_chars implementation This adds auto_locale and auto_ferounding types to use RAII for changing and restoring the local and floating-point environment when using strtod to implement std::from_chars. The destructors for the RAII objects run slightly later than the previous statements that restored the locale/fenv, but not the difference is not significant. After this change it would be safe to define USE_STRTOD_FOR_FROM_CHARS for single-threaded targets, where it's OK to change the global locale while we use strtod. This would be an ABI change for affected targets, but it's possible that targets with no thread support don't care about that anyway. It would also mean that AIX would use a different std::from_chars implementation depending whether -pthread was used or not, since it has separate multilibs for single-threaded and multi-threaded. That seems less desirable. libstdc++-v3/ChangeLog: * src/c++17/floating_from_chars.cc [USE_STRTOD_FOR_FROM_CHARS] (auto_locale, auto_ferounding): New class types. (from_chars_impl): Use auto_locale and auto_ferounding. diff --git a/libstdc++-v3/src/c++17/floating_from_chars.cc b/libstdc++-v3/src/c++17/floating_from_chars.cc index 78b9d92cdc0..234cacd872c 100644 --- a/libstdc++-v3/src/c++17/floating_from_chars.cc +++ b/libstdc++-v3/src/c++17/floating_from_chars.cc @@ -50,7 +50,7 @@ # include <xlocale.h> #endif -#if _GLIBCXX_HAVE_USELOCALE +#if _GLIBCXX_HAVE_USELOCALE // || !defined _GLIBCXX_HAS_GTHREADS // FIXME: This should be reimplemented so it doesn't use strtod and newlocale. // That will avoid the need for any memory allocation, meaning that the // non-conforming errc::not_enough_memory result cannot happen. @@ -597,6 +597,87 @@ namespace return buf.c_str(); } + // RAII type to change and restore the locale. + struct auto_locale + { +#if _GLIBCXX_HAVE_USELOCALE + // When we have uselocale we can change the current thread's locale. + locale_t loc; + locale_t orig; + + auto_locale() + : loc(::newlocale(LC_ALL_MASK, "C", (locale_t)0)) + { + if (loc) + orig = ::uselocale(loc); + else + ec = errc{errno}; + } + + ~auto_locale() + { + if (loc) + { + ::uselocale(orig); + ::freelocale(loc); + } + } +#elif !defined _GLIBCXX_HAS_GTHREADS + // For a single-threaded target it's safe to change the global locale. + string orig; + + auto_locale() + { + const char* curloc = setlocale(LC_ALL, nullptr); + if (curloc && setlocale(LC_ALL, "C")) + orig = curloc; + else + ec = errc::operation_not_supported; + } + + ~auto_locale() + { + if (*this) + ::setlocale(LC_ALL, orig.c_str()); + } +#else + // Otherwise, we can't change the locale and so strtod can't be used. + auto_locale() = delete; +#endif + + explicit operator bool() const noexcept { return ec == errc{}; } + + errc ec{}; + + auto_locale(const auto_locale&) = delete; + auto_locale& operator=(const auto_locale&) = delete; + }; + + // RAII type to change and restore the floating-point environment. + struct auto_ferounding + { +#if _GLIBCXX_USE_C99_FENV_TR1 && defined(FE_TONEAREST) + const int rounding = std::fegetround(); + + auto_ferounding() + { + if (rounding != FE_TONEAREST) + std::fesetround(FE_TONEAREST); + } + + ~auto_ferounding() + { + if (rounding != FE_TONEAREST) + std::fesetround(rounding); + } +#else + auto_ferounding() = default; +#endif + + auto_ferounding(const auto_ferounding&) = delete; + auto_ferounding& operator=(const auto_ferounding&) = delete; + }; + // Convert the NTBS `str` to a floating-point value of type `T`. // If `str` cannot be converted, `value` is unchanged and `0` is returned. // Otherwise, let N be the number of characters consumed from `str`. @@ -607,16 +688,11 @@ namespace ptrdiff_t from_chars_impl(const char* str, T& value, errc& ec) noexcept { - if (locale_t loc = ::newlocale(LC_ALL_MASK, "C", (locale_t)0)) [[likely]] + auto_locale loc; + + if (loc) { - locale_t orig = ::uselocale(loc); - -#if _GLIBCXX_USE_C99_FENV_TR1 && defined(FE_TONEAREST) - const int rounding = std::fegetround(); - if (rounding != FE_TONEAREST) - std::fesetround(FE_TONEAREST); -#endif - + auto_ferounding rounding; const int save_errno = errno; errno = 0; char* endptr; @@ -647,14 +723,6 @@ namespace #endif const int conv_errno = std::__exchange(errno, save_errno); -#if _GLIBCXX_USE_C99_FENV_TR1 && defined(FE_TONEAREST) - if (rounding != FE_TONEAREST) - std::fesetround(rounding); -#endif - - ::uselocale(orig); - ::freelocale(loc); - const ptrdiff_t n = endptr - str; if (conv_errno == ERANGE) [[unlikely]] { @@ -675,8 +743,8 @@ namespace } return n; } - else if (errno == ENOMEM) - ec = errc::not_enough_memory; + else + ec = loc.ec; return 0; }
On May 4, 2023, Jonathan Wakely <jwakely@redhat.com> wrote: > And we could use strtod for a target that doesn't support locales *at all* > (so strtod always behaves as specified for LANG=C). Oh, sorry, I misread the *_USELOCALE macro as *_USE_LOCALE, and I thought this was what I was doing. Nevermind, patch withdrawn. I guess I should look into how to xfail or skip the tests involving full-precision long doubles on targets that are limited to doubles with lower precision to convert chars to long doubles. It's a pity to xfail the whole tests over an expected issue. Thanks,
* Jonathan Wakely via Libstdc: > We could use strtod for a single-threaded target (i.e. > !defined(_GLIBCXX_HAS_GTHREADS) by changing the global locale using > setlocale, instead of changing the per-thread locale using uselocale. This is not generally safe because the call to setlocale is still observable to applications in principle because a previous pointer returned from setlocale they have store could be invalidated. Thanks, Florian
On Fri, 5 May 2023 at 10:43, Florian Weimer wrote: > * Jonathan Wakely via Libstdc: > > > We could use strtod for a single-threaded target (i.e. > > !defined(_GLIBCXX_HAS_GTHREADS) by changing the global locale using > > setlocale, instead of changing the per-thread locale using uselocale. > > This is not generally safe because the call to setlocale is still > observable to applications in principle because a previous pointer > returned from setlocale they have store could be invalidated. > > Ah yes, good point, thanks. I think that's a non-starter then. I still think using RAII makes the from_chars_impl function easier to read, so here's a version of that patch without the single-threaded conditions. commit 4dc5b8864ec527e699d35880fbc706157113f92b Author: Jonathan Wakely <jwakely@redhat.com> Date: Thu May 4 15:22:07 2023 libstdc++: Use RAII types in strtod-based std::from_chars implementation This adds auto_locale and auto_ferounding types to use RAII for changing and restoring the local and floating-point environment when using strtod to implement std::from_chars. The destructors for the RAII objects run slightly later than the previous statements that restored the locale/fenv, but the differences are just some trivial assignments and an isinf call. libstdc++-v3/ChangeLog: * src/c++17/floating_from_chars.cc [USE_STRTOD_FOR_FROM_CHARS] (auto_locale, auto_ferounding): New class types. (from_chars_impl): Use auto_locale and auto_ferounding. diff --git a/libstdc++-v3/src/c++17/floating_from_chars.cc b/libstdc++-v3/src/c++17/floating_from_chars.cc index 78b9d92cdc0..7b3bdf445e3 100644 --- a/libstdc++-v3/src/c++17/floating_from_chars.cc +++ b/libstdc++-v3/src/c++17/floating_from_chars.cc @@ -597,6 +597,69 @@ namespace return buf.c_str(); } + // RAII type to change and restore the locale. + struct auto_locale + { +#if _GLIBCXX_HAVE_USELOCALE + // When we have uselocale we can change the current thread's locale. + locale_t loc; + locale_t orig; + + auto_locale() + : loc(::newlocale(LC_ALL_MASK, "C", (locale_t)0)) + { + if (loc) + orig = ::uselocale(loc); + else + ec = errc{errno}; + } + + ~auto_locale() + { + if (loc) + { + ::uselocale(orig); + ::freelocale(loc); + } + } +#else + // Otherwise, we can't change the locale and so strtod can't be used. + auto_locale() = delete; +#endif + + explicit operator bool() const noexcept { return ec == errc{}; } + + errc ec{}; + + auto_locale(const auto_locale&) = delete; + auto_locale& operator=(const auto_locale&) = delete; + }; + + // RAII type to change and restore the floating-point environment. + struct auto_ferounding + { +#if _GLIBCXX_USE_C99_FENV_TR1 && defined(FE_TONEAREST) + const int rounding = std::fegetround(); + + auto_ferounding() + { + if (rounding != FE_TONEAREST) + std::fesetround(FE_TONEAREST); + } + + ~auto_ferounding() + { + if (rounding != FE_TONEAREST) + std::fesetround(rounding); + } +#else + auto_ferounding() = default; +#endif + + auto_ferounding(const auto_ferounding&) = delete; + auto_ferounding& operator=(const auto_ferounding&) = delete; + }; + // Convert the NTBS `str` to a floating-point value of type `T`. // If `str` cannot be converted, `value` is unchanged and `0` is returned. // Otherwise, let N be the number of characters consumed from `str`. @@ -607,16 +670,11 @@ namespace ptrdiff_t from_chars_impl(const char* str, T& value, errc& ec) noexcept { - if (locale_t loc = ::newlocale(LC_ALL_MASK, "C", (locale_t)0)) [[likely]] + auto_locale loc; + + if (loc) { - locale_t orig = ::uselocale(loc); - -#if _GLIBCXX_USE_C99_FENV_TR1 && defined(FE_TONEAREST) - const int rounding = std::fegetround(); - if (rounding != FE_TONEAREST) - std::fesetround(FE_TONEAREST); -#endif - + auto_ferounding rounding; const int save_errno = errno; errno = 0; char* endptr; @@ -647,14 +705,6 @@ namespace #endif const int conv_errno = std::__exchange(errno, save_errno); -#if _GLIBCXX_USE_C99_FENV_TR1 && defined(FE_TONEAREST) - if (rounding != FE_TONEAREST) - std::fesetround(rounding); -#endif - - ::uselocale(orig); - ::freelocale(loc); - const ptrdiff_t n = endptr - str; if (conv_errno == ERANGE) [[unlikely]] { @@ -675,8 +725,8 @@ namespace } return n; } - else if (errno == ENOMEM) - ec = errc::not_enough_memory; + else + ec = loc.ec; return 0; }
Here's a patch to skip/xfail the bits that are expected to fail on aarch64-vxworks. [libstdc++] [testsuite] xfail double-prec from_chars for ldbl When long double is wider than double, but from_chars is implemented in terms of double, tests that involve the full precision of long double are expected to fail. Mark them as such on aarch64-*-vxworks. for libstdc++-v3/ChangeLog * testsuite/20_util/from_chars/4.cc: Skip long double test06 on aarch64-vxworks. * testsuite/20_util/to_chars/long_double.cc: Xfail run on aarch64-vxworks. --- libstdc++-v3/testsuite/20_util/from_chars/4.cc | 3 ++- .../testsuite/20_util/to_chars/long_double.cc | 4 ++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/libstdc++-v3/testsuite/20_util/from_chars/4.cc b/libstdc++-v3/testsuite/20_util/from_chars/4.cc index dd55690eb6511..c3594f9014bd3 100644 --- a/libstdc++-v3/testsuite/20_util/from_chars/4.cc +++ b/libstdc++-v3/testsuite/20_util/from_chars/4.cc @@ -18,6 +18,7 @@ // <charconv> is supported in C++14 as a GNU extension // { dg-do run { target c++14 } } // { dg-add-options ieee } +// { dg-additional-options "-DSKIP_LONG_DOUBLE" { target aarch64-*-vxworks* } } #include <charconv> #include <string> @@ -354,7 +355,7 @@ test06() { test_max_mantissa<float, unsigned long>(); test_max_mantissa<double, unsigned long long>(); -#ifdef __GLIBCXX_TYPE_INT_N_0 +#if defined __GLIBCXX_TYPE_INT_N_0 && !defined SKIP_LONG_DOUBLE test_max_mantissa<long double, unsigned __GLIBCXX_TYPE_INT_N_0>(); #endif } diff --git a/libstdc++-v3/testsuite/20_util/to_chars/long_double.cc b/libstdc++-v3/testsuite/20_util/to_chars/long_double.cc index 880c98021876d..263144bd42cba 100644 --- a/libstdc++-v3/testsuite/20_util/to_chars/long_double.cc +++ b/libstdc++-v3/testsuite/20_util/to_chars/long_double.cc @@ -34,6 +34,10 @@ // more portable and robust to differences in system printf behavior. // { dg-xfail-run-if "Non-conforming printf (see PR98384)" { *-*-solaris* *-*-darwin* } } +// On systems that use double-precision from_chars for long double, +// this is expected to fail. +// { dg-xfail-run-if "from_chars limited to double-precision" { aarch64-*-vxworks* } } + // { dg-require-effective-target ieee_floats } // { dg-require-effective-target size32plus } // { dg-require-cmath "" }
On Fri, 5 May 2023 at 11:39, Alexandre Oliva <oliva@adacore.com> wrote: > Here's a patch to skip/xfail the bits that are expected to fail on > aarch64-vxworks. > OK for trunk and gcc-13, thanks. > > > [libstdc++] [testsuite] xfail double-prec from_chars for ldbl > > When long double is wider than double, but from_chars is implemented > in terms of double, tests that involve the full precision of long > double are expected to fail. Mark them as such on aarch64-*-vxworks. > > > for libstdc++-v3/ChangeLog > > * testsuite/20_util/from_chars/4.cc: Skip long double test06 > on aarch64-vxworks. > * testsuite/20_util/to_chars/long_double.cc: Xfail run on > aarch64-vxworks. > --- > libstdc++-v3/testsuite/20_util/from_chars/4.cc | 3 ++- > .../testsuite/20_util/to_chars/long_double.cc | 4 ++++ > 2 files changed, 6 insertions(+), 1 deletion(-) > > diff --git a/libstdc++-v3/testsuite/20_util/from_chars/4.cc > b/libstdc++-v3/testsuite/20_util/from_chars/4.cc > index dd55690eb6511..c3594f9014bd3 100644 > --- a/libstdc++-v3/testsuite/20_util/from_chars/4.cc > +++ b/libstdc++-v3/testsuite/20_util/from_chars/4.cc > @@ -18,6 +18,7 @@ > // <charconv> is supported in C++14 as a GNU extension > // { dg-do run { target c++14 } } > // { dg-add-options ieee } > +// { dg-additional-options "-DSKIP_LONG_DOUBLE" { target > aarch64-*-vxworks* } } > > #include <charconv> > #include <string> > @@ -354,7 +355,7 @@ test06() > { > test_max_mantissa<float, unsigned long>(); > test_max_mantissa<double, unsigned long long>(); > -#ifdef __GLIBCXX_TYPE_INT_N_0 > +#if defined __GLIBCXX_TYPE_INT_N_0 && !defined SKIP_LONG_DOUBLE > test_max_mantissa<long double, unsigned __GLIBCXX_TYPE_INT_N_0>(); > #endif > } > diff --git a/libstdc++-v3/testsuite/20_util/to_chars/long_double.cc > b/libstdc++-v3/testsuite/20_util/to_chars/long_double.cc > index 880c98021876d..263144bd42cba 100644 > --- a/libstdc++-v3/testsuite/20_util/to_chars/long_double.cc > +++ b/libstdc++-v3/testsuite/20_util/to_chars/long_double.cc > @@ -34,6 +34,10 @@ > // more portable and robust to differences in system printf behavior. > // { dg-xfail-run-if "Non-conforming printf (see PR98384)" { *-*-solaris* > *-*-darwin* } } > > +// On systems that use double-precision from_chars for long double, > +// this is expected to fail. > +// { dg-xfail-run-if "from_chars limited to double-precision" { > aarch64-*-vxworks* } } > + > // { dg-require-effective-target ieee_floats } > // { dg-require-effective-target size32plus } > // { dg-require-cmath "" } > > > -- > Alexandre Oliva, happy hacker https://FSFLA.org/blogs/lxo/ > Free Software Activist GNU Toolchain Engineer > Disinformation flourishes because many people care deeply about injustice > but very few check the facts. Ask me about <https://stallmansupport.org> > >
On Fri, 5 May 2023, Jonathan Wakely wrote: > > > On Fri, 5 May 2023 at 10:43, Florian Weimer wrote: > * Jonathan Wakely via Libstdc: > > > We could use strtod for a single-threaded target (i.e. > > !defined(_GLIBCXX_HAS_GTHREADS) by changing the global locale using > > setlocale, instead of changing the per-thread locale using uselocale. > > This is not generally safe because the call to setlocale is still > observable to applications in principle because a previous pointer > returned from setlocale they have store could be invalidated. > > > Ah yes, good point, thanks. I think that's a non-starter then. I still think using RAII makes the from_chars_impl function easier to read, so here's a version of that patch without the single-threaded > conditions. > > commit 4dc5b8864ec527e699d35880fbc706157113f92b > Author: Jonathan Wakely <jwakely@redhat.com> > Date: Thu May 4 15:22:07 2023 > > libstdc++: Use RAII types in strtod-based std::from_chars implementation > > This adds auto_locale and auto_ferounding types to use RAII for changing > and restoring the local and floating-point environment when using strtod > to implement std::from_chars. > > The destructors for the RAII objects run slightly later than the > previous statements that restored the locale/fenv, but the differences > are just some trivial assignments and an isinf call. > > libstdc++-v3/ChangeLog: > > * src/c++17/floating_from_chars.cc [USE_STRTOD_FOR_FROM_CHARS] > (auto_locale, auto_ferounding): New class types. > (from_chars_impl): Use auto_locale and auto_ferounding. > > diff --git a/libstdc++-v3/src/c++17/floating_from_chars.cc b/libstdc++-v3/src/c++17/floating_from_chars.cc > index 78b9d92cdc0..7b3bdf445e3 100644 > --- a/libstdc++-v3/src/c++17/floating_from_chars.cc > +++ b/libstdc++-v3/src/c++17/floating_from_chars.cc > @@ -597,6 +597,69 @@ namespace > return buf.c_str(); > } > > + // RAII type to change and restore the locale. > + struct auto_locale > + { > +#if _GLIBCXX_HAVE_USELOCALE > + // When we have uselocale we can change the current thread's locale. > + locale_t loc; > + locale_t orig; It's not a big deal, but we could consider making these members const too, like in auto_ferounding. LGTM. I noticed sprintf_ld from floating_to_chars.cc could benefit from auto_ferounding as well. > + > + auto_locale() > + : loc(::newlocale(LC_ALL_MASK, "C", (locale_t)0)) > + { > + if (loc) > + orig = ::uselocale(loc); > + else > + ec = errc{errno}; > + } > + > + ~auto_locale() > + { > + if (loc) > + { > + ::uselocale(orig); > + ::freelocale(loc); > + } > + } > +#else > + // Otherwise, we can't change the locale and so strtod can't be used. > + auto_locale() = delete; > +#endif > + > + explicit operator bool() const noexcept { return ec == errc{}; } > + > + errc ec{}; > + > + auto_locale(const auto_locale&) = delete; > + auto_locale& operator=(const auto_locale&) = delete; > + }; > + > + // RAII type to change and restore the floating-point environment. > + struct auto_ferounding > + { > +#if _GLIBCXX_USE_C99_FENV_TR1 && defined(FE_TONEAREST) > + const int rounding = std::fegetround(); > + > + auto_ferounding() > + { > + if (rounding != FE_TONEAREST) > + std::fesetround(FE_TONEAREST); > + } > + > + ~auto_ferounding() > + { > + if (rounding != FE_TONEAREST) > + std::fesetround(rounding); > + } > +#else > + auto_ferounding() = default; > +#endif > + > + auto_ferounding(const auto_ferounding&) = delete; > + auto_ferounding& operator=(const auto_ferounding&) = delete; > + }; > + > // Convert the NTBS `str` to a floating-point value of type `T`. > // If `str` cannot be converted, `value` is unchanged and `0` is returned. > // Otherwise, let N be the number of characters consumed from `str`. > @@ -607,16 +670,11 @@ namespace > ptrdiff_t > from_chars_impl(const char* str, T& value, errc& ec) noexcept > { > - if (locale_t loc = ::newlocale(LC_ALL_MASK, "C", (locale_t)0)) [[likely]] > + auto_locale loc; > + > + if (loc) > { > - locale_t orig = ::uselocale(loc); > - > -#if _GLIBCXX_USE_C99_FENV_TR1 && defined(FE_TONEAREST) > - const int rounding = std::fegetround(); > - if (rounding != FE_TONEAREST) > - std::fesetround(FE_TONEAREST); > -#endif > - > + auto_ferounding rounding; > const int save_errno = errno; > errno = 0; > char* endptr; > @@ -647,14 +705,6 @@ namespace > #endif > const int conv_errno = std::__exchange(errno, save_errno); > > -#if _GLIBCXX_USE_C99_FENV_TR1 && defined(FE_TONEAREST) > - if (rounding != FE_TONEAREST) > - std::fesetround(rounding); > -#endif > - > - ::uselocale(orig); > - ::freelocale(loc); > - > const ptrdiff_t n = endptr - str; > if (conv_errno == ERANGE) [[unlikely]] > { > @@ -675,8 +725,8 @@ namespace > } > return n; > } > - else if (errno == ENOMEM) > - ec = errc::not_enough_memory; > + else > + ec = loc.ec; > > return 0; > }
On Thu, 11 May 2023 at 17:04, Patrick Palka <ppalka@redhat.com> wrote: > On Fri, 5 May 2023, Jonathan Wakely wrote: > > > > > > > On Fri, 5 May 2023 at 10:43, Florian Weimer wrote: > > * Jonathan Wakely via Libstdc: > > > > > We could use strtod for a single-threaded target (i.e. > > > !defined(_GLIBCXX_HAS_GTHREADS) by changing the global locale > using > > > setlocale, instead of changing the per-thread locale using > uselocale. > > > > This is not generally safe because the call to setlocale is still > > observable to applications in principle because a previous pointer > > returned from setlocale they have store could be invalidated. > > > > > > Ah yes, good point, thanks. I think that's a non-starter then. I still > think using RAII makes the from_chars_impl function easier to read, so > here's a version of that patch without the single-threaded > > conditions. > > > > commit 4dc5b8864ec527e699d35880fbc706157113f92b > > Author: Jonathan Wakely <jwakely@redhat.com> > > Date: Thu May 4 15:22:07 2023 > > > > libstdc++: Use RAII types in strtod-based std::from_chars > implementation > > > > This adds auto_locale and auto_ferounding types to use RAII for > changing > > and restoring the local and floating-point environment when using > strtod > > to implement std::from_chars. > > > > The destructors for the RAII objects run slightly later than the > > previous statements that restored the locale/fenv, but the > differences > > are just some trivial assignments and an isinf call. > > > > libstdc++-v3/ChangeLog: > > > > * src/c++17/floating_from_chars.cc > [USE_STRTOD_FOR_FROM_CHARS] > > (auto_locale, auto_ferounding): New class types. > > (from_chars_impl): Use auto_locale and auto_ferounding. > > > > diff --git a/libstdc++-v3/src/c++17/floating_from_chars.cc > b/libstdc++-v3/src/c++17/floating_from_chars.cc > > index 78b9d92cdc0..7b3bdf445e3 100644 > > --- a/libstdc++-v3/src/c++17/floating_from_chars.cc > > +++ b/libstdc++-v3/src/c++17/floating_from_chars.cc > > @@ -597,6 +597,69 @@ namespace > > return buf.c_str(); > > } > > > > + // RAII type to change and restore the locale. > > + struct auto_locale > > + { > > +#if _GLIBCXX_HAVE_USELOCALE > > + // When we have uselocale we can change the current thread's locale. > > + locale_t loc; > > + locale_t orig; > > It's not a big deal, but we could consider making these members const > too, like in auto_ferounding. > Done for loc, but not for orig (which is currently init'd in the ctor body). > > LGTM. I noticed sprintf_ld from floating_to_chars.cc could benefit from > auto_ferounding as well. > Ah yes. Maybe we should share the class, so we don't have two different types with internal linkage, and two RTTI definitions etc. For now I'll just push this patch, and make a note to reuse auto_ferounding in the other file later. Thanks for the review. > > > + > > + auto_locale() > > + : loc(::newlocale(LC_ALL_MASK, "C", (locale_t)0)) > > + { > > + if (loc) > > + orig = ::uselocale(loc); > > + else > > + ec = errc{errno}; > > + } > > + > > + ~auto_locale() > > + { > > + if (loc) > > + { > > + ::uselocale(orig); > > + ::freelocale(loc); > > + } > > + } > > +#else > > + // Otherwise, we can't change the locale and so strtod can't be > used. > > + auto_locale() = delete; > > +#endif > > + > > + explicit operator bool() const noexcept { return ec == errc{}; } > > + > > + errc ec{}; > > + > > + auto_locale(const auto_locale&) = delete; > > + auto_locale& operator=(const auto_locale&) = delete; > > + }; > > + > > + // RAII type to change and restore the floating-point environment. > > + struct auto_ferounding > > + { > > +#if _GLIBCXX_USE_C99_FENV_TR1 && defined(FE_TONEAREST) > > + const int rounding = std::fegetround(); > > + > > + auto_ferounding() > > + { > > + if (rounding != FE_TONEAREST) > > + std::fesetround(FE_TONEAREST); > > + } > > + > > + ~auto_ferounding() > > + { > > + if (rounding != FE_TONEAREST) > > + std::fesetround(rounding); > > + } > > +#else > > + auto_ferounding() = default; > > +#endif > > + > > + auto_ferounding(const auto_ferounding&) = delete; > > + auto_ferounding& operator=(const auto_ferounding&) = delete; > > + }; > > + > > // Convert the NTBS `str` to a floating-point value of type `T`. > > // If `str` cannot be converted, `value` is unchanged and `0` is > returned. > > // Otherwise, let N be the number of characters consumed from `str`. > > @@ -607,16 +670,11 @@ namespace > > ptrdiff_t > > from_chars_impl(const char* str, T& value, errc& ec) noexcept > > { > > - if (locale_t loc = ::newlocale(LC_ALL_MASK, "C", (locale_t)0)) > [[likely]] > > + auto_locale loc; > > + > > + if (loc) > > { > > - locale_t orig = ::uselocale(loc); > > - > > -#if _GLIBCXX_USE_C99_FENV_TR1 && defined(FE_TONEAREST) > > - const int rounding = std::fegetround(); > > - if (rounding != FE_TONEAREST) > > - std::fesetround(FE_TONEAREST); > > -#endif > > - > > + auto_ferounding rounding; > > const int save_errno = errno; > > errno = 0; > > char* endptr; > > @@ -647,14 +705,6 @@ namespace > > #endif > > const int conv_errno = std::__exchange(errno, save_errno); > > > > -#if _GLIBCXX_USE_C99_FENV_TR1 && defined(FE_TONEAREST) > > - if (rounding != FE_TONEAREST) > > - std::fesetround(rounding); > > -#endif > > - > > - ::uselocale(orig); > > - ::freelocale(loc); > > - > > const ptrdiff_t n = endptr - str; > > if (conv_errno == ERANGE) [[unlikely]] > > { > > @@ -675,8 +725,8 @@ namespace > > } > > return n; > > } > > - else if (errno == ENOMEM) > > - ec = errc::not_enough_memory; > > + else > > + ec = loc.ec; > > > > return 0; > > } > >
diff --git a/libstdc++-v3/configure b/libstdc++-v3/configure [omitted] diff --git a/libstdc++-v3/crossconfig.m4 b/libstdc++-v3/crossconfig.m4 index b3269cb88e077..9db32f4d422da 100644 --- a/libstdc++-v3/crossconfig.m4 +++ b/libstdc++-v3/crossconfig.m4 @@ -293,7 +293,7 @@ dnl # switch to more elaborate tests. GLIBCXX_CHECK_MATH_DECLS([ acosl asinl atan2l atanl ceill cosl coshl expl fabsl floorl fmodl frexpl ldexpl log10l logl modfl powl sinl sinhl sqrtl tanl tanhl hypotl - ldexpf modff hypotf frexpf]) + ldexpf modff hypotf frexpf strtof strtold]) dnl # sincosl is the only one missing here, compared with the *l dnl # functions in the list guarded by dnl # long_double_math_on_this_cpu in configure.ac, right after @@ -323,6 +323,7 @@ AC_DEFUN([GLIBCXX_CHECK_MATH_DECL], [ AC_LANG_SAVE AC_LANG_C AC_TRY_COMPILE([ +#include <stdlib.h> #include <math.h> #ifdef HAVE_IEEEFP_H # include <ieeefp.h> diff --git a/libstdc++-v3/src/c++17/floating_from_chars.cc b/libstdc++-v3/src/c++17/floating_from_chars.cc index 78b9d92cdc0fa..15af811d198c4 100644 --- a/libstdc++-v3/src/c++17/floating_from_chars.cc +++ b/libstdc++-v3/src/c++17/floating_from_chars.cc @@ -80,6 +80,10 @@ extern "C" _Float128 __strtof128(const char*, char**) # if __LDBL_MANT_DIG__ == __DBL_MANT_DIG__ // No need to use strtold. # undef USE_STRTOD_FOR_FROM_CHARS +# elif !defined USE_STRTOD_FOR_FROM_CHARS \ + && defined _GLIBCXX_HAVE_STRTOLD && !defined _GLIBCXX_HAVE_BROKEN_STRTOLD +// A working strtold will be more compliant than fast_float's double. +# define USE_STRTOD_FOR_FROM_CHARS 1 # endif #endif @@ -607,9 +611,11 @@ namespace ptrdiff_t from_chars_impl(const char* str, T& value, errc& ec) noexcept { +#if _GLIBCXX_HAVE_USELOCALE if (locale_t loc = ::newlocale(LC_ALL_MASK, "C", (locale_t)0)) [[likely]] { locale_t orig = ::uselocale(loc); +#endif #if _GLIBCXX_USE_C99_FENV_TR1 && defined(FE_TONEAREST) const int rounding = std::fegetround(); @@ -652,8 +658,10 @@ namespace std::fesetround(rounding); #endif +#if _GLIBCXX_HAVE_USELOCALE ::uselocale(orig); ::freelocale(loc); +#endif const ptrdiff_t n = endptr - str; if (conv_errno == ERANGE) [[unlikely]] @@ -674,9 +682,11 @@ namespace ec = errc(); } return n; +#if _GLIBCXX_HAVE_USELOCALE } else if (errno == ENOMEM) ec = errc::not_enough_memory; +#endif return 0; }