Re: Tree-walker callbacks vs -Wdeprecated-non-prototype
As visible on seawasp and locally (16/main branch nightly packages), they decided to start warning about these casts with a new strict variant of the warning. Their discussion: https://reviews.llvm.org/D134831 There are also a few other cases unrelated to this thread's original problem, for example casts involving pg_funcptr_t, HashCompareFunc. I guess our options would be to turn that warning off, or reconsider and try shoving the cast of "generic" arguments pointers down into the functions?
Re: Tree-walker callbacks vs -Wdeprecated-non-prototype
Thomas Munro writes: > As visible on seawasp and locally (16/main branch nightly packages), > they decided to start warning about these casts with a new strict > variant of the warning. Their discussion: > https://reviews.llvm.org/D134831 > There are also a few other cases unrelated to this thread's original > problem, for example casts involving pg_funcptr_t, HashCompareFunc. I > guess our options would be to turn that warning off, or reconsider and > try shoving the cast of "generic" arguments pointers down into the > functions? I'm for "turn the warning off". Per previous discussion, adhering strictly to that rule would make our code worse (less legible AND less safe), not better. regards, tom lane
Re: Tree-walker callbacks vs -Wdeprecated-non-prototype
On Mon, Dec 12, 2022 at 4:07 PM Tom Lane wrote:
> Thomas Munro writes:
> > As visible on seawasp and locally (16/main branch nightly packages),
> > they decided to start warning about these casts with a new strict
> > variant of the warning. Their discussion:
>
> > https://reviews.llvm.org/D134831
>
> > There are also a few other cases unrelated to this thread's original
> > problem, for example casts involving pg_funcptr_t, HashCompareFunc. I
> > guess our options would be to turn that warning off, or reconsider and
> > try shoving the cast of "generic" arguments pointers down into the
> > functions?
>
> I'm for "turn the warning off". Per previous discussion, adhering
> strictly to that rule would make our code worse (less legible AND
> less safe), not better.
Alright, this seems to do the trick here.
From b01690ca859b5f27ece386577b7b9d9d7572b8d2 Mon Sep 17 00:00:00 2001
From: Thomas Munro
Date: Mon, 12 Dec 2022 16:28:24 +1300
Subject: [PATCH] Disable clang 16's -Wcast-function-type-strict.
Clang 16 is still in development, but seawasp reveals that it has
started warning about many of our casts of function pointers. Disable
the new warning for now, since otherwise buildfarm animal seawasp fails.
May be back-patched with other Clang/LLVM 16 changes around release
time.
Discussion: https://postgr.es/m/CA%2BhUKGJvX%2BL3aMN84ksT-cGy08VHErRNip3nV-WmTx7f6Pqhyw%40mail.gmail.com
---
configure| 44
configure.ac | 6 ++
meson.build | 3 +++
3 files changed, 53 insertions(+)
diff --git a/configure b/configure
index 62f382c1d1..5025448a05 100755
--- a/configure
+++ b/configure
@@ -6611,6 +6611,50 @@ fi
if test -n "$NOT_THE_CFLAGS"; then
CFLAGS="$CFLAGS -Wno-stringop-truncation"
fi
+ # Suppress clang 16's strict warnings about function casts
+ NOT_THE_CFLAGS=""
+ { $as_echo "$as_me:${as_lineno-$LINENO}: checking whether ${CC} supports -Wcast-function-type-strict, for NOT_THE_CFLAGS" >&5
+$as_echo_n "checking whether ${CC} supports -Wcast-function-type-strict, for NOT_THE_CFLAGS... " >&6; }
+if ${pgac_cv_prog_CC_cflags__Wcast_function_type_strict+:} false; then :
+ $as_echo_n "(cached) " >&6
+else
+ pgac_save_CFLAGS=$CFLAGS
+pgac_save_CC=$CC
+CC=${CC}
+CFLAGS="${NOT_THE_CFLAGS} -Wcast-function-type-strict"
+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_strict=yes
+else
+ pgac_cv_prog_CC_cflags__Wcast_function_type_strict=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_strict" >&5
+$as_echo "$pgac_cv_prog_CC_cflags__Wcast_function_type_strict" >&6; }
+if test x"$pgac_cv_prog_CC_cflags__Wcast_function_type_strict" = x"yes"; then
+ NOT_THE_CFLAGS="${NOT_THE_CFLAGS} -Wcast-function-type-strict"
+fi
+
+
+ if test -n "$NOT_THE_CFLAGS"; then
+CFLAGS="$CFLAGS -Wno-cast-function-type-strict"
+ fi
elif test "$ICC" = yes; then
# Intel's compiler has a bug/misoptimization in checking for
# division by NAN (NaN == 0), -mp1 fixes it, so add it to the CFLAGS.
diff --git a/configure.ac b/configure.ac
index cfb10f59ce..4cd328fc83 100644
--- a/configure.ac
+++ b/configure.ac
@@ -573,6 +573,12 @@ if test "$GCC" = yes -a "$ICC" = no; then
if test -n "$NOT_THE_CFLAGS"; then
CFLAGS="$CFLAGS -Wno-stringop-truncation"
fi
+ # Suppress clang 16's strict warnings about function casts
+ NOT_THE_CFLAGS=""
+ PGAC_PROG_CC_VAR_OPT(NOT_THE_CFLAGS, [-Wcast-function-type-strict])
+ if test -n "$NOT_THE_CFLAGS"; then
+CFLAGS="$CFLAGS -Wno-cast-function-type-strict"
+ fi
elif test "$ICC" = yes; then
# Intel's compiler has a bug/misoptimization in checking for
# division by NAN (NaN == 0), -mp1 fixes it, so add it to the CFLAGS.
diff --git a/meson.build b/meson.build
index a62d8171cc..9df8685dfd 100644
--- a/meson.build
+++ b/meson.build
@@ -1773,6 +1773,9 @@ negative_warning_flags = [
'format-truncation',
'stringop-truncation',
+ # Suppress clang 16's strict warnings about function casts
+ 'cast-function-type-strict',
+
# To make warning_level=2 / -Wextra work, we'd need at least the following
# 'clobbered',
# 'missing-field-initializers',
--
2.35.1
Re: Tree-walker callbacks vs -Wdeprecated-non-prototype
On Mon, Dec 12, 2022 at 4:43 PM Thomas Munro wrote: > On Mon, Dec 12, 2022 at 4:07 PM Tom Lane wrote: > > I'm for "turn the warning off". Per previous discussion, adhering > > strictly to that rule would make our code worse (less legible AND > > less safe), not better. > > Alright, this seems to do the trick here. That did fix that problem. But... seawasp also just recompiled its compiler and picked up new opaque pointer API changes. So no green today. I have more work to do to fix that, which might take some time to get back to.
Re: Tree-walker callbacks vs -Wdeprecated-non-prototype
I wrote:
> Ugh. I wonder if we can get away with declaring the walker arguments
> as something like "bool (*walker) (Node *, void *)" without having
> to change all the actual walkers to be exactly that signature.
> Having to insert casts in the walkers would be a major pain-in-the-butt.
No joy on that: both gcc and clang want the walkers to be declared
as taking exactly "void *".
Attached is an incomplete POC patch that suppresses these warnings
in nodeFuncs.c itself and in costsize.c, which I selected at random
as a typical caller. I'll push forward with converting the other
call sites if this way seems good to people.
In nodeFuncs.c, we can hide the newly-required casts inside macros;
indeed, the mutators barely need any changes because they already
had MUTATE() macros that contained casts. So on that side, it feels
to me that this is actually a bit nicer than before.
For the callers, we can either do it as I did below:
static bool
-cost_qual_eval_walker(Node *node, cost_qual_eval_context *context)
+cost_qual_eval_walker(Node *node, void *ctx)
{
+ cost_qual_eval_context *context = (cost_qual_eval_context *) ctx;
+
if (node == NULL)
return false;
or perhaps like this:
static bool
-cost_qual_eval_walker(Node *node, cost_qual_eval_context *context)
+cost_qual_eval_walker(Node *node, void *context)
{
+ cost_qual_eval_context *cqctx = (cost_qual_eval_context *) context;
+
if (node == NULL)
return false;
but the latter would require changing references further down in the
function, so I felt it more invasive.
It's sad to note that this exercise in hoop-jumping actually leaves
us with net LESS type safety, because the outside callers of
cost_qual_eval_walker are no longer constrained to call it with
the appropriate kind of context struct. Thanks, C committee.
regards, tom lane
diff --git a/src/backend/nodes/nodeFuncs.c b/src/backend/nodes/nodeFuncs.c
index 3bac350bf5..1e2ae3a5a4 100644
--- a/src/backend/nodes/nodeFuncs.c
+++ b/src/backend/nodes/nodeFuncs.c
@@ -27,10 +27,10 @@
static bool expression_returns_set_walker(Node *node, void *context);
static int leftmostLoc(int loc1, int loc2);
static bool fix_opfuncids_walker(Node *node, void *context);
-static bool planstate_walk_subplans(List *plans, bool (*walker) (),
+static bool planstate_walk_subplans(List *plans, tree_walker_callback walker,
void *context);
static bool planstate_walk_members(PlanState **planstates, int nplans,
- bool (*walker) (), void *context);
+ tree_walker_callback walker, void *context);
/*
@@ -1836,7 +1836,7 @@ check_functions_in_node(Node *node, check_function_callback checker,
* that modify nodes in-place but never add/delete/replace nodes).
* A walker routine should look like this:
*
- * bool my_walker (Node *node, my_struct *context)
+ * bool my_walker (Node *node, void *context)
* {
* if (node == NULL)
* return false;
@@ -1850,7 +1850,7 @@ check_functions_in_node(Node *node, check_function_callback checker,
* ... do special actions for other node types
* }
* // for any node type not specially processed, do:
- * return expression_tree_walker(node, my_walker, (void *) context);
+ * return expression_tree_walker(node, my_walker, context);
* }
*
* The "context" argument points to a struct that holds whatever context
@@ -1910,7 +1910,7 @@ check_functions_in_node(Node *node, check_function_callback checker,
bool
expression_tree_walker(Node *node,
- bool (*walker) (),
+ tree_walker_callback walker,
void *context)
{
ListCell *temp;
@@ -1923,6 +1923,10 @@ expression_tree_walker(Node *node,
* when we expect a List we just recurse directly to self without
* bothering to call the walker.
*/
+#define WALK(n) walker((Node *) (n), context)
+
+#define LIST_WALK(l) expression_tree_walker((Node *) (l), walker, context)
+
if (node == NULL)
return false;
@@ -1946,25 +1950,21 @@ expression_tree_walker(Node *node,
/* primitive node types with no expression subnodes */
break;
case T_WithCheckOption:
- return walker(((WithCheckOption *) node)->qual, context);
+ return WALK(((WithCheckOption *) node)->qual);
case T_Aggref:
{
Aggref *expr = (Aggref *) node;
-/* recurse directly on List */
-if (expression_tree_walker((Node *) expr->aggdirectargs,
- walker, context))
+/* recurse directly on Lists */
+if (LIST_WALK(expr->aggdirectargs))
return true;
-if (expression_tree_walker((Node *) expr->args,
- walker, context))
+if (LIST_WALK(expr->args))
return true;
-if (expression_tree_walker((Node *) expr->aggorder,
- walker, context))
+if (LIST_WALK(expr->aggorder))
return true;
-if (expression_tree_walker((Node *) expr->aggdistinct,
- walker, context))
+if (LIST_WALK(expr->aggdistinct))
return true;
-
Re: Tree-walker callbacks vs -Wdeprecated-non-prototype
I wrote:
> Attached is an incomplete POC patch that suppresses these warnings
> in nodeFuncs.c itself and in costsize.c, which I selected at random
> as a typical caller. I'll push forward with converting the other
> call sites if this way seems good to people.
Here's a fleshed-out patch that gets rid of all warnings of this sort
(tested on clang version 15.0.0).
While I remain happy enough with what has to be done in nodeFuncs.c,
I'm really not happy at all with this point:
> It's sad to note that this exercise in hoop-jumping actually leaves
> us with net LESS type safety, because the outside callers of
> cost_qual_eval_walker are no longer constrained to call it with
> the appropriate kind of context struct. Thanks, C committee.
There are a lot of these walker/mutator functions and hence a whole
lot of opportunity to pass the wrong thing, not only from the outer
non-recursive call points but during internal recursions in the
walkers/mutators themselves.
I think we ought to seriously consider the alternative of changing
nodeFuncs.c about like I have here, but not touching the walkers/mutators,
and silencing the resulting complaints about function type casting by
doing the equivalent of
-return expression_tree_walker(node, cost_qual_eval_walker,
- (void *) context);
+return expression_tree_walker(node,
+ (tree_walker_callback) cost_qual_eval_walker,
+ (void *) context);
We could avoid touching all the call sites by turning
expression_tree_walker and friends into macro wrappers that incorporate
these casts. This is fairly annoying, in that it gives up the function
type safety the C committee wants to impose on us; but I really think
the data type safety that we're giving up in this version of the patch
is a worse hazard.
BTW, I was distressed to discover that someone decided they could
use ExecShutdownNode as a planstate_tree_walker() walker even though
its argument list is not even the right length. I'm a bit flabbergasted
that we seem to have gotten away with that so far, because I'd have
thought for sure that it'd break some platform's convention for which
argument gets passed where. I think we need to fix that, independently
of what we do about the larger scope of these problems. To avoid an
API break, I propose making ExecShutdownNode just be a one-liner that
calls an internal ExecShutdownNode_walker() function. (I've not done
it that way in the attached, though.)
Thoughts?
regards, tom lane
diff --git a/src/backend/catalog/dependency.c b/src/backend/catalog/dependency.c
index 39768fa22b..f127f20fd8 100644
--- a/src/backend/catalog/dependency.c
+++ b/src/backend/catalog/dependency.c
@@ -206,8 +206,7 @@ static void reportDependentObjects(const ObjectAddresses *targetObjects,
static void deleteOneObject(const ObjectAddress *object,
Relation *depRel, int32 flags);
static void doDeletion(const ObjectAddress *object, int flags);
-static bool find_expr_references_walker(Node *node,
- find_expr_references_context *context);
+static bool find_expr_references_walker(Node *node, void *ctx);
static void process_function_rte_ref(RangeTblEntry *rte, AttrNumber attnum,
find_expr_references_context *context);
static void eliminate_duplicate_dependencies(ObjectAddresses *addrs);
@@ -1738,9 +1737,10 @@ recordDependencyOnSingleRelExpr(const ObjectAddress *depender,
* the collation is being freshly introduced to the expression.
*/
static bool
-find_expr_references_walker(Node *node,
- find_expr_references_context *context)
+find_expr_references_walker(Node *node, void *ctx)
{
+ find_expr_references_context *context = ctx;
+
if (node == NULL)
return false;
if (IsA(node, Var))
@@ -2283,7 +2283,7 @@ find_expr_references_walker(Node *node,
context->rtables = lcons(query->rtable, context->rtables);
result = query_tree_walker(query,
find_expr_references_walker,
- (void *) context,
+ ctx,
QTW_IGNORE_JOINALIASES |
QTW_EXAMINE_SORTGROUP);
context->rtables = list_delete_first(context->rtables);
@@ -2352,8 +2352,7 @@ find_expr_references_walker(Node *node,
/* fall through to examine arguments */
}
- return expression_tree_walker(node, find_expr_references_walker,
- (void *) context);
+ return expression_tree_walker(node, find_expr_references_walker, ctx);
}
/*
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index 053d2ca5ae..671a41d3b6 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -63,7 +63,7 @@ static void ExplainPrintJIT(ExplainState *es, int jit_flags,
static void report_triggers(ResultRelInfo *rInfo, bool show_relname,
ExplainState *es);
static double elapsed_time(instr_time *starttime);
-static bool ExplainPreScanNode(PlanState *planstate, Bitmapset **rels_used);
+static
Re: Tree-walker callbacks vs -Wdeprecated-non-prototype
On Mon, Sep 19, 2022 at 8:57 AM Tom Lane wrote: > I think we ought to seriously consider the alternative of changing > nodeFuncs.c about like I have here, but not touching the walkers/mutators, > and silencing the resulting complaints about function type casting by > doing the equivalent of > > -return expression_tree_walker(node, cost_qual_eval_walker, > - (void *) context); > +return expression_tree_walker(node, > + (tree_walker_callback) > cost_qual_eval_walker, > + (void *) context); > > We could avoid touching all the call sites by turning > expression_tree_walker and friends into macro wrappers that incorporate > these casts. This is fairly annoying, in that it gives up the function > type safety the C committee wants to impose on us; but I really think > the data type safety that we're giving up in this version of the patch > is a worse hazard. But is it defined behaviour? https://stackoverflow.com/questions/559581/casting-a-function-pointer-to-another-type > BTW, I was distressed to discover that someone decided they could > use ExecShutdownNode as a planstate_tree_walker() walker even though > its argument list is not even the right length. I'm a bit flabbergasted > that we seem to have gotten away with that so far, because I'd have > thought for sure that it'd break some platform's convention for which > argument gets passed where. I think we need to fix that, independently > of what we do about the larger scope of these problems. To avoid an > API break, I propose making ExecShutdownNode just be a one-liner that > calls an internal ExecShutdownNode_walker() function. (I've not done > it that way in the attached, though.) Huh... wouldn't systems that pass arguments right-to-left on the stack receive NULL for node? That'd include the SysV i386 convention used on Linux, *BSD etc. But that can't be right or we'd know about it... But certainly +1 for fixing that regardless.
Re: Tree-walker callbacks vs -Wdeprecated-non-prototype
Thomas Munro writes: > On Mon, Sep 19, 2022 at 8:57 AM Tom Lane wrote: >> ... This is fairly annoying, in that it gives up the function >> type safety the C committee wants to impose on us; but I really think >> the data type safety that we're giving up in this version of the patch >> is a worse hazard. > But is it defined behaviour? > https://stackoverflow.com/questions/559581/casting-a-function-pointer-to-another-type Well, what we're talking about is substituting "void *" (which is required to be compatible with "char *") for a struct pointer type. Standards legalese aside, that could only be a problem if the platform ABI handles "char *" differently from struct pointer types. The last architecture I can remember dealing with where that might actually be a thing was the PDP-10. Everybody has learned better since then, but the C committee is apparently still intent on making the world safe for crappy machine architectures. Also, if you want to argue that "void *" is not compatible with struct pointer types, then it's not real clear to me that we aren't full of other spec violations, because we sure do a lot of casting across that (and even more with this patch as it stands). I don't have the slightest hesitation about saying that if there's still an architecture out there that's like that, we won't support it. I also note that our existing code in this area would break pretty thoroughly on such a machine, so this isn't making it worse. regards, tom lane
Re: Tree-walker callbacks vs -Wdeprecated-non-prototype
On Mon, Sep 19, 2022 at 3:39 PM Tom Lane wrote: > Thomas Munro writes: > > On Mon, Sep 19, 2022 at 8:57 AM Tom Lane wrote: > >> ... This is fairly annoying, in that it gives up the function > >> type safety the C committee wants to impose on us; but I really think > >> the data type safety that we're giving up in this version of the patch > >> is a worse hazard. > > > But is it defined behaviour? > > https://stackoverflow.com/questions/559581/casting-a-function-pointer-to-another-type > > Well, what we're talking about is substituting "void *" (which is > required to be compatible with "char *") for a struct pointer type. > Standards legalese aside, that could only be a problem if the platform > ABI handles "char *" differently from struct pointer types. The last > architecture I can remember dealing with where that might actually be > a thing was the PDP-10. Everybody has learned better since then, but > the C committee is apparently still intent on making the world safe > for crappy machine architectures. > > Also, if you want to argue that "void *" is not compatible with struct > pointer types, then it's not real clear to me that we aren't full of > other spec violations, because we sure do a lot of casting across that > (and even more with this patch as it stands). > > I don't have the slightest hesitation about saying that if there's > still an architecture out there that's like that, we won't support it. > I also note that our existing code in this area would break pretty > thoroughly on such a machine, so this isn't making it worse. Yeah, I don't expect it to be a practical problem on any real system (that is, I don't expect any real calling convention to transfer a struct T * argument in a different place than void *). I just wanted to mention that it's a new liberty. It's one thing to cast struct T * to void * and back before dereferencing, and another to cast a pointer to a function that takes struct T * to a pointer to a function that takes void * and call it. I considered proposing that myself when first reporting this problem, but fear of language lawyers put me off.
Re: Tree-walker callbacks vs -Wdeprecated-non-prototype
Thomas Munro writes: > On Mon, Sep 19, 2022 at 3:39 PM Tom Lane wrote: >> I also note that our existing code in this area would break pretty >> thoroughly on such a machine, so this isn't making it worse. > Yeah, I don't expect it to be a practical problem on any real system > (that is, I don't expect any real calling convention to transfer a > struct T * argument in a different place than void *). I just wanted > to mention that it's a new liberty. No, it's not, because the existing coding here is already assuming that. The walker callbacks are generally declared as taking a "struct *" second parameter, but expression_tree_walker et al think they are passing a "void *" to them. Even if a platform ABI had some weird special rule about how to call functions that you don't know the argument list for, it wouldn't fix this because the walkers sure do know what their arguments are. The only reason this code works today is that in practice, "void *" *is* ABI-compatible with "struct *". I'm not excited about creating a demonstrable opportunity for bugs in order to make the code hypothetically more compatible with hardware designs that are thirty years obsolete. (Hypothetical in the sense that there's little reason to believe there would be no other problems.) regards, tom lane
Re: Tree-walker callbacks vs -Wdeprecated-non-prototype
On Mon, Sep 19, 2022 at 4:53 PM Tom Lane wrote: > Thomas Munro writes: > > On Mon, Sep 19, 2022 at 3:39 PM Tom Lane wrote: > >> I also note that our existing code in this area would break pretty > >> thoroughly on such a machine, so this isn't making it worse. > > > Yeah, I don't expect it to be a practical problem on any real system > > (that is, I don't expect any real calling convention to transfer a > > struct T * argument in a different place than void *). I just wanted > > to mention that it's a new liberty. > > No, it's not, because the existing coding here is already assuming that. > The walker callbacks are generally declared as taking a "struct *" > second parameter, but expression_tree_walker et al think they are > passing a "void *" to them. Even if a platform ABI had some weird > special rule about how to call functions that you don't know the > argument list for, it wouldn't fix this because the walkers sure do know > what their arguments are. The only reason this code works today is that > in practice, "void *" *is* ABI-compatible with "struct *". True. > I'm not excited about creating a demonstrable opportunity for bugs > in order to make the code hypothetically more compatible with > hardware designs that are thirty years obsolete. (Hypothetical > in the sense that there's little reason to believe there would > be no other problems.) Fair enough.
Re: Tree-walker callbacks vs -Wdeprecated-non-prototype
On Mon, Sep 19, 2022 at 10:16 AM Thomas Munro wrote: > On Mon, Sep 19, 2022 at 8:57 AM Tom Lane wrote: > > BTW, I was distressed to discover that someone decided they could > > use ExecShutdownNode as a planstate_tree_walker() walker even though > > its argument list is not even the right length. I'm a bit flabbergasted > > that we seem to have gotten away with that so far, because I'd have > > thought for sure that it'd break some platform's convention for which > > argument gets passed where. I think we need to fix that, independently > > of what we do about the larger scope of these problems. To avoid an > > API break, I propose making ExecShutdownNode just be a one-liner that > > calls an internal ExecShutdownNode_walker() function. (I've not done > > it that way in the attached, though.) > > Huh... wouldn't systems that pass arguments right-to-left on the stack > receive NULL for node? That'd include the SysV i386 convention used > on Linux, *BSD etc. But that can't be right or we'd know about it... I take that back after looking up some long forgotten details; it happily ignores extra arguments.
Re: Tree-walker callbacks vs -Wdeprecated-non-prototype
Thomas Munro writes: > On Mon, Sep 19, 2022 at 10:16 AM Thomas Munro wrote: >> Huh... wouldn't systems that pass arguments right-to-left on the stack >> receive NULL for node? That'd include the SysV i386 convention used >> on Linux, *BSD etc. But that can't be right or we'd know about it... > I take that back after looking up some long forgotten details; it > happily ignores extra arguments. Yeah; the fact that no one has complained in several years seems to indicate that there's not a problem on supported platforms. Still, unlike the quibbles over whether char and struct pointers are the same, it seems clear that this is the sort of inconsistency that C2x wants to forbid, presumably in the name of making the world safe for more-efficient function calling code. So I think we'd better go fix ExecShutdownNode before somebody breaks it. Whichever way we jump on the tree-walker API changes, those won't be back-patchable. I think the best we can do for the back branches is add a configure test to use -Wno-deprecated-non-prototype if available. But the ExecShutdownNode change could be back-patched, and I'm leaning to doing so even though that breakage is just hypothetical today. regards, tom lane
Re: Tree-walker callbacks vs -Wdeprecated-non-prototype
Here's a second-generation patch that fixes the warnings by inserting
casts into a layer of macro wrappers. I had supposed that this would
cause us to lose all detection of wrongly-chosen walker functions,
so I was very pleased to see this when applying it to yesterday's HEAD:
execProcnode.c:792:2: warning: cast from 'bool (*)(PlanState *)' (aka 'bool
(*)(struct PlanState *)') to 'planstate_tree_walker_callback' (aka 'bool
(*)(struct PlanState *, void *)') converts to incompatible function type
[-Wcast-function-type]
planstate_tree_walker(node, ExecShutdownNode, NULL);
^~~
../../../src/include/nodes/nodeFuncs.h:180:33: note: expanded from macro
'planstate_tree_walker'
planstate_tree_walker_impl(ps, (planstate_tree_walker_callback) (w), c)
^~~~
So we've successfully suppressed the pedantic -Wdeprecated-non-prototype
warnings, and we have activated the actually-useful -Wcast-function-type
warnings, which seem to do exactly what we want in this context:
'-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'.
(That verbiage is from the gcc manual; clang seems to act the same
except that -Wcast-function-type is selected by -Wall, or perhaps is
even on by default.)
So I'm pretty pleased with this formulation: no caller changes are
needed, and it does exactly what we want warning-wise.
regards, tom lane
diff --git a/src/backend/nodes/nodeFuncs.c b/src/backend/nodes/nodeFuncs.c
index 3bac350bf5..724d076674 100644
--- a/src/backend/nodes/nodeFuncs.c
+++ b/src/backend/nodes/nodeFuncs.c
@@ -27,10 +27,12 @@
static bool expression_returns_set_walker(Node *node, void *context);
static int leftmostLoc(int loc1, int loc2);
static bool fix_opfuncids_walker(Node *node, void *context);
-static bool planstate_walk_subplans(List *plans, bool (*walker) (),
+static bool planstate_walk_subplans(List *plans,
+ planstate_tree_walker_callback walker,
void *context);
static bool planstate_walk_members(PlanState **planstates, int nplans,
- bool (*walker) (), void *context);
+ planstate_tree_walker_callback walker,
+ void *context);
/*
@@ -1909,9 +1911,9 @@ check_functions_in_node(Node *node, check_function_callback checker,
*/
bool
-expression_tree_walker(Node *node,
- bool (*walker) (),
- void *context)
+expression_tree_walker_impl(Node *node,
+ tree_walker_callback walker,
+ void *context)
{
ListCell *temp;
@@ -1923,6 +1925,10 @@ expression_tree_walker(Node *node,
* when we expect a List we just recurse directly to self without
* bothering to call the walker.
*/
+#define WALK(n) walker((Node *) (n), context)
+
+#define LIST_WALK(l) expression_tree_walker_impl((Node *) (l), walker, context)
+
if (node == NULL)
return false;
@@ -1946,25 +1952,21 @@ expression_tree_walker(Node *node,
/* primitive node types with no expression subnodes */
break;
case T_WithCheckOption:
- return walker(((WithCheckOption *) node)->qual, context);
+ return WALK(((WithCheckOption *) node)->qual);
case T_Aggref:
{
Aggref *expr = (Aggref *) node;
-/* recurse directly on List */
-if (expression_tree_walker((Node *) expr->aggdirectargs,
- walker, context))
+/* recurse directly on Lists */
+if (LIST_WALK(expr->aggdirectargs))
return true;
-if (expression_tree_walker((Node *) expr->args,
- walker, context))
+if (LIST_WALK(expr->args))
return true;
-if (expression_tree_walker((Node *) expr->aggorder,
- walker, context))
+if (LIST_WALK(expr->aggorder))
return true;
-if (expression_tree_walker((Node *) expr->aggdistinct,
- walker, context))
+if (LIST_WALK(expr->aggdistinct))
return true;
-if (walker((Node *) expr->aggfilter, context))
+if (WALK(expr->aggfilter))
return true;
}
break;
@@ -1972,8 +1974,7 @@ expression_tree_walker(Node *node,
{
GroupingFunc *grouping = (GroupingFunc *) node;
-if (expression_tree_walker((No
Re: Tree-walker callbacks vs -Wdeprecated-non-prototype
On Sun, Sep 18, 2022 at 4:58 PM Tom Lane wrote: > BTW, I was distressed to discover that someone decided they could > use ExecShutdownNode as a planstate_tree_walker() walker even though > its argument list is not even the right length. I'm a bit flabbergasted > that we seem to have gotten away with that so far, because I'd have > thought for sure that it'd break some platform's convention for which > argument gets passed where. I think we need to fix that, independently > of what we do about the larger scope of these problems. To avoid an > API break, I propose making ExecShutdownNode just be a one-liner that > calls an internal ExecShutdownNode_walker() function. (I've not done > it that way in the attached, though.) I think this was brain fade on my part ... or possibly on Amit Kapila's part, but I believe it was probably me. I agree that it's impressive that it actually seemed to work that way. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Tree-walker callbacks vs -Wdeprecated-non-prototype
I wrote: > (That verbiage is from the gcc manual; clang seems to act the same > except that -Wcast-function-type is selected by -Wall, or perhaps is > even on by default.) Nah, scratch that: the reason -Wcast-function-type is on is that we explicitly enable it, and have done so since de8feb1f3 (v14). I did not happen to see this warning with gcc because the test runs I made with this patch already had c35ba141d, whereas I did my clang test on another machine that wasn't quite up to HEAD. So we should have good warning coverage for bogus walker signatures on both compilers. regards, tom lane
Re: Tree-walker callbacks vs -Wdeprecated-non-prototype
Thomas Munro writes: > As visible on seawasp (and noticed here in passing, while hacking on > the opaque pointer changes for bleeding edge LLVM), Clang 15 now warns > by default about our use of tree walkers functions with no function > prototype, because the next revision of C (C23?) will apparently be > harmonising with C++ in interpreting f() to mean f(void), not > f(anything goes). Ugh. I wonder if we can get away with declaring the walker arguments as something like "bool (*walker) (Node *, void *)" without having to change all the actual walkers to be exactly that signature. Having to insert casts in the walkers would be a major pain-in-the-butt. regards, tom lane
