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

login
register
mail settings
Submitter Paul Pluzhnikov
Date Sept. 4, 2013, 8:53 p.m.
Message ID <ye6qhae0qpf8.fsf@elbrus2.mtv.corp.google.com>
Download mbox | patch
Permalink /patch/272718/
State New
Headers show

Comments

Paul Pluzhnikov - Sept. 4, 2013, 8:53 p.m.
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.
Daniel Krügler - Sept. 4, 2013, 9:10 p.m.
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
Paul Pluzhnikov - Sept. 4, 2013, 11:16 p.m.
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,
Paolo Carlini - Sept. 4, 2013, 11:26 p.m.
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.
Paul Pluzhnikov - Sept. 4, 2013, 11:36 p.m.
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,
Paolo Carlini - Sept. 4, 2013, 11:44 p.m.
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.
Daniel Krügler - Sept. 5, 2013, 4:55 a.m.
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

Patch

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>