Re: warnings for invalid function casts

2020-07-14 Thread Peter Eisentraut

On 2020-07-07 18:08, Tom Lane wrote:

Peter Eisentraut  writes:

On 2020-07-04 16:16, Tom Lane wrote:

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.


I don't have a compiler handy that emits these warnings, but this
passes an eyeball check.


committed

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




Re: warnings for invalid function casts

2020-07-07 Thread Tom Lane
Peter Eisentraut  writes:
> On 2020-07-04 16:16, Tom Lane wrote:
>> 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.

I don't have a compiler handy that emits these warnings, but this
passes an eyeball check.

>>> 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. ;-)

I liked the first proposal better.  Not gonna fight about it though.

regards, tom lane




Re: warnings for invalid function casts

2020-07-07 Thread Peter Eisentraut

On 2020-07-04 16:16, Tom Lane wrote:

Peter Eisentraut  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 
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 

Re: warnings for invalid function casts

2020-07-04 Thread Tom Lane
Peter Eisentraut  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.

> But if we prefer a typedef then I'd propose 
> GenericFuncPtr like in the initial patch.

That name is OK by me.

regards, tom lane




Re: warnings for invalid function casts

2020-07-04 Thread Peter Eisentraut

On 2020-07-03 16:40, Tom Lane wrote:

Given that gcc explicitly documents "void (*) (void)" as being what
to use, they're going to have a hard time changing their minds about
that ... and gcc is dominant enough in this space that I suppose
other compilers would have to be compatible with it.  So even though
it's theoretically bogus, I suppose we might as well go along with
it.  The typedef will allow a centralized fix if we ever find a
better answer.


Do people prefer a typedef or just writing it out, like it's done in the 
Python code?


Attached is a provisional patch that has it written out.

I'm minimally in favor of that, since the Python code would be 
consistent with the Python core code, and the one other use is quite 
special and it might not be worth introducing a globally visible 
workaround for it.  But if we prefer a typedef then I'd propose 
GenericFuncPtr like in the initial patch.


--
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From 220bab24e5a58f1b5d66336427c0877329a5f88a Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Sat, 4 Jul 2020 13:15:12 +0200
Subject: [PATCH v2] 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/hash/dynahash.c |  9 ++-
 src/include/fmgr.h|  6 +-
 src/pl/plpython/plpy_plpymodule.c | 14 ++---
 6 files changed, 118 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 

Re: warnings for invalid function casts

2020-07-03 Thread Tom Lane
Peter Eisentraut  writes:
> On 2020-06-30 21:38, Tom Lane wrote:
>> In any case, I think the issue here is what is the escape hatch for saying
>> that "I know this cast is okay, don't warn about it, thanks".  Treating
>> "void (*) (void)" as special for that purpose is nothing more nor less
>> than a kluge, so another compiler might do it differently.  Given the
>> POSIX restriction, I think we could reasonably use "void *" instead.

> I think gcc had to pick some escape hatch that is valid also outside of 
> POSIX, so they just had to pick something.  If we're disregarding 
> support for these Harvard architecture type things, then we might as 
> well use void * for easier notation.

As long as it's behind a typedef, the code will look the same in any
case ;-).

> Btw., one of the hunks in my patch was in PL/Python.  I have found an 
> equivalent change in the core Python code, which does make use of void 
> (*) (void): https://github.com/python/cpython/commit/62be74290aca

Given that gcc explicitly documents "void (*) (void)" as being what
to use, they're going to have a hard time changing their minds about
that ... and gcc is dominant enough in this space that I suppose
other compilers would have to be compatible with it.  So even though
it's theoretically bogus, I suppose we might as well go along with
it.  The typedef will allow a centralized fix if we ever find a
better answer.

regards, tom lane




Re: warnings for invalid function casts

2020-07-03 Thread Peter Eisentraut

On 2020-06-30 21:38, Tom Lane wrote:

In any case, I think the issue here is what is the escape hatch for saying
that "I know this cast is okay, don't warn about it, thanks".  Treating
"void (*) (void)" as special for that purpose is nothing more nor less
than a kluge, so another compiler might do it differently.  Given the
POSIX restriction, I think we could reasonably use "void *" instead.


I think gcc had to pick some escape hatch that is valid also outside of 
POSIX, so they just had to pick something.  If we're disregarding 
support for these Harvard architecture type things, then we might as 
well use void * for easier notation.


Btw., one of the hunks in my patch was in PL/Python.  I have found an 
equivalent change in the core Python code, which does make use of void 
(*) (void): https://github.com/python/cpython/commit/62be74290aca


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




Re: warnings for invalid function casts

2020-06-30 Thread Tom Lane
Andres Freund  writes:
> On 2020-06-30 10:15:05 -0400, Tom Lane wrote:
>> I feel like what you propose to do here is just shifting the problem
>> around: we're still casting from a function pointer that describes one
>> concrete call ABI to a function pointer that describes some other concrete
>> call ABI.  That is, "void (*ptr) (void)" is *not* disclaiming knowledge
>> of the function's signature, in the way that "void *ptr" disclaims
>> knowledge of what a data pointer points to.  So if current gcc fails to
>> warn about that, that's just a random and indeed obviously wrong decision
>> that they might change someday.

> ISTM that it's unlikely that they'd warn about casting from one
> signature to another?

Uh, what?  Isn't that *exactly* what this warning class does?
If it doesn't do that, what good is it?  I mean, I can definitely
see the point of warning when you cast a function pointer to some
other not-ABI-compatible function pointer type, because that might
be a mistake, just like assigning "int *" to "double *" might be.

gcc 8's manual says

'-Wcast-function-type'
 Warn when a function pointer is cast to an incompatible function
 pointer.  In a cast involving function types with a variable
 argument list only the types of initial arguments that are provided
 are considered.  Any parameter of pointer-type matches any other
 pointer-type.  Any benign differences in integral types are
 ignored, like 'int' vs.  'long' on ILP32 targets.  Likewise type
 qualifiers are ignored.  The function type 'void (*) (void)' is
 special and matches everything, which can be used to suppress this
 warning.  In a cast involving pointer to member types this warning
 warns whenever the type cast is changing the pointer to member
 type.  This warning is enabled by '-Wextra'.

so it seems like they've already mostly crippled the type-safety of the
warning with the provision about "all pointer types are interchangeable"
:-(.  But they certainly are warning about *some* cases of casting one
signature to another.

In any case, I think the issue here is what is the escape hatch for saying
that "I know this cast is okay, don't warn about it, thanks".  Treating
"void (*) (void)" as special for that purpose is nothing more nor less
than a kluge, so another compiler might do it differently.  Given the
POSIX restriction, I think we could reasonably use "void *" instead.

regards, tom lane




Re: warnings for invalid function casts

2020-06-30 Thread Andres Freund
Hi,

On 2020-06-30 10:15:05 -0400, Tom Lane wrote:
> Peter Eisentraut  writes:
> > 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.
> 
> I feel like what you propose to do here is just shifting the problem
> around: we're still casting from a function pointer that describes one
> concrete call ABI to a function pointer that describes some other concrete
> call ABI.  That is, "void (*ptr) (void)" is *not* disclaiming knowledge
> of the function's signature, in the way that "void *ptr" disclaims
> knowledge of what a data pointer points to.  So if current gcc fails to
> warn about that, that's just a random and indeed obviously wrong decision
> that they might change someday.

ISTM that it's unlikely that they'd warn about casting from one
signature to another? That'd basically mean that you're not allowed to
cast function pointers at all anymore? There's a legitimate reason to
distinguish between pointers to functions and pointers to data - but
what'd be the point in forbidding all casts between different function
pointer types?


> > 2. There is a bit of cheating in dynahash.c.
> 
> It's slightly annoying that this fix introduces an extra layer of
> function-call indirection.  Maybe that's not worth worrying about,
> but I'm tempted to suggest that we could fix it on the same principle
> with

Hm. At first I was going to say that every compiler worth its salt
should be able to optimize the indirection, but that's probably not
generally true, due to returning dest "manually". If the wrapper instead
just added explicit cast to the return type it'd presumably be ok.

Greetings,

Andres Freund




Re: warnings for invalid function casts

2020-06-30 Thread Andres Freund
Hi,

On 2020-06-30 08:47:56 +0200, Peter Eisentraut wrote:
> 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.

Well, it's part of -Wextra. Which I think a fair number of people just
always enable...


> 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.

To a generic *function pointer type*, right?


> 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.

I agree that it's a low risk,


> Is there anything we want to pursue further here?

You mean whether we want to do further changes in the vein of yours, or
whether we want to apply your patch?

Greetings,

Andres Freund




Re: warnings for invalid function casts

2020-06-30 Thread Tom Lane
Peter Eisentraut  writes:
> 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.

I feel like what you propose to do here is just shifting the problem
around: we're still casting from a function pointer that describes one
concrete call ABI to a function pointer that describes some other concrete
call ABI.  That is, "void (*ptr) (void)" is *not* disclaiming knowledge
of the function's signature, in the way that "void *ptr" disclaims
knowledge of what a data pointer points to.  So if current gcc fails to
warn about that, that's just a random and indeed obviously wrong decision
that they might change someday.

Re-reading the original discussion, it seems like what we have to do
if we want to suppress these warnings is to fully buy into POSIX's
assertion that casting between data and function pointers is OK:

Note that conversion from a void * pointer to a function pointer as in:
fptr = (int (*)(int)) dlsym(handle, "my_function");
is not defined by the ISO C standard. This standard requires this
conversion to work correctly on conforming implementations.

I suggest therefore that a logically cleaner solution is to keep the
result type of load_external_function et al as "void *", and have
callers cast that to the required specific function-pointer type,
thus avoiding ever casting between two function-pointer types.
(We could keep most of your patch as-is, but typedef GenericFunctionPtr
as "void *" not a function pointer, with some suitable commentary.)

> 2. There is a bit of cheating in dynahash.c.

It's slightly annoying that this fix introduces an extra layer of
function-call indirection.  Maybe that's not worth worrying about,
but I'm tempted to suggest that we could fix it on the same principle
with

hashp->keycopy = (HashCopyFunc) (void *) strlcpy;

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

Again, it seems pretty random to me that this suppresses any warnings,
but it'd be less so if the intermediate cast were to "void *".

regards, tom lane




warnings for invalid function casts

2020-06-30 Thread Peter Eisentraut
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 
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"