Tree-walker callbacks vs -Wdeprecated-non-prototype

Thomas Munro <thomas.munro@gmail.com> 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

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

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

On Mon, Sep 19, 2022 at 8:57 AM Tom Lane <tgl@sss.pgh.pa.us> 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.

Thomas Munro <thomas.munro@gmail.com> writes:

On Mon, Sep 19, 2022 at 8:57 AM Tom Lane <tgl@sss.pgh.pa.us> 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

On Mon, Sep 19, 2022 at 3:39 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Thomas Munro <thomas.munro@gmail.com> writes:

On Mon, Sep 19, 2022 at 8:57 AM Tom Lane <tgl@sss.pgh.pa.us> 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.

Thomas Munro <thomas.munro@gmail.com> writes:

On Mon, Sep 19, 2022 at 3:39 PM Tom Lane <tgl@sss.pgh.pa.us> 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

On Mon, Sep 19, 2022 at 4:53 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Thomas Munro <thomas.munro@gmail.com> writes:

On Mon, Sep 19, 2022 at 3:39 PM Tom Lane <tgl@sss.pgh.pa.us> 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.

On Mon, Sep 19, 2022 at 10:16 AM Thomas Munro <thomas.munro@gmail.com> wrote:

On Mon, Sep 19, 2022 at 8:57 AM Tom Lane <tgl@sss.pgh.pa.us> 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.

Thomas Munro <thomas.munro@gmail.com> writes:

On Mon, Sep 19, 2022 at 10:16 AM Thomas Munro <thomas.munro@gmail.com> 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

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

On Sun, Sep 18, 2022 at 4:58 PM Tom Lane <tgl@sss.pgh.pa.us> 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

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

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?

Thomas Munro <thomas.munro@gmail.com> 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

On Mon, Dec 12, 2022 at 4:07 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Thomas Munro <thomas.munro@gmail.com> 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.

On Mon, Dec 12, 2022 at 4:43 PM Thomas Munro <thomas.munro@gmail.com> wrote:

On Mon, Dec 12, 2022 at 4:07 PM Tom Lane <tgl@sss.pgh.pa.us> 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.

Hi,

On 2022-12-13 14:18:48 +1300, Thomas Munro wrote:

On Mon, Dec 12, 2022 at 4:43 PM Thomas Munro <thomas.munro@gmail.com> wrote:

On Mon, Dec 12, 2022 at 4:07 PM Tom Lane <tgl@sss.pgh.pa.us> 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.

Presumably due to conflict resolution, this commit added an empty meson.build
to REL_13_STABLE. I'll remove that...

Greetings,

Andres Freund