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