| Message ID | 4FCCF671.6000403@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Mon, 4 Jun 2012, Florian Weimer wrote: > On 06/01/2012 01:34 PM, Jakub Jelinek wrote: >> Have you looked at the assembly differences with this in? > > It's not great. > > Here's an example: > > void > write(std::vector<float>& blob, unsigned n, float v1, float v2, float v3, > float v4) > { > blob[n] = v1; > blob[n + 1] = v2; > blob[n + 2] = v3; > blob[n + 3] = v4; > } Would be great if it ended up testing only n and n+3. __attribute__((__noreturn__)) is not quite strong enough to allow this optimization, it would require something like __attribute__((__crashing__)) to let the compiler know that if the function is called, you don't care what happens to blob. And possibly the use of a signed n. Note that even when the optimization would be legal, gcc seems to have a few difficulties: extern "C" void fail() __attribute((noreturn)); void write(signed m, signed n) { if((n+3)>m) fail(); if((n+2)>m) fail(); if((n+1)>m) fail(); if(n>m) fail(); } keeps 3 tests.
On 06/04/2012 09:07 PM, Marc Glisse wrote: > On Mon, 4 Jun 2012, Florian Weimer wrote: >> void >> write(std::vector<float>& blob, unsigned n, float v1, float v2, float >> v3, float v4) >> { >> blob[n] = v1; >> blob[n + 1] = v2; >> blob[n + 2] = v3; >> blob[n + 3] = v4; >> } > > Would be great if it ended up testing only n and n+3. True. > __attribute__((__noreturn__)) is not quite strong enough to allow this > optimization, it would require something like > __attribute__((__crashing__)) to let the compiler know that if the > function is called, you don't care what happens to blob. And possibly > the use of a signed n. Interesting point, I had not realized that before. Ada has a special rule for failures of language-defined checks, and they might give enough wiggle room to leave behind a partially updated vector in such situations. But even without that, you could clone the if sequence, that is, if (blob.size() - n >= 4) { blob[n] = v1; blob[n + 1] = v2; blob[n + 2] = v3; blob[n + 3] = v4; } else { ... // individual checks } Obviously, this has quite a bit of an impact on code size.
On Mon, Jun 4, 2012 at 9:07 PM, Marc Glisse <marc.glisse@inria.fr> wrote: > On Mon, 4 Jun 2012, Florian Weimer wrote: > >> On 06/01/2012 01:34 PM, Jakub Jelinek wrote: >>> >>> Have you looked at the assembly differences with this in? >> >> >> It's not great. >> >> Here's an example: >> >> void >> write(std::vector<float>& blob, unsigned n, float v1, float v2, float v3, >> float v4) >> { >> blob[n] = v1; >> blob[n + 1] = v2; >> blob[n + 2] = v3; >> blob[n + 3] = v4; >> } > > > Would be great if it ended up testing only n and n+3. > __attribute__((__noreturn__)) is not quite strong enough to allow this > optimization, it would require something like __attribute__((__crashing__)) > to let the compiler know that if the function is called, you don't care what > happens to blob. And possibly the use of a signed n. > > Note that even when the optimization would be legal, gcc seems to have a few > difficulties: > > extern "C" void fail() __attribute((noreturn)); > void write(signed m, signed n) > { > if((n+3)>m) fail(); > if((n+2)>m) fail(); > if((n+1)>m) fail(); > if(n>m) fail(); > } > > keeps 3 tests. Well, the issue is that we'd first need to commonize the fail () calls which we do now, but even then VRP fails to simplify the comparisons against the symbolic ranges (it's not very good at that). And that would only be at -O1. Note that such range-checks will defeat most, if not all, loop optimizations, too. So C++ code using std::vector in compute-intensive parts would be severely pessimized. So, I don't think fortifying libstdc++ is a good idea at all. Richard. > -- > Marc Glisse
On 06/05/2012 11:05 AM, Richard Guenther wrote: > And that would only be at -O1. Note that such range-checks will defeat > most, if not all, loop optimizations, too. So C++ code using std::vector > in compute-intensive parts would be severely pessimized. Array bounds check elimination could deal with that, but that would require to teach the compiler about std::vector internals. > So, I don't think fortifying libstdc++ is a good idea at all. For std::vector and operator[] I agree, but I wouldn't discard the entire idea completely. (std::array would be much cheaper to check, but then we'd lose consistency.) I'm going to look for tool support for locating such uses of operator[]. We might start recommending developers to switch to the at() member functions in most code.
Index: libstdc++-v3/include/bits/c++config =================================================================== --- libstdc++-v3/include/bits/c++config (revision 187951) +++ libstdc++-v3/include/bits/c++config (working copy) @@ -370,6 +370,13 @@ } while (false) #endif +// _FORTIFY_SOURCE support. Only available on GNU libc. + +namespace __gnu_cxx +{ + extern "C" void __chk_fail () __attribute__((noreturn)); +} + // Macros for race detectors. // _GLIBCXX_SYNCHRONIZATION_HAPPENS_BEFORE(A) and // _GLIBCXX_SYNCHRONIZATION_HAPPENS_AFTER(A) should be used to explain 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,17 @@ __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 __USE_FORTIFY_LEVEL && __USE_FORTIFY_LEVEL > 0 + if (__n >= this->size()) + __gnu_cxx::__chk_fail(); +#endif + } + public: /** * @brief Provides access to the data contained in the %vector. Index: libstdc++-v3/testsuite/23_containers/vector/element_access/2.cc =================================================================== --- libstdc++-v3/testsuite/23_containers/vector/element_access/2.cc (revision 0) +++ libstdc++-v3/testsuite/23_containers/vector/element_access/2.cc (revision 0) @@ -0,0 +1,100 @@ +// Copyright (C) 2012 Free Software Foundation, Inc. +// +// This file is part of the GNU ISO C++ Library. This library 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. + +// This library 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. + +// You should have received a copy of the GNU General Public License along +// with this library; see the file COPYING3. If not see +// <http://www.gnu.org/licenses/>. + +// 23.2.4 vector + +// { dg-add-options no_pch } +// { dg-target *-*-linux* } + +#undef _FORTIFY_SOURCE +#define _FORTIFY_SOURCE 2 + +#include <vector> + +#include <fcntl.h> +#include <paths.h> +#include <setjmp.h> +#include <signal.h> + +#include <testsuite_hooks.h> + +// Testing pattern follows debug/test-strcpy_chk.c in GNU libc. +volatile int chk_fail_ok; +jmp_buf chk_fail_buf; + +static void +handler(int) +{ + if (chk_fail_ok) + { + chk_fail_ok = 0; + longjmp(chk_fail_buf, 1); + } + else + _exit(127); +} + +static void +run_test(void (*t)()) +{ + bool test __attribute__((unused)) = true; + if (setjmp(chk_fail_buf) == 0) + { + t(); + VERIFY(false); + } +} + +void test01() +{ + std::vector<int> v(5); + v[0]; + v[4]; + chk_fail_ok = 1; + v[5]; +} + +void test02() +{ + std::vector<int> u(5); + const std::vector<int>& v(u); + v[0]; + v[4]; + chk_fail_ok = 1; + v[5]; +} + +int main() +{ + // See debug/test-strcpy_chk.c in GNU libc. + struct sigaction sa; + sa.sa_handler = andler; + sa.sa_flags = 0; + sigaction(SIGABRT, &sa, NULL); + int fd = open(_PATH_DEVNULL, O_WRONLY); + if (fd == -1) + close(STDERR_FILENO); + else + { + dup2(fd, STDERR_FILENO); + close(fd); + } + setenv("LIBC_FATAL_STDERR_", "1", 1); // do not use /dev/tty + run_test(test01); + run_test(test02); + return 0; +}
On 06/01/2012 01:34 PM, Jakub Jelinek wrote: > Have you looked at the assembly differences with this in? It's not great. Here's an example: void write(std::vector<float>& blob, unsigned n, float v1, float v2, float v3, float v4) { blob[n] = v1; blob[n + 1] = v2; blob[n + 2] = v3; blob[n + 3] = v4; } Without checks, it turns into: movq (%rdi), %rax movl %esi, %edx movss %xmm0, (%rax,%rdx,4) leal 1(%rsi), %edx movss %xmm1, (%rax,%rdx,4) leal 2(%rsi), %edx movss %xmm2, (%rax,%rdx,4) leal 3(%rsi), %edx movss %xmm3, (%rax,%rdx,4) ret With checks, it's: subq $8, %rsp movq (%rdi), %rdx movq 8(%rdi), %rax movl %esi, %ecx subq %rdx, %rax sarq $2, %rax cmpq %rax, %rcx jae .L8 movss %xmm0, (%rdx,%rcx,4) leal 1(%rsi), %ecx cmpq %rax, %rcx jae .L8 movss %xmm1, (%rdx,%rcx,4) leal 2(%rsi), %ecx cmpq %rax, %rcx jae .L8 movss %xmm2, (%rdx,%rcx,4) leal 3(%rsi), %ecx cmpq %rax, %rcx jae .L8 movss %xmm3, (%rdx,%rcx,4) addq $8, %rsp ret .L8: call __chk_fail For char/signed char/unsigned char, it's even worse because there's an additional pointer load per store (bringing up the total to two—the compiler cannot see that the _M_start pointer cannot point into the std::vector object). I'm no longer sure that's acceptable overhead. We really don't want programmers to switch off _FORTIFY_SOURCE because of its overhead. I'm attaching the patch just in case. I'm not sure the test case actually ran (if it did, it passed on first try, which would be unusual.)