Re: [PATCH 0/3] Work around icc miscompilation

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