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

[Jonathan Wakely]

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