Paths

Table of Contentst

-
stand/efi/loader/arch/
-
efi/
-
loader/
-
arch/
-
arm/
1/1
exec.c
-
riscv/
1/1
exec.c

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

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

Details

Reviewers

brooks
emaste
imp
kevans
mhorne

Commits

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

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

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 32408
Build 29887: arc lint + arc unit

Event Timeline

jrtc27 created this revision.Jul 18 2020, 4:42 PM

Herald added subscribers: bcran, andrew. · View Herald TranscriptJul 18 2020, 4:42 PM

jrtc27 requested review of this revision.Jul 18 2020, 4:42 PM

Harbormaster completed remote builds in B32404: Diff 74626.Jul 18 2020, 4:42 PM

emaste accepted this revision.Jul 18 2020, 4:45 PM

This revision is now accepted and ready to land.Jul 18 2020, 4:45 PM

imp added inline comments.Jul 18 2020, 5:52 PM

stand/efi/loader/arch/arm/exec.c
80	If it's a pointer, do you need this cast?
stand/efi/loader/arch/riscv/exec.c
66	ditto

jrtc27 added inline comments.Jul 18 2020, 5:59 PM

stand/efi/loader/arch/arm/exec.c
80	`-pedantic` still complains, because it's a function pointer not a data pointer.

jrtc27 added inline comments.Jul 18 2020, 6:12 PM

stand/efi/loader/arch/arm/exec.c
80	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.

imp added inline comments.Jul 18 2020, 6:33 PM

stand/efi/loader/arch/arm/exec.c
80	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?

jrtc27 added inline comments.Jul 18 2020, 6:36 PM

stand/efi/loader/arch/arm/exec.c
80	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

Harbormaster completed remote builds in B32408: Diff 74631.Jul 18 2020, 6:40 PM

jrtc27 marked 3 inline comments as done.Jul 18 2020, 6:40 PM

emaste accepted this revision.Jul 19 2020, 12:54 AM

This revision is now accepted and ready to land.Jul 19 2020, 12:54 AM

brooks accepted this revision.Jul 20 2020, 8:30 PM

Closed by commit rS363572: loader: Avoid -Wpointer-to-int cast warnings for Arm and RISC-V (authored by jrtc27). · Explain WhyJul 26 2020, 6:17 PM

This revision was automatically updated to reflect the committed changes.

jrtc27 added a commit: rS363572: loader: Avoid -Wpointer-to-int cast warnings for Arm and RISC-V.

Revision Contents
Changeset List

Path

Size

stand/

efi/

loader/

arch/

arm/

exec.c

2 lines

riscv/

exec.c

2 lines

Diff 74631

View Options

stand/efi/loader/arch/arm/exec.c

Show All 25 Lines
* SUCH DAMAGE.		* SUCH DAMAGE.
*		*
* $FreeBSD$		* $FreeBSD$
*/		*/

#ifndef _VMMAPI_H_		#ifndef _VMMAPI_H_
#define _VMMAPI_H_		#define _VMMAPI_H_

		#include <sys/cdefs.h>
#include <sys/param.h>		#include <sys/param.h>
#include <sys/cpuset.h>		#include <sys/cpuset.h>
#include <machine/vmm.h>		#include <machine/vmm.h>
		markjUnsubmitted Done Inline Actions Conventionally, cdefs.h is included first (see style(9)). So: #include <sys/cdefs.h> #include <sys/param.h> #include ... markj: Conventionally, cdefs.h is included first (see style(9)). So: ``` #include <sys/cdefs.h>…
#include <machine/vmm_dev.h>		#include <machine/vmm_dev.h>

#include <stdbool.h>		#include <stdbool.h>

/*		/*
* API version for out-of-tree consumers like grub-bhyve for making compile		* API version for out-of-tree consumers like grub-bhyve for making compile
* time decisions.		* time decisions.
*/		*/
Show All 27 Lines
*/		*/
enum {		enum {
VM_SYSMEM,		VM_SYSMEM,
VM_BOOTROM,		VM_BOOTROM,
VM_FRAMEBUFFER,		VM_FRAMEBUFFER,
VM_PCIROM,		VM_PCIROM,
};		};

		__BEGIN_DECLS
/*		/*
* Get the length and name of the memory segment identified by 'segid'.		* Get the length and name of the memory segment identified by 'segid'.
* Note that system memory segments are identified with a nul name.		* Note that system memory segments are identified with a nul name.
*		*
* Returns 0 on success and non-zero otherwise.		* Returns 0 on success and non-zero otherwise.
*/		*/
int vm_get_memseg(struct vmctx ctx, int ident, size_t lenp, char *name,		int vm_get_memseg(struct vmctx ctx, int ident, size_t lenp, char *name,
size_t namesiz);		size_t namesiz);
▲ Show 20 Lines • Show All 171 Lines • ▼ Show 20 Lines	int vm_setup_freebsd_registers_i386(struct vmctx *vmctx, int vcpu,
uint32_t esp);		uint32_t esp);
void vm_setup_freebsd_gdt(uint64_t *gdtr);		void vm_setup_freebsd_gdt(uint64_t *gdtr);

/*		/*
* Save and restore		* Save and restore
*/		*/
int vm_snapshot_req(struct vm_snapshot_meta *meta);		int vm_snapshot_req(struct vm_snapshot_meta *meta);
int vm_restore_time(struct vmctx *ctx);		int vm_restore_time(struct vmctx *ctx);
		__END_DECLS

#endif /* _VMMAPI_H_ */		#endif /* _VMMAPI_H_ */