[libstdc++] use strtold for from_chars even without locale

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(-)

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;
> >    }
>
>