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
From 7212f3e13107bf262dd0e0c4baeb2f8170ca6073 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pe...@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
