On Fri, Jun 01, 2012 at 01:00:46PM +0200, Florian Weimer wrote:
> I forgot to send this to the libstdc++ list the first time.
>
> This patch evaluates _FORTIFY_SOURCE in a way similar to GNU libc.
> If set, std::vector::operator[] throws if the index is out of bounds.
> This is compliant with the standard because such usage triggers
> undefined behavior. _FORTIFY_SOURCE users expect some performance hit.
>
> In contrast to debugging mode, this does not change ABI and is more
> widely applicable.
>
> Okay for trunk?
Have you looked at the assembly differences with this in?
I wonder e.g. if we don't need to add __builtin_expect around the
_M_range_check test. As it needs to read in many cases two pointers,
subtract them (+ divide by size of entry (or shift right if power of two))
and compare that to something likely in a register, it is likely to be more
expensive than most of other _FORTIFY_SOURCE checks, but perhaps bearable.
Also, I wonder if the addition of a throw spot (even in cold .text chunk)
doesn't add too much cost (code having to deal with possible EH even when
otherwise it wouldn't be expected) and whether calling __chk_fail () in
that case instead wouldn't be cheaper. Even in C++ memcpy etc. _FORTIFY_SOURCE
failures will result in __chk_fail, not throwing some exception, so
operator[] on std::vector could work the same. operator[] is documented
as not doing the range check (unlike ->at()), so you probably need to
tweak the documentation too.
> 2012-05-29 Florian Weimer <fwei...@redhat.com>
>
> * include/bits/stl_vector.h (vector::_M_fortify_range_check):
> New.
> * (vector::operator[]): Call it.
> * testsuite/23_containers/vector/element_access/2.cc: New.
>
> Index: libstdc++-v3/include/bits/stl_vector.h
> ===================================================================
> --- libstdc++-v3/include/bits/stl_vector.h (revision 187951)
> +++ libstdc++-v3/include/bits/stl_vector.h (working copy)
> @@ -768,7 +768,10 @@
> */
> reference
> operator[](size_type __n)
> - { return *(this->_M_impl._M_start + __n); }
> + {
> + _M_fortify_range_check(__n);
> + return *(this->_M_impl._M_start + __n);
> + }
>
> /**
> * @brief Subscript access to the data contained in the %vector.
> @@ -783,7 +786,10 @@
> */
> const_reference
> operator[](size_type __n) const
> - { return *(this->_M_impl._M_start + __n); }
> + {
> + _M_fortify_range_check(__n);
> + return *(this->_M_impl._M_start + __n);
> + }
>
> protected:
> /// Safety check used only from at().
> @@ -794,6 +800,16 @@
> __throw_out_of_range(__N("vector::_M_range_check"));
> }
>
> + /// Range check used by operator[].
> + /// No-op unless _FORTIFY_SOURCE is enabled.
> + void
> + _M_fortify_range_check(size_type __n) const
> + {
> +#if defined _FORTIFY_SOURCE && _FORTIFY_SOURCE > 0
> + _M_range_check(__n);
> +#endif
> + }
> +
> public:
> /**
> * @brief Provides access to the data contained in the %vector.
Jakub
