Patchwork Make vector::at() assertion message more useful (try #2)

login
register
mail settings
Submitter Paul Pluzhnikov
Date Sept. 13, 2013, 12:01 a.m.
Message ID <CALoOobO6XEsrSM6jJFCQ=n00iY8qM=eujs-=LixNZyuya5f-cA@mail.gmail.com>
Download mbox | patch
Permalink /patch/274622/
State New
Headers show

Comments

Paul Pluzhnikov - Sept. 13, 2013, 12:01 a.m.
On Wed, Sep 4, 2013 at 9:55 PM, Daniel Krügler
<daniel.kruegler@gmail.com> wrote:

>> Did you mean "pessimises code size", or something else?
>
> Yes.

Daniel's idea proved a good one, and I now have a patch that I am
happy with, and that will be easy to extend to string::at(), and other
__throw_... functions.

I've added the new snprintf.cc to c++11/ rather than c++98/ as Paolo
suggested, because the only current caller is in c++11/functexcept.cc

Thanks,
Paolo Carlini - Sept. 13, 2013, 10:02 a.m.
Hi,

On 09/13/2013 02:01 AM, Paul Pluzhnikov wrote:
> On Wed, Sep 4, 2013 at 9:55 PM, Daniel Krügler
> <daniel.kruegler@gmail.com> wrote:
>
>>> Did you mean "pessimises code size", or something else?
>> Yes.
> Daniel's idea proved a good one, and I now have a patch that I am
> happy with, and that will be easy to extend to string::at(), and other
> __throw_... functions.
>
> I've added the new snprintf.cc to c++11/ rather than c++98/ as Paolo
> suggested, because the only current caller is in c++11/functexcept.cc
Patch looks pretty good to me. Thanks for persisting. Some details:
- The game with the variadic and the non-variadic __throw_out_of_range 
makes me a little nervous. Let's just name the new one differently, like 
__throw_out_of_range_var.
- Please consistently use __builtin_alloca everywhere, alloca isn't a 
standard function.
- I would rather call the file itself snprintf_lite.cc, in order not to 
fool somebody that it actually implements the whole snprintf.
- I'm a bit confused about __concat_size_t returning -1. Since it only 
formats integers, I think we can be *sure* that the buffer is big 
enough. Then, if it returns -1 something is going *very badly* wrong, 
shouldn't we __builtin_abort() or something similar?
- In terms of buffer sizes, this comment:

     // enough for expanding up to 5 size_t's in the format.

and then the actual code in __snprintf_lite makes me a little nervous. 
Agreed, we are not going to overflow the buffer, but truncating with no 
diagnostic whatsoever seems rather gross. We can probably sort out this 
later, new ideas welcome, anyway.
- While we are at it, shouldn't we use the new facility at least in 
array, vector<bool> and deque too? For consistency over the containers.

Thanks,
Paolo.

Patch

Index: libstdc++-v3/src/c++11/Makefile.am
===================================================================
--- libstdc++-v3/src/c++11/Makefile.am	(revision 202545)
+++ libstdc++-v3/src/c++11/Makefile.am	(working copy)
@@ -42,6 +42,7 @@ 
 	random.cc \
 	regex.cc  \
 	shared_ptr.cc \
+	snprintf.cc \
 	system_error.cc \
 	thread.cc
 
Index: libstdc++-v3/src/c++11/functexcept.cc
===================================================================
--- libstdc++-v3/src/c++11/functexcept.cc	(revision 202545)
+++ libstdc++-v3/src/c++11/functexcept.cc	(working copy)
@@ -31,6 +31,7 @@ 
 #include <future>
 #include <functional>
 #include <bits/regex_error.h>
+#include <stdarg.h>
 
 #ifdef _GLIBCXX_USE_NLS
 # include <libintl.h>
@@ -39,6 +40,12 @@ 
 # define _(msgid)   (msgid)
 #endif
 
+namespace __gnu_cxx
+{
+  int __snprintf_lite(char *__buf, size_t __bufsize, const char *__fmt,
+		      va_list __ap);
+}
+
 namespace std _GLIBCXX_VISIBILITY(default)
 {
 _GLIBCXX_BEGIN_NAMESPACE_VERSION
@@ -75,11 +82,27 @@ 
   __throw_length_error(const char* __s __attribute__((unused)))
   { _GLIBCXX_THROW_OR_ABORT(length_error(_(__s))); }
 
+  // Left only to maintain backward-compatible ABI.
   void
   __throw_out_of_range(const char* __s __attribute__((unused)))
   { _GLIBCXX_THROW_OR_ABORT(out_of_range(_(__s))); }
 
   void
+  __throw_out_of_range(const char* __fmt, ...)
+  {
+    const size_t __len = __builtin_strlen(__fmt);
+    // enough for expanding up to 5 size_t's in the format.
+    const size_t __alloca_size = __len + 100;
+    char *const __s = static_cast<char*>(alloca(__alloca_size));
+    va_list __ap;
+
+    va_start(__ap, __fmt);
+    __gnu_cxx::__snprintf_lite(__s, __alloca_size, __fmt, __ap);
+    _GLIBCXX_THROW_OR_ABORT(out_of_range(_(__s)));
+    va_end(__ap);  // Not reached.
+  }
+
+  void
   __throw_runtime_error(const char* __s __attribute__((unused)))
   { _GLIBCXX_THROW_OR_ABORT(runtime_error(_(__s))); }
 
Index: libstdc++-v3/src/c++11/snprintf.cc
===================================================================
--- libstdc++-v3/src/c++11/snprintf.cc	(revision 0)
+++ libstdc++-v3/src/c++11/snprintf.cc	(revision 0)
@@ -0,0 +1,103 @@ 
+// Debugging support -*- C++ -*-
+
+// Copyright (C) 2013 Free Software Foundation, Inc.
+//
+// This file is part of GCC.
+//
+// GCC 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.
+//
+// GCC 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.
+//
+// Under Section 7 of GPL version 3, you are granted additional
+// permissions described in the GCC Runtime Library Exception, version
+// 3.1, as published by the Free Software Foundation.
+
+// You should have received a copy of the GNU General Public License and
+// a copy of the GCC Runtime Library Exception along with this program;
+// see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
+// <http://www.gnu.org/licenses/>.
+
+#include <stdarg.h>
+#include <bits/locale_facets.h>
+
+namespace std {
+  template<typename _CharT, typename _ValueT>
+  int
+  __int_to_char(_CharT* __bufend, _ValueT __v, const _CharT* __lit,
+                ios_base::fmtflags __flags, bool __dec);
+}
+
+namespace __gnu_cxx {
+
+  // Private routine to append decimal representation of VAL to the given
+  // BUFFER, but not more than BUFSIZE characters.
+  // Does not NUL-terminate the output buffer.
+  // Returns number of characters appended, or -1 if BUFSIZE is too small.
+  int __concat_size_t(char *__buf, size_t __bufsize, size_t __val)
+  {
+    // Long enough for decimal representation.
+    int __ilen = 3 * sizeof(__val);
+    char *__cs = static_cast<char*>(__builtin_alloca(__ilen));
+    size_t __len = std::__int_to_char(__cs + __ilen, __val,
+				      std::__num_base::_S_atoms_out,
+				      std::ios_base::dec, true);
+    if (__bufsize < __len)
+      return -1;
+
+    __builtin_memcpy(__buf, __cs + __ilen - __len, __len);
+    return __len;
+  }
+
+  // Private routine to print into __buf arguments according to format,
+  // not to exceed __bufsize.
+  // Only '%%' and '%zu' format specifiers are understood.
+  // Returns number of characters printed (excluding terminating NUL).
+  // Always NUL-terminates __buf.
+  int __snprintf_lite(char *__buf, size_t __bufsize, const char *__fmt,
+		      va_list __ap)
+  {
+    char *__d = __buf;
+    const char *__s = __fmt;
+    const char *const __limit = __d + __bufsize - 1;  // Leave space for NUL.
+
+    while (*__s != '\0' && __d < __limit)
+      {
+	if (*__s == '%')
+	  switch (__s[1])
+	    {
+	    default:  // Stray '%'. Just print it.
+	      break;
+	    case '%':  // '%%'
+	      __s += 1;
+	      break;
+	    case 'z':
+	      if (__s[2] == 'u')  // '%zu' -- expand next size_t arg.
+		{
+		  const int __len = __concat_size_t(__d, __limit - __d,
+						    va_arg(__ap, size_t));
+		  if (__len > 0)
+		    __d += __len;
+		  else
+		    goto __out;
+
+		  __s += 3;
+		  continue;
+		}
+	      // Stray '%zX'. Just print it.
+	      break;
+	    }
+	*__d++ = *__s++;
+      }
+
+  __out:
+    *__d = '\0';
+    return __d - __buf;
+  }
+
+}  // __gnu_cxx
Index: libstdc++-v3/include/bits/functexcept.h
===================================================================
--- libstdc++-v3/include/bits/functexcept.h	(revision 202545)
+++ libstdc++-v3/include/bits/functexcept.h	(working copy)
@@ -72,7 +72,7 @@ 
   __throw_length_error(const char*) __attribute__((__noreturn__));
 
   void
-  __throw_out_of_range(const char*) __attribute__((__noreturn__));
+  __throw_out_of_range(const char*, ...) __attribute__((__noreturn__));
 
   void
   __throw_runtime_error(const char*) __attribute__((__noreturn__));
Index: libstdc++-v3/include/bits/stl_vector.h
===================================================================
--- libstdc++-v3/include/bits/stl_vector.h	(revision 202545)
+++ libstdc++-v3/include/bits/stl_vector.h	(working copy)
@@ -791,7 +791,9 @@ 
       _M_range_check(size_type __n) const
       {
 	if (__n >= this->size())
-	  __throw_out_of_range(__N("vector::_M_range_check"));
+	  __throw_out_of_range(__N("vector::_M_range_check: __n (which is %zu) "
+				   ">= this->size() (which is %zu)"),
+			       __n, this->size());
       }
 
     public:
Index: libstdc++-v3/testsuite/23_containers/vector/requirements/dr438/assign_neg.cc
===================================================================
--- libstdc++-v3/testsuite/23_containers/vector/requirements/dr438/assign_neg.cc	(revision 202545)
+++ libstdc++-v3/testsuite/23_containers/vector/requirements/dr438/assign_neg.cc	(working copy)
@@ -18,7 +18,7 @@ 
 // <http://www.gnu.org/licenses/>.
 
 // { dg-do compile }
-// { dg-error "no matching" "" { target *-*-* } 1308 }
+// { dg-error "no matching" "" { target *-*-* } 1310 }
 
 #include <vector>
 
Index: libstdc++-v3/testsuite/23_containers/vector/requirements/dr438/insert_neg.cc
===================================================================
--- libstdc++-v3/testsuite/23_containers/vector/requirements/dr438/insert_neg.cc	(revision 202545)
+++ libstdc++-v3/testsuite/23_containers/vector/requirements/dr438/insert_neg.cc	(working copy)
@@ -18,7 +18,7 @@ 
 // <http://www.gnu.org/licenses/>.
 
 // { dg-do compile }
-// { dg-error "no matching" "" { target *-*-* } 1349 }
+// { dg-error "no matching" "" { target *-*-* } 1351 }
 
 #include <vector>
 
Index: libstdc++-v3/testsuite/23_containers/vector/requirements/dr438/constructor_1_neg.cc
===================================================================
--- libstdc++-v3/testsuite/23_containers/vector/requirements/dr438/constructor_1_neg.cc	(revision 202545)
+++ libstdc++-v3/testsuite/23_containers/vector/requirements/dr438/constructor_1_neg.cc	(working copy)
@@ -18,7 +18,7 @@ 
 // <http://www.gnu.org/licenses/>.
 
 // { dg-do compile }
-// { dg-error "no matching" "" { target *-*-* } 1234 }
+// { dg-error "no matching" "" { target *-*-* } 1236 }
 
 #include <vector>
 
Index: libstdc++-v3/testsuite/23_containers/vector/requirements/dr438/constructor_2_neg.cc
===================================================================
--- libstdc++-v3/testsuite/23_containers/vector/requirements/dr438/constructor_2_neg.cc	(revision 202545)
+++ libstdc++-v3/testsuite/23_containers/vector/requirements/dr438/constructor_2_neg.cc	(working copy)
@@ -18,7 +18,7 @@ 
 // <http://www.gnu.org/licenses/>.
 
 // { dg-do compile }
-// { dg-error "no matching" "" { target *-*-* } 1234 }
+// { dg-error "no matching" "" { target *-*-* } 1236 }
 
 #include <vector>
 #include <utility>
Index: libstdc++-v3/config/abi/pre/gnu.ver
===================================================================
--- libstdc++-v3/config/abi/pre/gnu.ver	(revision 202545)
+++ libstdc++-v3/config/abi/pre/gnu.ver	(working copy)
@@ -1365,6 +1365,9 @@ 
     # std::get_unexpected()
     _ZSt14get_unexpectedv;
 
+    # std::__throw_out_of_range(char const*, ...)
+    _ZSt20__throw_out_of_rangePKcz;
+
 } GLIBCXX_3.4.19;
 
 # Symbols in the support library (libsupc++) have their own tag.
Index: libstdc++-v3/src/c++11/Makefile.in
===================================================================
--- libstdc++-v3/src/c++11/Makefile.in	(revision 202545)
+++ libstdc++-v3/src/c++11/Makefile.in	(working copy)
@@ -70,7 +70,8 @@ 
 am__objects_1 = chrono.lo condition_variable.lo debug.lo \
 	functexcept.lo functional.lo future.lo hash_c++0x.lo \
 	hashtable_c++0x.lo limits.lo mutex.lo placeholders.lo \
-	random.lo regex.lo shared_ptr.lo system_error.lo thread.lo
+	random.lo regex.lo shared_ptr.lo snprintf.lo system_error.lo \
+	thread.lo
 @ENABLE_EXTERN_TEMPLATE_TRUE@am__objects_2 = fstream-inst.lo \
 @ENABLE_EXTERN_TEMPLATE_TRUE@	string-inst.lo wstring-inst.lo
 am_libc__11convenience_la_OBJECTS = $(am__objects_1) $(am__objects_2)
@@ -324,6 +325,7 @@ 
 	random.cc \
 	regex.cc  \
 	shared_ptr.cc \
+	snprintf.cc \
 	system_error.cc \
 	thread.cc