Re: [HACKERS] [RFC] overflow checks optimized away
On Fri, Dec 4, 2015 at 1:27 AM, Andres Freundwrote: > On 2015-12-03 12:10:27 +, Greg Stark wrote: >> I'm leaning towards using the builtin functions described here > > For performance reasons? Michael's version of the patch had the > necessary 'raw' macros, and they don't look *that* bad. Using the > __builtin variants when available, would be nice - and not hard. On > e.g. x86 the overflow checks can be done much more efficiently than both > the current and patched checks. Using the _builtin functions when available would be indeed a nice optimization that the previous patch missed. > I wonder though if we can replace > > #define PG_INT16_ADD_OVERFLOWS(a, b) ( \ > ((a) > 0 && (b) > 0 && (a) > PG_INT16_MAX - (b)) || \ > ((a) < 0 && (b) < 0 && (a) < PG_INT16_MIN - (b))) > > #define PG_INT32_ADD_OVERFLOWS(a, b) ( \ > ((a) > 0 && (b) > 0 && (a) > PG_INT32_MAX - (b)) || \ > ((a) < 0 && (b) < 0 && (a) < PG_INT32_MIN - (b))) > > ... > > with something like > #define PG_ADD_OVERFLOWS(a, b, MINVAL, MAXVAL) (\ > ((a) > 0 && (b) > 0 && (a) > MAXVAL - (b)) || \ > ((a) < 0 && (b) < 0 && (a) < MINVAL - (b))) > #define PG_INT16_ADD_OVERFLOWS(a, b)\ > PG_ADD_OVERFLOWS(a, b, PG_INT16_MIN, PG_INT16_MAX) > > especially for the MUL variants that'll save a bunch of long repetitions. Yeah, we should. Those would save quite a couple of lines in c.h. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [RFC] overflow checks optimized away
On Tue, Dec 1, 2015 at 5:17 PM, Greg Starkwrote: > Sorry, I didn't look at it since. At the time I was using Xi Wang's software > to find the overflow checks that need to be redone. He published a paper on > it and it's actually pretty impressive. It constructs a constraint problem > and then throws a kSAT solver at it to find out if there's any code that a > compiler could optimize away regardless of whether any existant compiler is > actually capable of detecting the case and optimizing it away. > https://pdos.csail.mit.edu/papers/stack:sosp13.pdf I did get this code running again (it was quite a hassle actually). Attached is the report. I'm leaning towards using the builtin functions described here http://clang.llvm.org/docs/LanguageExtensions.html#checked-arithmetic-builtins https://gcc.gnu.org/onlinedocs/gcc/Integer-Overflow-Builtins.html They aren't in older GCC and clang and probably won't be in icc and the like but we could probably implement replacements. The downside is that then we wouldn't be able to use the generic one and would have to use the type-specific ones which would be annoying. The Linux kernel folk wrote wrappers that don't have that problem but they depend on type_min() and type_max() which I've never heard of and have no idea what support they have? What version of GCC and other compilers did we decide we're targeting now? -- greg --- bug: anti-simplify model: | %cmp48 = icmp ne i8* %30, null, !dbg !1018 --> true stack: - /home/stark/src/pg/postgresql-master/src/backend/storage/lmgr/lmgr.c:712:0 ncore: 1 core: - /home/stark/src/pg/postgresql-master/src/backend/storage/lmgr/lmgr.c:713:0 - null pointer dereference --- bug: anti-algebra model: | %cmp = icmp ult i8* %add.ptr1, %start_address, !dbg !500 --> %16 = icmp slt i64 %15, 0, !dbg !500 (4 + (8 * (sext i32 %7 to i64)) + %start_address) (4 + (8 * (sext i32 %7 to i64))) false stack: - /home/stark/src/pg/postgresql-master/src/backend/utils/adt/int.c:756:0 ncore: 1 core: - /home/stark/src/pg/postgresql-master/src/backend/utils/adt/int.c:754:0 - signed addition overflow --- bug: anti-algebra model: | %cmp2 = icmp slt i32 %add1, %s, !dbg !570 --> %6 = icmp slt i32 %l, 0, !dbg !570 (%s + %l) %l false stack: - /home/stark/src/pg/postgresql-master/src/backend/utils/adt/varbit.c:1170:0 ncore: 1 core: - /home/stark/src/pg/postgresql-master/src/backend/utils/adt/varbit.c:1169:0 - signed addition overflow --- bug: anti-algebra model: | %cmp3 = icmp slt i32 %add, %start, !dbg !878 --> %2 = icmp slt i32 %length, 0, !dbg !878 (%start + %length) %length %29 = icmp slt i32 %length, 0, !dbg !907 (%start + %length) %length false stack: - /home/stark/src/pg/postgresql-master/src/backend/utils/adt/varlena.c:1047:0 ncore: 1 core: - /home/stark/src/pg/postgresql-master/src/backend/utils/adt/varlena.c:1046:0 - signed addition overflow --- bug: anti-algebra model: | %cmp1 = icmp slt i32 %add, %S, !dbg !875 --> %2 = icmp slt i32 %L, 0, !dbg !875 (%S + %L) %L false stack: - /home/stark/src/pg/postgresql-master/src/backend/utils/adt/varlena.c:2666:0 ncore: 1 core: - /home/stark/src/pg/postgresql-master/src/backend/utils/adt/varlena.c:2665:0 - signed addition overflow --- bug: anti-simplify model: | %cmp19 = icmp slt i64 %mul, 0, !dbg !583 --> false stack: - /home/stark/src/pg/postgresql-master/src/backend/utils/adt/int8.c:576:0 ncore: 1 core: - /home/stark/src/pg/postgresql-master/src/backend/utils/adt/int8.c:561:0 - signed multiplication overflow --- bug: anti-simplify model: | %cmp2 = icmp sgt i64 %1, 0, !dbg !580 --> false stack: - /home/stark/src/pg/postgresql-master/src/backend/utils/adt/int8.c:711:0 ncore: 1 core: - /home/stark/src/pg/postgresql-master/src/backend/utils/adt/int8.c:709:0 - signed addition overflow --- bug: anti-simplify model: | %cmp2 = icmp slt i64 %1, 0, !dbg !580 --> false stack: - /home/stark/src/pg/postgresql-master/src/backend/utils/adt/int8.c:755:0 ncore: 1 core: - /home/stark/src/pg/postgresql-master/src/backend/utils/adt/int8.c:753:0 - signed subtraction overflow --- bug: anti-dce model: | %cmp36 = icmp eq i64 %val.1, 0, !dbg !964 --> true lor.lhs.false38: %cmp39 = icmp slt i64 %val.0, 0, !dbg !964 br i1 %cmp39, label %if.then41, label %if.end42, !dbg !964 stack: - /home/stark/src/pg/postgresql-master/src/backend/utils/adt/numeric.c:5233:0 ncore: 1 core: - /home/stark/src/pg/postgresql-master/src/backend/utils/adt/numeric.c:5233:0 - signed subtraction overflow
Re: [HACKERS] [RFC] overflow checks optimized away
Greg Starkwrites: > What version of GCC and other compilers did we decide we're targeting now? I can't see us moving the compiler goalposts one inch for this. "I'm going to break building on your compiler in order to work around bugs in somebody else's compiler" isn't gonna fly. The original post pointed out that we haven't introduced the appropriate equivalents of -fwrapv for non-gcc compilers, which is a good point that we should fix. Beyond that, I'm not convinced. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [RFC] overflow checks optimized away
On Thu, Dec 3, 2015 at 2:51 PM, Tom Lanewrote: > I can't see us moving the compiler goalposts one inch for this. > "I'm going to break building on your compiler in order to work around > bugs in somebody else's compiler" isn't gonna fly. Fwiw the builtins offer a carrot as well. They promise to use architecture features like arithmetic status flags which can be faster than explicit comparisons and also avoid extra branches that can mess up cache and branch prediction. I was proposing to implement wrappers around them that do the checks manually if they're not present. -- greg -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [RFC] overflow checks optimized away
Greg Starkwrites: > On Thu, Dec 3, 2015 at 2:51 PM, Tom Lane wrote: >> I can't see us moving the compiler goalposts one inch for this. >> "I'm going to break building on your compiler in order to work around >> bugs in somebody else's compiler" isn't gonna fly. > I was proposing to implement wrappers around them that do the checks > manually if they're not present. As long as there's a fallback for compilers without the builtins, fine. I read your question as suggesting that you were thinking about not providing a fallback ... regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [RFC] overflow checks optimized away
On 2015-12-03 12:10:27 +, Greg Stark wrote: > I'm leaning towards using the builtin functions described here For performance reasons? Michael's version of the patch had the necessary 'raw' macros, and they don't look *that* bad. Using the __builtin variants when available, would be nice - and not hard. On e.g. x86 the overflow checks can be done much more efficiently than both the current and patched checks. I wonder though if we can replace #define PG_INT16_ADD_OVERFLOWS(a, b) ( \ ((a) > 0 && (b) > 0 && (a) > PG_INT16_MAX - (b)) || \ ((a) < 0 && (b) < 0 && (a) < PG_INT16_MIN - (b))) #define PG_INT32_ADD_OVERFLOWS(a, b) ( \ ((a) > 0 && (b) > 0 && (a) > PG_INT32_MAX - (b)) || \ ((a) < 0 && (b) < 0 && (a) < PG_INT32_MIN - (b))) ... with something like #define PG_ADD_OVERFLOWS(a, b, MINVAL, MAXVAL) (\ ((a) > 0 && (b) > 0 && (a) > MAXVAL - (b)) || \ ((a) < 0 && (b) < 0 && (a) < MINVAL - (b))) #define PG_INT16_ADD_OVERFLOWS(a, b)\ PG_ADD_OVERFLOWS(a, b, PG_INT16_MIN, PG_INT16_MAX) especially for the MUL variants that'll save a bunch of long repetitions. > The downside is that then we wouldn't be able to use the generic one > and would have to use the type-specific ones which would be annoying. Doesn't seem to be all that bad to me. If we do the fallback like in the above it should be fairly ok repetition wise. Greetings, Andres Freund -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [RFC] overflow checks optimized away
On Thu, Dec 3, 2015 at 2:51 PM, Tom Lanewrote: > Greg Stark writes: >> What version of GCC and other compilers did we decide we're targeting now? > > I can't see us moving the compiler goalposts one inch for this. That's not the question I asked :/ -- greg -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [RFC] overflow checks optimized away
On Fri, Oct 9, 2015 at 2:52 PM, Bruce Momjianwrote: > Any news on this item from 2013, worked on again 2014? > Sorry, I didn't look at it since. At the time I was using Xi Wang's software to find the overflow checks that need to be redone. He published a paper on it and it's actually pretty impressive. It constructs a constraint problem and then throws a kSAT solver at it to find out if there's any code that a compiler could optimize away regardless of whether any existant compiler is actually capable of detecting the case and optimizing it away. https://pdos.csail.mit.edu/papers/stack:sosp13.pdf Xi Wang actually went on to pursue the same issues in the Linux kernel, Clang, and elsewhere: https://css.csail.mit.edu/stack/ http://kqueue.org/blog/2012/03/16/fast-integer-overflow-detection/ http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20130617/082221.html I'm up for trying again but it's a long slong to replace all the overflow checks and the patch will be big -- greg
Re: [HACKERS] [RFC] overflow checks optimized away
On Tue, Oct 20, 2015 at 4:25 PM, Michael Paquier wrote: > I'll add that to the next CF, perhaps this will interest somebody. And nobody got interested into that, marking as returned with feedback. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [RFC] overflow checks optimized away
On Fri, Oct 16, 2015 at 11:29 PM, Michael Paquier wrote: > Well, I have played a bit with this patch and rebased it as attached. > One major change is the use of the variables PG_INT* that have been > added in 62e2a8d. Some places were not updated with those new checks, > in majority a couple of routines in int.c (I haven't finished > monitoring the whole code though). Also, I haven't played yet with my > compilers to optimize away some of the checks and break them, but I'll > give it a try with clang and gcc. For now, I guess that this patch is > a good thing to begin with though, I have checked that code compiles > and regression tests pass. Hm. Looking at the options of clang (http://clang.llvm.org/docs/UsersManual.html), there is no actual equivalent of fno-wrapv and strict-overflow, there are a couple of sanitizer functions though (-fsanitize=unsigned-integer-overflow -fsanitize=signed-integer-overflow) that can be used at run time, but that's not really useful for us. I also looked at the definition of SHRT_MIN/MAX in a couple of places (OSX, Linux, MinGW, Solaris, OpenBSD, FreeBSD, MSVC), and they are always set as respectively -7fff-1 and 7fff. Hence do we really need to worry about those two having potentially a different length, are there opinions about having customs PG_SHRT_MIN/PG_SHRT_MAX in c.h? Except that, attached is a result of all the hacking I have been doing regarding this item: - Addition of some macros to check overflows for INT16 - Addition of a couple of regression tests (Does testing int2inc & friends make sense?) - Addition of consistent overflow checks in more code paths, previous patch missing a couple of places in int8.c, int.c and btree_gist (+ alpha). I have screened the code for existing "out of range" errors. I'll add that to the next CF, perhaps this will interest somebody. Regards, -- Michael diff --git a/contrib/btree_gist/btree_int2.c b/contrib/btree_gist/btree_int2.c index 54dc1cc..202bd21 100644 --- a/contrib/btree_gist/btree_int2.c +++ b/contrib/btree_gist/btree_int2.c @@ -102,7 +102,7 @@ int2_dist(PG_FUNCTION_ARGS) ra = Abs(r); /* Overflow check. */ - if (ra < 0 || (!SAMESIGN(a, b) && !SAMESIGN(r, a))) + if (ra < 0 || PG_INT16_SUB_OVERFLOWS(a, b)) ereport(ERROR, (errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE), errmsg("smallint out of range"))); diff --git a/contrib/btree_gist/btree_int4.c b/contrib/btree_gist/btree_int4.c index ddbcf52..890a7d4 100644 --- a/contrib/btree_gist/btree_int4.c +++ b/contrib/btree_gist/btree_int4.c @@ -103,7 +103,7 @@ int4_dist(PG_FUNCTION_ARGS) ra = Abs(r); /* Overflow check. */ - if (ra < 0 || (!SAMESIGN(a, b) && !SAMESIGN(r, a))) + if (ra < 0 || PG_INT32_SUB_OVERFLOWS(a, b)) ereport(ERROR, (errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE), errmsg("integer out of range"))); diff --git a/contrib/btree_gist/btree_int8.c b/contrib/btree_gist/btree_int8.c index 44bf69a..91d4972 100644 --- a/contrib/btree_gist/btree_int8.c +++ b/contrib/btree_gist/btree_int8.c @@ -103,7 +103,7 @@ int8_dist(PG_FUNCTION_ARGS) ra = Abs(r); /* Overflow check. */ - if (ra < 0 || (!SAMESIGN(a, b) && !SAMESIGN(r, a))) + if (ra < 0 || PG_INT64_SUB_OVERFLOWS(a, b)) ereport(ERROR, (errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE), errmsg("bigint out of range"))); diff --git a/src/backend/executor/execQual.c b/src/backend/executor/execQual.c index 29f058c..9b4b890 100644 --- a/src/backend/executor/execQual.c +++ b/src/backend/executor/execQual.c @@ -3185,7 +3185,8 @@ ExecEvalArray(ArrayExprState *astate, ExprContext *econtext, /* Get sub-array details from first member */ elem_ndims = this_ndims; ndims = elem_ndims + 1; -if (ndims <= 0 || ndims > MAXDIM) + +if (PG_INT32_ADD_OVERFLOWS(elem_ndims,1) || ndims > MAXDIM) ereport(ERROR, (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED), errmsg("number of array dimensions (%d) exceeds " \ diff --git a/src/backend/utils/adt/array_userfuncs.c b/src/backend/utils/adt/array_userfuncs.c index c14ea23..6ad97da 100644 --- a/src/backend/utils/adt/array_userfuncs.c +++ b/src/backend/utils/adt/array_userfuncs.c @@ -113,8 +113,9 @@ array_append(PG_FUNCTION_ARGS) ub = dimv[0] + lb[0] - 1; indx = ub + 1; - /* overflow? */ - if (indx < ub) + /* check for overflow in upper bound (indx + 1) */ + if (PG_INT32_ADD_OVERFLOWS(dimv[0] ,lb[0]) || + PG_INT32_ADD_OVERFLOWS(dimv[0] + lb[0], 1)) ereport(ERROR, (errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE), errmsg("integer out of range"))); @@ -168,7 +169,7 @@ array_prepend(PG_FUNCTION_ARGS) lb0 = lb[0]; /* overflow? */ - if (indx > lb[0]) + if (PG_INT32_ADD_OVERFLOWS(lb[0], -1)) ereport(ERROR, (errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE), errmsg("integer out of range"))); diff --git a/src/backend/utils/adt/float.c b/src/backend/utils/adt/float.c index 4e927d8..cc4c570 100644 --- a/src/backend/utils/adt/float.c +++ b/src/backend/utils/adt/float.c @@ -2763,12
Re: [HACKERS] [RFC] overflow checks optimized away
On Fri, Oct 9, 2015 at 10:52 PM, Bruce Momjianwrote: > > Any news on this item from 2013, worked on again 2014? > > --- > > On Wed, Aug 6, 2014 at 12:55:59PM -0400, Bruce Momjian wrote: >> On Fri, Nov 29, 2013 at 02:04:10AM +, Greg Stark wrote: >> > Attached is what I have so far. I have to say I'm starting to come >> > around to Tom's point of view. This is a lot of hassle for not much >> > gain. i've noticed a number of other overflow checks that the llvm >> > optimizer is not picking up on so even after this patch it's not clear >> > that all the signed overflow checks that depend on -fwrapv will be >> > gone. >> > >> > This patch still isn't quite finished though. >> > >> > a) It triggers a spurious gcc warning about overflows on constant >> > expressions. These value of these expressions aren't actually being >> > used as they're used in the other branch of the overflow test. I think >> > I see how to work around it for gcc using __builtin_choose_expr but it >> > might be pretty grotty. >> > >> > b) I'm concerned these checks depend on INT_MIN/MAX and SHRT_MIN/MAX >> > which may not be exactly the right length. I'm kind of confused why >> > c.h assumes long is 32 bits and short is 16 bits though so I don't >> > think I'm making it any worse. It may be better for us to just define >> > our own limits since we know exactly how large we expect these data >> > types to be. >> > >> > c) I want to add regression tests that will ensure that the overflow >> > checks are all working. So far I haven't been able to catch any being >> > optimized away even with -fno-wrapv and -fstrict-overflow. I think I >> > just didn't build with the right options though and it should be >> > possible. >> > >> > The goal here imho is to allow building with -fno-wrapv >> > -fstrict-overflow safely. Whether we actually do or not is another >> > question but it means we would be able to use new compilers like clang >> > without worrying about finding the equivalent of -fwrapv for them. >> >> Where are we on this? Well, I have played a bit with this patch and rebased it as attached. One major change is the use of the variables PG_INT* that have been added in 62e2a8d. Some places were not updated with those new checks, in majority a couple of routines in int.c (I haven't finished monitoring the whole code though). Also, I haven't played yet with my compilers to optimize away some of the checks and break them, but I'll give it a try with clang and gcc. For now, I guess that this patch is a good thing to begin with though, I have checked that code compiles and regression tests pass. Regards, -- Michael diff --git a/src/backend/executor/execQual.c b/src/backend/executor/execQual.c index 29f058c..9b4b890 100644 --- a/src/backend/executor/execQual.c +++ b/src/backend/executor/execQual.c @@ -3185,7 +3185,8 @@ ExecEvalArray(ArrayExprState *astate, ExprContext *econtext, /* Get sub-array details from first member */ elem_ndims = this_ndims; ndims = elem_ndims + 1; -if (ndims <= 0 || ndims > MAXDIM) + +if (PG_INT32_ADD_OVERFLOWS(elem_ndims,1) || ndims > MAXDIM) ereport(ERROR, (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED), errmsg("number of array dimensions (%d) exceeds " \ diff --git a/src/backend/utils/adt/array_userfuncs.c b/src/backend/utils/adt/array_userfuncs.c index c14ea23..a6b0d1b 100644 --- a/src/backend/utils/adt/array_userfuncs.c +++ b/src/backend/utils/adt/array_userfuncs.c @@ -113,8 +113,9 @@ array_append(PG_FUNCTION_ARGS) ub = dimv[0] + lb[0] - 1; indx = ub + 1; - /* overflow? */ - if (indx < ub) + /* check for overflow in upper bound (indx+1) */ + if (PG_INT32_ADD_OVERFLOWS(dimv[0],lb[0]) || + PG_INT32_ADD_OVERFLOWS(dimv[0]+lb[0], 1)) ereport(ERROR, (errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE), errmsg("integer out of range"))); @@ -168,7 +169,7 @@ array_prepend(PG_FUNCTION_ARGS) lb0 = lb[0]; /* overflow? */ - if (indx > lb[0]) + if (PG_INT32_ADD_OVERFLOWS(lb[0], -1)) ereport(ERROR, (errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE), errmsg("integer out of range"))); diff --git a/src/backend/utils/adt/float.c b/src/backend/utils/adt/float.c index 4e927d8..cc4c570 100644 --- a/src/backend/utils/adt/float.c +++ b/src/backend/utils/adt/float.c @@ -2763,12 +2763,12 @@ width_bucket_float8(PG_FUNCTION_ARGS) result = 0; else if (operand >= bound2) { - result = count + 1; /* check for overflow */ - if (result < count) + if (PG_INT32_ADD_OVERFLOWS(count, 1)) ereport(ERROR, (errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE), errmsg("integer out of range"))); + result = count + 1; } else result = ((float8) count * (operand - bound1) / (bound2 - bound1)) + 1; @@ -2779,12 +2779,12 @@ width_bucket_float8(PG_FUNCTION_ARGS) result = 0; else if (operand <= bound2) { - result = count + 1; /*
Re: [HACKERS] [RFC] overflow checks optimized away
Any news on this item from 2013, worked on again 2014? --- On Wed, Aug 6, 2014 at 12:55:59PM -0400, Bruce Momjian wrote: > On Fri, Nov 29, 2013 at 02:04:10AM +, Greg Stark wrote: > > Attached is what I have so far. I have to say I'm starting to come > > around to Tom's point of view. This is a lot of hassle for not much > > gain. i've noticed a number of other overflow checks that the llvm > > optimizer is not picking up on so even after this patch it's not clear > > that all the signed overflow checks that depend on -fwrapv will be > > gone. > > > > This patch still isn't quite finished though. > > > > a) It triggers a spurious gcc warning about overflows on constant > > expressions. These value of these expressions aren't actually being > > used as they're used in the other branch of the overflow test. I think > > I see how to work around it for gcc using __builtin_choose_expr but it > > might be pretty grotty. > > > > b) I'm concerned these checks depend on INT_MIN/MAX and SHRT_MIN/MAX > > which may not be exactly the right length. I'm kind of confused why > > c.h assumes long is 32 bits and short is 16 bits though so I don't > > think I'm making it any worse. It may be better for us to just define > > our own limits since we know exactly how large we expect these data > > types to be. > > > > c) I want to add regression tests that will ensure that the overflow > > checks are all working. So far I haven't been able to catch any being > > optimized away even with -fno-wrapv and -fstrict-overflow. I think I > > just didn't build with the right options though and it should be > > possible. > > > > The goal here imho is to allow building with -fno-wrapv > > -fstrict-overflow safely. Whether we actually do or not is another > > question but it means we would be able to use new compilers like clang > > without worrying about finding the equivalent of -fwrapv for them. > > Where are we on this? > > -- > Bruce Momjianhttp://momjian.us > EnterpriseDB http://enterprisedb.com > > + Everyone has their own god. + > > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers -- Bruce Momjian http://momjian.us EnterpriseDB http://enterprisedb.com + As you are, so once was I. As I am, so you will be. + + Roman grave inscription + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [RFC] overflow checks optimized away
On Fri, Nov 29, 2013 at 02:04:10AM +, Greg Stark wrote: Attached is what I have so far. I have to say I'm starting to come around to Tom's point of view. This is a lot of hassle for not much gain. i've noticed a number of other overflow checks that the llvm optimizer is not picking up on so even after this patch it's not clear that all the signed overflow checks that depend on -fwrapv will be gone. This patch still isn't quite finished though. a) It triggers a spurious gcc warning about overflows on constant expressions. These value of these expressions aren't actually being used as they're used in the other branch of the overflow test. I think I see how to work around it for gcc using __builtin_choose_expr but it might be pretty grotty. b) I'm concerned these checks depend on INT_MIN/MAX and SHRT_MIN/MAX which may not be exactly the right length. I'm kind of confused why c.h assumes long is 32 bits and short is 16 bits though so I don't think I'm making it any worse. It may be better for us to just define our own limits since we know exactly how large we expect these data types to be. c) I want to add regression tests that will ensure that the overflow checks are all working. So far I haven't been able to catch any being optimized away even with -fno-wrapv and -fstrict-overflow. I think I just didn't build with the right options though and it should be possible. The goal here imho is to allow building with -fno-wrapv -fstrict-overflow safely. Whether we actually do or not is another question but it means we would be able to use new compilers like clang without worrying about finding the equivalent of -fwrapv for them. Where are we on this? -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [RFC] overflow checks optimized away
On 11/29/2013 04:04 AM, Greg Stark wrote: Attached is what I have so far. I have to say I'm starting to come around to Tom's point of view. This is a lot of hassle for not much gain. i've noticed a number of other overflow checks that the llvm optimizer is not picking up on so even after this patch it's not clear that all the signed overflow checks that depend on -fwrapv will be gone. This patch still isn't quite finished though. In addition to what you have in the patch, Coverity is complaining about the overflow checks in int4abs (which is just like int8abs), and in DetermineTimeZoneOffset. - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [RFC] overflow checks optimized away
Greg Stark st...@mit.edu writes: b) I'm concerned these checks depend on INT_MIN/MAX and SHRT_MIN/MAX which may not be exactly the right length. I'm kind of confused why c.h assumes long is 32 bits and short is 16 bits though so I don't think I'm making it any worse. I think it's something we figured we could make configure deal with if it ever proved necessary; which it hasn't yet. (In practice, unless an implementor is going to omit support for these integer widths entirely, he doesn't have too much choice but to assign them the names short and int --- C doesn't provide all that many names he can use.) It may be better for us to just define our own limits since we know exactly how large we expect these data types to be. Yeah, using INT16_MAX etc would likely be more transparent, if the code is declaring the variables as int16. c) I want to add regression tests that will ensure that the overflow checks are all working. So far I haven't been able to catch any being optimized away even with -fno-wrapv and -fstrict-overflow. This does not leave me with a warm feeling about whether this is a useful exercise. Maybe you need to try a different compiler? regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [RFC] overflow checks optimized away
On Fri, Nov 29, 2013 at 5:21 PM, Tom Lane t...@sss.pgh.pa.us wrote: c) I want to add regression tests that will ensure that the overflow checks are all working. So far I haven't been able to catch any being optimized away even with -fno-wrapv and -fstrict-overflow. This does not leave me with a warm feeling about whether this is a useful exercise. Maybe you need to try a different compiler? Just as an update I did get gcc to do the wrong thing on purpose. The only overflow check that the regression tests find missing is the one for int8abs() ie: *** /home/stark/src/postgresql/postgresql/src/test/regress/expected/int8.out Wed Jul 17 19:23:02 2013 --- /home/stark/src/postgresql/postgresql/src/test/regress/results/int8.out Fri Nov 29 14:22:31 2013 *** *** 674,680 select '9223372036854775800'::int8 % '0'::int8; ERROR: division by zero select abs('-9223372036854775808'::int8); ! ERROR: bigint out of range select '9223372036854775800'::int8 + '100'::int4; ERROR: bigint out of range select '-9223372036854775800'::int8 - '100'::int4; --- 674,684 select '9223372036854775800'::int8 % '0'::int8; ERROR: division by zero select abs('-9223372036854775808'::int8); ! abs ! -- ! -9223372036854775808 ! (1 row) ! select '9223372036854775800'::int8 + '100'::int4; ERROR: bigint out of range select '-9223372036854775800'::int8 - '100'::int4; == -- greg -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [RFC] overflow checks optimized away
On Fri, Nov 29, 2013 at 7:39 PM, Greg Stark st...@mit.edu wrote: Just as an update I did get gcc to do the wrong thing on purpose. The only overflow check that the regression tests find missing is the one for int8abs() ie: Also, one of the places GCC warns about optimizing away an overflow check (with -fno-wrapv) is inside the localtime.c file from the tz library. I fixed it in my patch but in fact I checked and it's already fixed upstream so I'm wondering whether you expect to merge in an updated tz library? Is there anything surprising about the process or do you just copy in the files? Would you be happy for someone else to do it? -- greg -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [RFC] overflow checks optimized away
On 11/29/2013 10:06 PM, Greg Stark wrote: On Fri, Nov 29, 2013 at 7:39 PM, Greg Stark st...@mit.edu wrote: Just as an update I did get gcc to do the wrong thing on purpose. The only overflow check that the regression tests find missing is the one for int8abs() ie: Also, one of the places GCC warns about optimizing away an overflow check (with -fno-wrapv) is inside the localtime.c file from the tz library. I fixed it in my patch but in fact I checked and it's already fixed upstream so I'm wondering whether you expect to merge in an updated tz library? Is there anything surprising about the process or do you just copy in the files? Would you be happy for someone else to do it? IIRC some files can be copied directly, but not all. Might be easiest to generate a diff between the last version in PostgreSQL and the latest upstream version, and apply that. - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [RFC] overflow checks optimized away
Greg Stark st...@mit.edu writes: Also, one of the places GCC warns about optimizing away an overflow check (with -fno-wrapv) is inside the localtime.c file from the tz library. I fixed it in my patch but in fact I checked and it's already fixed upstream so I'm wondering whether you expect to merge in an updated tz library? Is there anything surprising about the process or do you just copy in the files? Would you be happy for someone else to do it? We've made a number of changes in our copies, unfortunately. What you have to do is look at the upstream diffs since we last synchronized with them (which according to src/timezone/README was tzcode 2010c) and merge those diffs as appropriate. It should be reasonably mechanical, but don't forget to update the README. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [RFC] overflow checks optimized away
On Mon, Sep 9, 2013 at 12:21:56PM -0400, Robert Haas wrote: On Sat, Sep 7, 2013 at 6:55 PM, Greg Stark st...@mit.edu wrote: Should these patches be applied? I have a copy of the program and was going to take care of this. When? 2.5 months later, status report? -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [RFC] overflow checks optimized away
On Sat, Sep 7, 2013 at 6:55 PM, Greg Stark st...@mit.edu wrote: Should these patches be applied? I have a copy of the program and was going to take care of this. When? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [RFC] overflow checks optimized away
On Mon, Jul 15, 2013 at 06:19:27PM -0400, Alvaro Herrera wrote: Robert Haas escribió: On Mon, Jul 15, 2013 at 5:59 PM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Xi Wang escribió: Intel's icc and PathScale's pathcc compilers optimize away several overflow checks, since they consider signed integer overflow as undefined behavior. This leads to a vulnerable binary. This thread died without reaching a conclusion. Noah Misch, Robert Haas and Greg Stark each gave a +1 to the patches, but Tom Lane gave them a -inf; so they weren't applied. However, I think everyone walked away with the feeling that Tom is wrong on this. Meanwhile Xi Wang and team published a paper: http://pdos.csail.mit.edu/~xi/papers/stack-sosp13.pdf Postgres is mentioned a number of times in this paper -- mainly to talk about the bugs we leave unfixed. It might prove useful to have usable these guys' STACK checker output available continuously, so that if we happen to introduce more bugs in the future, it alerts us about that. Yeah, I think we ought to apply those patches. I don't suppose you have links handy? Sure, see this thread in the archives: first one is at http://www.postgresql.org/message-id/510100aa.4050...@gmail.com and you can see the others in the dropdown (though since the subjects are not shown, only date and author, it's a bit hard to follow. The flat URL is useful.) Should these patches be applied? -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [RFC] overflow checks optimized away
Should these patches be applied? I have a copy of the program and was going to take care of this. -- greg -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [RFC] overflow checks optimized away
Xi Wang escribió: Intel's icc and PathScale's pathcc compilers optimize away several overflow checks, since they consider signed integer overflow as undefined behavior. This leads to a vulnerable binary. This thread died without reaching a conclusion. Noah Misch, Robert Haas and Greg Stark each gave a +1 to the patches, but Tom Lane gave them a -inf; so they weren't applied. However, I think everyone walked away with the feeling that Tom is wrong on this. Meanwhile Xi Wang and team published a paper: http://pdos.csail.mit.edu/~xi/papers/stack-sosp13.pdf Postgres is mentioned a number of times in this paper -- mainly to talk about the bugs we leave unfixed. It might prove useful to have usable these guys' STACK checker output available continuously, so that if we happen to introduce more bugs in the future, it alerts us about that. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [RFC] overflow checks optimized away
On Mon, Jul 15, 2013 at 5:59 PM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Xi Wang escribió: Intel's icc and PathScale's pathcc compilers optimize away several overflow checks, since they consider signed integer overflow as undefined behavior. This leads to a vulnerable binary. This thread died without reaching a conclusion. Noah Misch, Robert Haas and Greg Stark each gave a +1 to the patches, but Tom Lane gave them a -inf; so they weren't applied. However, I think everyone walked away with the feeling that Tom is wrong on this. Meanwhile Xi Wang and team published a paper: http://pdos.csail.mit.edu/~xi/papers/stack-sosp13.pdf Postgres is mentioned a number of times in this paper -- mainly to talk about the bugs we leave unfixed. It might prove useful to have usable these guys' STACK checker output available continuously, so that if we happen to introduce more bugs in the future, it alerts us about that. Yeah, I think we ought to apply those patches. I don't suppose you have links handy? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [RFC] overflow checks optimized away
Robert Haas escribió: On Mon, Jul 15, 2013 at 5:59 PM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Xi Wang escribió: Intel's icc and PathScale's pathcc compilers optimize away several overflow checks, since they consider signed integer overflow as undefined behavior. This leads to a vulnerable binary. This thread died without reaching a conclusion. Noah Misch, Robert Haas and Greg Stark each gave a +1 to the patches, but Tom Lane gave them a -inf; so they weren't applied. However, I think everyone walked away with the feeling that Tom is wrong on this. Meanwhile Xi Wang and team published a paper: http://pdos.csail.mit.edu/~xi/papers/stack-sosp13.pdf Postgres is mentioned a number of times in this paper -- mainly to talk about the bugs we leave unfixed. It might prove useful to have usable these guys' STACK checker output available continuously, so that if we happen to introduce more bugs in the future, it alerts us about that. Yeah, I think we ought to apply those patches. I don't suppose you have links handy? Sure, see this thread in the archives: first one is at http://www.postgresql.org/message-id/510100aa.4050...@gmail.com and you can see the others in the dropdown (though since the subjects are not shown, only date and author, it's a bit hard to follow. The flat URL is useful.) -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] [RFC] overflow checks optimized away
Intel's icc and PathScale's pathcc compilers optimize away several overflow checks, since they consider signed integer overflow as undefined behavior. This leads to a vulnerable binary. Currently we use -fwrapv to disable such (mis)optimizations in gcc, but not in other compilers. Examples 1) x + 1 = 0 (assuming x 0). src/backend/executor/execQual.c:3088 Below is the simplified code. - void bar(void); void foo(int this_ndims) { if (this_ndims = 0) return; int elem_ndims = this_ndims; int ndims = elem_ndims + 1; if (ndims = 0) bar(); } - $ icc -S -o - sadd1.c ... foo: # parameter 1: %edi ..B1.1: ..___tag_value_foo.1: ..B1.2: ret 2) x + 1 x src/backend/utils/adt/float.c:2769 src/backend/utils/adt/float.c:2785 src/backend/utils/adt/oracle_compat.c:1045 (x + C x) Below is the simplified code. - void bar(void); void foo(int count) { int result = count + 1; if (result count) bar(); } - $ icc -S -o - sadd2.c ... foo: # parameter 1: %edi ..B1.1: ..___tag_value_foo.1: ret 3) x + y = x (assuming y 0) src/backend/utils/adt/varbit.c:1142 src/backend/utils/adt/varlena.c:1001 src/backend/utils/adt/varlena.c:2024 src/pl/plpgsql/src/pl_exec.c:1975 src/pl/plpgsql/src/pl_exec.c:1981 Below is the simplified code. - void bar(void); void foo(int sp, int sl) { if (sp = 0) return; int sp_pl_sl = sp + sl; if (sp_pl_sl = sl) bar(); } - $ icc -S -o - sadd3.c foo: # parameter 1: %edi # parameter 2: %esi ..B1.1: ..___tag_value_foo.1: ..B1.2: ret Possible fixes == * Recent versions of icc and pathcc support gcc's workaround option, -fno-strict-overflow, to disable some optimizations based on signed integer overflow. It's better to add this option to configure. They don't support gcc's -fwrapv yet. * This -fno-strict-overflow option cannot help in all cases: it cannot prevent the latest icc from (mis)compiling the 1st case. We could also fix the source code by avoiding signed integer overflows, as follows. x + y = 0 (assuming x 0, y 0) -- x INT_MAX - y x + y = x (assuming y 0) -- x INT_MAX - y I'd suggest to fix the code rather than trying to work around the compilers since the fix seems simple and portable. See two recent compiler bugs of -fwrapv/-fno-strict-overflow as well. http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55883 http://software.intel.com/en-us/forums/topic/358200 * I don't have access to IBM's xlc compiler. Not sure how it works for the above cases. - xi -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] [RFC] overflow checks optimized away
Intel's icc and PathScale's pathcc compilers optimize away several overflow checks, since they consider signed integer overflow as undefined behavior. This leads to a vulnerable binary. Currently we use -fwrapv to disable such (mis)optimizations in gcc, but not in other compilers. Examples 1) x + 1 = 0 (assuming x 0). src/backend/executor/execQual.c:3088 Below is the simplified code. - void bar(void); void foo(int this_ndims) { if (this_ndims = 0) return; int elem_ndims = this_ndims; int ndims = elem_ndims + 1; if (ndims = 0) bar(); } - $ icc -S -o - sadd1.c ... foo: # parameter 1: %edi ..B1.1: ..___tag_value_foo.1: ..B1.2: ret 2) x + 1 x src/backend/utils/adt/float.c:2769 src/backend/utils/adt/float.c:2785 src/backend/utils/adt/oracle_compat.c:1045 (x + C x) Below is the simplified code. - void bar(void); void foo(int count) { int result = count + 1; if (result count) bar(); } - $ icc -S -o - sadd2.c ... foo: # parameter 1: %edi ..B1.1: ..___tag_value_foo.1: ret 3) x + y = x (assuming y 0) src/backend/utils/adt/varbit.c:1142 src/backend/utils/adt/varlena.c:1001 src/backend/utils/adt/varlena.c:2024 src/pl/plpgsql/src/pl_exec.c:1975 src/pl/plpgsql/src/pl_exec.c:1981 Below is the simplified code. - void bar(void); void foo(int sp, int sl) { if (sp = 0) return; int sp_pl_sl = sp + sl; if (sp_pl_sl = sl) bar(); } - $ icc -S -o - sadd3.c foo: # parameter 1: %edi # parameter 2: %esi ..B1.1: ..___tag_value_foo.1: ..B1.2: ret Possible fixes == * Recent versions of icc and pathcc support gcc's workaround option, -fno-strict-overflow, to disable some optimizations based on signed integer overflow. It's better to add this option to configure. They don't support gcc's -fwrapv yet. * This -fno-strict-overflow option cannot help in all cases: it cannot prevent the latest icc from (mis)compiling the 1st case. We could also fix the source code by avoiding signed integer overflows, as follows. x + y = 0 (assuming x 0, y 0) -- x INT_MAX - y x + y = x (assuming y 0) -- x INT_MAX - y I'd suggest to fix the code rather than trying to work around the compilers since the fix seems simple and portable. See two recent compiler bugs of -fwrapv/-fno-strict-overflow as well. http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55883 http://software.intel.com/en-us/forums/topic/358200 * I don't have access to IBM's xlc compiler. Not sure how it works for the above cases. - xi -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers