Re: _FORTIFY_SOURCE for std::vector

2012-06-06 Thread Florian Weimer
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

Re: _FORTIFY_SOURCE for std::vector

2012-06-05 Thread Richard Guenther
On Mon, Jun 4, 2012 at 9:07 PM, Marc Glisse marc.gli...@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::vectorfloat blob,

Re: _FORTIFY_SOURCE for std::vector

2012-06-04 Thread Florian Weimer
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::vectorfloat blob, unsigned n, float v1, float v2, float v3, float v4) { blob[n] = v1; blob[n + 1] = v2; blob[n + 2] = v3; blob[n

Re: _FORTIFY_SOURCE for std::vector

2012-06-04 Thread Marc Glisse
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::vectorfloat blob, unsigned n, float v1, float v2, float v3, float v4) { blob[n] = v1;

Re: _FORTIFY_SOURCE for std::vector

2012-06-04 Thread Florian Weimer
On 06/04/2012 09:07 PM, Marc Glisse wrote: On Mon, 4 Jun 2012, Florian Weimer wrote: void write(std::vectorfloat 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

_FORTIFY_SOURCE for std::vector

2012-06-01 Thread Florian Weimer
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.

Re: _FORTIFY_SOURCE for std::vector

2012-06-01 Thread Jakub Jelinek
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

Re: _FORTIFY_SOURCE for std::vector

2012-06-01 Thread Paolo Carlini
Hi a bunch of minor comments, first: 1- What happens with make check-debug? I suppose the debug-mode checks trigger before the check in normal code, and the testcase doesn't pass. Maybe for this kind of work you need a // { dg-require-normal-mode } 2- Something sound weird with debug-mode

Re: _FORTIFY_SOURCE for std::vector

2012-06-01 Thread Florian Weimer
On 06/01/2012 01:42 PM, Paolo Carlini wrote: Hi a bunch of minor comments, first: 1- What happens with make check-debug? I suppose the debug-mode checks trigger before the check in normal code, and the testcase doesn't pass. Maybe for this kind of work you need a // { dg-require-normal-mode }

Re: _FORTIFY_SOURCE for std::vector

2012-06-01 Thread Paolo Carlini
Hi, On 06/01/2012 02:53 PM, Florian Weimer wrote: I've seen the variable, but I don't understand how to use it. I would like to add something about it to the testing documentation. Just copy what *all* the other testcase in the library testsuite have, just define it, with attribute unused. If

Re: _FORTIFY_SOURCE for std::vector

2012-06-01 Thread Paolo Carlini
On 06/01/2012 03:09 PM, Paolo Carlini wrote: You are right, sorry, I went through your changes too quickly and didn't realize that you are just using the existing _M_range_check. Anyway, I confirm that probably we want something more consistent with debug-mode, thus not throwing in this case.

Re: _FORTIFY_SOURCE for std::vector

2012-06-01 Thread Pedro Alves
On 06/01/2012 02:09 PM, Paolo Carlini wrote: 4- Maybe I'm misremembering, sorry in case, but Isn't: #if defined _FORTIFY_SOURCE _FORTIFY_SOURCE 0 the same as: #if _FORTIFY_SOURCE 0 ? I think you're right, but I'm just copying literally what GNU libc is doing. I can change it to

Re: _FORTIFY_SOURCE for std::vector

2012-06-01 Thread Jakub Jelinek
On Fri, Jun 01, 2012 at 03:09:19PM +0200, Paolo Carlini wrote: On 06/01/2012 02:53 PM, Florian Weimer wrote: I've seen the variable, but I don't understand how to use it. I would like to add something about it to the testing documentation. Just copy what *all* the other testcase in the

Re: _FORTIFY_SOURCE for std::vector

2012-06-01 Thread Paolo Carlini
On 06/01/2012 03:22 PM, Pedro Alves wrote: The shorter version triggers an -Wundef warning if _FORTIFY_SOURCE is not defined. Interesting, but does it happen in system headers too? Paolo.

Re: _FORTIFY_SOURCE for std::vector

2012-06-01 Thread Paolo Carlini
On 06/01/2012 03:34 PM, Jakub Jelinek wrote: The standard -D_FORTIFY_SOURCE failure is __chk_fail (), so IMNSHO if this is presented as _FORTIFY_SOURCE check, it should call that and not some other function. I understand. I don't know much about -D_FORTIFY_SOURCE, honestly. I hope the

Re: _FORTIFY_SOURCE for std::vector

2012-06-01 Thread Jakub Jelinek
On Fri, Jun 01, 2012 at 03:39:12PM +0200, Paolo Carlini wrote: On 06/01/2012 03:34 PM, Jakub Jelinek wrote: The standard -D_FORTIFY_SOURCE failure is __chk_fail (), so IMNSHO if this is presented as _FORTIFY_SOURCE check, it should call that and not some other function. I understand. I don't

Re: _FORTIFY_SOURCE for std::vector

2012-06-01 Thread Paolo Carlini
Hi, On 06/01/2012 03:49 PM, Jakub Jelinek wrote: On Fri, Jun 01, 2012 at 03:39:12PM +0200, Paolo Carlini wrote: On 06/01/2012 03:34 PM, Jakub Jelinek wrote: The standard -D_FORTIFY_SOURCE failure is __chk_fail (), so IMNSHO if this is presented as _FORTIFY_SOURCE check, it should call that

Re: _FORTIFY_SOURCE for std::vector

2012-06-01 Thread Jakub Jelinek
On Fri, Jun 01, 2012 at 03:52:24PM +0200, Paolo Carlini wrote: On 06/01/2012 03:49 PM, Jakub Jelinek wrote: On Fri, Jun 01, 2012 at 03:39:12PM +0200, Paolo Carlini wrote: On 06/01/2012 03:34 PM, Jakub Jelinek wrote: The standard -D_FORTIFY_SOURCE failure is __chk_fail (), so IMNSHO if this is

Re: _FORTIFY_SOURCE for std::vector

2012-06-01 Thread Pedro Alves
On 06/01/2012 02:36 PM, Paolo Carlini wrote: On 06/01/2012 03:22 PM, Pedro Alves wrote: The shorter version triggers an -Wundef warning if _FORTIFY_SOURCE is not defined. Interesting, but does it happen in system headers too? Good point. I just tried and it doesn't. -- Pedro Alves

Re: _FORTIFY_SOURCE for std::vector

2012-06-01 Thread Jakub Jelinek
On Fri, Jun 01, 2012 at 03:11:51PM +0100, Pedro Alves wrote: On 06/01/2012 02:36 PM, Paolo Carlini wrote: On 06/01/2012 03:22 PM, Pedro Alves wrote: The shorter version triggers an -Wundef warning if _FORTIFY_SOURCE is not defined. Interesting, but does it happen in system headers

Re: _FORTIFY_SOURCE for std::vector

2012-06-01 Thread Paolo Carlini
On 06/01/2012 04:04 PM, Jakub Jelinek wrote: Well, you have the core file often (unless disabled), or could use addr2line to decode the addresses. The point is that the fortification checks must be very lightweight (so that people can enable them by default for everything), and e.g. storing

Re: _FORTIFY_SOURCE for std::vector

2012-06-01 Thread Paolo Carlini
On 06/01/2012 04:13 PM, Jakub Jelinek wrote: On Fri, Jun 01, 2012 at 03:11:51PM +0100, Pedro Alves wrote: On 06/01/2012 02:36 PM, Paolo Carlini wrote: On 06/01/2012 03:22 PM, Pedro Alves wrote: The shorter version triggers an -Wundef warning if _FORTIFY_SOURCE is not defined. Interesting,

Re: _FORTIFY_SOURCE for std::vector

2012-06-01 Thread Florian Weimer
On 06/01/2012 03:34 PM, Jakub Jelinek wrote: The standard -D_FORTIFY_SOURCE failure is __chk_fail (), so IMNSHO if this is presented as _FORTIFY_SOURCE check, it should call that and not some other function. You'd need to use #if __USE_FORTIFY_LEVEL 0 test instead (as __chk_fail is only

Re: _FORTIFY_SOURCE for std::vector

2012-06-01 Thread Paolo Carlini
On 06/01/2012 05:00 PM, Florian Weimer wrote: On 06/01/2012 03:34 PM, Jakub Jelinek wrote: The standard -D_FORTIFY_SOURCE failure is __chk_fail (), so IMNSHO if this is presented as _FORTIFY_SOURCE check, it should call that and not some other function. You'd need to use #if

Re: _FORTIFY_SOURCE for std::vector

2012-06-01 Thread Jakub Jelinek
On Fri, Jun 01, 2012 at 05:00:58PM +0200, Florian Weimer wrote: On 06/01/2012 03:34 PM, Jakub Jelinek wrote: The standard -D_FORTIFY_SOURCE failure is __chk_fail (), so IMNSHO if this is presented as _FORTIFY_SOURCE check, it should call that and not some other function. You'd need to use

Re: _FORTIFY_SOURCE for std::vector

2012-06-01 Thread Florian Weimer
On 06/01/2012 05:09 PM, Jakub Jelinek wrote: __chk_fail it is, then. This means that the test case will be specific to GNU libc platforms. How can I mark it as such? { target *-*-linux* } or so? Wouldn't this match Bionic libc environments, too? Note, __chk_fail () isn't prototyped in

Re: _FORTIFY_SOURCE for std::vector

2012-05-30 Thread Florian Weimer
On 05/29/2012 06:45 PM, Paolo Carlini wrote: Hi, 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

_FORTIFY_SOURCE for std::vector

2012-05-29 Thread Florian Weimer
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. Okay for trunk?

Re: _FORTIFY_SOURCE for std::vector

2012-05-29 Thread Paolo Carlini
Hi, 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. Indeed. But at