[PATCH 3/3] exec: use char* for pointer arithmetic

[PATCH 3/3] exec: use char* for pointer arithmetic

[Roman Kiryanov]

void* pointer arithmetic is not in the
C standard. This change allows using
the QEMU headers with a C++ compiler.

Google-Bug-Id: 331190993
Change-Id: I5a064853429f627c17a9213910811dea4ced6174
Signed-off-by: Roman Kiryanov <rkir@google.com>
---
 include/exec/memory.h                 |  8 ++++----
 include/exec/memory_ldst_cached.h.inc | 12 ++++++------
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index d7591a60d9..738e4cef2c 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -2839,7 +2839,7 @@ static inline uint8_t address_space_ldub_cached(MemoryRegionCache *cache,
 {
     assert(addr < cache->len);
     if (likely(cache->ptr)) {
-        return ldub_p(cache->ptr + addr);
+        return ldub_p((char*)cache->ptr + addr);
     } else {
         return address_space_ldub_cached_slow(cache, addr, attrs, result);
     }
@@ -2850,7 +2850,7 @@ static inline void address_space_stb_cached(MemoryRegionCache *cache,
 {
     assert(addr < cache->len);
     if (likely(cache->ptr)) {
-        stb_p(cache->ptr + addr, val);
+        stb_p((char*)cache->ptr + addr, val);
     } else {
         address_space_stb_cached_slow(cache, addr, val, attrs, result);
     }
@@ -3123,7 +3123,7 @@ address_space_read_cached(MemoryRegionCache *cache, hwaddr addr,
     assert(addr < cache->len && len <= cache->len - addr);
     fuzz_dma_read_cb(cache->xlat + addr, len, cache->mrs.mr);
     if (likely(cache->ptr)) {
-        memcpy(buf, cache->ptr + addr, len);
+        memcpy(buf, (char*)cache->ptr + addr, len);
         return MEMTX_OK;
     } else {
         return address_space_read_cached_slow(cache, addr, buf, len);
@@ -3144,7 +3144,7 @@ address_space_write_cached(MemoryRegionCache *cache, hwaddr addr,
 {
     assert(addr < cache->len && len <= cache->len - addr);
     if (likely(cache->ptr)) {
-        memcpy(cache->ptr + addr, buf, len);
+        memcpy((char*)cache->ptr + addr, buf, len);
         return MEMTX_OK;
     } else {
         return address_space_write_cached_slow(cache, addr, buf, len);
diff --git a/include/exec/memory_ldst_cached.h.inc b/include/exec/memory_ldst_cached.h.inc
index d7834f852c..f767e53a3d 100644
--- a/include/exec/memory_ldst_cached.h.inc
+++ b/include/exec/memory_ldst_cached.h.inc
@@ -30,7 +30,7 @@ static inline uint16_t ADDRESS_SPACE_LD_CACHED(uw)(MemoryRegionCache *cache,
     assert(addr < cache->len && 2 <= cache->len - addr);
     fuzz_dma_read_cb(cache->xlat + addr, 2, cache->mrs.mr);
     if (likely(cache->ptr)) {
-        return LD_P(uw)(cache->ptr + addr);
+        return LD_P(uw)((char*)cache->ptr + addr);
     } else {
         return ADDRESS_SPACE_LD_CACHED_SLOW(uw)(cache, addr, attrs, result);
     }
@@ -42,7 +42,7 @@ static inline uint32_t ADDRESS_SPACE_LD_CACHED(l)(MemoryRegionCache *cache,
     assert(addr < cache->len && 4 <= cache->len - addr);
     fuzz_dma_read_cb(cache->xlat + addr, 4, cache->mrs.mr);
     if (likely(cache->ptr)) {
-        return LD_P(l)(cache->ptr + addr);
+        return LD_P(l)((char*)cache->ptr + addr);
     } else {
         return ADDRESS_SPACE_LD_CACHED_SLOW(l)(cache, addr, attrs, result);
     }
@@ -54,7 +54,7 @@ static inline uint64_t ADDRESS_SPACE_LD_CACHED(q)(MemoryRegionCache *cache,
     assert(addr < cache->len && 8 <= cache->len - addr);
     fuzz_dma_read_cb(cache->xlat + addr, 8, cache->mrs.mr);
     if (likely(cache->ptr)) {
-        return LD_P(q)(cache->ptr + addr);
+        return LD_P(q)((char*)cache->ptr + addr);
     } else {
         return ADDRESS_SPACE_LD_CACHED_SLOW(q)(cache, addr, attrs, result);
     }
@@ -76,7 +76,7 @@ static inline void ADDRESS_SPACE_ST_CACHED(w)(MemoryRegionCache *cache,
 {
     assert(addr < cache->len && 2 <= cache->len - addr);
     if (likely(cache->ptr)) {
-        ST_P(w)(cache->ptr + addr, val);
+        ST_P(w)((char*)cache->ptr + addr, val);
     } else {
         ADDRESS_SPACE_ST_CACHED_SLOW(w)(cache, addr, val, attrs, result);
     }
@@ -87,7 +87,7 @@ static inline void ADDRESS_SPACE_ST_CACHED(l)(MemoryRegionCache *cache,
 {
     assert(addr < cache->len && 4 <= cache->len - addr);
     if (likely(cache->ptr)) {
-        ST_P(l)(cache->ptr + addr, val);
+        ST_P(l)((char*)cache->ptr + addr, val);
     } else {
         ADDRESS_SPACE_ST_CACHED_SLOW(l)(cache, addr, val, attrs, result);
     }
@@ -98,7 +98,7 @@ static inline void ADDRESS_SPACE_ST_CACHED(q)(MemoryRegionCache *cache,
 {
     assert(addr < cache->len && 8 <= cache->len - addr);
     if (likely(cache->ptr)) {
-        ST_P(q)(cache->ptr + addr, val);
+        ST_P(q)((char*)cache->ptr + addr, val);
     } else {
         ADDRESS_SPACE_ST_CACHED_SLOW(q)(cache, addr, val, attrs, result);
     }
-- 
2.45.2.627.g7a2c4fd464-goog

Re: [PATCH 3/3] exec: use char* for pointer arithmetic

[Richard Henderson]

On 6/18/24 15:46, Roman Kiryanov wrote:
> @@ -2839,7 +2839,7 @@ static inline uint8_t address_space_ldub_cached(MemoryRegionCache *cache,
>   {
>       assert(addr < cache->len);
>       if (likely(cache->ptr)) {
> -        return ldub_p(cache->ptr + addr);
> +        return ldub_p((char*)cache->ptr + addr);

We require "char *" with a space.

With all of those fixed,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~


PS: I'm annoyed that standards never adopted arithmetic on void *.

Re: [PATCH 3/3] exec: use char* for pointer arithmetic

[Roman Kiryanov]

Hi Richard,

On Tue, Jun 18, 2024 at 4:05 PM Richard Henderson
<richard.henderson@linaro.org> wrote:
> We require "char *" with a space.

thank you for looking into this. I sent v2 for this one.

Re: [PATCH 3/3] exec: use char* for pointer arithmetic

[Daniel P. Berrangé]

On Tue, Jun 18, 2024 at 04:05:36PM -0700, Richard Henderson wrote:
> On 6/18/24 15:46, Roman Kiryanov wrote:
> > @@ -2839,7 +2839,7 @@ static inline uint8_t address_space_ldub_cached(MemoryRegionCache *cache,
> >   {
> >       assert(addr < cache->len);
> >       if (likely(cache->ptr)) {
> > -        return ldub_p(cache->ptr + addr);
> > +        return ldub_p((char*)cache->ptr + addr);
> 
> We require "char *" with a space.
> 
> With all of those fixed,
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> 
> PS: I'm annoyed that standards never adopted arithmetic on void *.

NB, QEMU is explicitly *NOT* targetting the C standard, we are
targetting the C dialect supported by GCC and CLang only. IOW,
if they have well defined behaviour for arithmetic on void *,
then we are free to use it.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 3/3] exec: use char* for pointer arithmetic

[Alex Bennée]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Tue, Jun 18, 2024 at 04:05:36PM -0700, Richard Henderson wrote:
>> On 6/18/24 15:46, Roman Kiryanov wrote:
>> > @@ -2839,7 +2839,7 @@ static inline uint8_t address_space_ldub_cached(MemoryRegionCache *cache,
>> >   {
>> >       assert(addr < cache->len);
>> >       if (likely(cache->ptr)) {
>> > -        return ldub_p(cache->ptr + addr);
>> > +        return ldub_p((char*)cache->ptr + addr);
>> 
>> We require "char *" with a space.
>> 
>> With all of those fixed,
>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>> 
>> PS: I'm annoyed that standards never adopted arithmetic on void *.
>
> NB, QEMU is explicitly *NOT* targetting the C standard, we are
> targetting the C dialect supported by GCC and CLang only. IOW,
> if they have well defined behaviour for arithmetic on void *,
> then we are free to use it.

It looks like GNU C does support it:

  https://gcc.gnu.org/onlinedocs/gcc/Pointer-Arith.html

>
> With regards,
> Daniel

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH 3/3] exec: use char* for pointer arithmetic

[Roman Kiryanov]

Hi Daniel and Alex,

On Thu, Jun 20, 2024 at 8:10 AM Alex Bennée <alex.bennee@linaro.org> wrote:
>
> Daniel P. Berrangé <berrange@redhat.com> writes:
> > NB, QEMU is explicitly *NOT* targetting the C standard, we are
> > targetting the C dialect supported by GCC and CLang only. IOW,
> > if they have well defined behaviour for arithmetic on void *,
> > then we are free to use it.
>
> It looks like GNU C does support it:

GCC does support void* pointer arithmetic as an extension. But if you
decide to use other compilers, you might not have the same luck. We
(and maybe other developers) would like to use the QEMU headers with a
C++ compiler where this extension is not available.

Regards,
Roman.

Re: [PATCH 3/3] exec: use char* for pointer arithmetic

[Peter Maydell]

On Thu, 20 Jun 2024 at 17:24, Roman Kiryanov <rkir@google.com> wrote:
>
> Hi Daniel and Alex,
>
> On Thu, Jun 20, 2024 at 8:10 AM Alex Bennée <alex.bennee@linaro.org> wrote:
> >
> > Daniel P. Berrangé <berrange@redhat.com> writes:
> > > NB, QEMU is explicitly *NOT* targetting the C standard, we are
> > > targetting the C dialect supported by GCC and CLang only. IOW,
> > > if they have well defined behaviour for arithmetic on void *,
> > > then we are free to use it.
> >
> > It looks like GNU C does support it:
>
> GCC does support void* pointer arithmetic as an extension. But if you
> decide to use other compilers, you might not have the same luck. We
> (and maybe other developers) would like to use the QEMU headers with a
> C++ compiler where this extension is not available.

I think this is the point where I would say "you're making the
code worse for upstream and the only benefit is to your out-of-tree
downstream code". If you want to build QEMU, use one of the compilers
that QEMU supports. There are lots and lots of places where we
assume the GCC-or-clang feature set over and above plain C.

thanks
-- PMM

Re: [PATCH 3/3] exec: use char* for pointer arithmetic

[Roman Kiryanov]

Hi Peter, thank you for looking.

On Thu, Jun 20, 2024 at 12:09 PM Peter Maydell <peter.maydell@linaro.org> wrote:
> I think this is the point where I would say "you're making the
> code worse for upstream and the only benefit is to your out-of-tree
> downstream code". If you want to build QEMU, use one of the compilers
> that QEMU supports.

I think there is a value in letting other developers (not just us) to build
their code with QEMU. I understand your concern and sent an
updated version of the patch, which is only two lines long.

> There are lots and lots of places where we
> assume the GCC-or-clang feature set over and above plain C.

I am not changing this part.

Re: [PATCH 3/3] exec: use char* for pointer arithmetic

[Paolo Bonzini]

On 6/19/24 00:46, Roman Kiryanov wrote:
> void* pointer arithmetic is not in the
> C standard. This change allows using
> the QEMU headers with a C++ compiler.
> 
> Google-Bug-Id: 331190993
> Change-Id: I5a064853429f627c17a9213910811dea4ced6174
> Signed-off-by: Roman Kiryanov <rkir@google.com>

Would it work instead to declare MemoryRegionCache's ptr field as char*?

Thanks,

Paolo

Re: [PATCH 3/3] exec: use char* for pointer arithmetic

[Richard Henderson]

On 6/20/24 11:06, Paolo Bonzini wrote:
> On 6/19/24 00:46, Roman Kiryanov wrote:
>> void* pointer arithmetic is not in the
>> C standard. This change allows using
>> the QEMU headers with a C++ compiler.
>>
>> Google-Bug-Id: 331190993
>> Change-Id: I5a064853429f627c17a9213910811dea4ced6174
>> Signed-off-by: Roman Kiryanov <rkir@google.com>
> 
> Would it work instead to declare MemoryRegionCache's ptr field as char*?

I prefer to use char* only where there are strings.  For unstructured data such as 
MemoryRegionCache, void* is more appropriate.


r~

Re: [PATCH 3/3] exec: use char* for pointer arithmetic

[Paolo Bonzini]

On Thu, Jun 20, 2024 at 8:14 PM Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 6/20/24 11:06, Paolo Bonzini wrote:
> > On 6/19/24 00:46, Roman Kiryanov wrote:
> >> void* pointer arithmetic is not in the
> >> C standard. This change allows using
> >> the QEMU headers with a C++ compiler.
> >>
> >> Google-Bug-Id: 331190993
> >> Change-Id: I5a064853429f627c17a9213910811dea4ced6174
> >> Signed-off-by: Roman Kiryanov <rkir@google.com>
> >
> > Would it work instead to declare MemoryRegionCache's ptr field as char*?
>
> I prefer to use char* only where there are strings.  For unstructured data such as
> MemoryRegionCache, void* is more appropriate.

Or uint8_t*... I agree about char*, but unless casts are needed, I
find uint8_t and void pointers to be more or less interchangeable.

The problem is that casts are a bit uglier and (while unlikely in this
particular case) more subject to bit rot.

Paolo

Re: [PATCH 3/3] exec: use char* for pointer arithmetic

[Roman Kiryanov]

On Thu, Jun 20, 2024 at 11:16 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
> > > Would it work instead to declare MemoryRegionCache's ptr field as char*?
> >
> > I prefer to use char* only where there are strings.  For unstructured data such as
> > MemoryRegionCache, void* is more appropriate.
>
> Or uint8_t*... I agree about char*, but unless casts are needed, I
> find uint8_t and void pointers to be more or less interchangeable.
>
> The problem is that casts are a bit uglier and (while unlikely in this
> particular case) more subject to bit rot.

Will `typedef char *MemoryRegionCachePtr;` or making the ptr field
pointing to `struct MemoryRegionCacheData { char unused; }` work
better?