On Thu, 4 May 2023 at 13:06, Alexandre Oliva via Libstdc++ <
libstd...@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 <jwak...@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;
}
