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

[Jonathan Wakely]

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 <jwak...@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 ...



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;