Re: [HACKERS] quiet inline configure check misses a step for clang
On 2014-05-01 16:17:17 -0400, Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: Patch attached. Committed with minor comment-smithing. Interestingly this seems to have allowed the quiet inline test to succeedd on HP-UX ac++ as well: http://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=anoledt=2014-05-02%2023%3A36%3A53stg=config Seems it uses a similar logic to clang. For a lot longer. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] quiet inline configure check misses a step for clang
Hi, On 2014-04-03 12:47:00 +0200, Andres Freund wrote: The current quiet inline test doesn't work for clang. As e.g. evidenced in http://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=gulldt=2014-04-03%2007%3A49%3A26stg=configure configure thinks it's not quiet. Which means that postgres compiled with a recent clang will be noticably slower than it needs to be. The reason for that is that clang is smart and warns about static inline if they are declared locally in the .c file, but not if they are declared in a #included file. That seems to be a reasonable behaviour... I think that needs to be fixed. We either can make the configure test considerably more complex or simply drop the requirement for quiet inline. I still think we really need to fix this. I have three possible solutions: a) Add an external file (in the source tree) that's included in the configure test. b) Have a compiler specific override and specify USE_INLINE there. c) Drop the requirement of quiet inlines. a) would probably the best, but I haven't yet found a non ugly solution :( Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] quiet inline configure check misses a step for clang
Andres Freund and...@2ndquadrant.com writes: I still think we really need to fix this. I have three possible solutions: a) Add an external file (in the source tree) that's included in the configure test. b) Have a compiler specific override and specify USE_INLINE there. c) Drop the requirement of quiet inlines. a) would probably the best, but I haven't yet found a non ugly solution :( Why is (a) so hard again? (If you're wondering where to put the file, I'd vote for the config/ directory.) regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] quiet inline configure check misses a step for clang
On 2014-05-01 10:10:46 -0400, Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: I still think we really need to fix this. I have three possible solutions: a) Add an external file (in the source tree) that's included in the configure test. b) Have a compiler specific override and specify USE_INLINE there. c) Drop the requirement of quiet inlines. a) would probably the best, but I haven't yet found a non ugly solution :( Why is (a) so hard again? (If you're wondering where to put the file, I'd vote for the config/ directory.) If we accept that the test includes a external file that's included in the source tree, it's not hard. What'd be hard is to generate a second file via autoconf. I looked at doing the latter and it seemed far to complex. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] quiet inline configure check misses a step for clang
On 2014-05-01 10:10:46 -0400, Tom Lane wrote:
Andres Freund and...@2ndquadrant.com writes:
I still think we really need to fix this. I have three possible
solutions:
a) Add an external file (in the source tree) that's included in the
configure test.
b) Have a compiler specific override and specify USE_INLINE there.
c) Drop the requirement of quiet inlines.
a) would probably the best, but I haven't yet found a non ugly solution :(
Why is (a) so hard again?
(If you're wondering where to put the file, I'd vote for the config/
directory.)
Patch attached.
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training Services
From 67a8595f07a93d50276f1f4dc967b33a5137ae25 Mon Sep 17 00:00:00 2001
From: Andres Freund and...@anarazel.de
Date: Thu, 1 May 2014 20:40:31 +0200
Subject: [PATCH] Fix quiet inline configure test on newer clang compilers.
The test used to just define a static inline function inline and check
whether that causes a warning. But newer clang versions warn about
unused static inline functions when defined inside a .c file, but not
when defined in a included header. Change the test to cope.
This caused postgres compiled with newer clang versions to not use
inline functions, potentially leading to performance degradations.
---
config/c-compiler.m4| 14 --
config/test_quiet_include.h | 5 +
configure | 2 +-
3 files changed, 18 insertions(+), 3 deletions(-)
create mode 100644 config/test_quiet_include.h
diff --git a/config/c-compiler.m4 b/config/c-compiler.m4
index 4ba3236..1b9a2db 100644
--- a/config/c-compiler.m4
+++ b/config/c-compiler.m4
@@ -19,7 +19,17 @@ fi])# PGAC_C_SIGNED
# PGAC_C_INLINE
# -
-# Check if the C compiler understands inline functions.
+# Check if the C compiler understands inline functions without being
+# noisy about unused static inline functions. Some older compilers
+# understand inline functions (tested by AC_C_INLINE) but warn about
+# them if they aren't used in a translation unit.
+# This test used to just define a inline function, but some compilers
+# (notably clang) got too smart and now warn about unused static
+# inline functions when defined inside a .c file, but not when defined
+# in a included header. Since the latter is what we want to use, test
+# that by excluding a header defined outside autoconf's confines. Not
+# pretty, but it works.
+#
# Defines: inline, PG_USE_INLINE
AC_DEFUN([PGAC_C_INLINE],
[AC_C_INLINE
@@ -28,7 +38,7 @@ AC_CACHE_CHECK([for quiet inline (no complaint if unreferenced)], pgac_cv_c_inli
if test $ac_cv_c_inline != no; then
pgac_c_inline_save_werror=$ac_c_werror_flag
ac_c_werror_flag=yes
-AC_LINK_IFELSE([AC_LANG_PROGRAM([static inline int fun () {return 0;}],[])],
+AC_LINK_IFELSE([AC_LANG_PROGRAM([#include $srcdir/config/test_quiet_include.h],[])],
[pgac_cv_c_inline_quietly=yes])
ac_c_werror_flag=$pgac_c_inline_save_werror
fi])
diff --git a/config/test_quiet_include.h b/config/test_quiet_include.h
new file mode 100644
index 000..d2060c1
--- /dev/null
+++ b/config/test_quiet_include.h
@@ -0,0 +1,5 @@
+/*
+ * For the raison d'etre of this file, check the comment above the definition
+ * of the PGAC_C_INLINE macro in config/c-compiler.m4.
+ */
+static inline int fun () {return 0;}
diff --git a/configure b/configure
index e0dbdfe..9953389 100755
--- a/configure
+++ b/configure
@@ -9725,7 +9725,7 @@ else
ac_c_werror_flag=yes
cat confdefs.h - _ACEOF conftest.$ac_ext
/* end confdefs.h. */
-static inline int fun () {return 0;}
+#include $srcdir/config/test_quiet_include.h
int
main ()
{
--
1.8.5.rc2.dirty
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] quiet inline configure check misses a step for clang
Andres Freund and...@2ndquadrant.com writes: Patch attached. Committed with minor comment-smithing. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] quiet inline configure check misses a step for clang
On 2014-05-01 16:17:17 -0400, Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: Patch attached. Committed with minor comment-smithing. Thanks. There's another problematic case: http://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=olinguitodt=2014-05-01%2021%3A22%3A43stg=config configure:9737: ccache gcc -o conftest -O2 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing -fwrapv -g -I/usr/local/include/libxml2 -I/usr/local/include -I/usr/include/kerberosV -L/usr/local/lib -lpthread -lcrypto -liconv -L/usr/local/lib -L/usr/local/lib conftest.c -lxslt -lxml2 -lssl -lcrypto -lz -lreadline -ltermcap -lm 5 /usr/local/lib/libxml2.so.14.0: warning: strcpy() is almost always misused, please use strlcpy() /usr/local/lib/libxml2.so.14.0: warning: strcat() is almost always misused, please use strlcat() /usr/local/lib/libxslt.so.3.8: warning: sprintf() is often misused, please use snprintf() configure:9737: $? = 0 configure: failed program was: That's not new. And it seems like a case of really misguided platform specific changes (actually smelling a bit of zealotry). So I'd be prepared to ignore it. Other opinions? How on earth could it be a good idea to warn about this when linking to libraries instead of warning when a function is actually used during compilation? Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] quiet inline configure check misses a step for clang
Andres Freund and...@2ndquadrant.com writes: There's another problematic case: http://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=olinguitodt=2014-05-01%2021%3A22%3A43stg=config /usr/local/lib/libxml2.so.14.0: warning: strcpy() is almost always misused, please use strlcpy() /usr/local/lib/libxml2.so.14.0: warning: strcat() is almost always misused, please use strlcat() /usr/local/lib/libxslt.so.3.8: warning: sprintf() is often misused, please use snprintf() That's not new. And it seems like a case of really misguided platform specific changes (actually smelling a bit of zealotry). So I'd be prepared to ignore it. Other opinions? Yeah, I've noticed those before, and concluded that misguided zealotry is the appropriate classification. I certainly won't do anything about those. (These particular ones, we *can't* do anything about, since the calls are inside libraries we don't control the source of...) How on earth could it be a good idea to warn about this when linking to libraries instead of warning when a function is actually used during compilation? Not only misguided, but crappily implemented zealotry. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] quiet inline configure check misses a step for clang
Andres Freund and...@2ndquadrant.com writes: The current quiet inline test doesn't work for clang. As e.g. evidenced in http://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=gulldt=2014-04-03%2007%3A49%3A26stg=configure configure thinks it's not quiet. Which means that postgres compiled with a recent clang will be noticably slower than it needs to be. The reason for that is that clang is smart and warns about static inline if they are declared locally in the .c file, but not if they are declared in a #included file. That seems to be a reasonable behaviour... I think that needs to be fixed. We either can make the configure test considerably more complex or simply drop the requirement for quiet inline. I object to the latter; you're proposing to greatly increase the warning noise seen with any compiler that issues a warning for this without caring about .h vs .c. For somebody who finds gcc -pedantic unusable, I would think you'd have a bit more sympathy for people using other compilers. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] quiet inline configure check misses a step for clang
On 2014-04-03 09:43:20 -0400, Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: The current quiet inline test doesn't work for clang. As e.g. evidenced in http://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=gulldt=2014-04-03%2007%3A49%3A26stg=configure configure thinks it's not quiet. Which means that postgres compiled with a recent clang will be noticably slower than it needs to be. The reason for that is that clang is smart and warns about static inline if they are declared locally in the .c file, but not if they are declared in a #included file. That seems to be a reasonable behaviour... I think that needs to be fixed. We either can make the configure test considerably more complex or simply drop the requirement for quiet inline. I object to the latter; you're proposing to greatly increase the warning noise seen with any compiler that issues a warning for this without caring about .h vs .c. For somebody who finds gcc -pedantic unusable, I would think you'd have a bit more sympathy for people using other compilers. Yea, but which compilers are that? The only one in the buildfarm I could find a couple weeks back was acc, and there's a flag we could add to the relevant template that silences it. I also don't think that very old platforms won't usually be used for active development, so a louder build there doesn't really have the same impact as noisy builds for actively developed on platforms. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] quiet inline configure check misses a step for clang
Andres Freund and...@2ndquadrant.com writes: On 2014-04-03 09:43:20 -0400, Tom Lane wrote: I object to the latter; you're proposing to greatly increase the warning noise seen with any compiler that issues a warning for this without caring about .h vs .c. For somebody who finds gcc -pedantic unusable, I would think you'd have a bit more sympathy for people using other compilers. Yea, but which compilers are that? The only one in the buildfarm I could find a couple weeks back was acc, and there's a flag we could add to the relevant template that silences it. I also don't think that very old platforms won't usually be used for active development, so a louder build there doesn't really have the same impact as noisy builds for actively developed on platforms. Didn't we already have this discussion last year? The main points are all mentioned in http://www.postgresql.org/message-id/ca+tgmoyjnc4b+8y01grnal52gtpbzc3zsc4sdnw4lgxhqt3...@mail.gmail.com regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] quiet inline configure check misses a step for clang
On 2014-04-03 09:43:20 -0400, Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: The current quiet inline test doesn't work for clang. As e.g. evidenced in http://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=gulldt=2014-04-03%2007%3A49%3A26stg=configure configure thinks it's not quiet. Which means that postgres compiled with a recent clang will be noticably slower than it needs to be. The reason for that is that clang is smart and warns about static inline if they are declared locally in the .c file, but not if they are declared in a #included file. That seems to be a reasonable behaviour... I think that needs to be fixed. We either can make the configure test considerably more complex or simply drop the requirement for quiet inline. I object to the latter; Do you have an idea how to make the former work? The whole autoconf getup doesn't really seem to support generating two files for a test. I've looked a bit around and it seems like it'd be a fair amount of effort to do it solely in autoconf. The easiest seems to be to just have a header for the test in the sourcetree, but that seems fairly ugly... Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] quiet inline configure check misses a step for clang
On 2014-04-03 10:15:33 -0400, Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: On 2014-04-03 09:43:20 -0400, Tom Lane wrote: I object to the latter; you're proposing to greatly increase the warning noise seen with any compiler that issues a warning for this without caring about .h vs .c. For somebody who finds gcc -pedantic unusable, I would think you'd have a bit more sympathy for people using other compilers. Yea, but which compilers are that? The only one in the buildfarm I could find a couple weeks back was acc, and there's a flag we could add to the relevant template that silences it. I also don't think that very old platforms won't usually be used for active development, so a louder build there doesn't really have the same impact as noisy builds for actively developed on platforms. Didn't we already have this discussion last year? The main points are all mentioned in http://www.postgresql.org/message-id/ca+tgmoyjnc4b+8y01grnal52gtpbzc3zsc4sdnw4lgxhqt3...@mail.gmail.com To which I replied: http://www.postgresql.org/message-id/20131224141631.gf26...@alap2.anarazel.de It really seems like an odd behaviour if a compiler behaved that way. But even if some decade+ old compiler gets this wrong: I am not going to shed many tears. We're talking about HP-UX's ac++ here. If binaries get a bit more bloated there... I really am not trying to win the inline fight here through the backdoor, I only want to make clang use inlines again. As just written in the other message, I just don't see any easy and nice way to write the autoconf test more robustly. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
