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

[Michael S. Tsirkin]

On Tue, Mar 08, 2022 at 11:18:38AM +0000, Peter Maydell wrote:
> 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

True too. I would be happy if this gave a warning:

struct {
        uint32_t foo;
} bar = { .foo = (uintptr_t)hdev->vqs[queue].avail }

but of course it doesn't, uintptr_t is just an integer
type as far as gcc is concerned :(

Maybe adding things like that to sparse might be practical.

Generally sparse seems to mainly gain features from the
kernel, QEMU's needs are different. Could be useful to collect
ideas e.g. for a GSoC project.

-- 
MST