Re: [PULL v2 00/47] virtio,pc,pci: features, cleanups, fixes

[Michael S. Tsirkin]

On Tue, Mar 08, 2022 at 09:05:27AM +0000, Peter Maydell wrote:
> On Mon, 7 Mar 2022 at 22:52, Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Mon, Mar 07, 2022 at 05:13:16PM +0000, Peter Maydell wrote:
> > > Also fails on cross-win64-system:
> > >
> > > https://gitlab.com/qemu-project/qemu/-/jobs/2172339938
> > >
> > > ../hw/virtio/virtio.c: In function 
> > > 'qmp_x_query_virtio_vhost_queue_status':
> > > ../hw/virtio/virtio.c:4358:30: error: cast from pointer to integer of
> > > different size [-Werror=pointer-to-int-cast]
> > > 4358 | status->desc = (uint64_t)(unsigned long)hdev->vqs[queue].desc;
> > > | ^
> > > ../hw/virtio/virtio.c:4359:31: error: cast from pointer to integer of
> > > different size [-Werror=pointer-to-int-cast]
> > > 4359 | status->avail = (uint64_t)(unsigned long)hdev->vqs[queue].avail;
> > > | ^
> > > ../hw/virtio/virtio.c:4360:30: error: cast from pointer to integer of
> > > different size [-Werror=pointer-to-int-cast]
> > > 4360 | status->used = (uint64_t)(unsigned long)hdev->vqs[queue].used;
> > > | ^
> > > cc1: all warnings being treated as errors
> 
> > I dropped these for now but I really question the value of this warning,
> > as you can see the reason we have the buggy cast to unsigned long
> > is because someone wanted to shut up the warning on a 32 bit system.
> >
> > Now, I could maybe get behind this if it simply warned about a cast that
> > loses information (cast to a smaller integer) or integer/pointer cast
> > that does not go through uintptr_t without regard to size.
> 
> This *is* warning about losing information. On 64-bit Windows
> pointers are 64 bits but 'long' is 32 bits, so the path
> pointer -> long -> uint64_t drops the top half of the pointer.
> 
> thanks
> -- PMM

Yes obviously. My point is that this:
(uint64_t)hdev->vqs[queue].avail
is always harmless but it warns on a 32 bit system.

And someone trying to fix that *is* what resulted in
(uint64_t)(unsigned long)hdev->vqs[queue].avail


IOW I don't really see how
(uint64_t)(uintptr_t)hdev->vqs[queue].avail
is better than
(uint64_t)hdev->vqs[queue].avail

except as a way to say "yes I do intend to cast pointer to integer
here, I did not forget to dereference the pointer". But if that
latter is what gcc is trying to warn about, then it should
just warn about any cast to integer except to uintptr_t,
without respect to size.

-- 
MST