From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id 0C1423858C54 for ; Thu, 11 May 2023 16:51:21 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 0C1423858C54 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1683823880; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=MJjgk8B0br5a2U3Tk71WBqV/tY5WYgVLZi+KWoMCFPY=; b=UxzdTNHUbQdit6JRYrQZZmHoDB7xzfVUbiA4PH4L3EZoYJx9XktUgt7MAzUgBq79mzRSq2 nef1qeZDyaX+x4ilx8oEe8djXI+ru5qS/OI0UZmTH6QEzssIgxQRhKkzo3P1NBT8rhTqtt gRsNl9t2VALrO5bd+XDM5F8CooVeIHk= Received: from mail-lj1-f197.google.com (mail-lj1-f197.google.com [209.85.208.197]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-76-MvZPMu70NPiZ7sTXXKF6gg-1; Thu, 11 May 2023 12:51:19 -0400 X-MC-Unique: MvZPMu70NPiZ7sTXXKF6gg-1 Received: by mail-lj1-f197.google.com with SMTP id 38308e7fff4ca-2acfaf0c1ffso30979621fa.3 for ; Thu, 11 May 2023 09:51:18 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1683823877; x=1686415877; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=MJjgk8B0br5a2U3Tk71WBqV/tY5WYgVLZi+KWoMCFPY=; b=fGHJQA5uaH5Cmd0So0+RTBKfuepEDlBqgtqcF6kGnSydxJxB4il5m7zEVc4Y+ODYUP sPUhNNGTf9UxIeqH/y326ZJ3WN+7/CMTw1SOP0HWd3U01Sulp+zJRUqRCCae3Xwe4FY6 zVzAZLcOI0SR0bjmlYAv7u7PbcLdn4Kr+KWFHI+3BZMlAV8HDlJkPJ7IC9yIiGeo8s5f WE7bqDnJ+Ml4TH+cFp/Gv76lMxR3ds1hS58rFwJHYELriUcSBgj9DglFMiqdUhN2Qb3S /JEDZAQSVJeG+ZDEA16e+MVCzyiXPQIajq0ZvvDvwkNjqgdWQYN94hzAKlG6pP+o1mYO U15A== X-Gm-Message-State: AC+VfDwMnCXah3XFp1NuziUgKCpdK0GUy4o5rnDfdG4mre0M1PAbEl0n gKj/CAwFPkg0SaIAvvxYuSCjBoP2c2p8eZ6Bbd1c5vvL9JTnwaat53xEPVfzeU7IaUZFqF8sAFW SjY8dADfJnJHXPSHnwxE7L1BbON2IVok= X-Received: by 2002:a05:651c:150:b0:2ad:9c26:3638 with SMTP id c16-20020a05651c015000b002ad9c263638mr3550917ljd.52.1683823877787; Thu, 11 May 2023 09:51:17 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ6ueIHVQDh3NOIxOykgSM2uouQaSgaovXp6hLRo0mTzUuQX3GuDGLw3evl4GwEyD8z4rOY+jxESPbHcFh1q6fw= X-Received: by 2002:a05:651c:150:b0:2ad:9c26:3638 with SMTP id c16-20020a05651c015000b002ad9c263638mr3550905ljd.52.1683823877401; Thu, 11 May 2023 09:51:17 -0700 (PDT) MIME-Version: 1.0 References: <87cz3fw1uh.fsf@oldenburg.str.redhat.com> <22cf6d46-517f-ae3f-4e35-3280b8bbd275@idea> In-Reply-To: <22cf6d46-517f-ae3f-4e35-3280b8bbd275@idea> From: Jonathan Wakely Date: Thu, 11 May 2023 17:51:06 +0100 Message-ID: Subject: Re: [libstdc++] use strtold for from_chars even without locale To: Patrick Palka Cc: Florian Weimer , "Jonathan Wakely via Libstdc++" , Alexandre Oliva , gcc-patches@gcc.gnu.org X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: multipart/alternative; boundary="00000000000013bf4305fb6dcc12" X-Spam-Status: No, score=-13.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,HTML_MESSAGE,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE,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 List-Id: --00000000000013bf4305fb6dcc12 Content-Type: text/plain; charset="UTF-8" On Thu, 11 May 2023 at 17:04, Patrick Palka 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 > > 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; > > } > > --00000000000013bf4305fb6dcc12--