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