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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
