[Peter Eisentraut <peter.eisentraut@2ndquadrant.com>]

Some time ago, there were some discussions about gcc warnings produced 
by -Wcast-function-type [0].  To clarify, while that thread seemed to 
imply that the warnings appear by default in some compiler version, this 
is not the case AFAICT, and the warnings are entirely optional.

So I took a look at what it would take to fix all the warnings and came 
up with the attached patch.

There are three subplots:

1. Changing the return type of load_external_function() and 
lookup_external_function() from PGFunction to a generic pointer type, 
which is what the discussion in [0] started out about.

2. There is a bit of cheating in dynahash.c.  They keycopy field is 
declared as a function pointer that returns a pointer to the 
destination, to match the signature of memcpy(), but then we assign 
strlcpy() to it, which returns size_t.  Even though we never use the 
return value, I'm not sure whether this could break if size_t and 
pointers are of different sizes, which in turn is very unlikely.

3. Finally, there is some nonsense necessary in plpython, which is 
annoying but otherwise uninteresting.

Is there anything we want to pursue further here?


[0]: 
https://www.postgresql.org/message-id/flat/20180206200205.f5kvbyn6jawtzi6s%40alap3.anarazel.de

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Attachments:

0001-Fix-Wcast-function-type-warnings.patch (text/plain, 10.5K) download
From 7212f3e13107bf262dd0e0c4baeb2f8170ca6073 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Tue, 30 Jun 2020 01:19:56 +0200
Subject: [PATCH] Fix -Wcast-function-type warnings

Discussion: https://www.postgresql.org/message-id/flat/20180206200205.f5kvbyn6jawtzi6s%40alap3.anarazel.de
---
 configure                         | 91 +++++++++++++++++++++++++++++++
 configure.in                      |  2 +
 src/backend/utils/fmgr/dfmgr.c    | 14 ++---
 src/backend/utils/fmgr/fmgr.c     |  2 +-
 src/backend/utils/hash/dynahash.c | 11 +++-
 src/include/fmgr.h                |  8 ++-
 src/pl/plpython/plpy_plpymodule.c | 14 ++---
 7 files changed, 123 insertions(+), 19 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..ccef74aae5 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 (GenericFuncPtr) 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
+GenericFuncPtr
 load_external_function(const char *filename, const char *funcname,
 					   bool signalNotFound, void **filehandle)
 {
 	char	   *fullname;
 	void	   *lib_handle;
-	PGFunction	retval;
+	GenericFuncPtr 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 (GenericFuncPtr) NULL if not found.
  */
-PGFunction
+GenericFuncPtr
 lookup_external_function(void *filehandle, const char *funcname)
 {
-	return (PGFunction) dlsym(filehandle, funcname);
+	return dlsym(filehandle, funcname);
 }
 
 
diff --git a/src/backend/utils/fmgr/fmgr.c b/src/backend/utils/fmgr/fmgr.c
index 03c614b234..648908de91 100644
--- a/src/backend/utils/fmgr/fmgr.c
+++ b/src/backend/utils/fmgr/fmgr.c
@@ -400,7 +400,7 @@ fmgr_info_C_lang(Oid functionId, FmgrInfo *finfo, HeapTuple procedureTuple)
 		probinstring = TextDatumGetCString(probinattr);
 
 		/* Look up the function itself */
-		user_fn = load_external_function(probinstring, prosrcstring, true,
+		user_fn = (PGFunction) load_external_function(probinstring, prosrcstring, true,
 										 &libraryhandle);
 
 		/* Get the function information record (real or default) */
diff --git a/src/backend/utils/hash/dynahash.c b/src/backend/utils/hash/dynahash.c
index 2688e27726..42e37abfde 100644
--- a/src/backend/utils/hash/dynahash.c
+++ b/src/backend/utils/hash/dynahash.c
@@ -297,6 +297,15 @@ string_compare(const char *key1, const char *key2, Size keysize)
 	return strncmp(key1, key2, keysize - 1);
 }
 
+/*
+ * HashCopyFunc for string keys
+ */
+static void *
+string_copy(void *dest, const void *src, Size keysize)
+{
+	strlcpy(dest, src, keysize);
+	return dest;
+}
 
 /************************** CREATE ROUTINES **********************/
 
@@ -398,7 +407,7 @@ 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;
+		hashp->keycopy = string_copy;
 	else
 		hashp->keycopy = memcpy;
 
diff --git a/src/include/fmgr.h b/src/include/fmgr.h
index d349510b7c..339b021cbb 100644
--- a/src/include/fmgr.h
+++ b/src/include/fmgr.h
@@ -18,6 +18,8 @@
 #ifndef FMGR_H
 #define FMGR_H
 
+typedef void (*GenericFuncPtr) (void);
+
 /* We don't want to include primnodes.h here, so make some stub references */
 typedef struct Node *fmNodePtr;
 typedef struct Aggref *fmAggrefPtr;
@@ -716,9 +718,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 GenericFuncPtr load_external_function(const char *filename, const char *funcname,
+											 bool signalNotFound, void **filehandle);
+extern GenericFuncPtr 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..f301073c7b 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) (GenericFuncPtr) PLy_debug, METH_VARARGS | METH_KEYWORDS, NULL},
+	{"log", (PyCFunction) (GenericFuncPtr) PLy_log, METH_VARARGS | METH_KEYWORDS, NULL},
+	{"info", (PyCFunction) (GenericFuncPtr) PLy_info, METH_VARARGS | METH_KEYWORDS, NULL},
+	{"notice", (PyCFunction) (GenericFuncPtr) PLy_notice, METH_VARARGS | METH_KEYWORDS, NULL},
+	{"warning", (PyCFunction) (GenericFuncPtr) PLy_warning, METH_VARARGS | METH_KEYWORDS, NULL},
+	{"error", (PyCFunction) (GenericFuncPtr) PLy_error, METH_VARARGS | METH_KEYWORDS, NULL},
+	{"fatal", (PyCFunction) (GenericFuncPtr) PLy_fatal, METH_VARARGS | METH_KEYWORDS, NULL},
 
 	/*
 	 * create a stored plan
-- 
2.27.0