From b853eb97182079dcd30b4f52576bd5d6c275ee71 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Sun, 13 Jan 2013 18:39:20 -0500 Subject: [PATCH] Improve handling of ereport(ERROR) and elog(ERROR). In commit 71450d7fd6c7cf7b3e38ac56e363bff6a681973c, we added code to inform suitably-intelligent compilers that ereport() doesn't return if the elevel is ERROR or higher. This patch extends that to elog(), and also fixes a double-evaluation hazard that the previous commit created in ereport(), as well as reducing the emitted code size. The elog() improvement requires the compiler to support __VA_ARGS__, which should be available in just about anything nowadays since it's required by C99. But our minimum language baseline is still C89, so add a configure test for that. The previous commit assumed that ereport's elevel could be evaluated twice, which isn't terribly safe --- there are already counterexamples in xlog.c. On compilers that have __builtin_constant_p, we can use that to protect the second test, since there's no possible optimization gain if the compiler doesn't know the value of elevel. Otherwise, use a local variable inside the macros to prevent double evaluation. The local-variable solution is inferior because (a) it leads to useless code being emitted when elevel isn't constant, and (b) it increases the optimization level needed for the compiler to recognize that subsequent code is unreachable. But it seems better than not teaching non-gcc compilers about unreachability at all. Lastly, if the compiler has __builtin_unreachable(), we can use that instead of abort(), resulting in a noticeable code savings since no function call is actually emitted. However, it seems wise to do this only in non-assert builds. In an assert build, continue to use abort(), so that the behavior will be predictable and debuggable if the "impossible" happens. These changes involve making the ereport and elog macros emit do-while statement blocks not just expressions, which forces small changes in a few call sites. Andres Freund, Tom Lane, Heikki Linnakangas --- config/c-compiler.m4 | 59 +++++++- configure | 178 +++++++++++++++++++++++++ configure.in | 3 + contrib/cube/cubescan.l | 8 +- contrib/seg/segscan.l | 8 +- src/backend/bootstrap/bootscanner.l | 8 +- src/backend/parser/scan.l | 8 +- src/backend/replication/repl_scanner.l | 8 +- src/include/c.h | 12 ++ src/include/pg_config.h.in | 9 ++ src/include/pg_config.h.win32 | 9 ++ src/include/utils/elog.h | 66 +++++++-- 12 files changed, 361 insertions(+), 15 deletions(-) diff --git a/config/c-compiler.m4 b/config/c-compiler.m4 index 7cbb8ec324..29db5b16b0 100644 --- a/config/c-compiler.m4 +++ b/config/c-compiler.m4 @@ -122,7 +122,7 @@ fi])# PGAC_C_FUNCNAME_SUPPORT # PGAC_C_STATIC_ASSERT -# ----------------------- +# -------------------- # Check if the C compiler understands _Static_assert(), # and define HAVE__STATIC_ASSERT if so. # @@ -161,6 +161,63 @@ fi])# PGAC_C_TYPES_COMPATIBLE +# PGAC_C_BUILTIN_CONSTANT_P +# ------------------------- +# Check if the C compiler understands __builtin_constant_p(), +# and define HAVE__BUILTIN_CONSTANT_P if so. +AC_DEFUN([PGAC_C_BUILTIN_CONSTANT_P], +[AC_CACHE_CHECK(for __builtin_constant_p, pgac_cv__builtin_constant_p, +[AC_TRY_COMPILE([static int x; static int y[__builtin_constant_p(x) ? x : 1];], +[], +[pgac_cv__builtin_constant_p=yes], +[pgac_cv__builtin_constant_p=no])]) +if test x"$pgac_cv__builtin_constant_p" = xyes ; then +AC_DEFINE(HAVE__BUILTIN_CONSTANT_P, 1, + [Define to 1 if your compiler understands __builtin_constant_p.]) +fi])# PGAC_C_BUILTIN_CONSTANT_P + + + +# PGAC_C_BUILTIN_UNREACHABLE +# -------------------------- +# Check if the C compiler understands __builtin_unreachable(), +# and define HAVE__BUILTIN_UNREACHABLE if so. +# +# NB: Don't get the idea of putting a for(;;); or such before the +# __builtin_unreachable() call. Some compilers would remove it before linking +# and only a warning instead of an error would be produced. +AC_DEFUN([PGAC_C_BUILTIN_UNREACHABLE], +[AC_CACHE_CHECK(for __builtin_unreachable, pgac_cv__builtin_unreachable, +[AC_TRY_LINK([], +[__builtin_unreachable();], +[pgac_cv__builtin_unreachable=yes], +[pgac_cv__builtin_unreachable=no])]) +if test x"$pgac_cv__builtin_unreachable" = xyes ; then +AC_DEFINE(HAVE__BUILTIN_UNREACHABLE, 1, + [Define to 1 if your compiler understands __builtin_unreachable.]) +fi])# PGAC_C_BUILTIN_UNREACHABLE + + + +# PGAC_C_VA_ARGS +# -------------- +# Check if the C compiler understands C99-style variadic macros, +# and define HAVE__VA_ARGS if so. +AC_DEFUN([PGAC_C_VA_ARGS], +[AC_CACHE_CHECK(for __VA_ARGS__, pgac_cv__va_args, +[AC_TRY_COMPILE([#include ], +[#define debug(...) fprintf(stderr, __VA_ARGS__) +debug("%s", "blarg"); +], +[pgac_cv__va_args=yes], +[pgac_cv__va_args=no])]) +if test x"$pgac_cv__va_args" = xyes ; then +AC_DEFINE(HAVE__VA_ARGS, 1, + [Define to 1 if your compiler understands __VA_ARGS__ in macros.]) +fi])# PGAC_C_VA_ARGS + + + # PGAC_PROG_CC_CFLAGS_OPT # ----------------------- # Given a string, check if the compiler supports the string as a diff --git a/configure b/configure index 10ae152304..9efd866059 100755 --- a/configure +++ b/configure @@ -15659,6 +15659,184 @@ cat >>confdefs.h <<\_ACEOF #define HAVE__BUILTIN_TYPES_COMPATIBLE_P 1 _ACEOF +fi +{ $as_echo "$as_me:$LINENO: checking for __builtin_constant_p" >&5 +$as_echo_n "checking for __builtin_constant_p... " >&6; } +if test "${pgac_cv__builtin_constant_p+set}" = set; then + $as_echo_n "(cached) " >&6 +else + cat >conftest.$ac_ext <<_ACEOF +/* confdefs.h. */ +_ACEOF +cat confdefs.h >>conftest.$ac_ext +cat >>conftest.$ac_ext <<_ACEOF +/* end confdefs.h. */ +static int x; static int y[__builtin_constant_p(x) ? x : 1]; +int +main () +{ + + ; + return 0; +} +_ACEOF +rm -f conftest.$ac_objext +if { (ac_try="$ac_compile" +case "(($ac_try" in + *\"* | *\`* | *\\*) ac_try_echo=\$ac_try;; + *) ac_try_echo=$ac_try;; +esac +eval ac_try_echo="\"\$as_me:$LINENO: $ac_try_echo\"" +$as_echo "$ac_try_echo") >&5 + (eval "$ac_compile") 2>conftest.er1 + ac_status=$? + grep -v '^ *+' conftest.er1 >conftest.err + rm -f conftest.er1 + cat conftest.err >&5 + $as_echo "$as_me:$LINENO: \$? = $ac_status" >&5 + (exit $ac_status); } && { + test -z "$ac_c_werror_flag" || + test ! -s conftest.err + } && test -s conftest.$ac_objext; then + pgac_cv__builtin_constant_p=yes +else + $as_echo "$as_me: failed program was:" >&5 +sed 's/^/| /' conftest.$ac_ext >&5 + + pgac_cv__builtin_constant_p=no +fi + +rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext +fi +{ $as_echo "$as_me:$LINENO: result: $pgac_cv__builtin_constant_p" >&5 +$as_echo "$pgac_cv__builtin_constant_p" >&6; } +if test x"$pgac_cv__builtin_constant_p" = xyes ; then + +cat >>confdefs.h <<\_ACEOF +#define HAVE__BUILTIN_CONSTANT_P 1 +_ACEOF + +fi +{ $as_echo "$as_me:$LINENO: checking for __builtin_unreachable" >&5 +$as_echo_n "checking for __builtin_unreachable... " >&6; } +if test "${pgac_cv__builtin_unreachable+set}" = set; then + $as_echo_n "(cached) " >&6 +else + cat >conftest.$ac_ext <<_ACEOF +/* confdefs.h. */ +_ACEOF +cat confdefs.h >>conftest.$ac_ext +cat >>conftest.$ac_ext <<_ACEOF +/* end confdefs.h. */ + +int +main () +{ +__builtin_unreachable(); + ; + return 0; +} +_ACEOF +rm -f conftest.$ac_objext conftest$ac_exeext +if { (ac_try="$ac_link" +case "(($ac_try" in + *\"* | *\`* | *\\*) ac_try_echo=\$ac_try;; + *) ac_try_echo=$ac_try;; +esac +eval ac_try_echo="\"\$as_me:$LINENO: $ac_try_echo\"" +$as_echo "$ac_try_echo") >&5 + (eval "$ac_link") 2>conftest.er1 + ac_status=$? + grep -v '^ *+' conftest.er1 >conftest.err + rm -f conftest.er1 + cat conftest.err >&5 + $as_echo "$as_me:$LINENO: \$? = $ac_status" >&5 + (exit $ac_status); } && { + test -z "$ac_c_werror_flag" || + test ! -s conftest.err + } && test -s conftest$ac_exeext && { + test "$cross_compiling" = yes || + $as_test_x conftest$ac_exeext + }; then + pgac_cv__builtin_unreachable=yes +else + $as_echo "$as_me: failed program was:" >&5 +sed 's/^/| /' conftest.$ac_ext >&5 + + pgac_cv__builtin_unreachable=no +fi + +rm -rf conftest.dSYM +rm -f core conftest.err conftest.$ac_objext conftest_ipa8_conftest.oo \ + conftest$ac_exeext conftest.$ac_ext +fi +{ $as_echo "$as_me:$LINENO: result: $pgac_cv__builtin_unreachable" >&5 +$as_echo "$pgac_cv__builtin_unreachable" >&6; } +if test x"$pgac_cv__builtin_unreachable" = xyes ; then + +cat >>confdefs.h <<\_ACEOF +#define HAVE__BUILTIN_UNREACHABLE 1 +_ACEOF + +fi +{ $as_echo "$as_me:$LINENO: checking for __VA_ARGS__" >&5 +$as_echo_n "checking for __VA_ARGS__... " >&6; } +if test "${pgac_cv__va_args+set}" = set; then + $as_echo_n "(cached) " >&6 +else + cat >conftest.$ac_ext <<_ACEOF +/* confdefs.h. */ +_ACEOF +cat confdefs.h >>conftest.$ac_ext +cat >>conftest.$ac_ext <<_ACEOF +/* end confdefs.h. */ +#include +int +main () +{ +#define debug(...) fprintf(stderr, __VA_ARGS__) +debug("%s", "blarg"); + + ; + return 0; +} +_ACEOF +rm -f conftest.$ac_objext +if { (ac_try="$ac_compile" +case "(($ac_try" in + *\"* | *\`* | *\\*) ac_try_echo=\$ac_try;; + *) ac_try_echo=$ac_try;; +esac +eval ac_try_echo="\"\$as_me:$LINENO: $ac_try_echo\"" +$as_echo "$ac_try_echo") >&5 + (eval "$ac_compile") 2>conftest.er1 + ac_status=$? + grep -v '^ *+' conftest.er1 >conftest.err + rm -f conftest.er1 + cat conftest.err >&5 + $as_echo "$as_me:$LINENO: \$? = $ac_status" >&5 + (exit $ac_status); } && { + test -z "$ac_c_werror_flag" || + test ! -s conftest.err + } && test -s conftest.$ac_objext; then + pgac_cv__va_args=yes +else + $as_echo "$as_me: failed program was:" >&5 +sed 's/^/| /' conftest.$ac_ext >&5 + + pgac_cv__va_args=no +fi + +rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext +fi +{ $as_echo "$as_me:$LINENO: result: $pgac_cv__va_args" >&5 +$as_echo "$pgac_cv__va_args" >&6; } +if test x"$pgac_cv__va_args" = xyes ; then + +cat >>confdefs.h <<\_ACEOF +#define HAVE__VA_ARGS 1 +_ACEOF + fi { $as_echo "$as_me:$LINENO: checking whether struct tm is in sys/time.h or time.h" >&5 $as_echo_n "checking whether struct tm is in sys/time.h or time.h... " >&6; } diff --git a/configure.in b/configure.in index e2682f3da5..f31f7efda0 100644 --- a/configure.in +++ b/configure.in @@ -1106,6 +1106,9 @@ AC_C_VOLATILE PGAC_C_FUNCNAME_SUPPORT PGAC_C_STATIC_ASSERT PGAC_C_TYPES_COMPATIBLE +PGAC_C_BUILTIN_CONSTANT_P +PGAC_C_BUILTIN_UNREACHABLE +PGAC_C_VA_ARGS PGAC_STRUCT_TIMEZONE PGAC_UNION_SEMUN PGAC_STRUCT_SOCKADDR_UN diff --git a/contrib/cube/cubescan.l b/contrib/cube/cubescan.l index c184930158..a74eb4ba37 100644 --- a/contrib/cube/cubescan.l +++ b/contrib/cube/cubescan.l @@ -11,7 +11,13 @@ /* Avoid exit() on fatal scanner errors (a bit ugly -- see yy_fatal_error) */ #undef fprintf -#define fprintf(file, fmt, msg) ereport(ERROR, (errmsg_internal("%s", msg))) +#define fprintf(file, fmt, msg) fprintf_to_ereport(fmt, msg) + +static void +fprintf_to_ereport(const char *fmt, const char *msg) +{ + ereport(ERROR, (errmsg_internal("%s", msg))); +} /* Handles to the buffer that the lexer uses internally */ static YY_BUFFER_STATE scanbufhandle; diff --git a/contrib/seg/segscan.l b/contrib/seg/segscan.l index e4feab39b3..ce5ff31e47 100644 --- a/contrib/seg/segscan.l +++ b/contrib/seg/segscan.l @@ -10,7 +10,13 @@ /* Avoid exit() on fatal scanner errors (a bit ugly -- see yy_fatal_error) */ #undef fprintf -#define fprintf(file, fmt, msg) ereport(ERROR, (errmsg_internal("%s", msg))) +#define fprintf(file, fmt, msg) fprintf_to_ereport(fmt, msg) + +static void +fprintf_to_ereport(const char *fmt, const char *msg) +{ + ereport(ERROR, (errmsg_internal("%s", msg))); +} /* Handles to the buffer that the lexer uses internally */ static YY_BUFFER_STATE scanbufhandle; diff --git a/src/backend/bootstrap/bootscanner.l b/src/backend/bootstrap/bootscanner.l index bdd7dcb1b3..ce57de61b2 100644 --- a/src/backend/bootstrap/bootscanner.l +++ b/src/backend/bootstrap/bootscanner.l @@ -42,7 +42,13 @@ /* Avoid exit() on fatal scanner errors (a bit ugly -- see yy_fatal_error) */ #undef fprintf -#define fprintf(file, fmt, msg) ereport(ERROR, (errmsg_internal("%s", msg))) +#define fprintf(file, fmt, msg) fprintf_to_ereport(fmt, msg) + +static void +fprintf_to_ereport(const char *fmt, const char *msg) +{ + ereport(ERROR, (errmsg_internal("%s", msg))); +} static int yyline = 1; /* line number for error reporting */ diff --git a/src/backend/parser/scan.l b/src/backend/parser/scan.l index 622cb1f57c..23c83c4fd9 100644 --- a/src/backend/parser/scan.l +++ b/src/backend/parser/scan.l @@ -42,7 +42,13 @@ /* Avoid exit() on fatal scanner errors (a bit ugly -- see yy_fatal_error) */ #undef fprintf -#define fprintf(file, fmt, msg) ereport(ERROR, (errmsg_internal("%s", msg))) +#define fprintf(file, fmt, msg) fprintf_to_ereport(fmt, msg) + +static void +fprintf_to_ereport(const char *fmt, const char *msg) +{ + ereport(ERROR, (errmsg_internal("%s", msg))); +} /* * GUC variables. This is a DIRECT violation of the warning given at the diff --git a/src/backend/replication/repl_scanner.l b/src/backend/replication/repl_scanner.l index 9ccf02a040..a124498306 100644 --- a/src/backend/replication/repl_scanner.l +++ b/src/backend/replication/repl_scanner.l @@ -19,7 +19,13 @@ /* Avoid exit() on fatal scanner errors (a bit ugly -- see yy_fatal_error) */ #undef fprintf -#define fprintf(file, fmt, msg) ereport(ERROR, (errmsg_internal("%s", msg))) +#define fprintf(file, fmt, msg) fprintf_to_ereport(fmt, msg) + +static void +fprintf_to_ereport(const char *fmt, const char *msg) +{ + ereport(ERROR, (errmsg_internal("%s", msg))); +} /* Handle to the buffer that the lexer uses internally */ static YY_BUFFER_STATE scanbufhandle; diff --git a/src/include/c.h b/src/include/c.h index f7db157f83..59af5b5cb8 100644 --- a/src/include/c.h +++ b/src/include/c.h @@ -748,6 +748,18 @@ typedef NameData *Name; #endif /* HAVE__BUILTIN_TYPES_COMPATIBLE_P */ +/* + * Mark a point as unreachable in a portable fashion. This should preferably + * be something that the compiler understands, to aid code generation. + * In assert-enabled builds, we prefer abort() for debugging reasons. + */ +#if defined(HAVE__BUILTIN_UNREACHABLE) && !defined(USE_ASSERT_CHECKING) +#define pg_unreachable() __builtin_unreachable() +#else +#define pg_unreachable() abort() +#endif + + /* * Function inlining support -- Allow modules to define functions that may be * inlined, if the compiler supports it. diff --git a/src/include/pg_config.h.in b/src/include/pg_config.h.in index edaf85319b..8aabf3c87a 100644 --- a/src/include/pg_config.h.in +++ b/src/include/pg_config.h.in @@ -635,12 +635,21 @@ /* Define to 1 if you have the header file. */ #undef HAVE_WINLDAP_H +/* Define to 1 if your compiler understands __builtin_constant_p. */ +#undef HAVE__BUILTIN_CONSTANT_P + /* Define to 1 if your compiler understands __builtin_types_compatible_p. */ #undef HAVE__BUILTIN_TYPES_COMPATIBLE_P +/* Define to 1 if your compiler understands __builtin_unreachable. */ +#undef HAVE__BUILTIN_UNREACHABLE + /* Define to 1 if your compiler understands _Static_assert. */ #undef HAVE__STATIC_ASSERT +/* Define to 1 if your compiler understands __VA_ARGS__ in macros. */ +#undef HAVE__VA_ARGS + /* Define to the appropriate snprintf format for 64-bit ints. */ #undef INT64_FORMAT diff --git a/src/include/pg_config.h.win32 b/src/include/pg_config.h.win32 index 626ad47883..31b9a49d4c 100644 --- a/src/include/pg_config.h.win32 +++ b/src/include/pg_config.h.win32 @@ -526,12 +526,21 @@ /* Define to 1 if you have the header file. */ /* #undef HAVE_WINLDAP_H */ +/* Define to 1 if your compiler understands __builtin_constant_p. */ +/* #undef HAVE__BUILTIN_CONSTANT_P */ + /* Define to 1 if your compiler understands __builtin_types_compatible_p. */ /* #undef HAVE__BUILTIN_TYPES_COMPATIBLE_P */ +/* Define to 1 if your compiler understands __builtin_unreachable. */ +/* #undef HAVE__BUILTIN_UNREACHABLE */ + /* Define to 1 if your compiler understands _Static_assert. */ /* #undef HAVE__STATIC_ASSERT */ +/* Define to 1 if your compiler understands __VA_ARGS__ in macros. */ +#define HAVE__VA_ARGS 1 + /* Define to the appropriate snprintf format for 64-bit ints, if any. */ #define INT64_FORMAT "%lld" diff --git a/src/include/utils/elog.h b/src/include/utils/elog.h index cbbda04005..5e937fb10c 100644 --- a/src/include/utils/elog.h +++ b/src/include/utils/elog.h @@ -101,15 +101,33 @@ * ereport_domain() directly, or preferably they can override the TEXTDOMAIN * macro. * - * When elevel >= ERROR, we add an abort() call to give the compiler a hint - * that the ereport() expansion will not return, but the abort() isn't actually - * reached because the longjmp happens in errfinish(). + * If elevel >= ERROR, the call will not return; we try to inform the compiler + * of that via pg_unreachable(). However, no useful optimization effect is + * obtained unless the compiler sees elevel as a compile-time constant, else + * we're just adding code bloat. So, if __builtin_constant_p is available, + * use that to cause the second if() to vanish completely for non-constant + * cases. We avoid using a local variable because it's not necessary and + * prevents gcc from making the unreachability deduction at optlevel -O0. *---------- */ +#ifdef HAVE__BUILTIN_CONSTANT_P #define ereport_domain(elevel, domain, rest) \ - (errstart(elevel, __FILE__, __LINE__, PG_FUNCNAME_MACRO, domain) ? \ - (errfinish rest) : (void) 0), \ - ((elevel) >= ERROR ? abort() : (void) 0) + do { \ + if (errstart(elevel, __FILE__, __LINE__, PG_FUNCNAME_MACRO, domain)) \ + errfinish rest; \ + if (__builtin_constant_p(elevel) && (elevel) >= ERROR) \ + pg_unreachable(); \ + } while(0) +#else /* !HAVE__BUILTIN_CONSTANT_P */ +#define ereport_domain(elevel, domain, rest) \ + do { \ + const int elevel_ = (elevel); \ + if (errstart(elevel_, __FILE__, __LINE__, PG_FUNCNAME_MACRO, domain)) \ + errfinish rest; \ + if (elevel_ >= ERROR) \ + pg_unreachable(); \ + } while(0) +#endif /* HAVE__BUILTIN_CONSTANT_P */ #define ereport(elevel, rest) \ ereport_domain(elevel, TEXTDOMAIN, rest) @@ -212,7 +230,37 @@ extern int getinternalerrposition(void); * elog(ERROR, "portal \"%s\" not found", stmt->portalname); *---------- */ -#define elog elog_start(__FILE__, __LINE__, PG_FUNCNAME_MACRO), elog_finish +#ifdef HAVE__VA_ARGS +/* + * If we have variadic macros, we can give the compiler a hint about the + * call not returning when elevel >= ERROR. See comments for ereport(). + * Note that historically elog() has called elog_start (which saves errno) + * before evaluating "elevel", so we preserve that behavior here. + */ +#ifdef HAVE__BUILTIN_CONSTANT_P +#define elog(elevel, ...) \ + do { \ + elog_start(__FILE__, __LINE__, PG_FUNCNAME_MACRO); \ + elog_finish(elevel, __VA_ARGS__); \ + if (__builtin_constant_p(elevel) && (elevel) >= ERROR) \ + pg_unreachable(); \ + } while(0) +#else /* !HAVE__BUILTIN_CONSTANT_P */ +#define elog(elevel, ...) \ + do { \ + int elevel_; \ + elog_start(__FILE__, __LINE__, PG_FUNCNAME_MACRO); \ + elevel_ = (elevel); \ + elog_finish(elevel_, __VA_ARGS__); \ + if (elevel_ >= ERROR) \ + pg_unreachable(); \ + } while(0) +#endif /* HAVE__BUILTIN_CONSTANT_P */ +#else /* !HAVE__VA_ARGS */ +#define elog \ + elog_start(__FILE__, __LINE__, PG_FUNCNAME_MACRO), \ + elog_finish +#endif /* HAVE__VA_ARGS */ extern void elog_start(const char *filename, int lineno, const char *funcname); extern void @@ -299,14 +347,14 @@ extern PGDLLIMPORT ErrorContextCallback *error_context_stack; /* * gcc understands __attribute__((noreturn)); for other compilers, insert - * a useless exit() call so that the compiler gets the point. + * pg_unreachable() so that the compiler gets the point. */ #ifdef __GNUC__ #define PG_RE_THROW() \ pg_re_throw() #else #define PG_RE_THROW() \ - (pg_re_throw(), exit(1)) + (pg_re_throw(), pg_unreachable()) #endif extern PGDLLIMPORT sigjmp_buf *PG_exception_stack;