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

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.

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

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,

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.

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,

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.

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