Page MenuHomeFreeBSD

loader: Avoid -Wpointer-to-int cast warnings for Arm and RISC-V
ClosedPublic

Authored by jrtc27 on Jul 18 2020, 4:42 PM.

Details

Summary

On RISC-V, Clang warns with:

cast to smaller integer type 'unsigned int' from 'void (*)(void *)'

Instead, use %p as the standard format specifier for printing pointers.
Whilst Arm's pointer size is the same as unsigned, it's still cleaner to
use the right thing there too.

Diff Detail

Repository
rS 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

This revision is now accepted and ready to land.Jul 18 2020, 4:45 PM
stand/efi/loader/arch/arm/exec.c
80 ↗(On Diff #74626)

If it's a pointer, do you need this cast?

stand/efi/loader/arch/riscv/exec.c
66 ↗(On Diff #74626)

ditto

stand/efi/loader/arch/arm/exec.c
80 ↗(On Diff #74626)

-pedantic still complains, because it's a function pointer not a data pointer.

stand/efi/loader/arch/arm/exec.c
80 ↗(On Diff #74626)

Actually it seems -pedantic complains for anything that's not void * or similar; even char * trips up -Wformat-pedantic. For function vs data pointers that makes sense (although POSIX's dlsym forces the two to be equivalent, ISO C is much more lax), though for other data pointers it's a bit questionable whether that warning adds value. Then GCC wants to complain about (void *)entry being forbidden in ISO C, which isn't _quite_ true, so we can't win with -pedantic. I personally feel the cast is the right thing, but given it's like a game of whack-a-mole I don't feel strongly and so am happy to drop it if preferred.

stand/efi/loader/arch/arm/exec.c
80 ↗(On Diff #74626)

I'm surprised there aren't hundreds of warnings for that then. We use %p to be 'any kind of pointer' all over the place. While a function point may be a little special, in the tree we rarely treat it as such because we assume sizeof(void *) == sizeof (void (*)())...

But the real question: does -pedantic or -Wformat-pedantic catch anything that's actually a bug?

stand/efi/loader/arch/arm/exec.c
80 ↗(On Diff #74626)

Probably not (at least for de-facto POSIX C) otherwise warnings for those bugs would be worth enabling for -Wall and/or -Wextra. I don't actually compile FreeBSD with -pedantic (I'm not _that_ crazy), I just passed it for testing whether (void *) was necessary.

Drop explicit (void *) cast. Clang's -pedantic wants it, but GCC's -pedantic complains about the cast itself as function pointer to object pointer casts are an optional extension in ISO C. Since we can't please everyone, go with the least-cluttered option and just drop it. Building FreeBSD with -pedantic is never going to happen anyway.

This revision now requires review to proceed.Jul 18 2020, 6:40 PM
This revision is now accepted and ready to land.Jul 19 2020, 12:54 AM