This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: _FORTIFY_SOURCE for std::vector


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  <fweimer@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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]