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

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

[Jonathan Wakely]

[-- Attachment #1: Type: text/plain, Size: 207 bytes --]

We set errno=0 in __gnu_cxx::__stoa in order to reliably detect when
it gets set to ERANGE. This restores the previous value when the
conversion is successful.

Tested powerpc64le-linux, committed to trunk.

[-- Attachment #2: patch.txt --]
[-- Type: text/x-patch, Size: 2430 bytes --]

commit 412f75dc37b1048e14996c9caafa46c00db8eb30
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Tue Sep 29 15:09:23 2015 +0100

    Leave errno unchanged by successful std::stoi etc
    
    	* include/ext/string_conversions.h (__stoa): Save and restore errno.
    	* testsuite/21_strings/basic_string/numeric_conversions/char/errno.cc:
    	New.

diff --git a/libstdc++-v3/include/ext/string_conversions.h b/libstdc++-v3/include/ext/string_conversions.h
index f4648a8..58387a2 100644
--- a/libstdc++-v3/include/ext/string_conversions.h
+++ b/libstdc++-v3/include/ext/string_conversions.h
@@ -58,6 +58,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       _Ret __ret;
 
       _CharT* __endptr;
+      const int __saved_errno = errno;
       errno = 0;
       const _TRet __tmp = __convf(__str, &__endptr, __base...);
 
@@ -70,6 +71,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	std::__throw_out_of_range(__name);
       else
 	__ret = __tmp;
+      errno = __saved_errno;
 
       if (__idx)
 	*__idx = __endptr - __str;
diff --git a/libstdc++-v3/testsuite/21_strings/basic_string/numeric_conversions/char/errno.cc b/libstdc++-v3/testsuite/21_strings/basic_string/numeric_conversions/char/errno.cc
new file mode 100644
index 0000000..4079744
--- /dev/null
+++ b/libstdc++-v3/testsuite/21_strings/basic_string/numeric_conversions/char/errno.cc
@@ -0,0 +1,36 @@
+// Copyright (C) 2015 Free Software Foundation, Inc.
+//
+// This file is part of the GNU ISO C++ Library.  This library is free
+// software; you can redistribute it and/or modify it under the
+// terms of the GNU General Public License as published by the
+// Free Software Foundation; either version 3, or (at your option)
+// any later version.
+
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License along
+// with this library; see the file COPYING3.  If not see
+// <http://www.gnu.org/licenses/>.
+
+// { dg-options "-std=gnu++11" }
+// { dg-require-string-conversions "" }
+
+#include <string>
+#include <testsuite_hooks.h>
+
+void
+test01()
+{
+  errno = ERANGE;
+  std::stoi("42");
+  VERIFY( errno == ERANGE ); // errno should not be altered by successful call
+}
+
+int
+main()
+{
+  test01();
+}

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

[Jakub Jelinek]

On Tue, Sep 29, 2015 at 04:15:41PM +0100, Jonathan Wakely wrote:
> We set errno=0 in __gnu_cxx::__stoa in order to reliably detect when
> it gets set to ERANGE. This restores the previous value when the
> conversion is successful.
> 
> Tested powerpc64le-linux, committed to trunk.

> commit 412f75dc37b1048e14996c9caafa46c00db8eb30
> Author: Jonathan Wakely <jwakely@redhat.com>
> Date:   Tue Sep 29 15:09:23 2015 +0100
> 
>     Leave errno unchanged by successful std::stoi etc
>     
>     	* include/ext/string_conversions.h (__stoa): Save and restore errno.
>     	* testsuite/21_strings/basic_string/numeric_conversions/char/errno.cc:
>     	New.
> 
> diff --git a/libstdc++-v3/include/ext/string_conversions.h b/libstdc++-v3/include/ext/string_conversions.h
> index f4648a8..58387a2 100644
> --- a/libstdc++-v3/include/ext/string_conversions.h
> +++ b/libstdc++-v3/include/ext/string_conversions.h
> @@ -58,6 +58,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>        _Ret __ret;
>  
>        _CharT* __endptr;
> +      const int __saved_errno = errno;
>        errno = 0;
>        const _TRet __tmp = __convf(__str, &__endptr, __base...);
>  
> @@ -70,6 +71,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>  	std::__throw_out_of_range(__name);
>        else
>  	__ret = __tmp;
> +      errno = __saved_errno;

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.
So, I'd say you want to restore it earlier, right after __convf, and
immediately before that copy the current errno to some other temporary
for the use in the condition?  Or restore errno = __saved_errno;
in all the 3 spots instead of just one.

	Jakub

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

[Jonathan Wakely]

[-- Attachment #1: Type: text/plain, Size: 2158 bytes --]

On 29/09/15 17:25 +0200, Jakub Jelinek wrote:
>On Tue, Sep 29, 2015 at 04:15:41PM +0100, Jonathan Wakely wrote:
>> We set errno=0 in __gnu_cxx::__stoa in order to reliably detect when
>> it gets set to ERANGE. This restores the previous value when the
>> conversion is successful.
>>
>> Tested powerpc64le-linux, committed to trunk.
>
>> commit 412f75dc37b1048e14996c9caafa46c00db8eb30
>> Author: Jonathan Wakely <jwakely@redhat.com>
>> Date:   Tue Sep 29 15:09:23 2015 +0100
>>
>>     Leave errno unchanged by successful std::stoi etc
>>
>>     	* include/ext/string_conversions.h (__stoa): Save and restore errno.
>>     	* testsuite/21_strings/basic_string/numeric_conversions/char/errno.cc:
>>     	New.
>>
>> diff --git a/libstdc++-v3/include/ext/string_conversions.h b/libstdc++-v3/include/ext/string_conversions.h
>> index f4648a8..58387a2 100644
>> --- a/libstdc++-v3/include/ext/string_conversions.h
>> +++ b/libstdc++-v3/include/ext/string_conversions.h
>> @@ -58,6 +58,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>>        _Ret __ret;
>>
>>        _CharT* __endptr;
>> +      const int __saved_errno = errno;
>>        errno = 0;
>>        const _TRet __tmp = __convf(__str, &__endptr, __base...);
>>
>> @@ -70,6 +71,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>>  	std::__throw_out_of_range(__name);
>>        else
>>  	__ret = __tmp;
>> +      errno = __saved_errno;
>
>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.

But I suppose it would be better to consistently set it to non-zero
when an exception is thrown, or consistently restore the original
value in all cases.

>So, I'd say you want to restore it earlier, right after __convf, and
>immediately before that copy the current errno to some other temporary
>for the use in the condition?  Or restore errno = __saved_errno;
>in all the 3 spots instead of just one.

Or in a destructor so it happens however we exit the function, like
this ...



[-- Attachment #2: patch.txt --]
[-- Type: text/x-patch, Size: 867 bytes --]

diff --git a/libstdc++-v3/include/ext/string_conversions.h b/libstdc++-v3/include/ext/string_conversions.h
index 58387a2..3b62c9a 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 _Restore_errno {
+	  _Restore_errno() : _M_errno(errno) { errno = 0; }
+	  ~_Restore_errno() { errno = _M_errno; }
+	  int _M_errno;
+      } const __restore;
+
       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;

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

[Jakub Jelinek]

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.
> 
> But I suppose it would be better to consistently set it to non-zero
> when an exception is thrown, or consistently restore the original
> value in all cases.
> 
> >So, I'd say you want to restore it earlier, right after __convf, and
> >immediately before that copy the current errno to some other temporary
> >for the use in the condition?  Or restore errno = __saved_errno;
> >in all the 3 spots instead of just one.
> 
> Or in a destructor so it happens however we exit the function, like
> this ...

Works for me.

	Jakub

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

[Martin Sebor]

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.

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.

Martin

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

[Jonathan Wakely]

[-- Attachment #1: Type: text/plain, Size: 2479 bytes --]

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 :-)


[-- Attachment #2: patch.txt --]
[-- Type: text/x-patch, Size: 4608 bytes --]

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;

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

[Jonathan Wakely]

>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.
>
Committed to trunk.