Обсуждение: 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 testabove 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: overflowSELECT '' 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 handlerand 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@gmx.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@nondot.org NEW False uninitialized warning due to control-dependency of flag 9152 nor P Linu unassignedclangbugs@nondot.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@momjian.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@gmx.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 isreturned, 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@gmx.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