On 2020-07-04 16:16, Tom Lane wrote:
Peter Eisentraut <peter.eisentr...@2ndquadrant.com> writes:
Do people prefer a typedef or just writing it out, like it's done in the
Python code?
I'm for a typedef. There is *nothing* readable about "(void (*) (void))",
and the fact that it's theoretically incorrect for the purpose doesn't
exactly aid intelligibility either. With a typedef, not only are
the uses more readable but there's a place to put a comment explaining
that this is notionally wrong but it's what gcc specifies to use
to suppress thus-and-such warnings.
Makes sense. New patch here.
But if we prefer a typedef then I'd propose
GenericFuncPtr like in the initial patch.
That name is OK by me.
I changed that to pg_funcptr_t, to look a bit more like C and less like
Java. ;-)
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From 96149ead0af749592d3f2eb87929533ceb416d76 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pe...@eisentraut.org>
Date: Tue, 7 Jul 2020 11:41:09 +0200
Subject: [PATCH v3] Fix -Wcast-function-type warnings
Three groups of issues needed to be addressed:
load_external_function() and related functions returned PGFunction,
even though not necessarily all callers are looking for a function of
type PGFunction. Since these functions are really just wrappers
around dlsym(), change to return void * just like dlsym().
In dynahash.c, we are using strlcpy() were a function with a signature
like memcpy() is expected. This should be safe, as the new comment
there explains, but the cast needs to be augmented to avoid the
warning.
In PL/Python, methods all need to be cast to PyCFunction, per Python
API, but this now runs afoul of these warnings. (This issue also
exists in core CPython.)
To fix the second and third case, we add a new type pg_funcptr_t that
is defined specifically so that gcc accepts it as a generic function
pointer that can be cast to any other function pointer.
Also add -Wcast-function-type to the standard warning flags, subject
to configure check.
Discussion:
https://www.postgresql.org/message-id/flat/1e97628e-6447-b4fd-e230-d109cec2d584%402ndquadrant.com
---
configure | 91 +++++++++++++++++++++++++++++++
configure.in | 2 +
src/backend/utils/fmgr/dfmgr.c | 14 ++---
src/backend/utils/hash/dynahash.c | 11 +++-
src/include/c.h | 9 +++
src/include/fmgr.h | 6 +-
src/pl/plpython/plpy_plpymodule.c | 14 ++---
7 files changed, 129 insertions(+), 18 deletions(-)
diff --git a/configure b/configure
index 2feff37fe3..9907637e31 100755
--- a/configure
+++ b/configure
@@ -5643,6 +5643,97 @@ if test
x"$pgac_cv_prog_CXX_cxxflags__Wimplicit_fallthrough_3" = x"yes"; then
fi
+
+{ $as_echo "$as_me:${as_lineno-$LINENO}: checking whether ${CC} supports
-Wcast-function-type, for CFLAGS" >&5
+$as_echo_n "checking whether ${CC} supports -Wcast-function-type, for
CFLAGS... " >&6; }
+if ${pgac_cv_prog_CC_cflags__Wcast_function_type+:} false; then :
+ $as_echo_n "(cached) " >&6
+else
+ pgac_save_CFLAGS=$CFLAGS
+pgac_save_CC=$CC
+CC=${CC}
+CFLAGS="${CFLAGS} -Wcast-function-type"
+ac_save_c_werror_flag=$ac_c_werror_flag
+ac_c_werror_flag=yes
+cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+/* end confdefs.h. */
+
+int
+main ()
+{
+
+ ;
+ return 0;
+}
+_ACEOF
+if ac_fn_c_try_compile "$LINENO"; then :
+ pgac_cv_prog_CC_cflags__Wcast_function_type=yes
+else
+ pgac_cv_prog_CC_cflags__Wcast_function_type=no
+fi
+rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext
+ac_c_werror_flag=$ac_save_c_werror_flag
+CFLAGS="$pgac_save_CFLAGS"
+CC="$pgac_save_CC"
+fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: result:
$pgac_cv_prog_CC_cflags__Wcast_function_type" >&5
+$as_echo "$pgac_cv_prog_CC_cflags__Wcast_function_type" >&6; }
+if test x"$pgac_cv_prog_CC_cflags__Wcast_function_type" = x"yes"; then
+ CFLAGS="${CFLAGS} -Wcast-function-type"
+fi
+
+
+ { $as_echo "$as_me:${as_lineno-$LINENO}: checking whether ${CXX} supports
-Wcast-function-type, for CXXFLAGS" >&5
+$as_echo_n "checking whether ${CXX} supports -Wcast-function-type, for
CXXFLAGS... " >&6; }
+if ${pgac_cv_prog_CXX_cxxflags__Wcast_function_type+:} false; then :
+ $as_echo_n "(cached) " >&6
+else
+ pgac_save_CXXFLAGS=$CXXFLAGS
+pgac_save_CXX=$CXX
+CXX=${CXX}
+CXXFLAGS="${CXXFLAGS} -Wcast-function-type"
+ac_save_cxx_werror_flag=$ac_cxx_werror_flag
+ac_cxx_werror_flag=yes
+ac_ext=cpp
+ac_cpp='$CXXCPP $CPPFLAGS'
+ac_compile='$CXX -c $CXXFLAGS $CPPFLAGS conftest.$ac_ext >&5'
+ac_link='$CXX -o conftest$ac_exeext $CXXFLAGS $CPPFLAGS $LDFLAGS
conftest.$ac_ext $LIBS >&5'
+ac_compiler_gnu=$ac_cv_cxx_compiler_gnu
+
+cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+/* end confdefs.h. */
+
+int
+main ()
+{
+
+ ;
+ return 0;
+}
+_ACEOF
+if ac_fn_cxx_try_compile "$LINENO"; then :
+ pgac_cv_prog_CXX_cxxflags__Wcast_function_type=yes
+else
+ pgac_cv_prog_CXX_cxxflags__Wcast_function_type=no
+fi
+rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext
+ac_ext=c
+ac_cpp='$CPP $CPPFLAGS'
+ac_compile='$CC -c $CFLAGS $CPPFLAGS conftest.$ac_ext >&5'
+ac_link='$CC -o conftest$ac_exeext $CFLAGS $CPPFLAGS $LDFLAGS conftest.$ac_ext
$LIBS >&5'
+ac_compiler_gnu=$ac_cv_c_compiler_gnu
+
+ac_cxx_werror_flag=$ac_save_cxx_werror_flag
+CXXFLAGS="$pgac_save_CXXFLAGS"
+CXX="$pgac_save_CXX"
+fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: result:
$pgac_cv_prog_CXX_cxxflags__Wcast_function_type" >&5
+$as_echo "$pgac_cv_prog_CXX_cxxflags__Wcast_function_type" >&6; }
+if test x"$pgac_cv_prog_CXX_cxxflags__Wcast_function_type" = x"yes"; then
+ CXXFLAGS="${CXXFLAGS} -Wcast-function-type"
+fi
+
+
# This was included in -Wall/-Wformat in older GCC versions
{ $as_echo "$as_me:${as_lineno-$LINENO}: checking whether ${CC} supports
-Wformat-security, for CFLAGS" >&5
diff --git a/configure.in b/configure.in
index 0188c6ff07..2e05ce2e4d 100644
--- a/configure.in
+++ b/configure.in
@@ -498,6 +498,8 @@ if test "$GCC" = yes -a "$ICC" = no; then
PGAC_PROG_CXX_CFLAGS_OPT([-Wmissing-format-attribute])
PGAC_PROG_CC_CFLAGS_OPT([-Wimplicit-fallthrough=3])
PGAC_PROG_CXX_CFLAGS_OPT([-Wimplicit-fallthrough=3])
+ PGAC_PROG_CC_CFLAGS_OPT([-Wcast-function-type])
+ PGAC_PROG_CXX_CFLAGS_OPT([-Wcast-function-type])
# This was included in -Wall/-Wformat in older GCC versions
PGAC_PROG_CC_CFLAGS_OPT([-Wformat-security])
PGAC_PROG_CXX_CFLAGS_OPT([-Wformat-security])
diff --git a/src/backend/utils/fmgr/dfmgr.c b/src/backend/utils/fmgr/dfmgr.c
index 9dff1f5e82..bd779fdaf7 100644
--- a/src/backend/utils/fmgr/dfmgr.c
+++ b/src/backend/utils/fmgr/dfmgr.c
@@ -95,7 +95,7 @@ static const Pg_magic_struct magic_data =
PG_MODULE_MAGIC_DATA;
* named funcname in it.
*
* If the function is not found, we raise an error if signalNotFound is true,
- * else return (PGFunction) NULL. Note that errors in loading the library
+ * else return NULL. Note that errors in loading the library
* will provoke ereport() regardless of signalNotFound.
*
* If filehandle is not NULL, then *filehandle will be set to a handle
@@ -103,13 +103,13 @@ static const Pg_magic_struct magic_data =
PG_MODULE_MAGIC_DATA;
* lookup_external_function to lookup additional functions in the same file
* at less cost than repeating load_external_function.
*/
-PGFunction
+void *
load_external_function(const char *filename, const char *funcname,
bool signalNotFound, void
**filehandle)
{
char *fullname;
void *lib_handle;
- PGFunction retval;
+ void *retval;
/* Expand the possibly-abbreviated filename to an exact path name */
fullname = expand_dynamic_library_name(filename);
@@ -122,7 +122,7 @@ load_external_function(const char *filename, const char
*funcname,
*filehandle = lib_handle;
/* Look up the function within the library. */
- retval = (PGFunction) dlsym(lib_handle, funcname);
+ retval = dlsym(lib_handle, funcname);
if (retval == NULL && signalNotFound)
ereport(ERROR,
@@ -165,12 +165,12 @@ load_file(const char *filename, bool restricted)
/*
* Lookup a function whose library file is already loaded.
- * Return (PGFunction) NULL if not found.
+ * Return NULL if not found.
*/
-PGFunction
+void *
lookup_external_function(void *filehandle, const char *funcname)
{
- return (PGFunction) dlsym(filehandle, funcname);
+ return dlsym(filehandle, funcname);
}
diff --git a/src/backend/utils/hash/dynahash.c
b/src/backend/utils/hash/dynahash.c
index 2688e27726..5948b01abc 100644
--- a/src/backend/utils/hash/dynahash.c
+++ b/src/backend/utils/hash/dynahash.c
@@ -398,7 +398,16 @@ hash_create(const char *tabname, long nelem, HASHCTL
*info, int flags)
if (flags & HASH_KEYCOPY)
hashp->keycopy = info->keycopy;
else if (hashp->hash == string_hash)
- hashp->keycopy = (HashCopyFunc) strlcpy;
+ {
+ /*
+ * The signature of keycopy is meant for memcpy(), which returns
+ * void*, but strlcpy() returns size_t. Since we never use the
return
+ * value of keycopy, and size_t is pretty much always the same
size as
+ * void *, this should be safe. The extra cast in the middle
is to
+ * avoid warnings from -Wcast-function-type.
+ */
+ hashp->keycopy = (HashCopyFunc) (pg_funcptr_t) strlcpy;
+ }
else
hashp->keycopy = memcpy;
diff --git a/src/include/c.h b/src/include/c.h
index a904b49a37..c83a09f2d3 100644
--- a/src/include/c.h
+++ b/src/include/c.h
@@ -1208,6 +1208,15 @@ typedef union PGAlignedXLogBlock
((underlying_type) (expr))
#endif
+
+/*
+ * Generic function pointer. This can be used in the rare cases where it's
+ * necessary to cast a function pointer to a seemingly incompatible type while
+ * avoiding gcc's -Wcast-function-type warnings.
+ */
+typedef void (*pg_funcptr_t) (void);
+
+
/* ----------------------------------------------------------------
* Section 9: system-specific hacks
*
diff --git a/src/include/fmgr.h b/src/include/fmgr.h
index d349510b7c..f25068fae2 100644
--- a/src/include/fmgr.h
+++ b/src/include/fmgr.h
@@ -716,9 +716,9 @@ extern bool CheckFunctionValidatorAccess(Oid validatorOid,
Oid functionOid);
*/
extern char *Dynamic_library_path;
-extern PGFunction load_external_function(const char *filename, const char
*funcname,
-
bool signalNotFound, void **filehandle);
-extern PGFunction lookup_external_function(void *filehandle, const char
*funcname);
+extern void *load_external_function(const char *filename, const char *funcname,
+ bool
signalNotFound, void **filehandle);
+extern void *lookup_external_function(void *filehandle, const char *funcname);
extern void load_file(const char *filename, bool restricted);
extern void **find_rendezvous_variable(const char *varName);
extern Size EstimateLibraryStateSpace(void);
diff --git a/src/pl/plpython/plpy_plpymodule.c
b/src/pl/plpython/plpy_plpymodule.c
index e308c61d50..7f54d093ac 100644
--- a/src/pl/plpython/plpy_plpymodule.c
+++ b/src/pl/plpython/plpy_plpymodule.c
@@ -61,13 +61,13 @@ static PyMethodDef PLy_methods[] = {
/*
* logging methods
*/
- {"debug", (PyCFunction) PLy_debug, METH_VARARGS | METH_KEYWORDS, NULL},
- {"log", (PyCFunction) PLy_log, METH_VARARGS | METH_KEYWORDS, NULL},
- {"info", (PyCFunction) PLy_info, METH_VARARGS | METH_KEYWORDS, NULL},
- {"notice", (PyCFunction) PLy_notice, METH_VARARGS | METH_KEYWORDS,
NULL},
- {"warning", (PyCFunction) PLy_warning, METH_VARARGS | METH_KEYWORDS,
NULL},
- {"error", (PyCFunction) PLy_error, METH_VARARGS | METH_KEYWORDS, NULL},
- {"fatal", (PyCFunction) PLy_fatal, METH_VARARGS | METH_KEYWORDS, NULL},
+ {"debug", (PyCFunction) (pg_funcptr_t) PLy_debug, METH_VARARGS |
METH_KEYWORDS, NULL},
+ {"log", (PyCFunction) (pg_funcptr_t) PLy_log, METH_VARARGS |
METH_KEYWORDS, NULL},
+ {"info", (PyCFunction) (pg_funcptr_t) PLy_info, METH_VARARGS |
METH_KEYWORDS, NULL},
+ {"notice", (PyCFunction) (pg_funcptr_t) PLy_notice, METH_VARARGS |
METH_KEYWORDS, NULL},
+ {"warning", (PyCFunction) (pg_funcptr_t) PLy_warning, METH_VARARGS |
METH_KEYWORDS, NULL},
+ {"error", (PyCFunction) (pg_funcptr_t) PLy_error, METH_VARARGS |
METH_KEYWORDS, NULL},
+ {"fatal", (PyCFunction) (pg_funcptr_t) PLy_fatal, METH_VARARGS |
METH_KEYWORDS, NULL},
/*
* create a stored plan
--
2.27.0
