Page MenuHomeFreeBSD

kobj: avoid gcc -Wcast-function-type
ClosedPublic

Authored by rlibby on Feb 18 2021, 8:18 AM.

Details

Summary

The actual type of kobjop_t is arbitrary, it is only used as a generic
function pointer type. Declare it as void (*)(void) in order to avoid
gcc's -Wcast-function-type, which is included in -Wextra.


There's nothing wrong the current code, but by declaring kobjop_t as a
(void (*)(void)), we can avoid gcc warning -Wcast-function-type, which
has an exception for that type.

To be clear, this doesn't fix a problem for code in-tree with current
warnings. I recently ran into an out-of-tree module which uses this
warning when compiled with gcc via -Wextra.

Other solutions could be either to change the cast in the assignment in
KOBJMETHOD to first cast through that type, or just to leave it to
clients to disable (or not enable) the warning (i.e. take no change
here).

https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html

Diff Detail

Repository
R10 FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

rlibby created this revision.
This revision is now accepted and ready to land.Feb 18 2021, 9:13 AM

The only thing that is kind of useful about the current declaration is that it documents that all kobj methods are expected to return int (and specifically kobj returns a default value if no method is found).

In D28769#644259, @jhb wrote:

The only thing that is kind of useful about the current declaration is that it documents that all kobj methods are expected to return int (and specifically kobj returns a default value if no method is found).

Hmm, I may be misunderstanding something here, but I think I see methods where that isn't true, that return void or pointers? For example, sys/kern/bus_if.m, child_deleted and get_dma_tag, then sys/dev/pci/pci.c, pci_child_deleted and pci_get_dma_tag (void and bus_dma_tag_t).

We do still get a type warning via the trick in KOBJMETHOD, but of course that's not documentation.

In D28769#644259, @jhb wrote:

The only thing that is kind of useful about the current declaration is that it documents that all kobj methods are expected to return int (and specifically kobj returns a default value if no method is found).

Hmm, I may be misunderstanding something here, but I think I see methods where that isn't true, that return void or pointers? For example, sys/kern/bus_if.m, child_deleted and get_dma_tag, then sys/dev/pci/pci.c, pci_child_deleted and pci_get_dma_tag (void and bus_dma_tag_t).

Yes, those functions do abuse the interface a bit, and it might be ok for the non-void case as long as the function has an explicit default method, but for methods without a default, kobj_error_method() is what gets used. I guess though that the generated BUS_CHILD_DELETED() does actually return void though, so perhaps it isn't really inherent to kobj aside from the default default.

In D28769#645024, @jhb wrote:
In D28769#644259, @jhb wrote:

The only thing that is kind of useful about the current declaration is that it documents that all kobj methods are expected to return int (and specifically kobj returns a default value if no method is found).

Hmm, I may be misunderstanding something here, but I think I see methods where that isn't true, that return void or pointers? For example, sys/kern/bus_if.m, child_deleted and get_dma_tag, then sys/dev/pci/pci.c, pci_child_deleted and pci_get_dma_tag (void and bus_dma_tag_t).

Yes, those functions do abuse the interface a bit, and it might be ok for the non-void case as long as the function has an explicit default method, but for methods without a default, kobj_error_method() is what gets used. I guess though that the generated BUS_CHILD_DELETED() does actually return void though, so perhaps it isn't really inherent to kobj aside from the default default.

Okay, so what I understand from that is: if a method has a return type other than int, then it ought to supply a default.

Terrible hack searching for this:

find sys -name '*.m' -exec awk '/^METHOD/ {arm = ($2 != "int" && $2 != "u_int"); spec = $0} /^};/ {if (arm) print FILENAME, spec, $0; arm = 0}' {} \;

I see 190 matches.

Should we be trying to enforce this? Should we maybe provide a kobj_error_method_void as a default for void (that would take care of 116 of 190), or maybe a NODEFAULT that panics?

Re addressing the warning, a few options:

  • Drop this, punt to clients.
  • Go ahead with current patch.
  • Current patch plus a comment and/or other documentation.
  • Cast in KOBJMETHOD instead.
  • Something else?

I'm not sure how to enforce this better. What would be nice is if the awk script that generates the foo_if.c file would emit an error if the return type is not void and not int and a default method isn't supplied. I think the current patch is ok and would be orthogonal from possible fixing the generation script. (I didn't know the breakage was that wide spread.)

This revision was automatically updated to reflect the committed changes.