Re: Compiling CVS HEAD with clang under OSX

I tried $subject recently, and noticed some minor issues:

(1) Two warnings that suggest bugs; in src/backend/utils/adt,

datetime.c:3101:27: warning: use of logical || with constant operand;
switch to bitwise | or remove constant

And similarly for src/interfaces/ecpg/pgtypeslib/interval.c. Attached
is a patch that replaces logical OR with bitwise OR, which seems to be
the intended coding.

(2) clang doesn't support (or require) "-no-cpp-precomp", which
src/template/darwin adds to $CC unconditionally. Adding the flag
unconditionally seems wrong regardless: e.g., -no-cpp-precomp isn't
supported by FSF GCC on OSX either. clang is happy to ignore the flag,
but it just emits countless "warning: argument unused during
compilation: '-no-cpp-precomp'" -- not sure the best way to fix this.
Perhaps have configure grep for "apple" in "gcc --version"?

(As an aside, is "no-cpp-precomp" still necessary for
reasonably-modern versions of Apple GCC?)

(3) There are countless warnings emitted during the compilation of
regcomp.c and related files, due to unused values returned by ERR(),
VERR(), FAILW(), and similar macros. Perhaps it is possible to rewrite
the macros to avoid the warning, although I didn't see an easy way to
do that. We could also specify -Wno-unused-value, but probably not
worth bothering just for clang.

Neil

Neil Conway <neil(dot)conway(at)gmail(dot)com> writes:
> I tried $subject recently, and noticed some minor issues:
> (1) Two warnings that suggest bugs; in src/backend/utils/adt,

> datetime.c:3101:27: warning: use of logical || with constant operand;
> switch to bitwise | or remove constant

> And similarly for src/interfaces/ecpg/pgtypeslib/interval.c. Attached
> is a patch that replaces logical OR with bitwise OR, which seems to be
> the intended coding.

Yeah, this seems like an ancient typo. Will apply.

> (2) clang doesn't support (or require) "-no-cpp-precomp", which
> src/template/darwin adds to $CC unconditionally.
> ...
> (As an aside, is "no-cpp-precomp" still necessary for
> reasonably-modern versions of Apple GCC?)

Good question, but before thinking about removing it, we'd have to agree
what is a "reasonably modern" version of Apple's gcc. OSX 10.4 is still
in use in the wild, for sure. Is 10.3 still interesting?

> (3) There are countless warnings emitted during the compilation of
> regcomp.c and related files, due to unused values returned by ERR(),
> VERR(), FAILW(), and similar macros. Perhaps it is possible to rewrite
> the macros to avoid the warning, although I didn't see an easy way to
> do that.

Perhaps a tack like this would shut it up?

#define VERR(vv,e) ((vv)->nexttype = EOS, \
(vv)->err = ((vv)->err ? (vv)->err : (e)))

What are you doing exactly to build with clang ... is "CC = clang"
sufficient?

regards, tom lane

Neil Conway <neil(dot)conway(at)gmail(dot)com> writes:
> *** src/backend/utils/adt/datetime.c 9 May 2010 02:15:59 -0000 1.212
> --- src/backend/utils/adt/datetime.c 1 Aug 2010 23:09:30 -0000
> ***************
> *** 3098,3104 ****
> break;

> case RESERV:
> ! tmask = (DTK_DATE_M || DTK_TIME_M);
> *dtype = val;
> break;

> --- 3098,3104 ----
> break;

> case RESERV:
> ! tmask = (DTK_DATE_M | DTK_TIME_M);
> *dtype = val;
> break;

BTW, some digging in the code shows that this line is reached only for
interval input "invalid", which hasn't been accepted for a long time
anyhow:

regression=# select 'invalid'::interval;
ERROR: date/time value "invalid" is no longer supported

However, the mistaken value given to tmask interferes with detecting
other errors, in particular the DTERR_BAD_FORMAT that you ought to get
for specifying conflicting fields. The DTK_DATE_M | DTK_TIME_M value
is intended to say that "invalid" conflicts with any date or time field.
But since the wrong value is assigned, you can do this:

regression=# select '3 day invalid'::interval;
ERROR: date/time value "3 day invalid" is no longer supported

and the syntax error fails to trigger, so that control gets as far as
complaining about the DTK_INVALID type instead. With the fix, you
get the intended behavior:

regression=# select '3 day invalid'::interval;
ERROR: invalid input syntax for type interval: "3 day invalid"

So this is a real bug, though surely one that's awfully far down the
severity scale, to the point that the compiler warning might be judged
more annoying than the actual bug. I'm inclined to fix it in HEAD and
9.0, which are the only branches that anyone's likely to be excited
about building with clang.

regards, tom lane

Neil Conway <neil(dot)conway(at)gmail(dot)com> writes:
> I tried $subject recently, and noticed some minor issues:

I tried to duplicate your results using what I believe to be the latest
version of clang,

$ clang -v
Apple clang version 1.5 (tags/Apple/clang-60)
Target: x86_64-apple-darwin10
Thread model: posix

(this is a 10.6.4 machine with the Xcode update that came out last
week). I got some curious discrepancies from your report.

> (1) Two warnings that suggest bugs; in src/backend/utils/adt,

> datetime.c:3101:27: warning: use of logical || with constant operand;
> switch to bitwise | or remove constant

I do *not* see these warnings. Were you using some nondefault compiler
option?

> (2) clang doesn't support (or require) "-no-cpp-precomp", which
> src/template/darwin adds to $CC unconditionally. Adding the flag
> unconditionally seems wrong regardless: e.g., -no-cpp-precomp isn't
> supported by FSF GCC on OSX either. clang is happy to ignore the flag,
> but it just emits countless "warning: argument unused during
> compilation: '-no-cpp-precomp'"

I do see that, but I also see it complaining about -fwrapv:

clang -no-cpp-precomp -O2 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -fno-strict-aliasing -fwrapv -g -I../../src/port -DFRONTEND -I../../src/include -c -o chklocale.o chklocale.c
clang: warning: argument unused during compilation: '-no-cpp-precomp'
clang: warning: argument unused during compilation: '-fwrapv'
clang -no-cpp-precomp -O2 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -fno-strict-aliasing -fwrapv -g -I../../src/port -DFRONTEND -I../../src/include -c -o dirmod.o dirmod.c
clang: warning: argument unused during compilation: '-no-cpp-precomp'
clang: warning: argument unused during compilation: '-fwrapv'

We're certainly not going to just drop -fwrapv, as that would break the
code on many modern versions of gcc. (I'm a bit surprised and concerned
that clang hasn't got this flag, btw.) So it would seem that what's
needed here is a configure test, not just supplying or not supplying
the flag based on environment.

> (3) There are countless warnings emitted during the compilation of
> regcomp.c and related files, due to unused values returned by ERR(),
> VERR(), FAILW(), and similar macros.

I fixed this in HEAD, or at least my copy of clang doesn't complain
anymore.

regards, tom lane

Neil Conway <neil(dot)conway(at)gmail(dot)com> writes:
> (As an aside, is "no-cpp-precomp" still necessary for
> reasonably-modern versions of Apple GCC?)

I looked into this point a little bit. Apple abandoned their
nonstandard precompiler as of gcc 3.3, so the switch is a no-op
in that version and later, as per release notes here:
http://developer.apple.com/legacy/mac/library/releasenotes/DeveloperTools/RN-GCC3/index.html

I have a copy of gcc-3.3 still in captivity, and have verified that it
builds 9.0beta4 just fine without the no-cpp-precomp switch.

So the question is whether anybody is likely to still be using
even-older versions of Apple's gcc. I think probably not: as of Xcode
2.0, released in early 2005, 3.3 was already the oldest supported branch
AFAICT. If you'd like to build code that will run on Intel Macs, you
have to be using gcc 4.0 or later, so even 3.3 is pretty much in the
dustbin of history.

So I think we might as well drop -no-cpp-precomp. If there is anyone
out there who really needs it, they can always push it in via a manual
setting of CPPFLAGS, but there seems little reason to include it by
default.

I'm still wondering about the bleats I saw for -fwrapv though.
configure already is set up to install that switch only conditionally:

# Disable optimizations that assume no overflow; needed for gcc 4.3+
PGAC_PROG_CC_CFLAGS_OPT([-fwrapv])

but apparently the test used for this does not notice warning messages.
Can we improve that?

regards, tom lane

On Sun, Aug 1, 2010 at 7:40 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> I tried to duplicate your results using what I believe to be the latest
> version of clang,

I'm using SVN tip of llvm+clang from ~one week ago.

>> (2) clang doesn't support (or require) "-no-cpp-precomp", which
>> src/template/darwin adds to $CC unconditionally. Adding the flag
>> unconditionally seems wrong regardless: e.g., -no-cpp-precomp isn't
>> supported by FSF GCC on OSX either. clang is happy to ignore the flag,
>> but it just emits countless "warning: argument unused during
>> compilation: '-no-cpp-precomp'"
>
> I do see that, but I also see it complaining about -fwrapv:
>
[...]
>
> We're certainly not going to just drop -fwrapv, as that would break the
> code on many modern versions of gcc.  (I'm a bit surprised and concerned
> that clang hasn't got this flag, btw.)

Support for -fwrapv was apparently implemented recently:

https://llvm.org/viewvc/llvm-project?view=rev&sortby=date&revision=106956

FWIW, I think we should aim to eventually remove the dependency on
-fwrapv, and instead make the code correct under the semantics
guaranteed by the C spec. That will be hard to do without a tool that
checks for code that makes incorrect assumptions about integer
overflow behavior, but there seems to be progress in that area
recently:

http://blog.regehr.org/archives/226

(As it happens, their checker uses llvm, which is what motivated me to
start poking around in the first place...)

>> (3) There are countless warnings emitted during the compilation of
>> regcomp.c and related files, due to unused values returned by ERR(),
>> VERR(), FAILW(), and similar macros.
>
> I fixed this in HEAD, or at least my copy of clang doesn't complain
> anymore.

Yep, looks good here. Thanks!

Neil

On Sun, Aug 1, 2010 at 9:40 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> I'm still wondering about the bleats I saw for -fwrapv though.
> configure already is set up to install that switch only conditionally:
>
>  # Disable optimizations that assume no overflow; needed for gcc 4.3+
>  PGAC_PROG_CC_CFLAGS_OPT([-fwrapv])
>
> but apparently the test used for this does not notice warning messages.
> Can we improve that?

I think this is a non-issue at least with respect to clang, since they
added support for -fwrapv recently. However, I wonder if the logic
should be the reverse: unless we have evidence to suggest that the
compiler provides the integer overflow behavior we require (e.g., it
supports -fwrapv, sufficiently old GCC, etc.), then we should emit a
warning to suggest that the resulting binary might be buggy.

Neil

Neil Conway <neil(dot)conway(at)gmail(dot)com> writes:
> FWIW, I think we should aim to eventually remove the dependency on
> -fwrapv, and instead make the code correct under the semantics
> guaranteed by the C spec.

[ shrug... ] We've gone around on that before. I can't get excited
about it, and the reason is that 99% of the places where we actually
need -fwrapv behavior is in places where we are trying to test for
overflow. The need for that can't be legislated away. As an example,
in int4pl we have code like

result = arg1 + arg2;
if (some-test-on-result-and-arg1-and-arg2)
elog(ERROR, "overflow");

Here's the problem: if the compiler is allowed to assume that overflow
cannot happen, it is always going to be able to "prove" that the
if-test is constant false. This is inherent. Anybody claiming to
exhibit a safe way to code the overflow test is really only telling
you that today's version of their compiler isn't smart enough to make
the proof. Next year, who knows? I'm uninterested in getting into
that kind of arms race with the compiler authors. I prefer to keep
the goalposts exactly where they've been for the past forty years,
namely -fwrapv semantics.

If the compiler guys actually were serious about helping people write
code that didn't need -fwrapv, they would provide primitives for
testing for arithmetic overflow. I would be ecstatic if we could write
int4pl like this:

if (sum_overflows(arg1, arg2))
elog(ERROR, "overflow");
else
return arg1 + arg2;

especially since with a halfway decent compiler this sort of
construction wouldn't even require doing the addition twice ---
presumably the compiler could see that it had a common subexpression
there, and generate code that consists of one addition plus a
branch-on-overflow test. This'd be clean, readable, and far more
efficient than the hacks we have to resort to now.

Short of that, though, -fwrapv is what it's gonna be.

regards, tom lane

On Mon, Aug 2, 2010 at 4:27 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Here's the problem: if the compiler is allowed to assume that overflow
> cannot happen, it is always going to be able to "prove" that the
> if-test is constant false.  This is inherent.  Anybody claiming to
> exhibit a safe way to code the overflow test is really only telling
> you that today's version of their compiler isn't smart enough to make
> the proof.

So I'll do the next two parts of the dialogue myself:

Questioner: So why not write that as:

if ((arg1 > 0 && arg2 > 0 && arg1 > MAXINT - arg2) ||
(arg1 < 0 && arg2 < 0 && arg1 < MININT + arg2))
elog("overflow")
else
return arg1+arg2;

Tom: Because that code is much more complex and prone to errors
especially when you start getting into multiplication and other
operations and it's also much slower than the code which allows
overflow to happen and then checks that the result makes sense.

I'm not entirely sure I agree. At least I haven't actually gone
through all the arithmetic operations and I'm not sure how much more
complex they get. If they were all at that level of complexity I think
I would say we should go ahead and bite the performance bullet and do
it the ultra-safe way.

--
greg

On Mon, Aug 02, 2010 at 05:34:57PM +0100, Greg Stark wrote:
> Tom: Because that code is much more complex and prone to errors
> especially when you start getting into multiplication and other
> operations and it's also much slower than the code which allows
> overflow to happen and then checks that the result makes sense.

> I'm not entirely sure I agree. At least I haven't actually gone
> through all the arithmetic operations and I'm not sure how much more
> complex they get. If they were all at that level of complexity I think
> I would say we should go ahead and bite the performance bullet and do
> it the ultra-safe way.

FWIW, here's a site with some gcc magic which will allow you to detect
overflows during addition. Ofcourse, the fact that it's gcc specific
makes it a lot less useful.

http://www.fefe.de/intof.html

Have a nice day,
--
Martijn van Oosterhout <kleptog(at)svana(dot)org> http://svana.org/kleptog/
> Patriotism is when love of your own people comes first; nationalism,
> when hate for people other than your own comes first.
> - Charles de Gaulle