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

[Peter Maydell]

On Tue, 8 Mar 2022 at 11:01, Michael S. Tsirkin <mst@redhat.com> wrote:
>
> 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.

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

True, I suppose. But compiler warnings are often like that: we
take the hit of having to tweak some things we know to be OK in
order to catch the real bugs in other cases.

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

Using 'unsigned long' in a cast (or anything else) is often
the wrong thing in QEMU...

> 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.

What is the uint64_t cast bringing to the table? Wouldn't
just status->desc = (uintptr_t)hdev->vqs[queue].desc;
work ?

thanks
-- PMM