From owner-svn-src-all@FreeBSD.ORG Sun Apr 6 08:57:55 2014 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 69A923FD; Sun, 6 Apr 2014 08:57:55 +0000 (UTC) Received: from mail108.syd.optusnet.com.au (mail108.syd.optusnet.com.au [211.29.132.59]) by mx1.freebsd.org (Postfix) with ESMTP id 1181F887; Sun, 6 Apr 2014 08:57:54 +0000 (UTC) Received: from c122-106-147-133.carlnfd1.nsw.optusnet.com.au (c122-106-147-133.carlnfd1.nsw.optusnet.com.au [122.106.147.133]) by mail108.syd.optusnet.com.au (Postfix) with ESMTPS id A29C21A28C7; Sun, 6 Apr 2014 18:57:47 +1000 (EST) Date: Sun, 6 Apr 2014 18:57:46 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Warner Losh Subject: Re: svn commit: r264177 - in head/sys/dev/hyperv: netvsc storvsc In-Reply-To: <201404052242.s35Mg1ES027714@svn.freebsd.org> Message-ID: <20140406183716.N5281@besplex.bde.org> References: <201404052242.s35Mg1ES027714@svn.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.1 cv=eojmkOZX c=1 sm=1 tr=0 a=7NqvjVvQucbO2RlWB8PEog==:117 a=PO7r1zJSAAAA:8 a=hhxHlBTYTMkA:10 a=kj9zAlcOel0A:10 a=JzwRw_2MAAAA:8 a=fr98tUtQFgwMT2tNaHwA:9 a=XS83lCpg9cdRuOTd:21 a=6n9Cz6lIPdhr3v_N:21 a=CjuIK1q_8ugA:10 Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.17 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 06 Apr 2014 08:57:55 -0000 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 > +__FBSDID("$FreeBSD$"); > + > #include > #include > #include > @@ -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