Обсуждение: [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
I'm sending three smaller patches for review, which try to fix icc
and pathscale (mis)compilation problems described in my last email.
Not sure if we need to fix the overflow check x + 1 <= 0 (x > 0) at
src/backend/executor/execQual.c:3088, which icc optimizes away, too.
Fix x + y < x overflow checks Fix x + 1 < x overflow checks Fix overflow checking in repeat()
src/backend/utils/adt/float.c | 8 ++++----src/backend/utils/adt/oracle_compat.c | 18
+++++++-----------src/backend/utils/adt/varbit.c | 7 +++++--src/backend/utils/adt/varlena.c | 10
++++++----src/pl/plpgsql/src/pl_exec.c | 4 ++--5 files changed, 24 insertions(+), 23 deletions(-)
On 1/20/13 2:58 AM, Xi Wang wrote:
> 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
>
icc optimizes away the overflow check x + y < x (y > 0), because
signed integer overflow is undefined behavior in C. Instead, use
a safe precondition test x > INT_MAX - y.
---src/backend/utils/adt/varbit.c | 7 +++++--src/backend/utils/adt/varlena.c | 10
++++++----src/pl/plpgsql/src/pl_exec.c | 4 ++--3 files changed, 13 insertions(+), 8 deletions(-)
diff --git a/src/backend/utils/adt/varbit.c b/src/backend/utils/adt/varbit.c
index 1712c12..af69200 100644
--- a/src/backend/utils/adt/varbit.c
+++ b/src/backend/utils/adt/varbit.c
@@ -16,6 +16,8 @@#include "postgres.h"
+#include <limits.h>
+#include "access/htup_details.h"#include "libpq/pqformat.h"#include "nodes/nodeFuncs.h"
@@ -1138,12 +1140,13 @@ bit_overlay(VarBit *t1, VarBit *t2, int sp, int sl) ereport(ERROR,
(errcode(ERRCODE_SUBSTRING_ERROR), errmsg("negative substring length not allowed")));
- sp_pl_sl = sp + sl;
- if (sp_pl_sl <= sl)
+ if (sl > INT_MAX - sp) ereport(ERROR, (errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
errmsg("integer out of range")));
+ sp_pl_sl = sp + sl;
+ s1 = bitsubstring(t1, 1, sp - 1, false); s2 = bitsubstring(t1, sp_pl_sl, -1, true); result =
bit_catenate(s1,t2);
diff --git a/src/backend/utils/adt/varlena.c b/src/backend/utils/adt/varlena.c
index 95e41bf..c907f44 100644
--- a/src/backend/utils/adt/varlena.c
+++ b/src/backend/utils/adt/varlena.c
@@ -997,12 +997,13 @@ text_overlay(text *t1, text *t2, int sp, int sl) ereport(ERROR,
(errcode(ERRCODE_SUBSTRING_ERROR), errmsg("negative substring length not allowed")));
- sp_pl_sl = sp + sl;
- if (sp_pl_sl <= sl)
+ if (sl > INT_MAX - sp) ereport(ERROR, (errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
errmsg("integer out of range")));
+ sp_pl_sl = sp + sl;
+ s1 = text_substring(PointerGetDatum(t1), 1, sp - 1, false); s2 = text_substring(PointerGetDatum(t1), sp_pl_sl,
-1,true); result = text_catenate(s1, t2);
@@ -2020,12 +2021,13 @@ bytea_overlay(bytea *t1, bytea *t2, int sp, int sl) ereport(ERROR,
(errcode(ERRCODE_SUBSTRING_ERROR), errmsg("negative substring length not allowed")));
- sp_pl_sl = sp + sl;
- if (sp_pl_sl <= sl)
+ if (sl > INT_MAX - sp) ereport(ERROR, (errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
errmsg("integer out of range")));
+ sp_pl_sl = sp + sl;
+ s1 = bytea_substring(PointerGetDatum(t1), 1, sp - 1, false); s2 = bytea_substring(PointerGetDatum(t1),
sp_pl_sl,-1, true); result = bytea_catenate(s1, t2);
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index 0ecf651..a9cf6df 100644
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -1972,13 +1972,13 @@ exec_stmt_fori(PLpgSQL_execstate *estate, PLpgSQL_stmt_fori *stmt) */ if
(stmt->reverse) {
- if ((int32) (loop_value - step_value) > loop_value)
+ if (loop_value < INT_MIN + step_value) break; loop_value -= step_value; }
else {
- if ((int32) (loop_value + step_value) < loop_value)
+ if (loop_value > INT_MAX - step_value) break; loop_value += step_value;
}
--
1.7.10.4
icc optimizes away x + 1 < x because signed integer overflow is
undefined behavior in C. Instead, simply check if x is INT_MAX.
---src/backend/utils/adt/float.c | 8 ++++----1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/backend/utils/adt/float.c b/src/backend/utils/adt/float.c
index b73e0d5..344b092 100644
--- a/src/backend/utils/adt/float.c
+++ b/src/backend/utils/adt/float.c
@@ -2764,12 +2764,12 @@ width_bucket_float8(PG_FUNCTION_ARGS) result = 0; else if (operand >= bound2)
{
- result = count + 1; /* check for overflow */
- if (result < count)
+ if (count == INT_MAX) 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;
@@ -2780,12 +2780,12 @@ width_bucket_float8(PG_FUNCTION_ARGS) result = 0; else if (operand <= bound2)
{
- result = count + 1; /* check for overflow */
- if (result < count)
+ if (count == INT_MAX) ereport(ERROR,
(errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE), errmsg("integer out of range")));
+ result = count + 1; } else result = ((float8) count * (bound1 - operand) /
(bound1- bound2)) + 1;
--
1.7.10.4
icc optimizes away `check + VARHDRSZ <= check' since signed integer
overflow is undefined behavior. Simplify the overflow check for
`VARHDRSZ + count * slen' as `count > (INT_MAX - VARHDRSZ) / slen'.
---src/backend/utils/adt/oracle_compat.c | 18 +++++++-----------1 file changed, 7 insertions(+), 11 deletions(-)
diff --git a/src/backend/utils/adt/oracle_compat.c b/src/backend/utils/adt/oracle_compat.c
index d448088..a85a672 100644
--- a/src/backend/utils/adt/oracle_compat.c
+++ b/src/backend/utils/adt/oracle_compat.c
@@ -15,6 +15,8 @@ */#include "postgres.h"
+#include <limits.h>
+#include "utils/builtins.h"#include "utils/formatting.h"#include "mb/pg_wchar.h"
@@ -1034,20 +1036,14 @@ repeat(PG_FUNCTION_ARGS) count = 0; slen = VARSIZE_ANY_EXHDR(string);
- tlen = VARHDRSZ + (count * slen); /* Check for integer overflow */
- if (slen != 0 && count != 0)
- {
- int check = count * slen;
- int check2 = check + VARHDRSZ;
-
- if ((check / slen) != count || check2 <= check)
- ereport(ERROR,
- (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
- errmsg("requested length too large")));
- }
+ if (slen != 0 && count > (INT_MAX - VARHDRSZ) / slen)
+ ereport(ERROR,
+ (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
+ errmsg("requested length too large")));
+ tlen = VARHDRSZ + (count * slen); result = (text *) palloc(tlen); SET_VARSIZE(result, tlen);
--
1.7.10.4
On 24.01.2013 11:33, Xi Wang wrote: > I'm sending three smaller patches for review, which try to fix icc > and pathscale (mis)compilation problems described in my last email. These patches look ok at a quick glance, but how do we ensure this kind of problems don't crop back again in the future? Does icc give a warning about these? Do we have a buildfarm animal that produces the warnings? If we fix these, can we stop using -frapv on gcc? Is there any way to get gcc to warn about these? - Heikki
On 1/24/13 5:02 AM, Heikki Linnakangas wrote: > These patches look ok at a quick glance, but how do we ensure this kind > of problems don't crop back again in the future? Does icc give a warning > about these? Do we have a buildfarm animal that produces the warnings? > > If we fix these, can we stop using -frapv on gcc? Is there any way to > get gcc to warn about these? Thanks for reviewing. gcc has this -Wstrict-overflow option to warn against overflow checks that may be optimized away. The result in inaccurate: it may produce a large number of false warnings, and it may also miss many cases (esp. when gcc's value-range-propagation fails to compute variables' ranges). Not sure if other compilers have similar options. I find these broken checks using a static checker I'm developing, and only report cases that existing compilers do miscompile. If you are interested, I'll post a complete list of overflow checks in pgsql that invoke undefined behavior and thus may be killed by future compilers. I believe we can get rid of -fwrapv once we fix all such checks. - xi
On Thu, Jan 24, 2013 at 6:36 AM, Xi Wang <xi.wang@gmail.com> wrote: > icc optimizes away the overflow check x + y < x (y > 0), because > signed integer overflow is undefined behavior in C. Instead, use > a safe precondition test x > INT_MAX - y. I should mention gcc 4.7 does the same, and it emits a warning.
Xi Wang <xi.wang@gmail.com> writes:
> On 1/24/13 5:02 AM, Heikki Linnakangas wrote:
>> If we fix these, can we stop using -frapv on gcc? Is there any way to
>> get gcc to warn about these?
> I believe we can get rid of -fwrapv once we fix all such checks.
TBH, I find that statement to be nonsense, and these patches to be
completely the wrong way to go about it.
The fundamental problem here is that the compiler, unless told otherwise
by a compilation switch, believes it is entitled to assume that no
integer overflow will happen anywhere in the program. Therefore, any
error check that is looking for overflow *should* get optimized away.
The only reason the compiler would fail to do that is if its optimizer
isn't quite smart enough to prove that the code is testing for an
overflow condition. So what you are proposing here is merely the next
move in an arms race with the compiler writers, and it will surely
break again in a future generation of compilers. Or even if these
particular trouble spots don't break, something else will. The only
reliable solution is to not let the compiler make that type of
assumption.
So I think we should just reject all of these, and instead fix configure
to make sure it turns on icc's equivalent of -fwrapv.
regards, tom lane
On 1/24/13 10:48 AM, Tom Lane wrote: > The fundamental problem here is that the compiler, unless told otherwise > by a compilation switch, believes it is entitled to assume that no > integer overflow will happen anywhere in the program. Therefore, any > error check that is looking for overflow *should* get optimized away. > The only reason the compiler would fail to do that is if its optimizer > isn't quite smart enough to prove that the code is testing for an > overflow condition. So what you are proposing here is merely the next > move in an arms race with the compiler writers, and it will surely > break again in a future generation of compilers. Or even if these > particular trouble spots don't break, something else will. The only > reliable solution is to not let the compiler make that type of > assumption. What I am proposing here is the opposite: _not_ to enter an arm race with the compiler writers. Instead, make the code conform to the C standard, something both sides can agree on. Particularly, avoid using (signed) overflowed results to detect overflows, which the C standard clearly specifies as undefined behavior and many compilers are actively exploiting. We could use either unsigned overflows (which is well defined) or precondition testing (like `x > INT_MAX - y' in the patches). > So I think we should just reject all of these, and instead fix configure > to make sure it turns on icc's equivalent of -fwrapv. While I agree it's better to turn on icc's -fno-strict-overflow as a workaround, the fundamental problem here is that we are _not_ programming in the C language. Rather, we are programming in some C-with-signed-wrapraround dialect. The worst part of this C dialect is that it has never been specified clearly what it means by "signed wraparound": gcc's -fwrapv assumes signed wraparound for add/sub/mul, but not for div (e.g., INT_MIN/-1 traps on x86) nor shift (e.g., 1<<32 produces undef with clang). clang's -fwrapv also assumes workaround for pointer arithmetic, while gcc's does not. I have no idea what icc and pathscale's -fno-strict-overflow option does (probably the closest thing to -fwrapv). Sometimes it prevents such checks from being optimized away, sometimes it doesn't. Anyway, it would not be surprising if future C compilers break this dialect again. But they shouldn't break standard-conforming code. - xi
On Thu, Jan 24, 2013 at 4:36 AM, Xi Wang <xi.wang@gmail.com> wrote: > icc optimizes away the overflow check x + y < x (y > 0), because > signed integer overflow is undefined behavior in C. Instead, use > a safe precondition test x > INT_MAX - y. As you post these patches, please add them to: https://commitfest.postgresql.org/action/commitfest_view/open This will ensure that they (eventually) get looked at. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Thu, Jan 24, 2013 at 11:40:41AM -0500, Xi Wang wrote: > On 1/24/13 10:48 AM, Tom Lane wrote: > > The fundamental problem here is that the compiler, unless told otherwise > > by a compilation switch, believes it is entitled to assume that no > > integer overflow will happen anywhere in the program. Therefore, any > > error check that is looking for overflow *should* get optimized away. > > The only reason the compiler would fail to do that is if its optimizer > > isn't quite smart enough to prove that the code is testing for an > > overflow condition. So what you are proposing here is merely the next > > move in an arms race with the compiler writers, and it will surely > > break again in a future generation of compilers. Or even if these > > particular trouble spots don't break, something else will. The only > > reliable solution is to not let the compiler make that type of > > assumption. > > What I am proposing here is the opposite: _not_ to enter an arm race > with the compiler writers. Instead, make the code conform to the C > standard, something both sides can agree on. > > Particularly, avoid using (signed) overflowed results to detect > overflows, which the C standard clearly specifies as undefined > behavior and many compilers are actively exploiting. > > We could use either unsigned overflows (which is well defined) or > precondition testing (like `x > INT_MAX - y' in the patches). > > > So I think we should just reject all of these, and instead fix configure > > to make sure it turns on icc's equivalent of -fwrapv. > > While I agree it's better to turn on icc's -fno-strict-overflow as a > workaround, the fundamental problem here is that we are _not_ > programming in the C language. Rather, we are programming in some > C-with-signed-wrapraround dialect. I could not have said it better. All other things being similar, I'd rather have PostgreSQL be written in C, not C-with-signed-wrapraround. The latter has been a second class citizen for over 20 years, and that situation won't be improving. However, compiler options selecting it are common and decently-maintained. Changes along these lines would become much more interesting if PostgreSQL has a live bug on a modern compiler despite the use of available options comparable to -fwrapv. When, if ever, to stop using -fwrapv (and use -ftrapv under --enable-cassert) is another question. GCC 4.7 reports 999 warnings at -fstrict-overflow -Wstrict-overflow=5. That doesn't mean 999 bugs to fix before we could remove -fwrapv, but it does mean 999 places where the compiler will start generating different code. That has a high chance of breaking something not covered by the regression tests, so I'd hope to see a notable upside, perhaps a benchmark improvement. It would also be instructive to know how much code actually needs to change. Fixing 20 code sites in exchange for standard-conformance and an X% performance improvement is a different proposition from fixing 200 code sites for the same benefit. If we do start off in this direction at any scale, I suggest defining macros like INT_MULT_OVERFLOWS(a, b) to wrap the checks. Then these changes would at least make the code clearer, not less so. Thanks, nm -- Noah Misch EnterpriseDB http://www.enterprisedb.com
On Thu, Jan 24, 2013 at 3:48 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> The fundamental problem here is that the compiler, unless told otherwise
> by a compilation switch, believes it is entitled to assume that no
> integer overflow will happen anywhere in the program. Therefore, any
> error check that is looking for overflow *should* get optimized away.
> The only reason the compiler would fail to do that is if its optimizer
> isn't quite smart enough to prove that the code is testing for an
> overflow condition.
He's changing things to do
if (INT_MAX - a > b) PG_THROW ("a+b would overflow")
else x=a+b;
Why would a smarter compiler be licensed to conclude that it can
optimize away anything? "INT_MAX-a > b" is always well defined. And
the x = a+b won't execute unless it's well defined too. (Actually
we'll probably depend on the non-local exit behaviour of PG_THROW but
any compiler has to be able to deal with that anyways).
The point that we have no way to be sure we've gotten rid of any such
case is a good one. Logically as long as we're afraid of such things
we should continue to use -fwrapv and if we're using -fwrapv there's
no urgency to fix the code. But if we do get rid of all the known ones
then at least we have the option if we decide we've inspected the code
enough and had enough compilers check it to feel confident.
--
greg
Greg Stark <stark@mit.edu> writes:
> He's changing things to do
> if (INT_MAX - a > b)
> PG_THROW ("a+b would overflow")
> else
> x=a+b;
> Why would a smarter compiler be licensed to conclude that it can
> optimize away anything? "INT_MAX-a > b" is always well defined.
Really? Can't "INT_MAX - a" overflow?
regards, tom lane
It depends on the context. In the patches, `a' is known to be
non-negative, so `INT_MAX - a' cannot overflow. If you ignore the
context and talk about the general case, then it can.
- xi
On Sat, Feb 23, 2013 at 12:25 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Greg Stark <stark@mit.edu> writes:
>> He's changing things to do
>
>> if (INT_MAX - a > b)
>> PG_THROW ("a+b would overflow")
>> else
>> x=a+b;
>
>> Why would a smarter compiler be licensed to conclude that it can
>> optimize away anything? "INT_MAX-a > b" is always well defined.
>
> Really? Can't "INT_MAX - a" overflow?
>
> regards, tom lane
On Sat, Feb 23, 2013 at 5:25 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Really? Can't "INT_MAX - a" overflow? I guess I shouldn't have tried summarizing the code and just pasted some real code in for fear of getting it wrong. I was thinking of unsigned arithmetic when I wrote that. The point being that the compiler isn't going to make optimization based on code that comes later if that code wouldn't have run in the same flow of execution. -- greg
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 Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On Mon, Jul 15, 2013 at 5:59 PM, Alvaro Herrera <alvherre@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
Robert Haas escribió: > On Mon, Jul 15, 2013 at 5:59 PM, Alvaro Herrera > <alvherre@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.4050105@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 Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
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 > > <alvherre@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.4050105@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 <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
> Should these patches be applied? I have a copy of the program and was going to take care of this. -- greg
On Sat, Sep 7, 2013 at 6:55 PM, Greg Stark <stark@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
On Mon, Sep 9, 2013 at 12:21:56PM -0400, Robert Haas wrote: > On Sat, Sep 7, 2013 at 6:55 PM, Greg Stark <stark@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 <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
On Wed, Nov 27, 2013 at 10:48 PM, Bruce Momjian <bruce@momjian.us> wrote: > > On Mon, Sep 9, 2013 at 12:21:56PM -0400, Robert Haas wrote: > > On Sat, Sep 7, 2013 at 6:55 PM, Greg Stark <stark@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? 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. -- greg
Вложения
Greg Stark <stark@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
On Fri, Nov 29, 2013 at 5:21 PM, Tom Lane <tgl@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
On Fri, Nov 29, 2013 at 7:39 PM, Greg Stark <stark@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
On 11/29/2013 10:06 PM, Greg Stark wrote: > On Fri, Nov 29, 2013 at 7:39 PM, Greg Stark <stark@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
Greg Stark <stark@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
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
On Fri, Nov 29, 2013 at 02:04:10AM +0000, 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 <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
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 +0000, 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 <bruce@momjian.us> 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 <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + As you are, so once was I. As I am, so you will be. + + Roman grave inscription +
On Fri, Oct 9, 2015 at 10:52 PM, Bruce Momjian <bruce@momjian.us> 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 +0000, 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
Вложения
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
Вложения
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
On Fri, Oct 9, 2015 at 2:52 PM, Bruce Momjian <bruce@momjian.us> 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:
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....
--
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
On Tue, Dec 1, 2015 at 5:17 PM, Greg Stark <stark@mit.edu> 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
Вложения
Greg Stark <stark@mit.edu> 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
On Thu, Dec 3, 2015 at 2:51 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Greg Stark <stark@mit.edu> 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
On 2015-12-03 12:10:27 +0000, 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
On Thu, Dec 3, 2015 at 2:51 PM, Tom Lane <tgl@sss.pgh.pa.us> 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
Greg Stark <stark@mit.edu> writes:
> On Thu, Dec 3, 2015 at 2:51 PM, Tom Lane <tgl@sss.pgh.pa.us> 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
On Fri, Dec 4, 2015 at 1:27 AM, Andres Freund <andres@anarazel.de> wrote: > On 2015-12-03 12:10:27 +0000, 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