Date: Sun, 6 Apr 2014 18:57:46 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Warner Losh <imp@freebsd.org> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r264177 - in head/sys/dev/hyperv: netvsc storvsc Message-ID: <20140406183716.N5281@besplex.bde.org> In-Reply-To: <201404052242.s35Mg1ES027714@svn.freebsd.org> References: <201404052242.s35Mg1ES027714@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sat, 5 Apr 2014, Warner Losh wrote:
> Log:
> Make some unwise casts. On i386 these casts wind up being safe. Rather
> than disturb the API, go with these casts to shut gcc up.
These casts seem to be correct, while the old ones were just wrong. Now
the old ones are still there, but seem to be just style bugs -- they
have no effect except to bloat the code and give some collateral
style bugs (long lines).
> Modified: head/sys/dev/hyperv/netvsc/hv_net_vsc.c
> ==============================================================================
> --- head/sys/dev/hyperv/netvsc/hv_net_vsc.c Sat Apr 5 22:28:46 2014 (r264176)
> +++ head/sys/dev/hyperv/netvsc/hv_net_vsc.c Sat Apr 5 22:42:00 2014 (r264177)
> @@ -182,7 +182,7 @@ hv_nv_init_rx_buffer_with_net_vsp(struct
> /* Send the gpadl notification request */
>
> ret = hv_vmbus_channel_send_packet(device->channel, init_pkt,
> - sizeof(nvsp_msg), (uint64_t)init_pkt,
> + sizeof(nvsp_msg), (uint64_t)(uintptr_t)init_pkt,
> HV_VMBUS_PACKET_TYPE_DATA_IN_BAND,
> HV_VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
> if (ret != 0) {
The cast was presumably to pass a pointer to a function expectly a uint64_t.
Casting to uint64_t was wrong for this. Pointer to integer casts should
only use uintptr_t or intptr_t.
After changing to the correct cast, the cast to uint64_t has no effect.
Conversion from one integer type to another is done automatically by
the prototype. It even works correctly on amd64 and i386, since uintptr_t
is no larger than uint64_t. Conversion from pointer to integer is not
done automatically by prototypes, even if it would work correctly. I
forget it the C standard requires this of if it is just a warning.
Probably the former.
> @@ -280,7 +280,7 @@ hv_nv_init_send_buffer_with_net_vsp(stru
> /* Send the gpadl notification request */
>
> ret = hv_vmbus_channel_send_packet(device->channel, init_pkt,
> - sizeof(nvsp_msg), (uint64_t)init_pkt,
> + sizeof(nvsp_msg), (uint64_t)(uintptr_t)init_pkt,
> HV_VMBUS_PACKET_TYPE_DATA_IN_BAND,
> HV_VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
> if (ret != 0) {
All the changes keep the bogus cast.
> ...
> Modified: head/sys/dev/hyperv/netvsc/hv_netvsc_drv_freebsd.c
> ==============================================================================
> --- head/sys/dev/hyperv/netvsc/hv_netvsc_drv_freebsd.c Sat Apr 5 22:28:46 2014 (r264176)
> +++ head/sys/dev/hyperv/netvsc/hv_netvsc_drv_freebsd.c Sat Apr 5 22:42:00 2014 (r264177)
> ...
> @@ -494,7 +494,7 @@ retry_send:
> /* Set the completion routine */
> packet->compl.send.on_send_completion = netvsc_xmit_completion;
> packet->compl.send.send_completion_context = packet;
> - packet->compl.send.send_completion_tid = (uint64_t)m_head;
> + packet->compl.send.send_completion_tid = (uint64_t)(uintptr_t)m_head;
>
> /* Removed critical_enter(), does not appear necessary */
> ret = hv_rf_on_send(device_ctx, packet);
Here the changes also break the formatting.
> @@ -682,7 +682,7 @@ netvsc_recv(struct hv_device *device_ctx
> */
> for (i=0; i < packet->page_buf_count; i++) {
> /* Shift virtual page number to form virtual page address */
> - uint8_t *vaddr = (uint8_t *)
> + uint8_t *vaddr = (uint8_t *)(uintptr_t)
> (packet->page_buffers[i].pfn << PAGE_SHIFT);
>
> hv_m_append(m_new, packet->page_buffers[i].length,
>
This one is a little different, and still broken. Oops, probably all the
casts are still broken. This one is just more obviously broken. uintptr_t
is only specified to work on pointers of type void *, but here the pointer
has type uint8_t *. From C99 (n869.txt draft):
% 7.18.1.4 Integer types capable of holding object pointers
%
% [#1] The following type designates a signed integer type
% with the property that any valid pointer to void can be
% converted to this type, then converted back to pointer to
% void, and the result will compare equal to the original
% pointer:
%
% intptr_t
%
% The following type designates an unsigned integer type with
% the property that any valid pointer to void can be converted
% to this type, then converted back to pointer to void, and
% the result will compare equal to the original pointer:
%
% uintptr_t
So almost all correct uses of uintptr_t need a pre- or post-cast of the
pointer to void * (or vice versa), and the code is verbose for another
reason.
In practice, uintptr_t and intptr_t work for casts of all pointer types,
and compilers only warn if there is a size mismatch. The C standard
requires just about all casts between pointers and integers to give
a result and doesn't require any warnings, but it only requires conversions
between void * and [u]intptr_t to be reversible. When abusing APIs to
pass pointers as integers, reversibility is exactly what is required.
> Modified: head/sys/dev/hyperv/netvsc/hv_rndis_filter.c
> ==============================================================================
> --- head/sys/dev/hyperv/netvsc/hv_rndis_filter.c Sat Apr 5 22:28:46 2014 (r264176)
> +++ head/sys/dev/hyperv/netvsc/hv_rndis_filter.c Sat Apr 5 22:42:00 2014 (r264177)
> @@ -26,6 +26,9 @@
> * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> */
>
> +#include <sys/cdefs.h>
> +__FBSDID("$FreeBSD$");
> +
> #include <sys/param.h>
> #include <sys/mbuf.h>
> #include <sys/socket.h>
> @@ -334,7 +337,7 @@ hv_rf_on_receive(struct hv_device *devic
> return (EINVAL);
>
> /* Shift virtual page number to form virtual page address */
> - rndis_hdr = (rndis_msg *)(pkt->page_buffers[0].pfn << PAGE_SHIFT);
> + rndis_hdr = (rndis_msg *)(uintptr_t)(pkt->page_buffers[0].pfn << PAGE_SHIFT);
>
> rndis_hdr = (void *)((unsigned long)rndis_hdr
> + pkt->page_buffers[0].offset);
>
Long line.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20140406183716.N5281>