From 2e30d77a196810f7e9d57709a942b1826fd1bb43 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Thu, 17 Feb 2022 22:45:34 -0500 Subject: [PATCH] Suppress warning about stack_base_ptr with late-model GCC. GCC 12 complains that set_stack_base is storing the address of a local variable in a long-lived pointer. This is an entirely reasonable warning (indeed, it just helped us find a bug); but that behavior is intentional here. We can work around it by using __builtin_frame_address(0) instead of a specific local variable; that produces an address a dozen or so bytes different, in my testing, but we don't care about such a small difference. Maybe someday a compiler lacking that function will start to issue a similar warning, but we'll worry about that when it happens. Patch by me, per a suggestion from Andres Freund. Back-patch to v12, which is as far back as the patch will go without some pain. (Recently-established project policy would permit a back-patch as far as 9.2, but I'm disinclined to expend the work until GCC 12 is much more widespread.) Discussion: https://postgr.es/m/3773792.1645141467@sss.pgh.pa.us --- config/c-compiler.m4 | 22 ++++++++++++++++ configure | 40 +++++++++++++++++++++++++++++ configure.ac | 3 +++ src/backend/postmaster/postmaster.c | 2 +- src/backend/tcop/postgres.c | 12 ++++++++- src/backend/utils/init/miscinit.c | 11 ++++---- src/include/pg_config.h.in | 3 +++ src/tools/msvc/Solution.pm | 1 + 8 files changed, 86 insertions(+), 8 deletions(-) diff --git a/config/c-compiler.m4 b/config/c-compiler.m4 index 780e906ecc..d3562d6fee 100644 --- a/config/c-compiler.m4 +++ b/config/c-compiler.m4 @@ -381,6 +381,28 @@ fi])# PGAC_CHECK_BUILTIN_FUNC +# PGAC_CHECK_BUILTIN_FUNC_PTR +# ----------------------- +# Like PGAC_CHECK_BUILTIN_FUNC, except that the function is assumed to +# return a pointer type, and the argument(s) should be given literally. +# This handles some cases that PGAC_CHECK_BUILTIN_FUNC doesn't. +AC_DEFUN([PGAC_CHECK_BUILTIN_FUNC_PTR], +[AC_CACHE_CHECK(for $1, pgac_cv$1, +[AC_LINK_IFELSE([AC_LANG_PROGRAM([ +void * +call$1(void) +{ + return $1($2); +}], [])], +[pgac_cv$1=yes], +[pgac_cv$1=no])]) +if test x"${pgac_cv$1}" = xyes ; then +AC_DEFINE_UNQUOTED(AS_TR_CPP([HAVE$1]), 1, + [Define to 1 if your compiler understands $1.]) +fi])# PGAC_CHECK_BUILTIN_FUNC_PTR + + + # PGAC_PROG_VARCC_VARFLAGS_OPT # ---------------------------- # Given a compiler, variable name and a string, check if the compiler diff --git a/configure b/configure index dcf03ce6a4..4267c7e1d5 100755 --- a/configure +++ b/configure @@ -15944,6 +15944,46 @@ cat >>confdefs.h <<_ACEOF #define HAVE__BUILTIN_POPCOUNT 1 _ACEOF +fi +# __builtin_frame_address may draw a diagnostic for non-constant argument, +# so it needs a different test function. +{ $as_echo "$as_me:${as_lineno-$LINENO}: checking for __builtin_frame_address" >&5 +$as_echo_n "checking for __builtin_frame_address... " >&6; } +if ${pgac_cv__builtin_frame_address+:} false; then : + $as_echo_n "(cached) " >&6 +else + cat confdefs.h - <<_ACEOF >conftest.$ac_ext +/* end confdefs.h. */ + +void * +call__builtin_frame_address(void) +{ + return __builtin_frame_address(0); +} +int +main () +{ + + ; + return 0; +} +_ACEOF +if ac_fn_c_try_link "$LINENO"; then : + pgac_cv__builtin_frame_address=yes +else + pgac_cv__builtin_frame_address=no +fi +rm -f core conftest.err conftest.$ac_objext \ + conftest$ac_exeext conftest.$ac_ext +fi +{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $pgac_cv__builtin_frame_address" >&5 +$as_echo "$pgac_cv__builtin_frame_address" >&6; } +if test x"${pgac_cv__builtin_frame_address}" = xyes ; then + +cat >>confdefs.h <<_ACEOF +#define HAVE__BUILTIN_FRAME_ADDRESS 1 +_ACEOF + fi # We require 64-bit fseeko() to be available, but run this check anyway diff --git a/configure.ac b/configure.ac index 4b9cf07ee7..225e5afd87 100644 --- a/configure.ac +++ b/configure.ac @@ -1769,6 +1769,9 @@ PGAC_CHECK_BUILTIN_FUNC([__builtin_bswap64], [long int x]) PGAC_CHECK_BUILTIN_FUNC([__builtin_clz], [unsigned int x]) PGAC_CHECK_BUILTIN_FUNC([__builtin_ctz], [unsigned int x]) PGAC_CHECK_BUILTIN_FUNC([__builtin_popcount], [unsigned int x]) +# __builtin_frame_address may draw a diagnostic for non-constant argument, +# so it needs a different test function. +PGAC_CHECK_BUILTIN_FUNC_PTR([__builtin_frame_address], [0]) # We require 64-bit fseeko() to be available, but run this check anyway # in case it finds that _LARGEFILE_SOURCE has to be #define'd for that. diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c index 581d552acf..ad35340a52 100644 --- a/src/backend/postmaster/postmaster.c +++ b/src/backend/postmaster/postmaster.c @@ -1033,7 +1033,7 @@ PostmasterMain(int argc, char *argv[]) /* * Set reference point for stack-depth checking. */ - set_stack_base(); + (void) set_stack_base(); /* * Initialize pipe (or process handle on Windows) that allows children to diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c index 171f3a9500..2c50575b37 100644 --- a/src/backend/tcop/postgres.c +++ b/src/backend/tcop/postgres.c @@ -3415,7 +3415,9 @@ ia64_get_bsp(void) pg_stack_base_t set_stack_base(void) { +#ifndef HAVE__BUILTIN_FRAME_ADDRESS char stack_base; +#endif pg_stack_base_t old; #if defined(__ia64__) || defined(__ia64) @@ -3425,8 +3427,16 @@ set_stack_base(void) old = stack_base_ptr; #endif - /* Set up reference point for stack depth checking */ + /* + * Set up reference point for stack depth checking. On recent gcc we use + * __builtin_frame_address() to avoid a warning about storing a local + * variable's address in a long-lived variable. + */ +#ifdef HAVE__BUILTIN_FRAME_ADDRESS + stack_base_ptr = __builtin_frame_address(0); +#else stack_base_ptr = &stack_base; +#endif #if defined(__ia64__) || defined(__ia64) register_stack_base_ptr = ia64_get_bsp(); #endif diff --git a/src/backend/utils/init/miscinit.c b/src/backend/utils/init/miscinit.c index 8b73850d0d..e3110cda7f 100644 --- a/src/backend/utils/init/miscinit.c +++ b/src/backend/utils/init/miscinit.c @@ -95,13 +95,12 @@ InitPostmasterChild(void) IsUnderPostmaster = true; /* we are a postmaster subprocess now */ /* - * Set reference point for stack-depth checking. We re-do that even in the - * !EXEC_BACKEND case, because there are some edge cases where processes - * are started with an alternative stack (e.g. starting bgworkers when - * running postgres using the rr debugger, as bgworkers are launched from - * signal handlers). + * Set reference point for stack-depth checking. This might seem + * redundant in !EXEC_BACKEND builds; but it's not because the postmaster + * launches its children from signal handlers, so we might be running on + * an alternative stack. */ - set_stack_base(); + (void) set_stack_base(); InitProcessGlobals(); diff --git a/src/include/pg_config.h.in b/src/include/pg_config.h.in index 6c7eea17d4..5d12b21ccd 100644 --- a/src/include/pg_config.h.in +++ b/src/include/pg_config.h.in @@ -745,6 +745,9 @@ /* Define to 1 if your compiler understands __builtin_ctz. */ #undef HAVE__BUILTIN_CTZ +/* Define to 1 if your compiler understands __builtin_frame_address. */ +#undef HAVE__BUILTIN_FRAME_ADDRESS + /* Define to 1 if your compiler understands __builtin_$op_overflow. */ #undef HAVE__BUILTIN_OP_OVERFLOW diff --git a/src/tools/msvc/Solution.pm b/src/tools/msvc/Solution.pm index c983f506f9..acd3b8de06 100644 --- a/src/tools/msvc/Solution.pm +++ b/src/tools/msvc/Solution.pm @@ -441,6 +441,7 @@ sub GenerateFiles HAVE__BUILTIN_CLZ => undef, HAVE__BUILTIN_CONSTANT_P => undef, HAVE__BUILTIN_CTZ => undef, + HAVE__BUILTIN_FRAME_ADDRESS => undef, HAVE__BUILTIN_OP_OVERFLOW => undef, HAVE__BUILTIN_POPCOUNT => undef, HAVE__BUILTIN_TYPES_COMPATIBLE_P => undef,