Re: [HACKERS] [RFC] overflow checks optimized away

2015-12-03 Thread Michael Paquier
On Fri, Dec 4, 2015 at 1:27 AM, Andres Freund  wrote:
> 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

2015-12-03 Thread Greg Stark
On Tue, Dec 1, 2015 at 5:17 PM, Greg Stark  wrote:
> 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

2015-12-03 Thread Tom Lane
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.
"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

2015-12-03 Thread Greg Stark
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.

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

2015-12-03 Thread Tom Lane
Greg Stark  writes:
> 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

2015-12-03 Thread Andres Freund
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

2015-12-03 Thread Greg Stark
On Thu, Dec 3, 2015 at 2:51 PM, Tom Lane  wrote:
> 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

2015-12-01 Thread Greg Stark
On Fri, Oct 9, 2015 at 2:52 PM, Bruce Momjian  wrote:

> 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

2015-12-01 Thread Michael Paquier
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

2015-10-20 Thread Michael Paquier
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

2015-10-16 Thread Michael Paquier
On Fri, Oct 9, 2015 at 10:52 PM, Bruce Momjian  wrote:
>
> 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

2015-10-09 Thread Bruce Momjian

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 Momjian  http://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

2014-08-06 Thread Bruce Momjian
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

2014-01-16 Thread Heikki Linnakangas

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

2013-11-29 Thread Tom Lane
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

2013-11-29 Thread Greg Stark
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

2013-11-29 Thread Greg Stark
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

2013-11-29 Thread Heikki Linnakangas

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

2013-11-29 Thread Tom Lane
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

2013-11-27 Thread Bruce Momjian
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

2013-09-09 Thread Robert Haas
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

2013-09-07 Thread Bruce Momjian
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

2013-09-07 Thread Greg Stark
 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

2013-07-15 Thread Alvaro Herrera
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

2013-07-15 Thread Robert Haas
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

2013-07-15 Thread Alvaro Herrera
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

2013-01-22 Thread Xi Wang
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

2013-01-19 Thread Xi Wang
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