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

[Jonathan Wakely <jwakely@redhat.com>]

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