Re: [patch] Leave errno unchanged by successful std::stoi etc

[Jonathan Wakely <jwakely at redhat dot com>]

On 29/09/15 10:50 -0600, Martin Sebor wrote:
> On 09/29/2015 10:15 AM, Jakub Jelinek wrote:
> > On Tue, Sep 29, 2015 at 05:10:20PM +0100, Jonathan Wakely wrote:
> > > > That looks wrong to me, you only restore errno if you don't throw :(.
> > > > If you throw, then errno might remain 0, which is IMHO undesirable.
> > >
> > > My thinking was that a failed conversion that throws an exception
> > > should be allowed to modify errno, and that the second case sets it to
> > > ERANGE sometimes anyway.
> >
> > Well, you can modify errno, you just shouldn't change it from non-zero to
> > zero as far as the user is concerned.
> >
> > http://pubs.opengroup.org/onlinepubs/009695399/functions/errno.html
> > "No function in this volume of IEEE Std 1003.1-2001 shall set errno to 0."
> > Of course, this part of STL is not POSIX, still, as you said, it would be
> > nice to guarantee the same.
>
> FWIW, I agree. It's a helpful property. If libstdc++ provides
> the POSIC/C guarantee it would be nice to document it in the
> manual.
>
> That said, this part of the C++ spec (stoi and related) is specified
> to such a level of detail that one might argue that the functions
> aren't allowed to reset errno in an observable way.

Yes, looking at the spec again it's not as simple as my original patch
or as simple as always restoring the previous value.

We are required to call one of the strtoX functions from the C
library, and if that sets errno C++ says nothing about resetting it,
see http://eel.is/c++draft/string.conversions

So we need to zero it to reliably detect ERANGE being set, but we
should undo that if it doesn't get set (i.e. is still zero).  But if
the strtoX function sets errno to anything non-zero then it looks as
though that should be left unchanged.

So the scope-guard type should do:

   ~_Restore_errno() { if (errno == 0) errno = _M_errno; }


I noticed that we're also zeroing errno in the generic c_locale.cc
file so we need something similar there.

> As an aside, I objected to this specification when it was first
> proposed, not because of the errno guarantee, but because the
> functions were meant to be light-weight, efficient, and certainly
> thread-safe means of converting strings to numbers. Specifying
> their effects as opposed to their postconditions means that can't
> be implemented independent of strtol and the C locale, which makes
> them anything but light-weight, and prone to data races in
> programs that call setlocale.

Agreed, but that's not a cause I want to fight for :-)

commit 502928c8061343e82e982e06299c11d465f64b6c
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Wed Sep 30 14:10:58 2015 +0100

    Save-and-restore errno more carefully in libstdc++
    
    	* doc/xml/manual/diagnostics.xml: Document use of errno.
    	* config/locale/generic/c_locale.cc (_Save_errno): New helper.
    	(__convert_to_v): Use _Save_errno.
    	* include/ext/string_conversions.h (__stoa): Only restore errno when
    	it isn't set to non-zero.

diff --git a/libstdc++-v3/config/locale/generic/c_locale.cc b/libstdc++-v3/config/locale/generic/c_locale.cc
index 6da5f22..8dfea6b 100644
--- a/libstdc++-v3/config/locale/generic/c_locale.cc
+++ b/libstdc++-v3/config/locale/generic/c_locale.cc
@@ -44,6 +44,16 @@ namespace std _GLIBCXX_VISIBILITY(default)
 {
 _GLIBCXX_BEGIN_NAMESPACE_VERSION
 
+  namespace
+  {
+    struct _Save_errno
+    {
+      _Save_errno() : _M_errno(errno) { errno = 0; }
+      ~_Save_errno() { if (errno == 0) errno = _M_errno; }
+      int _M_errno;
+    };
+  }
+
   template<>
     void
     __convert_to_v(const char* __s, float& __v, ios_base::iostate& __err,
@@ -59,7 +69,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       bool __overflow = false;
 
 #if !__FLT_HAS_INFINITY__
-      errno = 0;
+      const _Save_errno __save_errno;
 #endif
 
 #ifdef _GLIBCXX_HAVE_STRTOF
@@ -123,7 +133,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       char* __sanity;
 
 #if !__DBL_HAS_INFINITY__
-      errno = 0;
+      const _Save_errno __save_errno;
 #endif
 
       __v = strtod(__s, &__sanity);
@@ -167,7 +177,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       setlocale(LC_ALL, "C");
 
 #if !__LDBL_HAS_INFINITY__
-      errno = 0;
+      const _Save_errno __save_errno;
 #endif
 
 #if defined(_GLIBCXX_HAVE_STRTOLD) && !defined(_GLIBCXX_HAVE_BROKEN_STRTOLD)
diff --git a/libstdc++-v3/doc/xml/manual/diagnostics.xml b/libstdc++-v3/doc/xml/manual/diagnostics.xml
index 88ed2e2..3ceb5b3 100644
--- a/libstdc++-v3/doc/xml/manual/diagnostics.xml
+++ b/libstdc++-v3/doc/xml/manual/diagnostics.xml
@@ -71,6 +71,38 @@
   </section>
 </section>
 
+<section xml:id="std.diagnostics.errno" xreflabel="errno"><info><title>Use of errno by the library</title></info>
+
+  <para>
+    The C and POSIX standards guarantee that <varname>errno</varname>
+    is never set to zero by any library function.
+    The C++ standard has less to say about when <varname>errno</varname>
+    is or isn't set, but libstdc++ follows the same rule and never sets
+    it to zero.
+  </para>
+
+  <para>
+    On the other hand, there are few guarantees about when the C++ library
+    sets <varname>errno</varname> on error, beyond what is specified for
+    functions that come from the C library.
+    For example, when <function>std::stoi</function> throws an exception of
+    type <classname>std::out_of_range</classname>, <varname>errno</varname>
+    may or may not have been set to <constant>ERANGE</constant>.
+  </para>
+
+  <para>
+    Parts of the C++ library may be implemented in terms of C library
+    functions, which may result in <varname>errno</varname> being set
+    with no explicit call to a C function. For example, on a target where
+    <function>operator new</function> uses <function>malloc</function>
+    a failed memory allocation with <function>operator new</function> might
+    set <varname>errno</varname> to <constant>ENOMEM</constant>.
+    Which C++ library functions can set <varname>errno</varname> in this way
+    is unspecified because it may vary between platforms and between releases.
+  </para>
+
+</section>
+
 <section xml:id="std.diagnostics.concept_checking" xreflabel="Concept Checking"><info><title>Concept Checking</title></info>
   <?dbhtml filename="concept_checking.html"?>
   
diff --git a/libstdc++-v3/include/ext/string_conversions.h b/libstdc++-v3/include/ext/string_conversions.h
index 58387a2..7f37e69 100644
--- a/libstdc++-v3/include/ext/string_conversions.h
+++ b/libstdc++-v3/include/ext/string_conversions.h
@@ -58,8 +58,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       _Ret __ret;
 
       _CharT* __endptr;
-      const int __saved_errno = errno;
-      errno = 0;
+
+      struct _Save_errno {
+	_Save_errno() : _M_errno(errno) { errno = 0; }
+	~_Save_errno() { if (errno == 0) errno = _M_errno; }
+	int _M_errno;
+      } const __save_errno;
+
       const _TRet __tmp = __convf(__str, &__endptr, __base...);
 
       if (__endptr == __str)
@@ -71,7 +76,6 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	std::__throw_out_of_range(__name);
       else
 	__ret = __tmp;
-      errno = __saved_errno;
 
       if (__idx)
 	*__idx = __endptr - __str;