Re: new clang report

The lastest clang svn tip (2.9-to-be, I guess) builds PostgreSQL out of
the box and most tests pass. Specifically, it no longer chokes on
-D_GNU_SOURCE on Linux, which was the previously reported blocker.

Warnings:

Lots of these:
clang: warning: argument unused during compilation: '-mthreads'
clang: warning: argument unused during compilation: '-mt'

Possible fix, check both link and compile invocations for warnings in
configure:

diff --git i/config/acx_pthread.m4 w/config/acx_pthread.m4
index ceb161a..ee181f9 100644
--- i/config/acx_pthread.m4
+++ w/config/acx_pthread.m4
@@ -142,7 +142,7 @@ main (int argc, char **argv)
}
_ACEOF
rm -f conftest.$ac_objext conftest$ac_exeext
- if test "`(eval $ac_link 2>&1 1>&5)`" = ""; then
+ if test "`(eval $ac_link 2>&1 1>&5)`" = "" && test "`(eval $ac_compile 2>&1 1>&5)`" = ""; then
# we continue with more flags because Linux needs -lpthread
# for libpq builds on PostgreSQL. The test above only
# tests for building binaries, not shared libraries.

The usual flex warning:

In file included from gram.y:12460:
scan.c:16256:23: warning: unused variable 'yyg' [-Wunused-variable]
struct yyguts_t * yyg = (struct yyguts_t*)yyscanner; /* This var may be unused depending upon options. */

And then only these two:

fe-exec.c:2408:13: warning: comparison of unsigned enum expression < 0 is always false [-Wtautological-compare]
if (status < 0 || status >= sizeof pgresStatus / sizeof pgresStatus[0])
~~~~~~ ^ ~

pg_standby.c:347:22: warning: comparison of unsigned expression >= 0 is always true [-Wtautological-compare]
if (tli > 0 && log >= 0 && seg > 0)
~~~ ^ ~

Regression tests (world):

--- src/test/regress/expected/float8.out
+++ src/test/regress/results/float8.out
@@ -384,7 +384,15 @@
SELECT '' AS bad, f.f1 * '1e200' from FLOAT8_TBL f;
ERROR: value out of range: overflow
SELECT '' AS bad, f.f1 ^ '1e200' from FLOAT8_TBL f;
-ERROR: value out of range: overflow
+ bad | ?column?
+-----+----------
+ | 0
+ | NaN
+ | NaN
+ | NaN
+ | NaN
+(5 rows)
+
SELECT 0 ^ 0 + 0 ^ 1 + 0 ^ 0.0 + 0 ^ 0.5;
?column?
----------

PL/Python test crashes. I was able to make it work either by using -O0
or by applying the following patch:

diff --git i/src/pl/plpython/plpython.c w/src/pl/plpython/plpython.c
index fff7de7..8eaee36 100644
--- i/src/pl/plpython/plpython.c
+++ w/src/pl/plpython/plpython.c
@@ -1019,12 +1019,13 @@ PLy_trigger_build_args(FunctionCallInfo fcinfo, PLyProcedure *proc, HeapTuple *r

/* function handler and friends */
static Datum
-PLy_function_handler(FunctionCallInfo fcinfo, PLyProcedure *proc)
+PLy_function_handler(FunctionCallInfo fcinfo, PLyProcedure *proc2)
{
Datum rv;
PyObject *volatile plargs = NULL;
PyObject *volatile plrv = NULL;
ErrorContextCallback plerrcontext;
+ PLyProcedure *volatile proc = proc2;

PG_TRY();
{

Hmmm.

On Wed, Feb 9, 2011 at 6:30 PM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
> The lastest clang svn tip (2.9-to-be, I guess) builds PostgreSQL out of
> the box and most tests pass.  Specifically, it no longer chokes on
> -D_GNU_SOURCE on Linux, which was the previously reported blocker.

Odd, I tried the same thing just a couple days ago and reported two bugs:

9161 nor P Linu unassignedbugs(at)nondot(dot)org NEW False uninitialized
warning due to control-dependency of flag
9152 nor P Linu unassignedclangbugs(at)nondot(dot)org NEW File takes 1
minute to compile much longer than with gcc or other similar files
with llvm

The latter is much better on svn head than the version I reported it
on but it's still a problem. It took 16s to compile with svn head. Was
the first one recently fixed?

--
greg

Peter Eisentraut wrote:
> The lastest clang svn tip (2.9-to-be, I guess) builds PostgreSQL out of
> the box and most tests pass. Specifically, it no longer chokes on
> -D_GNU_SOURCE on Linux, which was the previously reported blocker.
>
> Warnings:
>
> Lots of these:
> clang: warning: argument unused during compilation: '-mthreads'
> clang: warning: argument unused during compilation: '-mt'

FYI, our threading code throws every flag it can at the compiler --- it
is very imprecise.

> Possible fix, check both link and compile invocations for warnings in
> configure:
>
> diff --git i/config/acx_pthread.m4 w/config/acx_pthread.m4
> index ceb161a..ee181f9 100644
> --- i/config/acx_pthread.m4
> +++ w/config/acx_pthread.m4
> @@ -142,7 +142,7 @@ main (int argc, char **argv)
> }
> _ACEOF
> rm -f conftest.$ac_objext conftest$ac_exeext
> - if test "`(eval $ac_link 2>&1 1>&5)`" = ""; then
> + if test "`(eval $ac_link 2>&1 1>&5)`" = "" && test "`(eval $ac_compile 2>&1 1>&5)`" = ""; then
> # we continue with more flags because Linux needs -lpthread
> # for libpq builds on PostgreSQL. The test above only
> # tests for building binaries, not shared libraries.

Yep, looks good.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

On ons, 2011-02-09 at 20:30 +0200, Peter Eisentraut wrote:
> Regression tests (world):
>
> --- src/test/regress/expected/float8.out
> +++ src/test/regress/results/float8.out
> @@ -384,7 +384,15 @@
> SELECT '' AS bad, f.f1 * '1e200' from FLOAT8_TBL f;
> ERROR: value out of range: overflow
> SELECT '' AS bad, f.f1 ^ '1e200' from FLOAT8_TBL f;
> -ERROR: value out of range: overflow
> + bad | ?column?
> +-----+----------
> + | 0
> + | NaN
> + | NaN
> + | NaN
> + | NaN
> +(5 rows)
> +
> SELECT 0 ^ 0 + 0 ^ 1 + 0 ^ 0.0 + 0 ^ 0.5;
> ?column?
> ----------

So issue here is actually that clang has an option

-fmath-errno
Indicate that math functions should be treated as updating errno.

If you pass this option, then the regression tests pass. If not, you
get the above difference. So the question is, do we

a) legislate that -fmath-errno is required, or

b) fix dpow() to handle this case somehow (how?), or

c) provide an alternative expected file?

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> On ons, 2011-02-09 at 20:30 +0200, Peter Eisentraut wrote:
>> Regression tests (world):
>>
>> --- src/test/regress/expected/float8.out
>> +++ src/test/regress/results/float8.out
>> @@ -384,7 +384,15 @@
>> SELECT '' AS bad, f.f1 * '1e200' from FLOAT8_TBL f;
>> ERROR: value out of range: overflow
>> SELECT '' AS bad, f.f1 ^ '1e200' from FLOAT8_TBL f;
>> -ERROR: value out of range: overflow
>> + bad | ?column?
>> +-----+----------
>> + | 0
>> + | NaN
>> + | NaN
>> + | NaN
>> + | NaN
>> +(5 rows)
>> +
>> SELECT 0 ^ 0 + 0 ^ 1 + 0 ^ 0.0 + 0 ^ 0.5;
>> ?column?
>> ----------

> So issue here is actually that clang has an option

> -fmath-errno
> Indicate that math functions should be treated as updating errno.

Really? It looks to me like the issue is that pow() is returning NaN
instead of Inf for an out-of-range result. That's a bug: the correct
result is *not* ill-defined, it's simply too large to represent.
If that has anything to do with errno, it's an implementation artifact
that's unrelated to the claimed meaning of the switch.

But I would also note that the Single Unix Spec is unequivocal about
this case:

If the correct value would cause overflow, +-HUGE_VAL is
returned, and errno is set to [ERANGE].

That's "IS set", not "may be set" as in some other cases. So this
behavior should not depend on any such compiler switch anyway, unless
the intent of the switch is "ignore the standard and do whatever we
feel like".

regards, tom lane

On mån, 2011-05-02 at 14:45 -0400, Tom Lane wrote:
> > So issue here is actually that clang has an option
>
> > -fmath-errno
> > Indicate that math functions should be treated as
> updating errno.
>
> Really? It looks to me like the issue is that pow() is returning NaN
> instead of Inf for an out-of-range result. That's a bug: the correct
> result is *not* ill-defined, it's simply too large to represent.
> If that has anything to do with errno, it's an implementation artifact
> that's unrelated to the claimed meaning of the switch.
>
> But I would also note that the Single Unix Spec is unequivocal about
> this case:
>
> If the correct value would cause overflow, +-HUGE_VAL is
> returned, and errno is set to [ERANGE].
>
> That's "IS set", not "may be set" as in some other cases. So this
> behavior should not depend on any such compiler switch anyway, unless
> the intent of the switch is "ignore the standard and do whatever we
> feel like".

Well, the intent of the switch actually appears to be rather "follow the
standard and do what it says" with the default behavior being the
questionable one. So we could easily settle on that you need to use
that switch, otherwise it's not supported. (This is actually quite
similar to the old days when some systems had -ffast-math the default.)

Btw., when you build a simple test program in the default mode, pow()
indeed returns Inf on overflow. There appear to be some code generation
or optimization problems when it builds the postgres code, because the
problem goes away with either -O0 or by inserting an elog or something
like that after the pow() call.

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> Btw., when you build a simple test program in the default mode, pow()
> indeed returns Inf on overflow. There appear to be some code generation
> or optimization problems when it builds the postgres code, because the
> problem goes away with either -O0 or by inserting an elog or something
> like that after the pow() call.

Hmm. Sounds to me like clang is trying to insert an inlined version of
pow() that gets this case wrong. Any of -fmath-errno, -O0, or possibly
other things discourage it from doing that, and then the non-inline code
gets it right. Bug for sure.

regards, tom lane