[patch] Make vector::at() assertion message more useful (try #2)

[patch] Make vector::at() assertion message more useful (try #2)

[Paul Pluzhnikov]

Greetings,

This is a followup to:
http://gcc.gnu.org/ml/libstdc++/2013-08/msg00096.html

Without this patch, the user on vector::at out of bounds sees:

terminate called after throwing an instance of 'std::out_of_range'
  what():  vector::_M_range_check
Aborted (core dumped)

With the patch:

terminate called after throwing an instance of 'std::out_of_range'
  what():  vector::_M_range_check: __n (which is 3) >= this->size() (which is 2)
Aborted (core dumped)


I am not at all sure the names I choose here are good ones. Suggestions
welcome.

I also shudder at the idea of repeating _M_range_check code in
e.g. string::at(), and elsewhere. Perhaps we need a snprintf_lite, that
only understands '%zu' and literal characters, e.g.:

  snprintf_lite(__s, sizeof(__s),
                _N("vector::_M_range_check: __n (which is %zu) >= "
                   "this->size() (which is %zu)"), __n, this->size());


[The patch also doesn't include libstdc++-v3/libsupc++/Makefile.in,
which I'll regenerate before submitting.]

[Please CC me on any replies.]

--
Paul Pluzhnikov

2013-09-04  Paul Pluzhnikov  <ppluzhnikov@google.com>

	* libstdc++-v3/config/abi/pre/gnu.ver: Add
        _ZN9__gnu_cxx13concat_size_tEPcm
	* libstdc++-v3/include/bits/stl_vector.h (_M_range_check): Print
        additional assertion details
	* libstdc++-v3/libsupc++/Makefile.am: Add support.cc
	* libstdc++-v3/libsupc++/support.cc: New
        * libstdc++-v3/testsuite/23_containers/vector/requirements/dr438/assign_neg.cc: 
	Adjust.
	* testsuite/23_containers/vector/requirements/dr438/insert_neg.cc:
	Likewise.
	* testsuite/23_containers/vector/requirements/dr438/constructor_1_neg.cc:
	Likewise.
	* testsuite/23_containers/vector/requirements/dr438/constructor_2_neg.cc:
	Likewise.


Index: libstdc++-v3/config/abi/pre/gnu.ver
===================================================================
--- libstdc++-v3/config/abi/pre/gnu.ver	(revision 202262)
+++ libstdc++-v3/config/abi/pre/gnu.ver	(working copy)
@@ -1365,6 +1365,9 @@
     # std::get_unexpected()
     _ZSt14get_unexpectedv;
 
+    # __gnu_cxx::concat_size_t(char*, unsigned long)
+    _ZN9__gnu_cxx13concat_size_tEPcm;
+
 } GLIBCXX_3.4.19;
 
 # Symbols in the support library (libsupc++) have their own tag.
Index: libstdc++-v3/include/bits/stl_vector.h
===================================================================
--- libstdc++-v3/include/bits/stl_vector.h	(revision 202262)
+++ libstdc++-v3/include/bits/stl_vector.h	(working copy)
@@ -63,6 +63,10 @@
 #include <initializer_list>
 #endif
 
+namespace __gnu_cxx {
+  int concat_size_t(char *, size_t);
+}
+
 namespace std _GLIBCXX_VISIBILITY(default)
 {
 _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
@@ -791,7 +795,26 @@
       _M_range_check(size_type __n) const
       {
 	if (__n >= this->size())
-	  __throw_out_of_range(__N("vector::_M_range_check"));
+	  {
+	    const char __p[] = __N("vector::_M_range_check: __n (which is ");
+	    const char __q[] = __N(") >= this->size() (which is ");
+
+	    // Enough space for __p, __q, size and __n (in decimal).
+	    const int __alloc_size =
+	      sizeof(__p) + sizeof(__q) + 3 * 2 * sizeof(size_type) + 10;
+
+	    char *__s = static_cast<char*>(__builtin_alloca(__alloc_size));
+	    char *__ps = __s;
+	    __builtin_memcpy(__ps, __p, sizeof(__p));
+	    __ps += sizeof(__p) - 1;
+	    __ps += __gnu_cxx::concat_size_t(__ps, __n);
+	    __builtin_memcpy(__ps, __q, sizeof(__q));
+	    __ps += sizeof(__q) - 1;
+	    __ps += __gnu_cxx::concat_size_t(__ps, this->size());
+	    *(__ps++) = __N(')');
+	    *(__ps++) = '\0';
+	    __throw_out_of_range(__s);
+	  }
       }
 
     public:
Index: libstdc++-v3/libsupc++/Makefile.am
===================================================================
--- libstdc++-v3/libsupc++/Makefile.am	(revision 202262)
+++ libstdc++-v3/libsupc++/Makefile.am	(working copy)
@@ -91,6 +91,7 @@
 	pointer_type_info.cc \
 	pure.cc \
 	si_class_type_info.cc \
+	support.cc \
 	tinfo.cc \
 	tinfo2.cc \
 	vec.cc \
Index: libstdc++-v3/libsupc++/support.cc
===================================================================
--- libstdc++-v3/libsupc++/support.cc	(revision 0)
+++ libstdc++-v3/libsupc++/support.cc	(revision 0)
@@ -0,0 +1,53 @@
+// 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 <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 {
+
+// Exported routine to append decimal representation of __val to the given
+// buffer, which must be at least 21 characters long in 64-bit mode.
+// Does not NUL-terminate the output buffer.
+// Returns number of characters appended.
+  int concat_size_t(char *__buf, size_t __val)
+  {
+    // Long enough for decimal representation.
+    int __ilen = 3 * sizeof(__val);
+    char *__cs = static_cast<char*>(__builtin_alloca(__ilen));
+    int __len = std::__int_to_char(__cs + __ilen, __val,
+				   std::__num_base::_S_atoms_out,
+				   std::ios_base::dec, true);
+    __builtin_memcpy(__buf, __cs + __ilen - __len, __len);
+    return __len;
+  }
+
+}  // __gnu_cxx
Index: libstdc++-v3/testsuite/23_containers/vector/requirements/dr438/assign_neg.cc
===================================================================
--- libstdc++-v3/testsuite/23_containers/vector/requirements/dr438/assign_neg.cc	(revision 202262)
+++ 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 *-*-* } 1331 }
 
 #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 202262)
+++ 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 *-*-* } 1372 }
 
 #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 202262)
+++ 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 *-*-* } 1257 }
 
 #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 202262)
+++ 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 *-*-* } 1257 }
 
 #include <vector>
 #include <utility>

Re: [patch] Make vector::at() assertion message more useful (try #2)

[Daniel Krügler]

2013/9/4 Paul Pluzhnikov <ppluzhnikov@google.com>:
> Greetings,
>
> This is a followup to:
> http://gcc.gnu.org/ml/libstdc++/2013-08/msg00096.html
>
> Without this patch, the user on vector::at out of bounds sees:
>
> terminate called after throwing an instance of 'std::out_of_range'
>   what():  vector::_M_range_check
> Aborted (core dumped)
>
> With the patch:
>
> terminate called after throwing an instance of 'std::out_of_range'
>   what():  vector::_M_range_check: __n (which is 3) >= this->size() (which is 2)
> Aborted (core dumped)
>
>
> I am not at all sure the names I choose here are good ones. Suggestions
> welcome.
>
> I also shudder at the idea of repeating _M_range_check code in
> e.g. string::at(), and elsewhere. Perhaps we need a snprintf_lite, that
> only understands '%zu' and literal characters, e.g.:

I expect that this kind of index violation is a rather often occurring
pattern and should be isolated. IMO the _M_range
_check now pessimisms the normal, non-violating case. Why not simply
replacing it by something along the lines of

 _M_range_check(size_type __n) const
       {
        if (__n >= this->size())
         __throw_out_of_range(__index_out_of_range_msg(__N("vector::_M_range_check"),
__n, this->size()));
       }

where __index_out_of_range_msg is a reusable function that uses
something like snprintf_lite internally?

- Daniel

Re: [patch] Make vector::at() assertion message more useful (try #2)

[Paul Pluzhnikov]

On Wed, Sep 4, 2013 at 2:10 PM, Daniel Krügler
<daniel.kruegler@gmail.com> wrote:

> I expect that this kind of index violation is a rather often occurring
> pattern and should be isolated. IMO the _M_range
> _check now pessimisms the normal, non-violating case.

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


> Why not simply
> replacing it by something along the lines of
>
>  _M_range_check(size_type __n) const
>        {
>         if (__n >= this->size())
>          __throw_out_of_range(__index_out_of_range_msg(__N("vector::_M_range_check"),
>                                                        __n, this->size()));
>        }
>
> where __index_out_of_range_msg is a reusable function that uses
> something like snprintf_lite internally?

Some disadvantages to doing that:
1. The actual message is now "magically" transformed inside
   __index_out_of_range_msg into the final message, making translation
   more difficult.
2. __index_out_of_range_msg() would have to return a string, which is heavier
   weight (in try#1 I just used snprintf, which was considered "too heavy").

Thanks,
-- 
Paul Pluzhnikov

Re: [patch] Make vector::at() assertion message more useful (try #2)

[Paolo Carlini]

Hi,

On 09/04/2013 10:53 PM, Paul Pluzhnikov wrote:
> I am not at all sure the names I choose here are good ones. Suggestions
> welcome.
For sure concat_size would not be Ok, isn't uglified. Thanks for the 
code, you understand isn't really something we can imagine committing.
> I also shudder at the idea of repeating _M_range_check code in
> e.g. string::at(), and elsewhere. Perhaps we need a snprintf_lite, that
> only understands '%zu' and literal characters, e.g.:
>
>    snprintf_lite(__s, sizeof(__s),
>                  _N("vector::_M_range_check: __n (which is %zu) >= "
>                     "this->size() (which is %zu)"), __n, this->size());
That seems worth exploring, I agree.

Paolo.

Re: [patch] Make vector::at() assertion message more useful (try #2)

[Paul Pluzhnikov]

On Wed, Sep 4, 2013 at 4:26 PM, Paolo Carlini <paolo.carlini@oracle.com> wrote:

> For sure concat_size would not be Ok, isn't uglified.

I didn't uglify it because it's inside __gnu_cxx namespace.
Does it still need uglification?

>>    snprintf_lite(__s, sizeof(__s),
>>                  _N("vector::_M_range_check: __n (which is %zu) >= "
>>                     "this->size() (which is %zu)"), __n, this->size());
>
> That seems worth exploring, I agree.

Should snprintf_lite be in __gnu_cxx namespace, or be global and be called
__snprintf_lite(), or ...?

Is the location of the out-of-line code in libstdc++-v3/libsupc++/ ok?
(Would probably be called snprintf_lite.cc or some such.)

Is the version I've assigned to the symbol -- GLIBCXX_3.4.20 -- ok?

Thanks,
-- 
Paul Pluzhnikov

Re: [patch] Make vector::at() assertion message more useful (try #2)

[Paolo Carlini]

Hi,

On 09/05/2013 01:36 AM, Paul Pluzhnikov wrote:
> On Wed, Sep 4, 2013 at 4:26 PM, Paolo Carlini <paolo.carlini@oracle.com> wrote:
>
>> For sure concat_size would not be Ok, isn't uglified.
> I didn't uglify it because it's inside __gnu_cxx namespace.
> Does it still need uglification?
Yes.
>
>>>     snprintf_lite(__s, sizeof(__s),
>>>                   _N("vector::_M_range_check: __n (which is %zu) >= "
>>>                      "this->size() (which is %zu)"), __n, this->size());
>> That seems worth exploring, I agree.
> Should snprintf_lite be in __gnu_cxx namespace, or be global and be called
> __snprintf_lite(), or ...?
In any case it needs the __ in front. Like the rest of the library, to 
protect vs

#define snprintf_lite 1

in user code. Well known issue...
> Is the location of the out-of-line code in libstdc++-v3/libsupc++/ ok?
> (Would probably be called snprintf_lite.cc or some such.)
I don't think we want to fiddle with libsupc++, for the time being at 
least. src/c++98 seem ok to me.
> Is the version I've assigned to the symbol -- GLIBCXX_3.4.20 -- ok?
Is a release out with 3.4.20? If not, it's fine. I think it's fine.

Paolo.

Re: [patch] Make vector::at() assertion message more useful (try #2)

[Daniel Krügler]

2013/9/5 Paul Pluzhnikov <ppluzhnikov@google.com>:
> On Wed, Sep 4, 2013 at 2:10 PM, Daniel Krügler
> <daniel.kruegler@gmail.com> wrote:
>
>> I expect that this kind of index violation is a rather often occurring
>> pattern and should be isolated. IMO the _M_range
>> _check now pessimisms the normal, non-violating case.
>
> Did you mean "pessimises code size", or something else?

Yes.

- Daniel

Re: [patch] Make vector::at() assertion message more useful (try #2)

[Paul Pluzhnikov]

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

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,
-- 
Paul Pluzhnikov

libstdc++-v3/ChangeLog:

2013-09-12  Paul Pluzhnikov  <ppluzhnikov@google.com>

	* src/c++11/Makefile.am: Add snprintf.cc
	* src/c++11/Makefile.in: Regenerate.
	* include/bits/functexcept.h (__throw_out_of_range): Adjust.
	* src/c++11/functexcept.cc (__throw_out_of_range): New overload.
	* src/c++11/snprintf.cc: New.
	* config/abi/pre/gnu.ver: Add _ZSt20__throw_out_of_rangePKcz.
	* include/bits/stl_vector.h (_M_range_check): Print
	additional assertion details.
	* testsuite/23_containers/vector/requirements/dr438/assign_neg.cc:
	Adjust.
	* testsuite/23_containers/vector/requirements/dr438/insert_neg.cc:
	Likewise.
	* testsuite/23_containers/vector/requirements/dr438/constructor_1_neg.cc:
	Likewise.
	* testsuite/23_containers/vector/requirements/dr438/constructor_2_neg.cc:
	Likewise.

[-- Attachment #2: gcc-20130912.txt --]
[-- Type: text/plain, Size: 10207 bytes --]

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
 

Re: [patch] Make vector::at() assertion message more useful (try #2)

[Paolo Carlini]

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.

Re: [patch] Make vector::at() assertion message more useful (try #2)

[Paul Pluzhnikov]

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

On Fri, Sep 13, 2013 at 3:02 AM, Paolo Carlini <paolo.carlini@oracle.com> wrote:

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

Replaced with __throw_out_of_range_fmt.

> - Please consistently use __builtin_alloca everywhere, alloca isn't a
> standard function.

Done.

> - I would rather call the file itself snprintf_lite.cc, in order not to fool
> somebody that it actually implements the whole snprintf.

Done.

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

How so? The caller could do this:

  __snprintf_lite("aaa: %zu, 8, 12345678);

By the time we get to __concat_size_t, we only have 3 characters left.

> Then, if it returns -1 something is going *very badly* wrong, shouldn't we
> __builtin_abort() or something similar?

I've re-worked this part -- if this ever happens, throw a logic_error with
a message to file a bug.

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

Re-worked.

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

Done.

I also expanded snprintf_lite to handle '%s', as that comes handy inside
string _M_check.

Thanks,

P.S. In the process of updating callers of __throw_out_of_range, I've
discovered that two of them had already used __builtin_sprintf.

P.P.S. Sorry this patch grew ... I can split it into parts if that's easier
to review.


-- 
Paul Pluzhnikov


libstdc++-v3/ChangeLog:

	* include/bits/functexcept.h (__throw_out_of_range_fmt): New.
	* src/c++11/functexcept.cc (__throw_out_of_range_fmt): New.
	* src/c++11/snprintf_lite.cc: New.
	* src/c++11/Makefile.am: Add snprintf_lite.cc.
	* src/c++11/Makefile.in: Regenerate.
	* config/abi/pre/gnu.ver: Add _ZSt24__throw_out_of_range_fmtPKcz.
	* include/std/array (at): Use __throw_out_of_range_fmt.
	* include/debug/array (at): Likewise.
	* include/profile/array (at): Likewise.
	* include/std/bitset (_M_check_initial_position, _M_check): New.
	(bitset::bitset): Use _M_check_initial_position.
	(set, reset, flip, test): Use _M_check.
	* include/ext/vstring.h (_M_check, at): Use __throw_out_of_range_fmt.
	* include/bits/stl_vector.h (_M_range_check): Likewise.
	* include/bits/stl_bvector.h (_M_range_check): Likewise.
	* include/bits/stl_deque.h (_M_range_check): Likewise.
	* include/bits/basic_string.h (_M_check, at): Likewise.
	* testsuite/23_containers/vector/requirements/dr438/assign_neg.cc: Adjust.
	* testsuite/23_containers/vector/requirements/dr438/insert_neg.cc: Likewise.
	* testsuite/23_containers/vector/requirements/dr438/constructor_1_neg.cc:
Likewise.
	* testsuite/23_containers/vector/requirements/dr438/constructor_2_neg.cc:
Likewise.
	* testsuite/23_containers/deque/requirements/dr438/assign_neg.cc: Likewise.
	* testsuite/23_containers/deque/requirements/dr438/insert_neg.cc: Likewise.
	* testsuite/23_containers/deque/requirements/dr438/constructor_1_neg.cc:
Likewise.
	* testsuite/23_containers/deque/requirements/dr438/constructor_2_neg.cc:
Likewise.
	* testsuite/23_containers/array/tuple_interface/tuple_element_neg.cc: Likewise.
	* testsuite/23_containers/array/tuple_interface/get_neg.cc: Likewise.
	* testsuite/util/exception/safety.h (generate): Use __throw_out_of_range_fmt.

[-- Attachment #2: gcc-20130918.txt --]
[-- Type: text/plain, Size: 26327 bytes --]

Index: libstdc++-v3/include/bits/functexcept.h
===================================================================
--- libstdc++-v3/include/bits/functexcept.h	(revision 202709)
+++ libstdc++-v3/include/bits/functexcept.h	(working copy)
@@ -75,6 +75,10 @@
   __throw_out_of_range(const char*) __attribute__((__noreturn__));
 
   void
+  __throw_out_of_range_fmt(const char*, ...) __attribute__((__noreturn__))
+    __attribute__((__format__(__printf__, 1, 2)));
+
+  void
   __throw_runtime_error(const char*) __attribute__((__noreturn__));
 
   void
Index: libstdc++-v3/src/c++11/functexcept.cc
===================================================================
--- libstdc++-v3/src/c++11/functexcept.cc	(revision 202709)
+++ 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
@@ -80,6 +87,22 @@
   { _GLIBCXX_THROW_OR_ABORT(out_of_range(_(__s))); }
 
   void
+  __throw_out_of_range_fmt(const char* __fmt, ...)
+  {
+    const size_t __len = __builtin_strlen(__fmt);
+    // We expect at most 2 numbers, and 1 short string. The additional
+    // 512 bytes should provide more than enough space for expansion.
+    const size_t __alloca_size = __len + 512;
+    char *const __s = static_cast<char*>(__builtin_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_lite.cc
===================================================================
--- libstdc++-v3/src/c++11/snprintf_lite.cc	(revision 0)
+++ libstdc++-v3/src/c++11/snprintf_lite.cc	(revision 0)
@@ -0,0 +1,152 @@
+// 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/functexcept.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 helper to throw logic error if snprintf_lite runs out
+  // of space (which is not expected to ever happen).
+  // NUL-terminates __buf.
+  void
+  __throw_insufficient_space(const char *__buf, const char *__bufend)
+    __attribute__((__noreturn__));
+
+  void
+  __throw_insufficient_space(const char *__buf, const char *__bufend)
+  {
+    // Include space for trailing NUL.
+    const size_t __len = __bufend - __buf + 1;
+
+    const char __err[] = "not enough space for format expansion "
+      "(Please submit full bug report at http://gcc.gnu.org/bugs.html):\n    ";
+    const size_t __errlen = sizeof(__err) - 1;
+
+    char *const __e
+      = static_cast<char*>(__builtin_alloca(__errlen + __len));
+
+    __builtin_memcpy(__e, __err, __errlen);
+    __builtin_memcpy(__e + __errlen, __buf, __len - 1);
+    __e[__errlen + __len - 1] = '\0';
+    std::__throw_logic_error(__e);
+  }
+
+
+  // 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 '%%', '%s' and '%zu' format specifiers are understood.
+  // Returns number of characters printed (excluding terminating NUL).
+  // Always NUL-terminates __buf.
+  // Throws logic_error on insufficient space.
+  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] != '\0' && __d < __limit)
+      {
+	if (__s[0] == '%')
+	  switch (__s[1])
+	    {
+	    default:  // Stray '%'. Just print it.
+	      break;
+	    case '%':  // '%%'
+	      __s += 1;
+	      break;
+	    case 's':  // '%s'.
+	      {
+		const char *__v = va_arg(__ap, const char *);
+
+		while (__v[0] != '\0' && __d < __limit)
+		  *__d++ = *__v++;
+
+		if (__v[0] != '\0')
+		  // Not enough space for __fmt expansion.
+		  __throw_insufficient_space(__buf, __d);
+
+		__s += 2;  // Step over %s.
+		continue;
+	      }
+	      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
+		    // Not enough space for __fmt expansion.
+		    __throw_insufficient_space(__buf, __d);
+
+		  __s += 3;  // Step over %zu
+		  continue;
+		}
+	      // Stray '%zX'. Just print it.
+	      break;
+	    }
+	*__d++ = *__s++;
+      }
+
+    if (__s[0] != '\0')
+      // Not enough space for __fmt expansion.
+      __throw_insufficient_space(__buf, __d);
+
+    *__d = '\0';
+    return __d - __buf;
+  }
+
+}  // __gnu_cxx
Index: libstdc++-v3/src/c++11/Makefile.am
===================================================================
--- libstdc++-v3/src/c++11/Makefile.am	(revision 202709)
+++ libstdc++-v3/src/c++11/Makefile.am	(working copy)
@@ -42,6 +42,7 @@
 	random.cc \
 	regex.cc  \
 	shared_ptr.cc \
+	snprintf_lite.cc \
 	system_error.cc \
 	thread.cc
 
Index: libstdc++-v3/src/c++11/Makefile.in
===================================================================
--- libstdc++-v3/src/c++11/Makefile.in	(revision 202709)
+++ 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_lite.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_lite.cc \
 	system_error.cc \
 	thread.cc
 
Index: libstdc++-v3/config/abi/pre/gnu.ver
===================================================================
--- libstdc++-v3/config/abi/pre/gnu.ver	(revision 202709)
+++ libstdc++-v3/config/abi/pre/gnu.ver	(working copy)
@@ -1365,6 +1365,9 @@
     # std::get_unexpected()
     _ZSt14get_unexpectedv;
 
+    # std::__throw_out_of_range_fmt(char const*, ...)
+    _ZSt24__throw_out_of_range_fmtPKcz;
+
 } GLIBCXX_3.4.19;
 
 # Symbols in the support library (libsupc++) have their own tag.
Index: libstdc++-v3/include/std/array
===================================================================
--- libstdc++-v3/include/std/array	(revision 202709)
+++ libstdc++-v3/include/std/array	(working copy)
@@ -180,7 +180,9 @@
       at(size_type __n)
       {
 	if (__n >= _Nm)
-	  std::__throw_out_of_range(__N("array::at"));
+	  std::__throw_out_of_range_fmt(__N("array::at: __n (which is %zu) "
+					    ">= _Nm (which is %zu)"),
+					__n, _Nm);
 	return _AT_Type::_S_ref(_M_elems, __n);
       }
 
@@ -190,7 +192,9 @@
 	// Result of conditional expression must be an lvalue so use
 	// boolean ? lvalue : (throw-expr, lvalue)
 	return __n < _Nm ? _AT_Type::_S_ref(_M_elems, __n)
-	  : (std::__throw_out_of_range(__N("array::at")),
+	  : (std::__throw_out_of_range_fmt(__N("array::at: __n (which is %zu) "
+					       ">= _Nm (which is %zu)"),
+					   __n, _Nm),
 	     _AT_Type::_S_ref(_M_elems, 0));
       }
 
Index: libstdc++-v3/include/debug/array
===================================================================
--- libstdc++-v3/include/debug/array	(revision 202709)
+++ libstdc++-v3/include/debug/array	(working copy)
@@ -165,7 +165,10 @@
       at(size_type __n)
       {
 	if (__n >= _Nm)
-	  std::__throw_out_of_range(__N("array::at"));
+	  std::__throw_out_of_range_fmt(__N("array::at: __n "
+				            "(which is %zu) >= _Nm "
+					    "(which is %zu)"),
+					__n, _Nm);
 	return _AT_Type::_S_ref(_M_elems, __n);
       }
 
@@ -175,7 +178,9 @@
 	// Result of conditional expression must be an lvalue so use
 	// boolean ? lvalue : (throw-expr, lvalue)
 	return __n < _Nm ? _AT_Type::_S_ref(_M_elems, __n)
-	  : (std::__throw_out_of_range(__N("array::at")),
+	  : (std::__throw_out_of_range_fmt(__N("array::at: __n (which is %zu) "
+					       ">= _Nm (which is %zu)"),
+					   __n, _Nm),
 	     _AT_Type::_S_ref(_M_elems, 0));
       }
 
Index: libstdc++-v3/include/profile/array
===================================================================
--- libstdc++-v3/include/profile/array	(revision 202709)
+++ libstdc++-v3/include/profile/array	(working copy)
@@ -138,7 +138,10 @@
       at(size_type __n)
       {
 	if (__n >= _Nm)
-	  std::__throw_out_of_range(__N("array::at"));
+	  std::__throw_out_of_range_fmt(__N("array::at: __n "
+				            "(which is %zu) >= _Nm "
+					    "(which is %zu)"),
+					__n, _Nm);
 	return _AT_Type::_S_ref(_M_elems, __n);
       }
 
@@ -148,7 +151,9 @@
 	// Result of conditional expression must be an lvalue so use
 	// boolean ? lvalue : (throw-expr, lvalue)
 	return __n < _Nm ? _AT_Type::_S_ref(_M_elems, __n)
-	  : (std::__throw_out_of_range(__N("array::at")),
+	  : (std::__throw_out_of_range_fmt(__N("array::at: __n (which is %zu) "
+					       ">= _Nm (which is %zu)"),
+					   __n, _Nm),
 	     _AT_Type::_S_ref(_M_elems, 0));
       }
 
Index: libstdc++-v3/include/std/bitset
===================================================================
--- libstdc++-v3/include/std/bitset	(revision 202709)
+++ libstdc++-v3/include/std/bitset	(working copy)
@@ -752,7 +752,27 @@
       typedef _Base_bitset<_GLIBCXX_BITSET_WORDS(_Nb)> _Base;
       typedef unsigned long _WordT;
 
+      template<class _CharT, class _Traits, class _Alloc>
       void
+      _M_check_initial_position(const std::basic_string<_CharT, _Traits, _Alloc>& __s,
+				size_t __position) const
+      {
+	if (__position > __s.size())
+	  __throw_out_of_range_fmt(__N("bitset::bitset: __position "
+				       "(which is %zu) > __s.size() "
+				       "(which is %zu)"),
+				   __position, __s.size());
+      }
+
+      void _M_check(size_t __position, const char *__s) const
+      {
+	if (__position >= _Nb)
+	  __throw_out_of_range_fmt(__N("%s: __position (which is %zu) "
+				       ">= _Nb (which is %zu)"),
+				   __s, __position, _Nb);
+      }
+
+      void
       _M_do_sanitize() _GLIBCXX_NOEXCEPT
       { 
 	typedef _Sanitize<_Nb % _GLIBCXX_BITSET_BITS_PER_WORD> __sanitize_type;
@@ -867,9 +887,7 @@
 	       size_t __position = 0)
 	: _Base()
 	{
-	  if (__position > __s.size())
-	    __throw_out_of_range(__N("bitset::bitset initial position "
-				     "not valid"));
+	  _M_check_initial_position(__s, __position);
 	  _M_copy_from_string(__s, __position,
 			      std::basic_string<_CharT, _Traits, _Alloc>::npos,
 			      _CharT('0'), _CharT('1'));
@@ -890,9 +908,7 @@
 	       size_t __position, size_t __n)
 	: _Base()
 	{
-	  if (__position > __s.size())
-	    __throw_out_of_range(__N("bitset::bitset initial position "
-				     "not valid"));
+	  _M_check_initial_position(__s, __position);
 	  _M_copy_from_string(__s, __position, __n, _CharT('0'), _CharT('1'));
 	}
 
@@ -904,9 +920,7 @@
 	       _CharT __zero, _CharT __one = _CharT('1'))
 	: _Base()
 	{
-	  if (__position > __s.size())
-	    __throw_out_of_range(__N("bitset::bitset initial position "
-				     "not valid"));
+	  _M_check_initial_position(__s, __position);
 	  _M_copy_from_string(__s, __position, __n, __zero, __one);
 	}
 
@@ -1067,8 +1081,7 @@
       bitset<_Nb>&
       set(size_t __position, bool __val = true)
       {
-	if (__position >= _Nb)
-	  __throw_out_of_range(__N("bitset::set"));
+	this->_M_check(__position, __N("bitset::set"));
 	return _Unchecked_set(__position, __val);
       }
 
@@ -1092,8 +1105,7 @@
       bitset<_Nb>&
       reset(size_t __position)
       {
-	if (__position >= _Nb)
-	  __throw_out_of_range(__N("bitset::reset"));
+	this->_M_check(__position, __N("bitset::reset"));
 	return _Unchecked_reset(__position);
       }
       
@@ -1116,8 +1128,7 @@
       bitset<_Nb>&
       flip(size_t __position)
       {
-	if (__position >= _Nb)
-	  __throw_out_of_range(__N("bitset::flip"));
+	this->_M_check(__position, __N("bitset::flip"));
 	return _Unchecked_flip(__position);
       }
       
@@ -1302,8 +1313,7 @@
       bool
       test(size_t __position) const
       {
-	if (__position >= _Nb)
-	  __throw_out_of_range(__N("bitset::test"));
+	this->_M_check(__position, __N("bitset::test"));
 	return _Unchecked_test(__position);
       }
 
Index: libstdc++-v3/include/ext/vstring.h
===================================================================
--- libstdc++-v3/include/ext/vstring.h	(revision 202709)
+++ libstdc++-v3/include/ext/vstring.h	(working copy)
@@ -85,7 +85,9 @@
       _M_check(size_type __pos, const char* __s) const
       {
 	if (__pos > this->size())
-	  std::__throw_out_of_range(__N(__s));
+	  std::__throw_out_of_range_fmt(__N("%s: __pos (which is %zu) > "
+					    "this->size() (which is %zu)"),
+					__s, __pos, this->size());
 	return __pos;
       }
 
@@ -581,7 +583,10 @@
       at(size_type __n) const
       {
 	if (__n >= this->size())
-	  std::__throw_out_of_range(__N("__versa_string::at"));
+	  std::__throw_out_of_range_fmt(__N("__versa_string::at: __n "
+					    "(which is %zu) >= this->size() "
+					    "(which is %zu)"),
+					__n, this->size());
 	return this->_M_data()[__n];
       }
 
@@ -600,7 +605,10 @@
       at(size_type __n)
       {
 	if (__n >= this->size())
-	  std::__throw_out_of_range(__N("__versa_string::at"));
+	  std::__throw_out_of_range_fmt(__N("__versa_string::at: __n "
+					    "(which is %zu) >= this->size() "
+					    "(which is %zu)"),
+					__n, this->size());
 	this->_M_leak();
 	return this->_M_data()[__n];
       }
Index: libstdc++-v3/include/bits/stl_deque.h
===================================================================
--- libstdc++-v3/include/bits/stl_deque.h	(revision 202709)
+++ libstdc++-v3/include/bits/stl_deque.h	(working copy)
@@ -1269,7 +1269,10 @@
       _M_range_check(size_type __n) const
       {
 	if (__n >= this->size())
-	  __throw_out_of_range(__N("deque::_M_range_check"));
+	  __throw_out_of_range_fmt(__N("deque::_M_range_check: __n "
+				       "(which is %zu)>= this->size() "
+				       "(which is %zu)"),
+				   __n, this->size());
       }
 
     public:
Index: libstdc++-v3/include/bits/stl_bvector.h
===================================================================
--- libstdc++-v3/include/bits/stl_bvector.h	(revision 202709)
+++ libstdc++-v3/include/bits/stl_bvector.h	(working copy)
@@ -785,7 +785,10 @@
     _M_range_check(size_type __n) const
     {
       if (__n >= this->size())
-        __throw_out_of_range(__N("vector<bool>::_M_range_check"));
+	__throw_out_of_range_fmt(__N("vector<bool>::_M_range_check: __n "
+				     "(which is %zu) >= this->size() "
+				     "(which is %zu)"),
+				 __n, this->size());
     }
 
   public:
Index: libstdc++-v3/include/bits/basic_string.h
===================================================================
--- libstdc++-v3/include/bits/basic_string.h	(revision 202709)
+++ libstdc++-v3/include/bits/basic_string.h	(working copy)
@@ -321,7 +321,9 @@
       _M_check(size_type __pos, const char* __s) const
       {
 	if (__pos > this->size())
-	  __throw_out_of_range(__N(__s));
+	  __throw_out_of_range_fmt(__N("%s: __pos (which is %zu) > "
+				       "this->size() (which is %zu)"),
+				   __s, __pos, this->size());
 	return __pos;
       }
 
@@ -865,7 +867,10 @@
       at(size_type __n) const
       {
 	if (__n >= this->size())
-	  __throw_out_of_range(__N("basic_string::at"));
+	  __throw_out_of_range_fmt(__N("basic_string::at: __n "
+				       "(which is %zu) >= this->size() "
+				       "(which is %zu)"),
+				   __n, this->size());
 	return _M_data()[__n];
       }
 
@@ -884,7 +889,10 @@
       at(size_type __n)
       {
 	if (__n >= size())
-	  __throw_out_of_range(__N("basic_string::at"));
+	  __throw_out_of_range_fmt(__N("basic_string::at: __n "
+				       "(which is %zu) >= this->size() "
+				       "(which is %zu)"),
+				   __n, this->size());
 	_M_leak();
 	return _M_data()[__n];
       }
Index: libstdc++-v3/include/bits/stl_vector.h
===================================================================
--- libstdc++-v3/include/bits/stl_vector.h	(revision 202709)
+++ libstdc++-v3/include/bits/stl_vector.h	(working copy)
@@ -785,7 +785,10 @@
       _M_range_check(size_type __n) const
       {
 	if (__n >= this->size())
-	  __throw_out_of_range(__N("vector::_M_range_check"));
+	  __throw_out_of_range_fmt(__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 202709)
+++ 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 *-*-* } 1302 }
+// { dg-error "no matching" "" { target *-*-* } 1305 }
 
 #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 202709)
+++ 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 *-*-* } 1343 }
+// { dg-error "no matching" "" { target *-*-* } 1346 }
 
 #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 202709)
+++ 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 *-*-* } 1228 }
+// { dg-error "no matching" "" { target *-*-* } 1231 }
 
 #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 202709)
+++ 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 *-*-* } 1228 }
+// { dg-error "no matching" "" { target *-*-* } 1231 }
 
 #include <vector>
 #include <utility>
Index: libstdc++-v3/testsuite/23_containers/deque/requirements/dr438/assign_neg.cc
===================================================================
--- libstdc++-v3/testsuite/23_containers/deque/requirements/dr438/assign_neg.cc	(revision 202709)
+++ libstdc++-v3/testsuite/23_containers/deque/requirements/dr438/assign_neg.cc	(working copy)
@@ -18,7 +18,7 @@
 // <http://www.gnu.org/licenses/>.
 
 // { dg-do compile }
-// { dg-error "no matching" "" { target *-*-* } 1760 }
+// { dg-error "no matching" "" { target *-*-* } 1763 }
 
 #include <deque>
 
Index: libstdc++-v3/testsuite/23_containers/deque/requirements/dr438/insert_neg.cc
===================================================================
--- libstdc++-v3/testsuite/23_containers/deque/requirements/dr438/insert_neg.cc	(revision 202709)
+++ libstdc++-v3/testsuite/23_containers/deque/requirements/dr438/insert_neg.cc	(working copy)
@@ -18,7 +18,7 @@
 // <http://www.gnu.org/licenses/>.
 
 // { dg-do compile }
-// { dg-error "no matching" "" { target *-*-* } 1844 }
+// { dg-error "no matching" "" { target *-*-* } 1847 }
 
 #include <deque>
 
@@ -32,4 +32,3 @@
   std::deque<A> d;
   d.insert(d.begin(), 10, 1);
 }
-
Index: libstdc++-v3/testsuite/23_containers/deque/requirements/dr438/constructor_1_neg.cc
===================================================================
--- libstdc++-v3/testsuite/23_containers/deque/requirements/dr438/constructor_1_neg.cc	(revision 202709)
+++ libstdc++-v3/testsuite/23_containers/deque/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 *-*-* } 1693 }
+// { dg-error "no matching" "" { target *-*-* } 1696 }
 
 #include <deque>
 
Index: libstdc++-v3/testsuite/23_containers/deque/requirements/dr438/constructor_2_neg.cc
===================================================================
--- libstdc++-v3/testsuite/23_containers/deque/requirements/dr438/constructor_2_neg.cc	(revision 202709)
+++ libstdc++-v3/testsuite/23_containers/deque/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 *-*-* } 1693 }
+// { dg-error "no matching" "" { target *-*-* } 1696 }
 
 #include <deque>
 #include <utility>
Index: libstdc++-v3/testsuite/23_containers/array/tuple_interface/get_neg.cc
===================================================================
--- libstdc++-v3/testsuite/23_containers/array/tuple_interface/get_neg.cc	(revision 202709)
+++ libstdc++-v3/testsuite/23_containers/array/tuple_interface/get_neg.cc	(working copy)
@@ -28,6 +28,6 @@
 int n2 = std::get<1>(std::move(a));
 int n3 = std::get<1>(ca);
 
-// { dg-error "static assertion failed" "" { target *-*-* } 270 }
-// { dg-error "static assertion failed" "" { target *-*-* } 279 }
-// { dg-error "static assertion failed" "" { target *-*-* } 287 }
+// { dg-error "static assertion failed" "" { target *-*-* } 274 }
+// { dg-error "static assertion failed" "" { target *-*-* } 283 }
+// { dg-error "static assertion failed" "" { target *-*-* } 291 }
Index: libstdc++-v3/testsuite/23_containers/array/tuple_interface/tuple_element_neg.cc
===================================================================
--- libstdc++-v3/testsuite/23_containers/array/tuple_interface/tuple_element_neg.cc	(revision 202709)
+++ libstdc++-v3/testsuite/23_containers/array/tuple_interface/tuple_element_neg.cc	(working copy)
@@ -23,4 +23,4 @@
 
 typedef std::tuple_element<1, std::array<int, 1>>::type type;
 
-// { dg-error "static assertion failed" "" { target *-*-* } 316 }
+// { dg-error "static assertion failed" "" { target *-*-* } 320 }
Index: libstdc++-v3/testsuite/util/exception/safety.h
===================================================================
--- libstdc++-v3/testsuite/util/exception/safety.h	(revision 202709)
+++ libstdc++-v3/testsuite/util/exception/safety.h	(working copy)
@@ -47,22 +47,12 @@
       const typename distribution_type::param_type p(0, __max_size);
       size_type random = generator(p);
       if (random < distribution.min() || random > distribution.max())
-	{
-	  std::string __s("setup_base::generate");
-	  __s += "\n";
-	  __s += "random number generated is: ";
-	  char buf[40];
-	  __builtin_sprintf(buf, "%lu", (unsigned long)random);
-	  __s += buf;
-	  __s += " on range [";
-	  __builtin_sprintf(buf, "%lu", (unsigned long)distribution.min());
-	  __s += buf;
-	  __s += ", ";
-	  __builtin_sprintf(buf, "%lu", (unsigned long)distribution.max());
-	  __s += buf;
-	  __s += "]\n";
-	  std::__throw_out_of_range(__s.c_str());
-	}
+	std::__throw_out_of_range_fmt(__N("setup_base::generate\n"
+					  "random number generated is: %zu "
+					  "out of range [%zu, %zu]\n"),
+				      (size_t)random,
+				      (size_t)distribution.min(),
+				      (size_t)distribution.max());
       return random;
     }
 

Re: [patch] Make vector::at() assertion message more useful (try #2)

[Paolo Carlini]

Hi,

> Il giorno 18/set/2013, alle ore 21:38, Paul Pluzhnikov <ppluzhnikov@google.com> ha scritto:
> 
>> On Fri, Sep 13, 2013 at 3:02 AM, Paolo Carlini <paolo.carlini@oracle.com> wrote:
>> 
>> - 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.
> 
> Replaced with __throw_out_of_range_fmt.
> 
>> - Please consistently use __builtin_alloca everywhere, alloca isn't a
>> standard function.
> 
> Done.
> 
>> - I would rather call the file itself snprintf_lite.cc, in order not to fool
>> somebody that it actually implements the whole snprintf.
> 
> Done.
> 
>> - 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.
> 
> How so? The caller could do this:
> 
>  __snprintf_lite("aaa: %zu, 8, 12345678);
> 
> By the time we get to __concat_size_t, we only have 3 characters left.

Ok, thanks, I think I misread the code. In general I meant that you can bound a priori the number of chars you need to format an integer - that isn't the case for floats, for example (the user may want a ridiculously large number of decimal digits). But you are in fact already exploiting that for the value of __ilen.

> 
>> Then, if it returns -1 something is going *very badly* wrong, shouldn't we
>> __builtin_abort() or something similar?
> 
> I've re-worked this part -- if this ever happens, throw a logic_error with
> a message to file a bug.
> 
>> - 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.
> 
> Re-worked.
> 
>> - 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.
> 
> Done.
> 
> I also expanded snprintf_lite to handle '%s', as that comes handy inside
> string _M_check.
> 
> Thanks,
> 
> P.S. In the process of updating callers of __throw_out_of_range, I've
> discovered that two of them had already used __builtin_sprintf.
> 
> P.P.S. Sorry this patch grew ... I can split it into parts if that's easier
> to review.

In general I like this version of the patch a lot. Thus assuming nobody else has further comments over the next day or so, please commit it at your ease. In the meanwhile, since you are also touching debug-mode and profile-mode, make sure to run check-debug and check-profile too.


Thanks,
Paolo

Re: [patch] Make vector::at() assertion message more useful (try #2)

[Paul Pluzhnikov]

On Wed, Sep 18, 2013 at 3:00 PM, Paolo Carlini <paolo.carlini@oracle.com> wrote:

> In the meanwhile, since you are also touching debug-mode and
> profile-mode, make sure to run check-debug and check-profile too.

Thanks for mentioning that. Several more tests needed line number
adjustments.

There are also tests that are failing there independently of my patch.

Committed as r202818.

Thanks,
-- 
Paul Pluzhnikov

Re: [patch] Make vector::at() assertion message more useful (try #2)

[Andreas Schwab]

Paul Pluzhnikov <ppluzhnikov@google.com> writes:

> Index: libstdc++-v3/src/c++11/snprintf_lite.cc
> ===================================================================
> --- libstdc++-v3/src/c++11/snprintf_lite.cc	(revision 0)
> +++ libstdc++-v3/src/c++11/snprintf_lite.cc	(revision 0)
> @@ -0,0 +1,152 @@
> +// 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/functexcept.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);
> +}
> +

m68k-linux/./libstdc++-v3/src/.libs/libstdc++.so: undefined reference to `int std::__int_to_char<char, unsigned int>(char*, unsigned int, char const*, std::_Ios_Fmtflags, bool)'

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [patch] Make vector::at() assertion message more useful (try #2)

[Paolo Carlini]

Hi,

thanks Andreas.

On 9/23/13 3:53 AM, Andreas Schwab wrote:
> Paul Pluzhnikov<ppluzhnikov@google.com>  writes:
>
>> Index: libstdc++-v3/src/c++11/snprintf_lite.cc
>> ===================================================================
>> --- libstdc++-v3/src/c++11/snprintf_lite.cc	(revision 0)
>> +++ libstdc++-v3/src/c++11/snprintf_lite.cc	(revision 0)
>> @@ -0,0 +1,152 @@
>> +// 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/functexcept.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);
>> +}
>> +
> m68k-linux/./libstdc++-v3/src/.libs/libstdc++.so: undefined reference to `int std::__int_to_char<char, unsigned int>(char*, unsigned int, char const*, std::_Ios_Fmtflags, bool)'
Paul, the *.cc are built with implicit instantiations disabled and we 
are missing explicit instantiations of this function template. If I 
remember correctly, we normally instantiate it in locale-inst.cc only 
for unsigned long and unsigned long long as second template argument. 
Thus, I would say, either make sure to use only those two in the new 
code, or add explicit instantiations. I would rather prefer the former, 
for example casting __val to (unsigned long long) in __concat_size_t 
isn't particularly elegant (vs for example checking if size_t isn't 
either unsigned long neither unsigned long long and adding an explicit 
instantiation only in that case) but would work I think. In any case, if 
it works for Andreas he is also preapproved to commit this kind change 
to unbreak his target.

Paolo.

Re: [patch] Make vector::at() assertion message more useful (try #2)

[Richard Biener]

On Mon, Sep 23, 2013 at 1:26 PM, Paolo Carlini <paolo.carlini@oracle.com> wrote:
> Hi,
>
> thanks Andreas.
>
>
> On 9/23/13 3:53 AM, Andreas Schwab wrote:
>>
>> Paul Pluzhnikov<ppluzhnikov@google.com>  writes:
>>
>>> Index: libstdc++-v3/src/c++11/snprintf_lite.cc
>>> ===================================================================
>>> --- libstdc++-v3/src/c++11/snprintf_lite.cc     (revision 0)
>>> +++ libstdc++-v3/src/c++11/snprintf_lite.cc     (revision 0)
>>> @@ -0,0 +1,152 @@
>>> +// 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/functexcept.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);
>>> +}
>>> +
>>
>> m68k-linux/./libstdc++-v3/src/.libs/libstdc++.so: undefined reference to
>> `int std::__int_to_char<char, unsigned int>(char*, unsigned int, char
>> const*, std::_Ios_Fmtflags, bool)'
>
> Paul, the *.cc are built with implicit instantiations disabled and we are
> missing explicit instantiations of this function template. If I remember
> correctly, we normally instantiate it in locale-inst.cc only for unsigned
> long and unsigned long long as second template argument. Thus, I would say,
> either make sure to use only those two in the new code, or add explicit
> instantiations. I would rather prefer the former, for example casting __val
> to (unsigned long long) in __concat_size_t isn't particularly elegant (vs
> for example checking if size_t isn't either unsigned long neither unsigned
> long long and adding an explicit instantiation only in that case) but would
> work I think. In any case, if it works for Andreas he is also preapproved to
> commit this kind change to unbreak his target.

Btw, i?86 is broken as well (or -m32 testing on x86_64).

Richard.

> Paolo.

Re: [patch] Make vector::at() assertion message more useful (try #2)

[Andreas Schwab]

Paolo Carlini <paolo.carlini@oracle.com> writes:

> Paul, the *.cc are built with implicit instantiations disabled and we are
> missing explicit instantiations of this function template. If I remember
> correctly, we normally instantiate it in locale-inst.cc only for unsigned
> long and unsigned long long as second template argument. Thus, I would
> say, either make sure to use only those two in the new code, or add
> explicit instantiations. I would rather prefer the former, for example
> casting __val to (unsigned long long) in __concat_size_t isn't
> particularly elegant (vs for example checking if size_t isn't either
> unsigned long neither unsigned long long and adding an explicit
> instantiation only in that case)

What's wrong with always adding the instantiation?

> but would work I think. In any case, if it works for Andreas he is
> also preapproved to commit this kind change to unbreak his target.

There are a lot of targets using unsigned int for size_t, which would
have been uncovered by proper testing.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [patch] Make vector::at() assertion message more useful (try #2)

[Paolo Carlini]

Hi

Andreas Schwab <schwab@suse.de> ha scritto:

>What's wrong with always adding the instantiation?

That quite a few targets will not use it and will remain dead in the .so? But I agree that it's an option, that you can also pursue immediately if you like, also preapproved (I'm traveling, either you or Paul or somebody else has to do the work, sorry)

>
>> but would work I think. In any case, if it works for Andreas he is
>> also preapproved to commit this kind change to unbreak his target.
>
>There are a lot of targets using unsigned int for size_t, which would
>have been uncovered by proper testing.

That's true, just remember to test *both* -m32 and -m64, for non trivial changes. How many times I have to repeat that?

Paolo

Re: [patch] Make vector::at() assertion message more useful (try #2)

[Andreas Schwab]

Paolo Carlini <paolo.carlini@oracle.com> writes:

> Hi
>
> Andreas Schwab <schwab@suse.de> ha scritto:
>
>>What's wrong with always adding the instantiation?
>
> That quite a few targets will not use it and will remain dead in the .so?

How many uses are there for the unsigned long version?

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [patch] Make vector::at() assertion message more useful (try #2)

[Marc Glisse]

On Mon, 23 Sep 2013, Paolo Carlini wrote:

>> There are a lot of targets using unsigned int for size_t, which would
>> have been uncovered by proper testing.

We can't test all patches on 3-4 different targets... It wasn't obvious 
this patch could be that sensitive to the target.

> That's true, just remember to test *both* -m32 and -m64, for non trivial 
> changes.

So how do you do that in practice ? Is it done by default if multilib is 
enabled? You also mentioned doing something special to check debug/profile 
modes recently, is there a make target to really perform all the tests 
necessary for a submission?

http://gcc.gnu.org/contribute.html has an outdated section on testing. It 
mentions that you should do a bootstrap for a change to the C front-end 
(should also be for the C++ front-end and I guess libstdc++ even if it 
isn't used much inside gcc).

-- 
Marc Glisse

Re: [patch] Make vector::at() assertion message more useful (try #2)

[Paolo Carlini]

On 9/23/13 8:22 AM, Andreas Schwab wrote:
> Paolo Carlini<paolo.carlini@oracle.com>  writes:
>
>> Hi
>>
>> Andreas Schwab<schwab@suse.de>  ha scritto:
>>
>>> What's wrong with always adding the instantiation?
>> That quite a few targets will not use it and will remain dead in the .so?
> How many uses are there for the unsigned long version?
I would say *all* the users of the library. Remember that these 
instantiations are in locale-inst.cc (I think an additional 
instantiation also belongs there, with a comment), and are now being 
"hijacked" for the new "pretty printing", but normally are used for 
formatted integers I/O.

Paolo.

Re: [patch] Make vector::at() assertion message more useful (try #2)

[Jakub Jelinek]

On Mon, Sep 23, 2013 at 03:55:26PM +0200, Marc Glisse wrote:
> On Mon, 23 Sep 2013, Paolo Carlini wrote:
> 
> >>There are a lot of targets using unsigned int for size_t, which would
> >>have been uncovered by proper testing.
> 
> We can't test all patches on 3-4 different targets... It wasn't
> obvious this patch could be that sensitive to the target.
> 
> >That's true, just remember to test *both* -m32 and -m64, for non
> >trivial changes.
> 
> So how do you do that in practice ? Is it done by default if
> multilib is enabled? You also mentioned doing something special to
> check debug/profile modes recently, is there a make target to really
> perform all the tests necessary for a submission?

It isn't done by default, but you can easily do that, by running
make check RUNTESTFLAGS='--target_board=unix\{-m32,-m64\}'
(either toplevel, or more specific, e.g. just in libstdc++-v3/
dir, or even
make check-gcc RUNTESTFLAGS='--target_board=unix\{-m32,-m64\} dg.exp=pr12345.c'
etc.).
Or of course you can do two separate bootstraps/make check (that is what I'm usually
doing, so that both bootstraps are tested).
> 
> http://gcc.gnu.org/contribute.html has an outdated section on
> testing. It mentions that you should do a bootstrap for a change to
> the C front-end (should also be for the C++ front-end and I guess
> libstdc++ even if it isn't used much inside gcc).

	Jakub

Re: [patch] Make vector::at() assertion message more useful (try #2)

[Paul Pluzhnikov]

On 9/23/13 4:26 AM, Paolo Carlini wrote:

>> m68k-linux/./libstdc++-v3/src/.libs/libstdc++.so: undefined reference
>> to `int std::__int_to_char<char, unsigned int>(char*, unsigned int,
>> char const*, std::_Ios_Fmtflags, bool)'

Reproduced on i686.
Sorry about the trouble ...

>I would say, either make sure to use only those two in the new
> code

Testing this patch:

Index: libstdc++-v3/src/c++11/snprintf_lite.cc
===================================================================
--- libstdc++-v3/src/c++11/snprintf_lite.cc	(revision 202830)
+++ libstdc++-v3/src/c++11/snprintf_lite.cc	(working copy)
@@ -70,9 +70,10 @@
    int __concat_size_t(char *__buf, size_t __bufsize, size_t __val)
    {
      // Long enough for decimal representation.
-    int __ilen = 3 * sizeof(__val);
+    unsigned long long __val_ull = __val;
+    int __ilen = 3 * sizeof(__val_ull);
      char *__cs = static_cast<char*>(__builtin_alloca(__ilen));
-    size_t __len = std::__int_to_char(__cs + __ilen, __val,
+    size_t __len = std::__int_to_char(__cs + __ilen, __val_ull,
  				      std::__num_base::_S_atoms_out,
  				      std::ios_base::dec, true);
      if (__bufsize < __len)

Re: [patch] Make vector::at() assertion message more useful (try #2)

[Paolo Carlini]

Hi,

On 9/23/13 9:48 AM, Paul Pluzhnikov wrote:
> On 9/23/13 4:26 AM, Paolo Carlini wrote:
>
>>> m68k-linux/./libstdc++-v3/src/.libs/libstdc++.so: undefined reference
>>> to `int std::__int_to_char<char, unsigned int>(char*, unsigned int,
>>> char const*, std::_Ios_Fmtflags, bool)'
>
> Reproduced on i686.
> Sorry about the trouble ...
>
>> I would say, either make sure to use only those two in the new
>> code
>
> Testing this patch:
In fact, however, that unsigned long long instantiation isn't 
*unconditionally* available, see locale-inst.cc. I think we have to use 
unsigned long as a fall back controlled by the same macro. And please 
add a comment explaining those contortions having to do with the 
available instantiations. Patch pre-approved.

Thanks again,
Paolo.

Re: [patch] Make vector::at() assertion message more useful (try #2)

[Paul Pluzhnikov]

On 9/23/13 7:48 AM, Paul Pluzhnikov wrote:

> Testing this patch:

libstdc++ tests finished with
RUNTESTFLAGS='--target_board=unix\{-m32,-m64\}'

Committed as r202832.

Sorry about the trouble.
--


2013-09-23  Paul Pluzhnikov  <ppluzhnikov@google.com>

	* src/c++11/snprintf_lite.cc (__concat_size_t): Use only
	std::__int_to_char<unsigned long long>()


Index: libstdc++-v3/src/c++11/snprintf_lite.cc
===================================================================
--- libstdc++-v3/src/c++11/snprintf_lite.cc	(revision 202830)
+++ libstdc++-v3/src/c++11/snprintf_lite.cc	(working copy)
@@ -70,9 +70,10 @@
    int __concat_size_t(char *__buf, size_t __bufsize, size_t __val)
    {
      // Long enough for decimal representation.
-    int __ilen = 3 * sizeof(__val);
+    unsigned long long __val_ull = __val;
+    int __ilen = 3 * sizeof(__val_ull);
      char *__cs = static_cast<char*>(__builtin_alloca(__ilen));
-    size_t __len = std::__int_to_char(__cs + __ilen, __val,
+    size_t __len = std::__int_to_char(__cs + __ilen, __val_ull,
  				      std::__num_base::_S_atoms_out,
  				      std::ios_base::dec, true);
      if (__bufsize < __len)

Re: [patch] Make vector::at() assertion message more useful (try #2)

[Paul Pluzhnikov]

On 9/23/13 7:54 AM, Paolo Carlini wrote:

>> Testing this patch:
> In fact, however, that unsigned long long instantiation isn't
> *unconditionally* available, see locale-inst.cc.

Thanks,

> I think we have to use
> unsigned long as a fall back controlled by the same macro. And please
> add a comment explaining those contortions having to do with the
> available instantiations. Patch pre-approved.

Does this look right?

Index: libstdc++-v3/src/c++11/snprintf_lite.cc
===================================================================
--- libstdc++-v3/src/c++11/snprintf_lite.cc	(revision 202832)
+++ libstdc++-v3/src/c++11/snprintf_lite.cc	(working copy)
@@ -69,11 +69,17 @@
    // Returns number of characters appended, or -1 if BUFSIZE is too small.
    int __concat_size_t(char *__buf, size_t __bufsize, size_t __val)
    {
+    // __int_to_char is explicitly instantiated and available only for
+    // some, but not all, types.
+#ifdef _GLIBCXX_USE_LONG_LONG
+    unsigned long long __val2 = __val;
+#else
+    unsigned long __val2 = __val;
+#endif
      // Long enough for decimal representation.
-    unsigned long long __val_ull = __val;
-    int __ilen = 3 * sizeof(__val_ull);
+    int __ilen = 3 * sizeof(__val2);
      char *__cs = static_cast<char*>(__builtin_alloca(__ilen));
-    size_t __len = std::__int_to_char(__cs + __ilen, __val_ull,
+    size_t __len = std::__int_to_char(__cs + __ilen, __val2,
  				      std::__num_base::_S_atoms_out,
  				      std::ios_base::dec, true);
      if (__bufsize < __len)

Re: [patch] Make vector::at() assertion message more useful (try #2)

[Paolo Carlini]

Hi,

On 9/23/13 11:02 AM, Paul Pluzhnikov wrote:
> On 9/23/13 7:54 AM, Paolo Carlini wrote:
>
>>> Testing this patch:
>> In fact, however, that unsigned long long instantiation isn't
>> *unconditionally* available, see locale-inst.cc.
>
> Thanks,
>
>> I think we have to use
>> unsigned long as a fall back controlled by the same macro. And please
>> add a comment explaining those contortions having to do with the
>> available instantiations. Patch pre-approved.
>
> Does this look right?
Yes. Nit, in the comment I would also explicitly mention locale-inst.cc.

Thanks!
Paolo.

Re: [patch] Make vector::at() assertion message more useful (try #2)

[Paul Pluzhnikov]

On 9/23/13 9:24 AM, Paolo Carlini wrote:

>> Does this look right?
> Yes. Nit, in the comment I would also explicitly mention locale-inst.cc.

Done, submitted as r202836.

2013-09-23  Paul Pluzhnikov  <ppluzhnikov@google.com>

	* src/c++11/snprintf_lite.cc (__concat_size_t): Use
	unsigned long long conditionally.



Index: libstdc++-v3/src/c++11/snprintf_lite.cc
===================================================================
--- libstdc++-v3/src/c++11/snprintf_lite.cc	(revision 202832)
+++ libstdc++-v3/src/c++11/snprintf_lite.cc	(working copy)
@@ -69,11 +69,17 @@
    // Returns number of characters appended, or -1 if BUFSIZE is too small.
    int __concat_size_t(char *__buf, size_t __bufsize, size_t __val)
    {
+    // __int_to_char is explicitly instantiated and available only for
+    // some, but not all, types. See locale-inst.cc.
+#ifdef _GLIBCXX_USE_LONG_LONG
+    unsigned long long __val2 = __val;
+#else
+    unsigned long __val2 = __val;
+#endif
      // Long enough for decimal representation.
-    unsigned long long __val_ull = __val;
-    int __ilen = 3 * sizeof(__val_ull);
+    int __ilen = 3 * sizeof(__val2);
      char *__cs = static_cast<char*>(__builtin_alloca(__ilen));
-    size_t __len = std::__int_to_char(__cs + __ilen, __val_ull,
+    size_t __len = std::__int_to_char(__cs + __ilen, __val2,
  				      std::__num_base::_S_atoms_out,
  				      std::ios_base::dec, true);
      if (__bufsize < __len)

[wwwdocs] Testing C++ changes (was: [patch] Make vector::at() assertion message more useful)

[Gerald Pfeifer]

On Mon, 23 Sep 2013, Marc Glisse wrote:
> http://gcc.gnu.org/contribute.html has an outdated section on testing. 
> It mentions that you should do a bootstrap for a change to the C 
> front-end (should also be for the C++ front-end and I guess libstdc++ 
> even if it isn't used much inside gcc).

Somehow nobody bit or did update contribute.html anyway, so here
is a patch to adjust this.

Jason, any other thoughts?

Gerald

Index: contribute.html
===================================================================
RCS file: /cvs/gcc/wwwdocs/htdocs/contribute.html,v
retrieving revision 1.85
diff -u -r1.85 contribute.html
--- contribute.html	27 Jun 2014 11:12:18 -0000	1.85
+++ contribute.html	6 Apr 2015 11:32:05 -0000
@@ -100,9 +100,9 @@
 <h3>Which tests to perform</h3>
 
 <p>If your change is to code that is not in a front end, or is to the
-C front end, you must perform a complete build of GCC and the runtime
-libraries included with it, on at least one target.  You must
-bootstrap all default languages, not just C, and run all testsuites.
+C or C++ front ends, you must perform a complete build of GCC and the
+runtime libraries included with it, on at least one target.  You must
+bootstrap all default languages, not just C and C++, and run all testsuites.
 For a normal native configuration, running</p>
 <blockquote><pre>
 make bootstrap
@@ -111,17 +111,6 @@
 <p>from the top level of the GCC tree (<strong>not</strong> the
 <code>gcc</code> subdirectory) will accomplish this.</p>
 
-<p>If your change is to the C++ front end, you should rebuild the compiler,
-<code>libstdc++</code>, <code>libjava</code> and run the C++ testsuite.
-If you already did a complete C,C++,Java bootstrap from your build
-directory before, you can use the following:</p>
-<blockquote><pre>
-make clean-target-libstdc++-v3                    # clean libstdc++ and ...
-test -d */libjava &amp;&amp; make -C */libjava clean-nat  # ... parts of libjava
-make all-target-libstdc++-v3 all-target-libjava   # rebuild compiler and libraries
-make -k check-c++                                 # run C++/libstdc++ testsuite
-</pre></blockquote>
-
 <p>If your change is to a front end other than the C or C++ front end,
 or a runtime library other than <code>libgcc</code>, you need to verify
 only that the runtime library for that language still builds and the

Re: [wwwdocs] Testing C++ changes

[Jason Merrill]

That looks fine.

Jason