Re: [PATCH v2] Drop more useless casts from void * to pointer

[BALATON Zoltan]

On Wed, 23 Nov 2022, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> > On Wed, Nov 23, 2022 at 02:51:49PM +0100, BALATON Zoltan wrote:
> > > On Wed, 23 Nov 2022, Markus Armbruster wrote:
> > > > Signed-off-by: Markus Armbruster <armbru@redhat.com>
> > > > Reviewed-by: Laurent Vivier <laurent@vivier.eu>
> > > > ---
> > > > v2:
> > > > * PATCH 1+2 merged as commit 0a553c12c7 and 3f7febc937
> > > > * PATCH 3 change to util/coroutine-ucontext.c dropped [Laurent]
> > > >
> > > > bsd-user/elfload.c                      | 2 +-
> > > > contrib/plugins/cache.c                 | 8 ++++----
> > > > contrib/vhost-user-blk/vhost-user-blk.c | 2 +-
> > > > hw/core/qdev-clock.c                    | 2 +-
> > > > hw/hyperv/vmbus.c                       | 2 +-
> > > > hw/net/cadence_gem.c                    | 2 +-
> > > > hw/net/virtio-net.c                     | 2 +-
> > > > hw/nvme/ctrl.c                          | 4 ++--
> > > > hw/rdma/vmw/pvrdma_cmd.c                | 9 +++------
> > > > hw/rdma/vmw/pvrdma_qp_ops.c             | 6 +++---
> > > > hw/virtio/virtio-iommu.c                | 3 +--
> > > > linux-user/syscall.c                    | 2 +-
> > > > target/i386/hax/hax-all.c               | 2 +-
> > > > tests/tcg/aarch64/system/semiheap.c     | 4 ++--
> > > > util/vfio-helpers.c                     | 2 +-
> > > > 15 files changed, 24 insertions(+), 28 deletions(-)
> > > >
> > > > diff --git a/bsd-user/elfload.c b/bsd-user/elfload.c
> > > > index f8edb22f2a..fbcdc94b96 100644
> > > > --- a/bsd-user/elfload.c
> > > > +++ b/bsd-user/elfload.c
> > > > @@ -156,7 +156,7 @@ static abi_ulong copy_elf_strings(int argc, char **argv, 
> > > > void **page,
> > > >             --p; --tmp; --len;
> > > >             if (--offset < 0) {
> > > >                 offset = p % TARGET_PAGE_SIZE;
> > > > -                pag = (char *)page[p / TARGET_PAGE_SIZE];
> > > > +                pag = page[p / TARGET_PAGE_SIZE];
> > >
> > > I think arithmetic on void pointer was undefined at least in the past so
> > > some compilers may warn for it but not sure if this is still the case for
> > > the compilers we care about. Apparently not if this now compiles but that
> > > explains why this cast was not useless.
>
> I don't think so :)
>
> @pag is char *.
>
> @page is void **.
>
> page[p / TARGET_PAGE_SIZE] is void *.  No need to cast to char * before
> assigning to @pag.

You are right. Although I'm not sure what page[] counts as because it adds 
an offset to the pointer but [] is higher priority than (type) cast so it 
does not matter and that cast is not needed here then. Maybe I should be 
more attentive to details but I did not take the time to look at it more 
carefully. I did not say we should keep the cast anyway (considering only 
gcc and clang are targeted), I was just trying to understand why it might 
have been there in the first place.

Regards,
BALATON Zoltan

> No pointer arithmetic so far.  There's some further down: pag + offset.
> @pag is char * before and after my patch.
>
> > >                                         Found some more info on this here:
> > >
> > > https://stackoverflow.com/questions/3523145/pointer-arithmetic-for-void-pointer-in-c
> >
> > QEMU explicitly only targets GCC + Clang, so portability to other
> > compilers is not required.
>
> Correct.  We do arithmentic with void * in many places already.
>
> If we cared for portability to other compilers, we'd enable
>
> '-Wpointer-arith'
>     Warn about anything that depends on the "size of" a function type
>     or of 'void'.  GNU C assigns these types a size of 1, for
>     convenience in calculations with 'void *' pointers and pointers to
>     functions.  In C++, warn also when an arithmetic operation involves
>     'NULL'.  This warning is also enabled by '-Wpedantic'.
>
> But we don't.